This is an archive of the discontinued LLVM Phabricator instance.

Add Soft/Hard RSS Limits to Scudo Standalone

Authored by bsdaemon on May 31 2022, 10:33 PM.

Diff Detail

Event Timeline

bsdaemon created this revision.May 31 2022, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 10:33 PM
bsdaemon requested review of this revision.May 31 2022, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 10:33 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
bsdaemon added a reviewer: mcgrathr.

This needs a stub GetRSS for Fuchsia, at least.
Since there's no way to get a useful value for this on Fuchsia as yet, it would be preferable to conditionalize the machinery so it isn't doing no-op work.

bsdaemon updated this revision to Diff 435038.Jun 7 2022, 8:25 PM

The feature right now is only supported on LINUX. isRssLimitExceeded() is protected by #if and inlined in any non-linux system and a generic GetRSS symbol is exported in any non-Linux systems (so even with LD_BIND_NOW the symbol exists/resolves). GetRSS() never gets called since it is only called by isRssLimitExceeded() and by a flow conditioned in the isRssLimitExceeded result.

With optimizations on, even the references to the functions and the entire conditional block get removed.

Can you reupload the patch with full context?

185 ↗(On Diff #435038)

The opening brace should be on the previous line.

190 ↗(On Diff #435038)

The opening brace should be on the previous line.

195 ↗(On Diff #435038)

The opening brace should be on the previous line.


The opening brace should be on the previous line.

bsdaemon updated this revision to Diff 435201.Jun 8 2022, 9:10 AM

Fixed the few style errors pointed out (thank you!) and submitting a full context patch.

Fixed the few style errors pointed out (thank you!) and submitting a full context patch.

bsdaemon updated this revision to Diff 439333.Jun 23 2022, 4:44 AM

Fixed compiler warnings due to implicit type conversions, got more pedantic on using the right types (even for compatible ones).

This comment was removed by bsdaemon.
bsdaemon updated this revision to Diff 440238.Jun 27 2022, 8:16 AM

Added LLVM code-style fixes

mcgrathr added inline comments.Jul 6 2022, 11:21 AM
127 ↗(On Diff #440238)

This should use the call above with true instead of false for the bool argument.


excess space between parens


It's better not to use ALWAYS_INLINE and leave it to the compiler to decide (which it will).
Provide the stub GetRSS function even when you expect calls to it to be optimized away.


Use false, not 0.


Put spaces around the =.

vitalybuka requested changes to this revision.Jul 7 2022, 10:25 AM
vitalybuka added inline comments.
127 ↗(On Diff #440238)

please move this into a separate patch and extend compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp

for context, you can organize patches into "stack" using "edit related revision" in the Phabricator menu.


just clangformat entire patch




limits need own tests as well

196 ↗(On Diff #440238)

How about moving GetRSS() (with a test) implementation into a separate patch?

This revision now requires changes to proceed.Jul 7 2022, 10:25 AM

Bastian Kersting started splitting the patch and making the suggested changes. The first in the series is here: - I appreciate if folks here help reviewing/merging.

Can you please rebase this one?


can you measure a meaningful difference or real apps between weak vs strong here?

1c3t3a added a subscriber: 1c3t3a.Dec 19 2022, 3:28 AM
1c3t3a updated this revision to Diff 483931.Dec 19 2022, 5:49 AM

Rebase on commit chain and latest main.

1c3t3a updated this revision to Diff 484194.Dec 20 2022, 2:02 AM

Add requested changes and tests for Hard and Soft RSS limits

1c3t3a added inline comments.Dec 20 2022, 2:10 AM

I benchmarked this method and found not a big difference but a difference between weak and strong here. I benchmarked this on an ARM (aarch64) machine with the following findings:

Running third_party/llvm/llvm-project/compiler-rt/scudo_benchmarks
Run on (64 X 3000 MHz CPU s)
CPU Caches:
  L1 Data 64 KiB (x64)
  L1 Instruction 64 KiB (x64)
  L2 Unified 1024 KiB (x64)
  L3 Unified 32768 KiB (x1)
Load Average: 4.02, 9.90, 14.71
Benchmark                       Time             CPU   Iterations
BM_IsRssExceeded             47.6 ns         47.5 ns     14772023
BM_IsRssExceededStrong       47.8 ns         47.7 ns     14673854

I get that this is not a big difference, but I would still argue that it is enough to use weak here. We could introduce a compilation parameter with the default weak.

vitalybuka added inline comments.Dec 20 2022, 10:31 PM

zero initialized value will work the same


Probably nicer to put this method and related fields into another class, e.g

class RssLimitChecker {
 atomic_u64 RssLastCheckedAtNS = {};
 atomic_u8 RssLimitExceeded = {};

  // Slow part in cpp file
  bool Check();

 // Inline fast path.
 bool isExceeded() {
    u64 LastCheck = atomic_load_relaxed(&RssLastCheckedAtNS);
    const u64 CurrentCheck = getMonotonicTime();
     if (UNLIKELY(CurrentCheck >= LastCheck + (250ULL * 1000000ULL)))
     return atomic_load_relaxed(&RssLimitExceeded);


instead of adding interval here,
add it when you store it in compare_exchange
then it should be named LastCheck -> NextCheck

CurrentCheck could be named e.g. "Now".


Then please just switch to strong


This relies on the order of the test execution
everything after this will see scudo::getFlags()->soft_rss_limit_mb = 1

for avoid this the test should backup and restore flags


this code does not use {} for one line loop bodies


restore flags

1c3t3a updated this revision to Diff 484551.Dec 21 2022, 5:55 AM

Add requested changes

This looks good to me with nits


remove {}


redundant ()

222 ↗(On Diff #484551)

atomic_compare_exchange_strong is in UNLIKELY branch already
sink it into the Check() to avoid unnecessary inlining it

240 ↗(On Diff #484551)

Remove redundant ULL as result fits into int32
Remove redundant ()

230 ↗(On Diff #484551)

remove {}

231 ↗(On Diff #484551)

maybe nicer to RssLimitExceeded -> enum {Neither, Soft, Hard}
and move reportRSSCheck() same place where you report soft limit

you don't need getOptionsForTesting if you can run init after updating flags


get from RssChecker


please add and use RssChecker.getSoftRssLimit() RssChecker.getHardRssLimit()
So we have not concerns if IsExceeded and this message use the same value.

217 ↗(On Diff #484551)

Return here if they are both zeroes and eliminate CheckRssLimit?

1c3t3a updated this revision to Diff 484837.Dec 22 2022, 7:41 AM

Add requested changes and reorganize tests

I also restructured the tests to get rid of getOptionsForTesting and the flags since this is now read at the creation of the RssChecker.

222 ↗(On Diff #484551)

That would mean Check needs the NextCheck variable for context and that would become a parameter of Check right?

217 ↗(On Diff #484551)

Done. What do you mean by "eliminate CheckRssLimit"?

1c3t3a updated this revision to Diff 484844.Dec 22 2022, 7:55 AM

Remove unused headers

1c3t3a updated this revision to Diff 484861.Dec 22 2022, 9:19 AM

Fix init of limits

vitalybuka edited the summary of this revision. (Show Details)Dec 22 2022, 11:39 AM
vitalybuka accepted this revision.Dec 22 2022, 7:32 PM
This revision is now accepted and ready to land.Dec 22 2022, 7:32 PM
This revision was landed with ongoing or failed builds.Dec 22 2022, 7:45 PM
This revision was automatically updated to reflect the committed changes.