[scudo] update Pushedblocks/PoppedBlocks in Impl functions

[cferris]

A couple of comment nits.

compiler-rt/lib/scudo/standalone/primary32.h

712

is the internal

713

Would this be better as something like:

values back to avoid incorrectly setting the stats.

[Chia-hungDuan]

Revise comment in both primary32.h and primary64.h

[cferris]

LGTM.

compiler-rt/lib/scudo/standalone/primary32.h

TransferBatch *popBatch(CacheT *C, uptr ClassId) {
     TransferBatch *B = popBatchImpl(C, ClassId, Sci);
     if (UNLIKELY(!B)) {
       if (UNLIKELY(!populateFreeList(C, ClassId, Sci)))
         return nullptr;
       B = popBatchImpl(C, ClassId, Sci);
       // if `populateFreeList` succeeded, we are supposed to get free blocks.
       DCHECK_NE(B, nullptr);
     }
-    Sci->FreeListInfo.PoppedBlocks += B->getCount();
     return B;
   }
 
   // Push the array of free blocks to the designated batch group.
   void pushBlocks(CacheT *C, uptr ClassId, CompactPtrT *Array, u32 Size) {
     DCHECK_LT(ClassId, NumClasses);
     DCHECK_GT(Size, 0);
 
     SizeClassInfo *Sci = getSizeClassInfo(ClassId);
     if (ClassId == SizeClassMap::BatchClassId) {
       ScopedLock L(Sci->Mutex);
       // Constructing a batch group in the free list will use two blocks in
       // BatchClassId. If we are pushing BatchClassId blocks, we will use the
       // blocks in the array directly (can't delegate local cache which will
       // cause a recursive allocation). However, The number of free blocks may
       // be less than two. Therefore, populate the free list before inserting
       // the blocks.
       if (Size == 1 && !populateFreeList(C, ClassId, Sci))
         return;
       pushBlocksImpl(C, ClassId, Sci, Array, Size);
-      Sci->FreeListInfo.PushedBlocks += Size;
       return;
     }
 
     // TODO(chiahungduan): Consider not doing grouping if the group size is not
     // greater than the block size with a certain scale.
 
     // Sort the blocks so that blocks belonging to the same group can be pushed
     // together.
for (u32 I = 1; I < Size; ++I) {
         --J;
       }
       Array[J] = Cur;
     }
 
     ScopedLock L(Sci->Mutex);
     pushBlocksImpl(C, ClassId, Sci, Array, Size, SameGroup);
 
