[PA] Allow straightening of free lists even if no unprovisioning

PartitionPurgeSlotSpan would skip straightening free lists, if there is
nothing to unprovision. It still can be useful, and I suspect it was
skipped only because it happened to be under the same |if| condition,
out of convenience.

Worth evaluating it, under a feature param attached to
PartitionAllocStraightenLargerSlotSpanFreeLists. Keep the existing
behavior the default (kOnlyWhenUnprovisioning).

Bug: 1471015
Change-Id: Ic38b67208719d7e5a995cc894b1d7b2e0a960772
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4809836
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kalvin Lee <kdlee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188180}

base/allocator/partition_alloc_features.cc

@@ -5,6 +5,7 @@
 #include "base/allocator/partition_alloc_features.h"
 
 #include "base/allocator/partition_allocator/partition_alloc_buildflags.h"
+#include "base/allocator/partition_allocator/partition_root.h"
 #include "base/base_export.h"
 #include "base/feature_list.h"
 #include "base/features.h"
@@ -280,6 +281,23 @@ BASE_FEATURE(kPartitionAllocDCScan,
 BASE_FEATURE(kPartitionAllocStraightenLargerSlotSpanFreeLists,
              "PartitionAllocStraightenLargerSlotSpanFreeLists",
              FEATURE_ENABLED_BY_DEFAULT);
+const base::FeatureParam<
+    partition_alloc::StraightenLargerSlotSpanFreeListsMode>::Option
+    kPartitionAllocStraightenLargerSlotSpanFreeListsModeOption[] = {
+        {partition_alloc::StraightenLargerSlotSpanFreeListsMode::
+             kOnlyWhenUnprovisioning,
+         "only-when-unprovisioning"},
+        {partition_alloc::StraightenLargerSlotSpanFreeListsMode::kAlways,
+         "always"},
+};
+const base::FeatureParam<partition_alloc::StraightenLargerSlotSpanFreeListsMode>
+    kPartitionAllocStraightenLargerSlotSpanFreeListsMode = {
+        &kPartitionAllocStraightenLargerSlotSpanFreeLists,
+        "mode",
+        partition_alloc::StraightenLargerSlotSpanFreeListsMode::
+            kOnlyWhenUnprovisioning,
+        &kPartitionAllocStraightenLargerSlotSpanFreeListsModeOption,
+};
 
 // Whether to sort free lists for smaller slot spans in PurgeMemory().
 BASE_FEATURE(kPartitionAllocSortSmallerSlotSpanFreeLists,

base/allocator/partition_alloc_features.h

@@ -6,6 +6,7 @@
 #define BASE_ALLOCATOR_PARTITION_ALLOC_FEATURES_H_
 
 #include "base/allocator/partition_allocator/partition_alloc_buildflags.h"
+#include "base/allocator/partition_allocator/partition_root.h"
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
 #include "base/feature_list.h"
@@ -164,15 +165,18 @@ BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocPCScanStackScanning);
 BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocDCScan);
 BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocPCScanImmediateFreeing);
 BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocPCScanEagerClearing);
-BASE_EXPORT BASE_DECLARE_FEATURE(
-    kPartitionAllocStraightenLargerSlotSpanFreeLists);
-BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocSortSmallerSlotSpanFreeLists);
-BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocSortActiveSlotSpans);
 BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocUseDenserDistribution);
 
 BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocMemoryReclaimer);
 extern const BASE_EXPORT base::FeatureParam<TimeDelta>
     kPartitionAllocMemoryReclaimerInterval;
+BASE_EXPORT BASE_DECLARE_FEATURE(
+    kPartitionAllocStraightenLargerSlotSpanFreeLists);
+extern const BASE_EXPORT
+    base::FeatureParam<partition_alloc::StraightenLargerSlotSpanFreeListsMode>
+        kPartitionAllocStraightenLargerSlotSpanFreeListsMode;
+BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocSortSmallerSlotSpanFreeLists);
+BASE_EXPORT BASE_DECLARE_FEATURE(kPartitionAllocSortActiveSlotSpans);
 
 #if BUILDFLAG(IS_WIN)
 BASE_EXPORT BASE_DECLARE_FEATURE(kPageAllocatorRetryOnCommitFailure);

base/allocator/partition_alloc_support.cc

