Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[compiler-rt][asan][Fuchsia] Tune the 64-bit asan allocator for riscv+fuchsia

Authored by leonardchan on May 22 2023, 2:51 PM.



This uses a custom size class map and primary allocator arena size that allows us to run all bringup tests on riscv64 with asan instrumentation reliabely.

Diff Detail

Event Timeline

leonardchan created this revision.May 22 2023, 2:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2023, 2:51 PM
leonardchan requested review of this revision.May 22 2023, 2:51 PM
mcgrathr added inline comments.May 24 2023, 5:14 PM

I don't know anything about the allocator. Is this making 128G the upper limit on how much the allocator can ever hand out? Or is it just setting 128G as a size of contiguous address space to use, and more than one such chunk might ever be used in the process?


I don't know what the rationales for choosing between {Very,}{Compact,Dense}SizeClassMap are. It would be nice if there were more comments about the meanings of the tuning parameters and the rationale for the choices.

Aside from the opaqueness of the code, the concern I'm getting at is whether this is making things compatible with a 39-bit address space (38-bit user address space) but still able to take advantage of a larger (48/47, 57/56) address space at runtime if that's what's available, or making things such that no matter how much is available this will never use more than it would use in the 39-bit address space?

For all Fuchsia platforms, there is theoretically variable options here and it won't be a change in the Fuchsia System ABI if more bits are supported on a future version and/or on future hardware. On x86 and AArch64 48 bits is the smallest size, but 57 bits may happen later, and for AArch64 maybe even 39 bits might happen later on some future hardware configuration (less likely). On RISC-V, 39, 48, and 57 are all likely to be supported whenever the hardware supports them, but the choice may well be dynamic based on how much actual RAM a machine has and other such factors.

fabio-d added inline comments.May 26 2023, 6:59 AM

Judging from compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h's Init(), this number is used by the 64-bit primary allocator as the size of the *single* contiguous reserved address space region, so this looks safe to me.

I think it's worth observing that we (@Caslyn in D148475) recently reduced the equivalent value in Scudo (PrimarySize = RegionSize * NumClasses on Fuchsia RISC-V from 1 GiB * 45 = 45 GiB to 256 MiB * 45 ~= 11 GB). Overall the values in this file seem to be a few order of magnitudes bigger than the corresponding Scudo values, but I don't know what the reason could be.


I don't know the details either. I can see that, with your change, Fuchsia RISC-V is moving to a different configuration:

typedef SizeClassMap<3, 4, 8, 17, 128, 16> DefaultSizeClassMap; // <- the old one
typedef SizeClassMap<2, 5, 9, 16, 8, 10> VeryDenseSizeClassMap; // <- the new one

template <uptr kNumBits, uptr kMinSizeLog, uptr kMidSizeLog, uptr kMaxSizeLog,
          uptr kMaxNumCachedHintT, uptr kMaxBytesCachedLog>
class SizeClassMap;

If it's similar to Scudo (as it seems to be), the first four values control the distribution of the size classes and the last two numbers control how big each per-thread cache is allowed to be. Intuitively, I would say that we shouldn't need to have different parameters here depending on the cpu architecture, but I don't have concrete data to support this intuition.

Changing the distribution of the size classes does not limit on the amount of *total* memory allocated by the primary allocator (which is limited by kAllocatorSize), but the maximum amount of memory allocated from each class is limited by kAllocatorSize / round_to_next_pow_of_two(NumClasses) (see kRegionSize in sanitizer_allocator_primary64.h:625).

Not marking as Done because I too would like to hear from other people :)

leonardchan retitled this revision from [compiler-rt][asan] Have RISCV allocator settings take precedence over Fuchsia's settings to [compiler-rt][asan][Fuchsia] Tune the 64-bit asan allocator for riscv+fuchsia.Aug 24 2023, 5:51 PM
leonardchan edited the summary of this revision. (Show Details)
leonardchan added inline comments.Aug 24 2023, 5:55 PM

I updated this patch with a more fine-grained and tested configuration and comments which hopefully explain the justification/impact each setting will have. TBH a lot of this was driven by getting the combined scudo tests running in an asan-instrumented build. If we didn't have to run those tests with asan, the SizeClassMap could be "looser" in that we could have something closer to what scudo has or the DefaultSizeClassMap.

ping for thoughts on this

fabio-d accepted this revision.Sep 28 2023, 11:57 AM
This revision is now accepted and ready to land.Sep 28 2023, 11:57 AM
This revision was landed with ongoing or failed builds.Sep 28 2023, 12:00 PM
This revision was automatically updated to reflect the committed changes.