[scudo] Acquire FLLock in mergeGroupsToReleaseBack

[Chia-hungDuan]

Summary

This removes the need of ScopedUnlock and make the future use of
condition variable easier to wake up waiting threads.

[cferris]

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

1057

Would it make sense to change this to Recorder.getReleasedBytes() and remove the uptr ReleasedBytes up above? Since this isn't under a lock any more, that variable isn't needed.

[Chia-hungDuan]

Address review comment

[cferris]

LGTM.

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

Str->append(
         Region->ReleaseInfo.LastReleasedBytes >> 10,
         RegionPushedBytesDelta >> 10, Region->RegionBeg,
         getRegionBaseByClassId(ClassId));
   }
 
   NOINLINE uptr releaseToOSMaybe(RegionInfo *Region, uptr ClassId,
                                  ReleaseToOS ReleaseType = ReleaseToOS::Normal)
       REQUIRES(Region->MMLock) EXCLUDES(Region->FLLock) {
+    const uptr BlockSize = getSizeByClassId(ClassId);
+    uptr BytesInFreeList;
+    const uptr AllocatedUserEnd =
+        Region->MemMapInfo.AllocatedUser + Region->RegionBeg;
+    SinglyLinkedList<BatchGroup> GroupsToRelease;
+
+    {
       ScopedLock L(Region->FLLock);
 
-    const uptr BlockSize = getSizeByClassId(ClassId);
-    const uptr BytesInFreeList =
-        Region->MemMapInfo.AllocatedUser - (Region->FreeListInfo.PoppedBlocks -
-                                            Region->FreeListInfo.PushedBlocks) *
-                                               BlockSize;
+      BytesInFreeList = Region->MemMapInfo.AllocatedUser -
+                        (Region->FreeListInfo.PoppedBlocks -
       if (UNLIKELY(BytesInFreeList == 0))
         return false;
 
-    const uptr AllocatedUserEnd =
-        Region->MemMapInfo.AllocatedUser + Region->RegionBeg;
-    const uptr CompactPtrBase = getCompactPtrBaseByClassId(ClassId);
-
-    // ====================================================================== //
+      // ==================================================================== //
       // 1. Check if we have enough free blocks and if it's worth doing a page
       //    release.
-    // ====================================================================== //
+      // ==================================================================== //
       if (ReleaseType != ReleaseToOS::ForceAll &&
           !hasChanceToReleasePages(Region, BlockSize, BytesInFreeList,
                                    ReleaseType)) {
         return 0;
       }
 
-    // ====================================================================== //
+      // ==================================================================== //
       // 2. Determine which groups can release the pages. Use a heuristic to
       //    gather groups that are candidates for doing a release.
-    // ====================================================================== //
-    SinglyLinkedList<BatchGroup> GroupsToRelease;
+      // ==================================================================== //
       if (ReleaseType == ReleaseToOS::ForceAll) {
         GroupsToRelease = Region->FreeListInfo.BlockList;
         Region->FreeListInfo.BlockList.clear();
       } else {
-      GroupsToRelease = collectGroupsToRelease(
-          Region, BlockSize, AllocatedUserEnd, CompactPtrBase);
+        GroupsToRelease =
+            collectGroupsToRelease(Region, BlockSize, AllocatedUserEnd,
+                                   getCompactPtrBaseByClassId(ClassId));
       }
       if (GroupsToRelease.empty())
         return 0;
-
-    // Ideally, we should use a class like `ScopedUnlock`. However, this form of
-    // unlocking is not supported by the thread-safety analysis. See
-    // https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis
-    // for more details.
-    // Put it as local class so that we can mark the ctor/dtor with proper
-    // annotations associated to the target lock. Note that accessing the
-    // function variable in local class only works in thread-safety annotations.
-    // TODO: Implement general `ScopedUnlock` when it's supported.
-    class FLLockScopedUnlock {
-    public:
-      FLLockScopedUnlock(RegionInfo *Region) RELEASE(Region->FLLock)
-          : R(Region) {
-        R->FLLock.assertHeld();
-        R->FLLock.unlock();
-      }
-      ~FLLockScopedUnlock() ACQUIRE(Region->FLLock) { R->FLLock.lock(); }
-
-    private:
-      RegionInfo *R;
-    };
 
     // Note that we have extracted the `GroupsToRelease` from region freelist.
     // It's safe to let pushBlocks()/popBatches() access the remaining region
     // freelist. In the steps 3 and 4, we will temporarily release the FLLock
     // and lock it again before step 5.
 
-    uptr ReleasedBytes = 0;
-    {
-      FLLockScopedUnlock UL(Region);
-      // ==================================================================== //
-      // 3. Mark the free blocks in `GroupsToRelease` in the
-      //    `PageReleaseContext`. Then we can tell which pages are in-use by
-      //    querying `PageReleaseContext`.
-      // ==================================================================== //
-      PageReleaseContext Context = markFreeBlocks(
-          Region, BlockSize, AllocatedUserEnd, CompactPtrBase, GroupsToRelease);
-      if (UNLIKELY(!Context.hasBlockMarked())) {
-        ScopedLock L(Region->FLLock);
-        mergeGroupsToReleaseBack(Region, GroupsToRelease);
-        return 0;
-      }
+    // 3. Mark the free blocks in `GroupsToRelease` in the `PageReleaseContext`.
+    //    Then we can tell which pages are in-use by querying
+    //    `PageReleaseContext`.
+    PageReleaseContext Context =
+        markFreeBlocks(Region, BlockSize, AllocatedUserEnd,
+                       getCompactPtrBaseByClassId(ClassId), GroupsToRelease);
 
-      // ==================================================================== //
-      // 4. Release the unused physical pages back to the OS.
-      // ==================================================================== //
-      RegionReleaseRecorder<MemMapT> Recorder(&Region->MemMapInfo.MemMap,
-                                              Region->RegionBeg,
-                                              Context.getReleaseOffset());
-      auto SkipRegion = [](UNUSED uptr RegionIndex) { return false; };
-      releaseFreeMemoryToOS(Context, Recorder, SkipRegion);
-      if (Recorder.getReleasedRangesCount() > 0) {
-        Region->ReleaseInfo.BytesInFreeListAtLastCheckpoint = BytesInFreeList;
-        Region->ReleaseInfo.RangesReleased += Recorder.getReleasedRangesCount();
-        Region->ReleaseInfo.LastReleasedBytes = Recorder.getReleasedBytes();
-      }
-      Region->ReleaseInfo.LastReleaseAtNs = getMonotonicTimeFast();
-      ReleasedBytes = Recorder.getReleasedBytes();
-    }
 
     // ====================================================================== //
     // 5. Merge the `GroupsToRelease` back to the freelist.
     // ====================================================================== //
     mergeGroupsToReleaseBack(Region, GroupsToRelease);
 
-    return ReleasedBytes;
+    return Recorder.getReleasedBytes();
   }
 
   bool hasChanceToReleasePages(RegionInfo *Region, uptr BlockSize,
                                uptr BytesInFreeList, ReleaseToOS ReleaseType)
       REQUIRES(Region->MMLock, Region->FLLock) {
     DCHECK_GE(Region->FreeListInfo.PoppedBlocks,
               Region->FreeListInfo.PushedBlocks);
     const uptr PageSize = getPageSizeCached();
markFreeBlocks(RegionInfo *Region, const uptr BlockSize,
 
     DCHECK(Context.hasBlockMarked());
 
     return Context;
   }
 
   void mergeGroupsToReleaseBack(RegionInfo *Region,
                                 SinglyLinkedList<BatchGroup> &GroupsToRelease)
-      REQUIRES(Region->MMLock, Region->FLLock) {
+      REQUIRES(Region->MMLock) EXCLUDES(Region->FLLock) {
+    ScopedLock L(Region->FLLock);
+
     // After merging two freelists, we may have redundant `BatchGroup`s that
     // need to be recycled. The number of unused `BatchGroup`s is expected to be
     // small. Pick a constant which is inferred from real programs.
     constexpr uptr MaxUnusedSize = 8;
     CompactPtrT Blocks[MaxUnusedSize];
     u32 Idx = 0;
     RegionInfo *BatchClassRegion = getRegionInfo(SizeClassMap::BatchClassId);
     // We can't call pushBatchClassBlocks() to recycle the unused `BatchGroup`s