-    Sci->FreeListInfo.PushedBlocks += Size;
     if (ClassId != SizeClassMap::BatchClassId)
       releaseToOSMaybe(Sci, ClassId);
   }
 
   void disable() NO_THREAD_SAFETY_ANALYSIS {
     // The BatchClassId must be locked last since other classes can use it.
     for (sptr I = static_cast<sptr>(NumClasses) - 1; I >= 0; I--) {
       if (static_cast<uptr>(I) == SizeClassMap::BatchClassId)
auto InsertBlocks = [&](BatchGroup *BG, CompactPtrT *Array, u32 Size) {
         u16 AppendSize = static_cast<u16>(Min<u32>(UnusedSlots, Size - I));
         CurBatch->appendFromArray(&Array[I], AppendSize);
         I += AppendSize;
       }
 
       BG->PushedBlocks += Size;
     };
 
+    Sci->FreeListInfo.PushedBlocks += Size;
     BatchGroup *Cur = Sci->FreeListInfo.BlockList.front();
 
     if (ClassId == SizeClassMap::BatchClassId) {
       if (Cur == nullptr) {
         // Don't need to classify BatchClassId.
         Cur = CreateGroup(/*CompactPtrGroupBase=*/0);
         Sci->FreeListInfo.BlockList.push_front(Cur);
       }
if (Batches.empty()) {
       // allocating. Note that block used by constructing BatchGroup is recorded
       // as free blocks in the last element of BatchGroup::Batches. Which means,
       // once we pop the last TransferBatch, the block is implicitly
       // deallocated.
       if (ClassId != SizeClassMap::BatchClassId)
         C->deallocate(SizeClassMap::BatchClassId, BG);
     }
 
+    Sci->FreeListInfo.PoppedBlocks += B->getCount();
     return B;
   }
 
   NOINLINE bool populateFreeList(CacheT *C, uptr ClassId, SizeClassInfo *Sci)
       REQUIRES(Sci->Mutex) {
     uptr Region;
     uptr Offset;
     // If the size-class currently has a region associated to it, use it. The
if (ClassId != SizeClassMap::BatchClassId) {
       shuffle(ShuffleArray + NumberOfBlocks - N, N, &Sci->RandState);
       pushBlocksImpl(C, ClassId, Sci, &ShuffleArray[NumberOfBlocks - N], N,
                      /*SameGroup=*/true);
     } else {
       pushBlocksImpl(C, ClassId, Sci, ShuffleArray, NumberOfBlocks,
                      /*SameGroup=*/true);
     }
 
+    // Note that `PushedBlocks` and `PoppedBlocks` are supposed to only record
+    // the requests from `PushBlocks` and `PopBatch` which are external
+    // interfaces. `populateFreeList` is the internal interface so we should set
+    // the values back to avoid incorrectly setting the stats.
+    Sci->FreeListInfo.PushedBlocks -= NumberOfBlocks;
+
     const uptr AllocatedUser = Size * NumberOfBlocks;
     C->getStats().add(StatFree, AllocatedUser);
     DCHECK_LE(Sci->CurrentRegionAllocated + AllocatedUser, RegionSize);
     // If there is not enough room in the region currently associated to fit
     // more blocks, we deassociate the region by resetting CurrentRegion and
     // CurrentRegionAllocated. Otherwise, update the allocated amount.
     if (RegionSize - (Sci->CurrentRegionAllocated + AllocatedUser) < Size) {
       Sci->CurrentRegion = 0;

compiler-rt/lib/scudo/standalone/primary64.h

public:
 
   TransferBatch *popBatch(CacheT *C, uptr ClassId) {
     DCHECK_LT(ClassId, NumClasses);
     RegionInfo *Region = getRegionInfo(ClassId);
 
     {
       ScopedLock L(Region->FLLock);
       TransferBatch *B = popBatchImpl(C, ClassId, Region);
-      if (LIKELY(B)) {
-        Region->FreeListInfo.PoppedBlocks += B->getCount();
+      if (LIKELY(B))
         return B;
-      }
-    }
 
     bool PrintStats = false;
 
     while (true) {
       // When two threads compete for `Region->MMLock`, we only want one of them
       // to call populateFreeList(). To avoid both of them doing that, always
       // check the freelist before mapping new pages.
       //
       // TODO(chiahungduan): Use a condition variable so that we don't need to
       // hold `Region->MMLock` here.
       ScopedLock ML(Region->MMLock);
       {
         ScopedLock FL(Region->FLLock);
         TransferBatch *B = popBatchImpl(C, ClassId, Region);
-        if (LIKELY(B)) {
-          Region->FreeListInfo.PoppedBlocks += B->getCount();
+        if (LIKELY(B))
           return B;
-        }
-      }
 
       const bool RegionIsExhausted = Region->Exhausted;
       if (!RegionIsExhausted) {
         // TODO: Make sure the one who does populateFreeList() gets one Batch
         // here.
         populateFreeList(C, ClassId, Region);
       }
       PrintStats = !RegionIsExhausted && Region->Exhausted;
if (ClassId == SizeClassMap::BatchClassId) {
         // the comment of popBatch().
         ScopedLock L(Region->MMLock);
         {
           ScopedLock L(Region->FLLock);
           const bool NeedToRefill =
               Size == 1U && Region->FreeListInfo.BlockList.empty();
           if (!NeedToRefill) {
             pushBlocksImpl(C, SizeClassMap::BatchClassId, Region, Array, Size);
-            Region->FreeListInfo.PushedBlocks += Size;
             return;
           }
         }
 
         if (UNLIKELY(!populateFreeList(C, SizeClassMap::BatchClassId, Region)))
           break;
       }
 
for (u32 I = 1; I < Size; ++I) {
         --J;
       }
       Array[J] = Cur;
     }
 
     {
       ScopedLock L(Region->FLLock);
       pushBlocksImpl(C, ClassId, Region, Array, Size, SameGroup);
-      Region->FreeListInfo.PushedBlocks += Size;
     }
 
     // Only non-BatchClass will be here, try to release the pages in the region.
 
     // Note that the tryLock() may fail spuriously, given that it should rarely
     // happen and page releasing is fine to skip, we don't take certain
     // approaches to ensure one page release is done.
     if (Region->MMLock.tryLock()) {
auto InsertBlocks = [&](BatchGroup *BG, CompactPtrT *Array, u32 Size) {
         u16 AppendSize = static_cast<u16>(Min<u32>(UnusedSlots, Size - I));
         CurBatch->appendFromArray(&Array[I], AppendSize);
         I += AppendSize;
       }
 
       BG->PushedBlocks += Size;
     };
 
+    Region->FreeListInfo.PushedBlocks += Size;
     BatchGroup *Cur = Region->FreeListInfo.BlockList.front();
 
     if (ClassId == SizeClassMap::BatchClassId) {
       if (Cur == nullptr) {
         // Don't need to classify BatchClassId.
         Cur = CreateGroup(/*CompactPtrGroupBase=*/0);
         Region->FreeListInfo.BlockList.push_front(Cur);
       }
if (Batches.empty()) {
       // allocating. Note that block used by constructing BatchGroup is recorded
       // as free blocks in the last element of BatchGroup::Batches. Which means,
       // once we pop the last TransferBatch, the block is implicitly
       // deallocated.
       if (ClassId != SizeClassMap::BatchClassId)
         C->deallocate(SizeClassMap::BatchClassId, BG);
     }
 
+    Region->FreeListInfo.PoppedBlocks += B->getCount();
+
     return B;
   }
 
   NOINLINE bool populateFreeList(CacheT *C, uptr ClassId, RegionInfo *Region)
       REQUIRES(Region->MMLock) EXCLUDES(Region->FLLock) {
     const uptr Size = getSizeByClassId(ClassId);
     const u16 MaxCount = TransferBatch::getMaxCached(Size);
 
if (ClassId != SizeClassMap::BatchClassId) {
       shuffle(ShuffleArray + NumberOfBlocks - N, N, &Region->RandState);
       pushBlocksImpl(C, ClassId, Region, &ShuffleArray[NumberOfBlocks - N], N,
                      /*SameGroup=*/true);
     } else {
       pushBlocksImpl(C, ClassId, Region, ShuffleArray, NumberOfBlocks,
                      /*SameGroup=*/true);
     }
 
+    // Note that `PushedBlocks` and `PoppedBlocks` are supposed to only record
+    // the requests from `PushBlocks` and `PopBatch` which are external
+    // interfaces. `populateFreeListAndPopBatch` is the internal interface so we
+    // should set the values back to avoid incorrectly setting the stats.
+    Region->FreeListInfo.PushedBlocks -= NumberOfBlocks;
+
     const uptr AllocatedUser = Size * NumberOfBlocks;
     C->getStats().add(StatFree, AllocatedUser);
     Region->MemMapInfo.AllocatedUser += AllocatedUser;
 
     return true;
   }
 
   void getStats(ScopedString *Str, uptr ClassId, RegionInfo *Region, uptr Rss)