@@ -21,6 +21,7 @@
 #include "base/allocator/partition_allocator/partition_alloc_check.h"
 #include "base/allocator/partition_allocator/partition_alloc_config.h"
 #include "base/allocator/partition_allocator/partition_lock.h"
+#include "base/allocator/partition_allocator/partition_root.h"
 #include "base/allocator/partition_allocator/pointers/raw_ptr.h"
 #include "base/allocator/partition_allocator/shim/allocator_shim.h"
 #include "base/allocator/partition_allocator/shim/allocator_shim_default_dispatch_to_partition_alloc.h"
@@ -1329,9 +1330,11 @@ void PartitionAllocSupport::ReconfigureAfterTaskRunnerInit(
       base::SingleThreadTaskRunner::GetCurrentDefault());
 #endif
 
-  partition_alloc::PartitionRoot::SetStraightenLargerSlotSpanFreeListsEnabled(
+  partition_alloc::PartitionRoot::SetStraightenLargerSlotSpanFreeListsMode(
       base::FeatureList::IsEnabled(
-          base::features::kPartitionAllocStraightenLargerSlotSpanFreeLists));
+          base::features::kPartitionAllocStraightenLargerSlotSpanFreeLists)
+          ? features::kPartitionAllocStraightenLargerSlotSpanFreeListsMode.Get()
+          : partition_alloc::StraightenLargerSlotSpanFreeListsMode::kNever);
   partition_alloc::PartitionRoot::SetSortSmallerSlotSpanFreeListsEnabled(
       base::FeatureList::IsEnabled(
           base::features::kPartitionAllocSortSmallerSlotSpanFreeLists));

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -430,7 +430,8 @@ class PartitionAllocTest
   }
 
   void SetUp() override {
-    PartitionRoot::SetStraightenLargerSlotSpanFreeListsEnabled(true);
+    PartitionRoot::SetStraightenLargerSlotSpanFreeListsMode(
+        StraightenLargerSlotSpanFreeListsMode::kOnlyWhenUnprovisioning);
     PartitionRoot::SetSortSmallerSlotSpanFreeListsEnabled(true);
     PartitionRoot::SetSortActiveSlotSpansEnabled(true);
     PartitionAllocGlobalInit(HandleOOM);
@@ -3303,7 +3304,7 @@ TEST_P(PartitionAllocTest, PurgeDiscardableManyPages) {
   }
 }
 
