[scudo][fuchsia] Precommit pages when going beyond static buffer size

[yaneury]

Summary

This revision is superseded by https://reviews.llvm.org/D140320

[phosek]

When uploading patches to Phabricator via web interface, it's recommended that you include as much context as possible (by using e.g. git show HEAD -U999999 > mypatch.patch), see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for more details.

[cryptoad]

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

150

Uppercase U for consistency

compiler-rt/lib/scudo/standalone/fuchsia.cpp

117

Looks long, was that formatted with clang-format?

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

85

Probably | rather than &?

[Chia-hungDuan]

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

85

We support partial page release which only releases paritial memory groups but we haven't supported allocating page map only for subrange of region (I'm working on that). Which means, we may prefault more pages than necessary.

As discussed with Luke (Luke Nicholson), Fuchsia will still benefit from the prefault even with those unnecessary pages. I'm wondering if we can do this for Fuchsia only now. I think we will end up prefault the page map here so I'm inclined to do that for all platforms after we can allocate page map for subrange of region

I know it only does real prefault work on Fuchsia now but the code structure may give the impression that we will always prefault the pages.

What do you think?

[yaneury]

Fixed suggested errors

[yaneury]

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

85

Are you suggesting we pass the flag "MAP_PRECOMMIT" only when running on Fuchsia? If so, what's the pattern for doing that?

#ifdef SCUDO_FUCHSIA

...

#endif

[Chia-hungDuan]

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

85

Yes, I may suggest using SCUDO_FUCHSIA now.

BTW, can you also share some performance number before/after this change?

[yaneury]

This revision is superseded by https://reviews.llvm.org/D140320

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

 // as many bytes as requested, and avoid interruptions (on Linux).
 constexpr uptr MaxRandomLength = 256U;
 bool getRandom(void *Buffer, uptr Length, bool Blocking = false);
 
 // Platform memory mapping functions.
 
 #define MAP_ALLOWNOMEM (1U << 0)
 #define MAP_NOACCESS (1U << 1)
 #define MAP_RESIZABLE (1U << 2)
 #define MAP_MEMTAG (1U << 3)
+#define MAP_PRECOMMIT (1U << 4)
 
 // Our platform memory mapping use is restricted to 3 scenarios:
 // - reserve memory at a random address (MAP_NOACCESS);
 // - commit memory in a previously reserved space;
 // - commit memory at a random address.
 // As such, only a subset of parameters combinations is valid, which is checked
 // by the function implementation. The Data parameter allows to pass opaque
 // platform specific data to the function.

compiler-rt/lib/scudo/standalone/fuchsia.cpp

void *map(void *Addr, uptr Size, const char *Name, uptr Flags,
   } else {
     CHECK_EQ(_zx_handle_close(Vmo), ZX_OK);
   }
   if (UNLIKELY(Status != ZX_OK)) {
     if (Status != ZX_ERR_NO_MEMORY || !AllowNoMem)
       dieOnMapUnmapError(Status == ZX_ERR_NO_MEMORY ? Size : 0);
     return nullptr;
   }
+
+  if (Flags & MAP_PRECOMMIT) {
+    Status = _zx_vmo_op_range(Vmo, ZX_VMO_OP_COMMIT, VmoSize, Size,
+                              /*buffer=*/nullptr, /*buffer_size=*/0);
+  }
+  if (UNLIKELY(Status != ZX_OK)) {
+    if (Status != ZX_ERR_NO_MEMORY || !AllowNoMem)
+      dieOnMapUnmapError(Status == ZX_ERR_NO_MEMORY ? Size : 0);
+    return nullptr;
+  }
+
   if (Data)
     Data->VmoSize += Size;
 
   return reinterpret_cast<void *>(P);
 }
 
 void unmap(void *Addr, uptr Size, uptr Flags, MapPlatformData *Data) {
   if (Flags & UNMAP_ALL) {

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

public:
   }
 
   void reset(uptr NumberOfRegion, uptr CountersPerRegion, uptr MaxValue) {
     DCHECK_GT(NumberOfRegion, 0);
     DCHECK_GT(CountersPerRegion, 0);
     DCHECK_GT(MaxValue, 0);
 
     Regions = NumberOfRegion;
     NumCounters = CountersPerRegion;
 
     constexpr uptr MaxCounterBits = sizeof(*Buffer) * 8UL;
     // Rounding counter storage size up to the power of two allows for using
     // bit shifts calculating particular counter's Index and offset.
     const uptr CounterSizeBits =
         roundUpToPowerOfTwo(getMostSignificantSetBitIndex(MaxValue) + 1);
     DCHECK_LE(CounterSizeBits, MaxCounterBits);
     CounterSizeBitsLog = getLog2(CounterSizeBits);
void reset(uptr NumberOfRegion, uptr CountersPerRegion, uptr MaxValue) {
     BufferSize = SizePerRegion * sizeof(*Buffer) * Regions;
     if (BufferSize <= (StaticBufferCount * sizeof(Buffer[0])) &&
         Mutex.tryLock()) {
       Buffer = &StaticBuffer[0];
       memset(Buffer, 0, BufferSize);
     } else {
       Buffer = reinterpret_cast<uptr *>(
           map(nullptr, roundUpTo(BufferSize, getPageSizeCached()),
-              "scudo:counters", MAP_ALLOWNOMEM, &MapData));
+              "scudo:counters", MAP_ALLOWNOMEM | MAP_PRECOMMIT, &MapData));
     }
   }
 
   bool isAllocated() const { return !!Buffer; }
 
   uptr getCount() const { return NumCounters; }
 
   uptr get(uptr Region, uptr I) const {