[PA] Make direct-map "slot" behavior match single-slot spans

Prior to this CL, slot (or slot span) would be the raw size aligned up
to the page boundary, even though there may be plenty of space to the
right for the allocation to grow.

This CL reassigns meaning:
- slot_size -> committed_size
- available_reservation_size -> slot_size

This changes the behavior of AllocationCapacityFromPtr which now
uses the new meaning of slot_size. AllocationCapacityFromRequestedSize
was adjusted to match it. These are now consistent with single-slot span
behavior.

There is still a difference between direct-map and single-slot spans.
The unused part of the slot in the former case isn't committed, whereas
in the latter case it is committed (at least in the current
implementation), but may be discarded.

Change-Id: I970deccb5e821db672bbed016854416cdbd5b6c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2636008
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#891545}

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -104,7 +104,8 @@ const size_t kTestSizes[] = {
     100,
     base::SystemPageSize(),
     base::SystemPageSize() + 1,
-    base::PartitionRoot<base::internal::ThreadSafe>::GetDirectMapSlotSize(100),
+    base::kMaxBucketed,
+    base::kMaxBucketed + 1,
     1 << 20,
     1 << 21,
 };
@@ -1110,13 +1111,12 @@ TEST_F(PartitionAllocTest, Realloc) {
   size_t actual_capacity = allocator.root()->AllocationCapacityFromPtr(ptr);
   ptr2 = allocator.root()->Realloc(ptr, size - SystemPageSize(), type_name);
   EXPECT_EQ(ptr, ptr2);
-  EXPECT_EQ(actual_capacity - SystemPageSize(),
-            allocator.root()->AllocationCapacityFromPtr(ptr2));
+  // While pages may have been decommitted, the capacity shouldn't change.
+  EXPECT_EQ(actual_capacity, allocator.root()->AllocationCapacityFromPtr(ptr2));
   void* ptr3 =
       allocator.root()->Realloc(ptr2, size - 32 * SystemPageSize(), type_name);
   EXPECT_EQ(ptr2, ptr3);
-  EXPECT_EQ(actual_capacity - 32 * SystemPageSize(),
-            allocator.root()->AllocationCapacityFromPtr(ptr3));
+  EXPECT_EQ(actual_capacity, allocator.root()->AllocationCapacityFromPtr(ptr3));
 
   // Test that a previously in-place shrunk direct mapped allocation can be
   // expanded up again up to its original size.
@@ -1157,12 +1157,13 @@ TEST_F(PartitionAllocTest, ReallocDirectMapAligned) {
     void* ptr2 =
         allocator.root()->Realloc(ptr, size - SystemPageSize(), type_name);
     EXPECT_EQ(ptr, ptr2);
-    EXPECT_EQ(actual_capacity - SystemPageSize(),
+    // While pages may have been decommitted, the capacity shouldn't change.
+    EXPECT_EQ(actual_capacity,
               allocator.root()->AllocationCapacityFromPtr(ptr2));
     void* ptr3 = allocator.root()->Realloc(ptr2, size - 32 * SystemPageSize(),
                                            type_name);
     EXPECT_EQ(ptr2, ptr3);
-    EXPECT_EQ(actual_capacity - 32 * SystemPageSize(),
+    EXPECT_EQ(actual_capacity,
               allocator.root()->AllocationCapacityFromPtr(ptr3));
 
     // Test that a previously in-place shrunk direct mapped allocation can be
@@ -2037,10 +2038,16 @@ TEST_F(PartitionAllocTest, DumpMemoryStats) {
   {
     size_t size_smaller = kMaxBucketed + 1;
     size_t size_bigger = (kMaxBucketed * 2) + 1;
-    size_t real_size_smaller =
-        (size_smaller + SystemPageOffsetMask()) & SystemPageBaseMask();
-    size_t real_size_bigger =
-        (size_bigger + SystemPageOffsetMask()) & SystemPageBaseMask();
+    size_t raw_size_smaller =
+        allocator.root()->AdjustSizeForExtrasAdd(size_smaller);
+    size_t raw_size_bigger =
+        allocator.root()->AdjustSizeForExtrasAdd(size_bigger);
+    size_t bucket_size_smaller =
+        allocator.root()->GetDirectMapReservationSize(raw_size_smaller) -
+        allocator.root()->GetDirectMapMetadataAndGuardPagesSize();
+    size_t bucket_size_bigger =
+        allocator.root()->GetDirectMapReservationSize(raw_size_bigger) -
+        allocator.root()->GetDirectMapMetadataAndGuardPagesSize();
     void* ptr = allocator.root()->Alloc(size_smaller, type_name);
     void* ptr2 = allocator.root()->Alloc(size_bigger, type_name);
 
@@ -2051,27 +2058,27 @@ TEST_F(PartitionAllocTest, DumpMemoryStats) {
       EXPECT_TRUE(dumper.IsMemoryAllocationRecorded());
 
       const PartitionBucketMemoryStats* stats =
-          dumper.GetBucketStats(real_size_smaller);
+          dumper.GetBucketStats(bucket_size_smaller);
       EXPECT_TRUE(stats);
       EXPECT_TRUE(stats->is_valid);
       EXPECT_TRUE(stats->is_direct_map);
-      EXPECT_EQ(real_size_smaller, stats->bucket_slot_size);
-      EXPECT_EQ(real_size_smaller, stats->active_bytes);
-      EXPECT_EQ(real_size_smaller, stats->resident_bytes);
+      EXPECT_EQ(bucket_size_smaller, stats->bucket_slot_size);
+      EXPECT_EQ(bucket_size_smaller, stats->active_bytes);
+      EXPECT_EQ(bucket_size_smaller, stats->resident_bytes);
       EXPECT_EQ(0u, stats->decommittable_bytes);
       EXPECT_EQ(0u, stats->discardable_bytes);
       EXPECT_EQ(1u, stats->num_full_slot_spans);
       EXPECT_EQ(0u, stats->num_active_slot_spans);
       EXPECT_EQ(0u, stats->num_empty_slot_spans);
       EXPECT_EQ(0u, stats->num_decommitted_slot_spans);
 
-      stats = dumper.GetBucketStats(real_size_bigger);
+      stats = dumper.GetBucketStats(bucket_size_bigger);
       EXPECT_TRUE(stats);
       EXPECT_TRUE(stats->is_valid);
       EXPECT_TRUE(stats->is_direct_map);
-      EXPECT_EQ(real_size_bigger, stats->bucket_slot_size);
-      EXPECT_EQ(real_size_bigger, stats->active_bytes);
-      EXPECT_EQ(real_size_bigger, stats->resident_bytes);
+      EXPECT_EQ(bucket_size_bigger, stats->bucket_slot_size);
+      EXPECT_EQ(bucket_size_bigger, stats->active_bytes);
+      EXPECT_EQ(bucket_size_bigger, stats->resident_bytes);
       EXPECT_EQ(0u, stats->decommittable_bytes);
       EXPECT_EQ(0u, stats->discardable_bytes);
       EXPECT_EQ(1u, stats->num_full_slot_spans);

base/allocator/partition_allocator/partition_bucket.cc

@@ -207,8 +207,20 @@ SlotSpanMetadata<thread_safe>* PartitionDirectMap(
     // small ones (e.g. WTF::String), and does not have a thread cache.
     ScopedUnlockGuard<thread_safe> scoped_unlock{root->lock_};
 
-    const size_t slot_size =
-        PartitionRoot<thread_safe>::GetDirectMapSlotSize(raw_size);
+    // Layout inside a direct map reservation:
+    //  |guard&metadata|[padding]|slot==slot_span|guard|
+    //                           <------(a)------>
+    //                 <---(b)--->
+    //  <------(c)----->            +            <-(c)->
+    //  <----------------------(d)--------------------->
+    //   (a) slot_size (slot and slot span are equivalent for direct map)
+    //   (b) padding_for_alignment
+    //   (c) GetDirectMapMetadataAndGuardPagesSize()
+    //   (d) reservation_size
+    //
+    // See also the slot layout diagram in AllocFlagsNoHooks() under RawAlloc()
+    // call. This will explain |committed_size| and |raw_size|.
+    size_t committed_size = bits::AlignUp(raw_size, SystemPageSize());
     // The super page starts with a partition page worth of metadata and guard
     // pages, hence alignment requests ==PartitionPageSize() will be
     // automatically satisfied. Padding is needed for higher-order alignment
@@ -217,13 +229,11 @@ SlotSpanMetadata<thread_safe>* PartitionDirectMap(
         slot_span_alignment - PartitionPageSize();
     const size_t reservation_size =
         PartitionRoot<thread_safe>::GetDirectMapReservationSize(
-            raw_size + padding_for_alignment);
-#if DCHECK_IS_ON()
-    const size_t available_reservation_size =
+            raw_size, padding_for_alignment);
+    const size_t slot_size =
         reservation_size - padding_for_alignment -
         PartitionRoot<thread_safe>::GetDirectMapMetadataAndGuardPagesSize();
-    PA_DCHECK(slot_size <= available_reservation_size);
-#endif
+    PA_DCHECK(committed_size <= slot_size);
 
     // Allocate from GigaCage. Route to the appropriate GigaCage pool based on
     // BackupRefPtr support.
@@ -270,8 +280,8 @@ SlotSpanMetadata<thread_safe>* PartitionDirectMap(
     // Note that we didn't check above, because if we cannot even commit a
     // single page, then this is likely hopeless anyway, and we will crash very
     // soon.
-    const bool ok = root->TryRecommitSystemPagesForData(slot_start, slot_size,
-                                                        PageUpdatePermissions);
+    const bool ok = root->TryRecommitSystemPagesForData(
+        slot_start, committed_size, PageUpdatePermissions);
     if (!ok) {
       if (!return_null) {
         PartitionOutOfMemoryCommitFailure(root, slot_size);

base/allocator/partition_allocator/partition_direct_map_extent.h

@@ -18,11 +18,11 @@ struct PartitionDirectMapExtent {
   PartitionDirectMapExtent<thread_safe>* prev_extent;
   PartitionBucket<thread_safe>* bucket;
   // Size of the entire reservation, including guard pages, meta-data,
-  // padding for alignment before allocation, and padding for granularity at the
-  // end of the allocation.
+  // padding for alignment before the slot, and padding for granularity at the
+  // end of the slot.
   size_t reservation_size;
   // Padding between the first partition page (guard pages + meta-data) and
-  // the allocation.
+  // the slot.
   size_t padding_for_alignment;
 
   ALWAYS_INLINE static PartitionDirectMapExtent<thread_safe>* FromSlotSpan(

base/allocator/partition_allocator/partition_page.cc

@@ -43,10 +43,14 @@ PartitionDirectUnmap(SlotSpanMetadata<thread_safe>* slot_span) {
     extent->next_extent->prev_extent = extent->prev_extent;
   }
 
+  size_t raw_size = slot_span->GetRawSize();
+  size_t committed_size = bits::AlignUp(raw_size, SystemPageSize());
+  size_t reservation_size = extent->reservation_size;
+  PA_DCHECK(committed_size <= slot_span->bucket->slot_size);
+
   // The actual decommit is deferred, when releasing the reserved memory region.
-  root->DecreaseCommittedPages(slot_span->bucket->slot_size);
+  root->DecreaseCommittedPages(committed_size);
 
-  size_t reservation_size = extent->reservation_size;
   PA_DCHECK(!(reservation_size & DirectMapAllocationGranularityOffsetMask()));
   PA_DCHECK(root->total_size_of_direct_mapped_pages >= reservation_size);
   root->total_size_of_direct_mapped_pages -= reservation_size;

base/allocator/partition_allocator/partition_root.cc

@@ -7,6 +7,7 @@
 #include "base/allocator/partition_allocator/address_pool_manager_bitmap.h"
 #include "base/allocator/partition_allocator/oom.h"
 #include "base/allocator/partition_allocator/page_allocator.h"
+#include "base/allocator/partition_allocator/page_allocator_constants.h"
 #include "base/allocator/partition_allocator/partition_address_space.h"
 #include "base/allocator/partition_allocator/partition_alloc_check.h"
 #include "base/allocator/partition_allocator/partition_alloc_config.h"
@@ -677,65 +678,60 @@ bool PartitionRoot<thread_safe>::TryReallocInPlaceForDirectMap(
 
   // Don't reallocate in-place if new reservation size would be less than 80 %
   // of the current one, to avoid holding on to too much unused address space.
-  // Make this check before comparing slot sizes, as even with equal or similar
-  // slot sizes we can save a lot if the original allocation was heavily padded
-  // for alignment.
+  // Make this check before comparing committed sizes, as even with equal or
+  // similar committed sizes we can save a lot if the original allocation was
+  // heavily padded for alignment.
   if ((new_reservation_size / SystemPageSize()) * 5 <
       (current_reservation_size / SystemPageSize()) * 4)
     return false;
 
-  // Note that the new size isn't a bucketed size; this function is called
-  // whenever we're reallocating a direct mapped allocation, so calculate it
-  // the way PartitionDirectMap() would.
-  size_t new_slot_size = GetDirectMapSlotSize(raw_size);
-  if (new_slot_size < kMinDirectMappedDownsize)
+  // Calculate committed size the way PartitionDirectMap() would.
+  size_t new_committed_size = bits::AlignUp(raw_size, SystemPageSize());
+  if (new_committed_size < kMinDirectMappedDownsize)
     return false;
 
   // Past this point, we decided we'll attempt to reallocate without relocating,
   // so we have to honor the padding for alignment in front of the original
   // allocation, even though this function isn't requesting any alignment.
 
-  // bucket->slot_size is the currently committed size of the allocation.
-  size_t current_slot_size = slot_span->bucket->slot_size;
+  size_t current_committed_size =
+      bits::AlignUp(slot_span->GetRawSize(), SystemPageSize());
+  // Slot and slot span are equivalent for direct map.
   char* slot_start =
       static_cast<char*>(SlotSpan::ToSlotSpanStartPtr(slot_span));
-  // This is the available part of the reservation up to which the new
-  // allocation can grow.
-  size_t available_reservation_size =
-      current_reservation_size - extent->padding_for_alignment -
-      PartitionRoot<thread_safe>::GetDirectMapMetadataAndGuardPagesSize();
 #if DCHECK_IS_ON()
   char* reservation_start = reinterpret_cast<char*>(
       reinterpret_cast<uintptr_t>(slot_start) & kSuperPageBaseMask);
   PA_DCHECK(internal::IsReservationStart(reservation_start));
-  PA_DCHECK(slot_start + available_reservation_size ==
+  PA_DCHECK(slot_start + slot_span->bucket->slot_size ==
             reservation_start + current_reservation_size -
                 GetDirectMapMetadataAndGuardPagesSize() + PartitionPageSize());
 #endif
 
-  if (new_slot_size == current_slot_size) {
+  if (new_committed_size == current_committed_size) {
     // No need to move any memory around, but update size and cookie below.
     // That's because raw_size may have changed.
-  } else if (new_slot_size < current_slot_size) {
+  } else if (new_committed_size < current_committed_size) {
     // Shrink by decommitting unneeded pages and making them inaccessible.
-    size_t decommit_size = current_slot_size - new_slot_size;
-    DecommitSystemPagesForData(slot_start + new_slot_size, decommit_size,
+    size_t decommit_size = current_committed_size - new_committed_size;
+    DecommitSystemPagesForData(slot_start + new_committed_size, decommit_size,
                                PageUpdatePermissions);
     // Since the decommited system pages are still reserved, we don't need to
     // change the entries for decommitted pages in the reservation offset table.
-  } else if (new_slot_size <= available_reservation_size) {
+  } else if (new_committed_size <= slot_span->bucket->slot_size) {
     // Grow within the actually reserved address space. Just need to make the
     // pages accessible again.
-    size_t recommit_slot_size_growth = new_slot_size - current_slot_size;
-    RecommitSystemPagesForData(slot_start + current_slot_size,
+    size_t recommit_slot_size_growth =
+        new_committed_size - current_committed_size;
+    RecommitSystemPagesForData(slot_start + current_committed_size,
                                recommit_slot_size_growth,
                                PageUpdatePermissions);
     // The recommited system pages had been already reserved and all the
     // entries in the reservation offset table (for entire reservation_size
     // region) have been already initialized.
 
 #if DCHECK_IS_ON()
-    memset(slot_start + current_slot_size, kUninitializedByte,
+    memset(slot_start + current_committed_size, kUninitializedByte,
            recommit_slot_size_growth);
 #endif
   } else {
@@ -745,7 +741,6 @@ bool PartitionRoot<thread_safe>::TryReallocInPlaceForDirectMap(
   }
 
   slot_span->SetRawSize(raw_size);
-  slot_span->bucket->slot_size = new_slot_size;
 
 #if DCHECK_IS_ON()
   // Write a new trailing cookie.

base/allocator/partition_allocator/partition_root.h

@@ -413,17 +413,12 @@ struct BASE_EXPORT PartitionRoot {
 #endif
   }
 
-  static PAGE_ALLOCATOR_CONSTANTS_DECLARE_CONSTEXPR ALWAYS_INLINE size_t
-  GetDirectMapSlotSize(size_t raw_size) {
-    // Caller must check that the size is not above the MaxDirectMapped()
-    // limit before calling. This also guards against integer overflow in the
-    // calculation here.
-    PA_DCHECK(raw_size <= MaxDirectMapped());
-    return bits::AlignUp(raw_size, SystemPageSize());
-  }
-
+  // |padding_for_alignment| is between the first partition page (guard pages +
+  // meta-data) and the allocation (where |raw_size| starts).
   static ALWAYS_INLINE size_t
-  GetDirectMapReservationSize(size_t padded_raw_size) {
+  GetDirectMapReservationSize(size_t raw_size,
+                              size_t padding_for_alignment = 0) {
+    size_t padded_raw_size = raw_size + padding_for_alignment;
     // Caller must check that the size is not above the MaxDirectMapped()
     // limit before calling. This also guards against integer overflow in the
     // calculation here.
@@ -1105,7 +1100,7 @@ ALWAYS_INLINE void PartitionRoot<thread_safe>::FreeNoHooksImmediate(
 
   // |ptr| points after the ref-count and the cookie.
   //
-  // Layout inside the slot:
+  // Layout inside a slot:
   //  <------extras----->                  <-extras->
   //  <--------------utilized_slot_size------------->
   //                    <----usable_size--->
@@ -1115,7 +1110,7 @@ ALWAYS_INLINE void PartitionRoot<thread_safe>::FreeNoHooksImmediate(
   //
   // Note: ref-count and cookies can be 0-sized.
   //
-  // For more context, see the other "Layout inside the slot" comment below.
+  // For more context, see the other Layout comment in AllocFlagsNoHooks().
 #if EXPENSIVE_DCHECKS_ARE_ON() || defined(PA_ZERO_RANDOMLY_ON_FREE)
   const size_t utilized_slot_size = slot_span->GetUtilizedSlotSize();
 #endif
@@ -1481,20 +1476,22 @@ ALWAYS_INLINE void* PartitionRoot<thread_safe>::AllocFlagsNoHooks(
   if (UNLIKELY(!slot_start))
     return nullptr;
 
-  // Layout inside the slot:
-  //  |[refcnt]|[cookie]|...data...|[empty]|[cookie]|[unused]|
+  // Layout inside a slot:
+  //  |[refcnt]|[cookie]|...data...|[empty]|[cookie]|[unused]|[uncommitted]|
   //                    <---(a)---->
   //                    <-------(b)-------->
   //  <-------(c)------->                  <--(c)--->
   //  <-------------(d)------------>   +   <--(d)--->
   //  <---------------------(e)--------------------->
   //  <-------------------------(f)-------------------------->
+  //  <--------------------------------(g)--------------------------------->
   //   (a) requested_size
   //   (b) usable_size
   //   (c) extras
   //   (d) raw_size
   //   (e) utilized_slot_size
-  //   (f) slot_size
+  //   (f) slot's committed region (==slot_size for normal buckets)
+  //   (g) slot_size
   //
   // - Ref-count may or may not exist in the slot, depending on CheckedPtr
   //   implementation.
@@ -1511,6 +1508,9 @@ ALWAYS_INLINE void* PartitionRoot<thread_safe>::AllocFlagsNoHooks(
   //   to save raw_size, i.e. only for large allocations. For small allocations,
   //   we have no other choice than putting the cookie at the very end of the
   //   slot, thus creating the "empty" space.
+  // - "uncommitted" space occurs only in the direct map case, if more space
+  //   than needed is reserved due to allocation granularity, or when downsized
+  //   by realloc.
   //
   // If BUILDFLAG(PUT_REF_COUNT_IN_PREVIOUS_SLOT) is true, Layout inside the
   // slot of small buckets:
@@ -1699,8 +1699,9 @@ ALWAYS_INLINE void* PartitionRoot<thread_safe>::TryRealloc(
 // Return the capacity of the underlying slot (adjusted for extras) that'd be
 // used to satisfy a request of |size|. This doesn't mean this capacity would be
 // readily available. It merely means that if an allocation happened with that
-// returned value, it'd use the same amount of underlying memory as the
-// allocation with |size|.
+// returned value, it'd use the same amount of underlying address space as the
+// allocation with |size|. This isn't necessarily true for committed memory,
+// because single-slot spans and direct map may commit less than reserved pages.
 template <bool thread_safe>
 ALWAYS_INLINE size_t
 PartitionRoot<thread_safe>::AllocationCapacityFromRequestedSize(
@@ -1719,7 +1720,9 @@ PartitionRoot<thread_safe>::AllocationCapacityFromRequestedSize(
   } else if (size > MaxDirectMapped()) {
     // Too large to allocate => return the size unchanged.
   } else {
-    size = GetDirectMapSlotSize(size);
+    PA_DCHECK(size > kMaxBucketed);
+    size = GetDirectMapReservationSize(size) -
+           GetDirectMapMetadataAndGuardPagesSize();
   }
   size = AdjustSizeForExtrasSubtract(size);
   return size;