-TEST_P(PartitionAllocTest, PurgeDiscardableWithFreeListRewrite) {
+TEST_P(PartitionAllocTest, PurgeDiscardableWithFreeListStraightening) {
   // This sub-test tests truncation of the provisioned slots in a trickier
   // case where the freelist is rewritten.
   allocator.root()->PurgeMemory(PurgeFlags::kDecommitEmptySlotSpans);
@@ -3363,11 +3364,84 @@ TEST_P(PartitionAllocTest, PurgeDiscardableWithFreeListRewrite) {
   void* ptr2b = allocator.root()->Alloc(
       SystemPageSize() - ExtraAllocSize(allocator), type_name);
   PA_EXPECT_PTR_EQ(ptr2, ptr2b);
+  EXPECT_FALSE(slot_span->get_freelist_head());  // ptr4 was unprovisioned
+  void* ptr4b = allocator.root()->Alloc(
+      SystemPageSize() - ExtraAllocSize(allocator), type_name);
+  PA_EXPECT_PTR_EQ(ptr4, ptr4b);
+  EXPECT_FALSE(slot_span->get_freelist_head());
+
+  // Free objects such that they're in this order on the list:
+  //   head -> ptr2 -> ptr3 -> ptr1
+  // However, ptr4 is still unfreed preventing any unprovisioning.
+  allocator.root()->Free(ptr1);
+  allocator.root()->Free(ptr3);
+  allocator.root()->Free(ptr2);
+  allocator.root()->PurgeMemory(PurgeFlags::kDiscardUnusedSystemPages);
+  // The test by default runs in
+  // StraightenLargerSlotSpanFreeListsMode::kOnlyWhenUnprovisioning mode, so the
+  // freelist wasn't modified, and the allocations will happen in LIFO order.
+  ptr2b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr2, ptr2b);
+  void* ptr3b = allocator.root()->Alloc(
+      SystemPageSize() - ExtraAllocSize(allocator), type_name);
+  PA_EXPECT_PTR_EQ(ptr3, ptr3b);
+  ptr1b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr1, ptr1b);
+  EXPECT_FALSE(slot_span->get_freelist_head());
+
+  // Free objects such that they're in this order on the list:
+  //   head -> ptr2 -> ptr3 -> ptr1
+  // However, ptr4 is still unfreed preventing any unprovisioning.
+  allocator.root()->Free(ptr1);
+  allocator.root()->Free(ptr3);
+  allocator.root()->Free(ptr2);
+  PartitionRoot::SetStraightenLargerSlotSpanFreeListsMode(
+      StraightenLargerSlotSpanFreeListsMode::kAlways);
+  allocator.root()->PurgeMemory(PurgeFlags::kDiscardUnusedSystemPages);
+  // In StraightenLargerSlotSpanFreeListsMode::kAlways mode, the freelist is
+  // ordered from left to right.
+  ptr1b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr1, ptr1b);
+  ptr2b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr2, ptr2b);
+  ptr3b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr3, ptr3b);
+  EXPECT_FALSE(slot_span->get_freelist_head());
+
+  // Free objects such that they're in this order on the list:
+  //   head -> ptr2 -> ptr4 -> ptr1
+  // ptr3 is still unfreed preventing unprovisioning of ptr1 and ptr2, but not
+  // ptr4.
+  allocator.root()->Free(ptr1);
+  allocator.root()->Free(ptr4);
+  allocator.root()->Free(ptr2);
+  PartitionRoot::SetStraightenLargerSlotSpanFreeListsMode(
+      StraightenLargerSlotSpanFreeListsMode::kNever);
+  allocator.root()->PurgeMemory(PurgeFlags::kDiscardUnusedSystemPages);
+  // In StraightenLargerSlotSpanFreeListsMode::kNever mode, unprovisioned
+  // entries willbe removed form the freelist but the list won't be reordered.
+  ptr2b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr2, ptr2b);
+  ptr1b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr1, ptr1b);
+  EXPECT_FALSE(slot_span->get_freelist_head());
+  ptr4b = allocator.root()->Alloc(SystemPageSize() - ExtraAllocSize(allocator),
+                                  type_name);
+  PA_EXPECT_PTR_EQ(ptr4, ptr4b);
   EXPECT_FALSE(slot_span->get_freelist_head());
 
+  // Clean up.
   allocator.root()->Free(ptr1);
   allocator.root()->Free(ptr2);
   allocator.root()->Free(ptr3);
