[PATCH] D93731: [wip] scudo: Support memory tagging in the secondary allocator.

Peter Collingbourne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 22 16:34:55 PST 2020


pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:259
+    auto PtrInt = reinterpret_cast<uptr>(Ptr);
+    return reinterpret_cast<void *>(addFixedTag(PtrInt, 2));
   }
----------------
eugenis wrote:
> Do you think it will be too expensive to make the tag random and use LDG to read it in the allocator?
I didn't see a huge advantage in making it random. I think we need to be careful about loads which follow the pattern of LDG followed by a load as that effectively makes MTE ineffective for those loads. It seemed better to use a fixed tag since that provides at least a little more protection than none at all.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:1051
   // Return the size of a chunk as requested during its allocation.
   inline uptr getSize(const void *Ptr, Chunk::UnpackedHeader *Header) {
     const uptr SizeOrUnusedBytes = Header->SizeOrUnusedBytes;
----------------
eugenis wrote:
> Rename the argument to "headerTaggedPtr" or something like that
Will do.


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:129
+      Entry.Time = Time;
+      setMemoryPermission(Entry.CommitBase, Entry.CommitSize, MAP_NOACCESS,
+                          &Entry.Data);
----------------
eugenis wrote:
> Do you expect this to be faster than the other branch? If we are doing an mm syscall anyway, why not release the memory immediately?
The advantage of not releasing the memory is that we don't have to pay the cost of faulting the memory once it is made writable again. (Or at least that's the theory. It turns out that the Linux kernel currently faults the accesses anyway, and I have a [patch](https://lore.kernel.org/linux-mm/20201212053152.3783250-1-pcc@google.com/) to get rid of the faults.) With that patch in place I did continue to measure a significant performance improvement for decay=1 versus decay=0.


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:464
+    const uptr NewMapEnd =
+        CommitBase + PageSize + roundUpTo(Size, PageSize) + PageSize;
     DCHECK_LE(NewMapEnd, MapEnd);
----------------
eugenis wrote:
> is this an unrelated bugfix?
No, this is a consequence of the fact that we are now passing the original size into this function rather than the size padded for alignment.


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:474
+  map(reinterpret_cast<void *>(CommitBase), CommitSize, "scudo:secondary",
+      MAP_RESIZABLE | MAP_MEMTAG, &Data);
+  const uptr HeaderPos =
----------------
eugenis wrote:
> I don't know if it is worth the cost of the extra syscall, but we only need MAP_MEMTAG on the first page of the mapping.
> 
It's the first four pages I think, because the cache may end up reusing an allocation and throwing away the first four pages. I'm inclined to wait for data before trying to optimize this.

That being said, one simple thing that we can do here is to make the flag conditional on whether memory tagging is enabled, similar to what we're already doing in the primary allocator. I'll do that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93731/new/

https://reviews.llvm.org/D93731



More information about the llvm-commits mailing list