+  allocator.root()->Free(ptr4);
 }
 
 TEST_P(PartitionAllocTest, PurgeDiscardableDoubleTruncateFreeList) {

base/allocator/partition_allocator/partition_root.cc

@@ -434,12 +434,10 @@ static size_t PartitionPurgeSlotSpan(PartitionRoot* root,
   }
   // First, do the work of calculating the discardable bytes. Don't actually
   // discard anything if `accounting_only` is set.
+  size_t unprovisioned_bytes = 0;
+  uintptr_t begin_addr = slot_span_start + (num_provisioned_slots * slot_size);
+  uintptr_t end_addr = begin_addr + (slot_size * truncated_slots);
   if (truncated_slots) {
-    size_t unprovisioned_bytes = 0;
-    uintptr_t begin_addr =
-        slot_span_start + (num_provisioned_slots * slot_size);
-    uintptr_t end_addr = begin_addr + (slot_size * truncated_slots);
-
     // The slots that do not contain discarded pages should not be included to
     // |truncated_slots|. Detects those slots and fixes |truncated_slots| and
     // |num_provisioned_slots| accordingly.
@@ -461,77 +459,90 @@ static size_t PartitionPurgeSlotSpan(PartitionRoot* root,
       unprovisioned_bytes = end_addr - begin_addr;
       discardable_bytes += unprovisioned_bytes;
     }
+  }
 
-    // If there are any unprovisioned slots, and `accounting_only` isn't set,
-    // then take action to remove these slots from the free list. "straighten"
-    // it, while at it (if requested), to help reduce fragmentation in the
-    // future.
-    if (unprovisioned_bytes && !accounting_only) {
-      PA_DCHECK(truncated_slots > 0);
-      size_t new_unprovisioned_slots =
-          truncated_slots + slot_span->num_unprovisioned_slots;
-      PA_DCHECK(new_unprovisioned_slots <= bucket->get_slots_per_span());
-      slot_span->num_unprovisioned_slots = new_unprovisioned_slots;
-
-      size_t num_new_freelist_entries = 0;
-      internal::EncodedNextFreelistEntry* back = nullptr;
-      if (PartitionRoot::IsStraightenLargerSlotSpanFreeListsEnabled()) {
-        // Rewrite the freelist to "straighten" it. The entries will be ordered
-        // based on how close they're to the slot span start. This reduce
-        // chances of allocating further slots, in hope that we'll get some
-        // unused pages at the end of the span that can be unprovisioned.
-        for (size_t slot_index = 0; slot_index < num_provisioned_slots;
-             ++slot_index) {
-          if (slot_usage[slot_index]) {
-            continue;
-          }
-          // Add the slot to the end of the list. The most proper thing to do
-          // would be to null-terminate the new entry with:
-          //   auto* entry = EncodedNextFreelistEntry::EmplaceAndInitNull(
-          //       slot_span_start + (slot_size * slot_index));
-          // But no need to do this, as it's last-ness is likely temporary, and
-          // the next iteration's back->SetNext(), or the post-loop
-          // EncodedNextFreelistEntry::EmplaceAndInitNull(back) will override it
-          // anyway.
-          auto* entry = static_cast<EncodedNextFreelistEntry*>(
-              SlotStartAddr2Ptr(slot_span_start + (slot_size * slot_index)));
+  // If `accounting_only` isn't set, then take action to remove unprovisioned
+  // slots from the free list (if any) and "straighten" the list (if
+  // requested) to help reduce fragmentation in the future. Then
+  // discard/decommit the pages hosting the unprovisioned slots.
+  if (!accounting_only) {
+    auto straighten_mode =
+        PartitionRoot::GetStraightenLargerSlotSpanFreeListsMode();
+    bool straighten =
+        straighten_mode == StraightenLargerSlotSpanFreeListsMode::kAlways ||
+        (straighten_mode ==
+             StraightenLargerSlotSpanFreeListsMode::kOnlyWhenUnprovisioning &&
+         unprovisioned_bytes);
+
+    PA_DCHECK((unprovisioned_bytes > 0) == (truncated_slots > 0));
+    size_t new_unprovisioned_slots =
+        truncated_slots + slot_span->num_unprovisioned_slots;
+    PA_DCHECK(new_unprovisioned_slots <= bucket->get_slots_per_span());
+    slot_span->num_unprovisioned_slots = new_unprovisioned_slots;
+
+    size_t num_new_freelist_entries = 0;
+    internal::EncodedNextFreelistEntry* back = nullptr;
+    if (straighten) {
+      // Rewrite the freelist to "straighten" it. This achieves two things:
+      // getting rid of unprovisioned entries, ordering etnries based on how
+      // close they're to the slot span start. This reduces chances of
+      // allocating further slots, in hope that we'll get some unused pages at
+      // the end of the span that can be unprovisioned, thus reducing
+      // fragmentation.
+      for (size_t slot_index = 0; slot_index < num_provisioned_slots;
+           ++slot_index) {
+        if (slot_usage[slot_index]) {
+          continue;
+        }
+        // Add the slot to the end of the list. The most proper thing to do
+        // would be to null-terminate the new entry with:
+        //   auto* entry = EncodedNextFreelistEntry::EmplaceAndInitNull(
+        //       slot_span_start + (slot_size * slot_index));
+        // But no need to do this, as it's last-ness is likely temporary, and
+        // the next iteration's back->SetNext(), or the post-loop
+        // EncodedNextFreelistEntry::EmplaceAndInitNull(back) will override it
+        // anyway.
+        auto* entry = static_cast<EncodedNextFreelistEntry*>(
+            SlotStartAddr2Ptr(slot_span_start + (slot_size * slot_index)));
+        if (num_new_freelist_entries) {
+          back->SetNext(entry);
+        } else {
+          slot_span->SetFreelistHead(entry);
+        }
+        back = entry;
+        num_new_freelist_entries++;
+      }
+    } else if (unprovisioned_bytes) {
+      // If there are any unprovisioned entries, scan the list to remove them,
+      // without "straightening" it.
+      uintptr_t first_unprovisioned_slot =
+          slot_span_start + (num_provisioned_slots * slot_size);
+      bool skipped = false;
+      for (EncodedNextFreelistEntry* entry = slot_span->get_freelist_head();
+           entry; entry = entry->GetNext(slot_size)) {
+        uintptr_t entry_addr = SlotStartPtr2Addr(entry);
+        if (entry_addr >= first_unprovisioned_slot) {
+          skipped = true;
+          continue;
+        }
+        // If the last visited entry was skipped (due to being unprovisioned),
+        // update the next pointer of the last not skipped entry (or the head
+        // if no entry exists). Otherwise the link is already correct.
+        if (skipped) {
           if (num_new_freelist_entries) {
             back->SetNext(entry);
           } else {
             slot_span->SetFreelistHead(entry);
           }
-          back = entry;
-          num_new_freelist_entries++;
-        }
-      } else {
-        // Scan the list to remove unprovisioned entries, without
-        // "straightening" it.
-        uintptr_t first_unprovisioned_slot =
-            slot_span_start + (num_provisioned_slots * slot_size);
-        bool skipped = false;
-        for (EncodedNextFreelistEntry* entry = slot_span->get_freelist_head();
-             entry; entry = entry->GetNext(slot_size)) {
-          uintptr_t entry_addr = SlotStartPtr2Addr(entry);
-          if (entry_addr >= first_unprovisioned_slot) {
-            skipped = true;
-            continue;
-          }
-          // If the last visited entry was skipped (due to being unprovisioned),
-          // updated the next pointer of the last not skipped entry (or the head
-          // if no entry exists). Otherwise the link is already correct.
-          if (skipped) {
-            if (num_new_freelist_entries) {
-              back->SetNext(entry);
-            } else {
-              slot_span->SetFreelistHead(entry);
-            }
-            skipped = false;
-          }
-          back = entry;
-          num_new_freelist_entries++;
+          skipped = false;
         }
+        back = entry;
+        num_new_freelist_entries++;
       }
-      // Now null-terminate the last entry, or the head if no entry exists.
+    }
+    // If any of the above loops were executed, null-terminate the last entry,
+    // or the head if no entry exists.
+    if (straighten || unprovisioned_bytes) {
       if (num_new_freelist_entries) {
         PA_DCHECK(back);
         EncodedNextFreelistEntry::EmplaceAndInitNull(back);
@@ -548,12 +559,14 @@ static size_t PartitionPurgeSlotSpan(PartitionRoot* root,
       }
       PA_DCHECK(num_new_freelist_entries ==
                 num_provisioned_slots - slot_span->num_allocated_slots);
+    }
 
 #if BUILDFLAG(USE_FREESLOT_BITMAP)
-      FreeSlotBitmapReset(slot_span_start + (slot_size * num_provisioned_slots),
-                          end_addr, slot_size);
+    FreeSlotBitmapReset(slot_span_start + (slot_size * num_provisioned_slots),
+                        end_addr, slot_size);
 #endif
 
+    if (unprovisioned_bytes) {
       if (!kUseLazyCommit) {
         // Discard the memory.
         ScopedSyscallTimer timer{root};
@@ -591,8 +604,8 @@ static size_t PartitionPurgeSlotSpan(PartitionRoot* root,
     // The first address we can safely discard is just after the freelist
     // pointer. There's one quirk: if the freelist pointer is actually nullptr,
     // we can discard that pointer value too (except on Windows).
-    uintptr_t begin_addr = slot_span_start + (i * slot_size);
-    uintptr_t end_addr = begin_addr + slot_size;
+    begin_addr = slot_span_start + (i * slot_size);
+    end_addr = begin_addr + slot_size;
 
     bool can_discard_free_list_pointer = false;
 #if !BUILDFLAG(IS_WIN)
@@ -1749,8 +1762,8 @@ ThreadCache* PartitionRoot::MaybeInitThreadCache() {
 }
 
 // static
-void PartitionRoot::SetStraightenLargerSlotSpanFreeListsEnabled(
-    bool new_value) {
+void PartitionRoot::SetStraightenLargerSlotSpanFreeListsMode(
+    StraightenLargerSlotSpanFreeListsMode new_value) {
   straighten_larger_slot_span_free_lists_ = new_value;
 }