This is an archive of the discontinued LLVM Phabricator instance.

Add Soft/Hard RSS Limits to Scudo Standalone
ClosedPublic

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?

compiler-rt/lib/scudo/standalone/linux.cpp
185

The opening brace should be on the previous line.

190

The opening brace should be on the previous line.

195

The opening brace should be on the previous line.

compiler-rt/lib/scudo/standalone/report.cpp
40

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
compiler-rt/lib/scudo/standalone/atomic_helpers.h
127 ↗(On Diff #440238)

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

compiler-rt/lib/scudo/standalone/combined.h
353

excess space between parens

732

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.

736

Use false, not 0.

1021

Put spaces around the =.

vitalybuka requested changes to this revision.Jul 7 2022, 10:25 AM
vitalybuka added inline comments.
compiler-rt/lib/scudo/standalone/atomic_helpers.h
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.

compiler-rt/lib/scudo/standalone/combined.h
353

just clangformat entire patch

357

static_cast

compiler-rt/lib/scudo/standalone/flags.inc
49

limits need own tests as well

compiler-rt/lib/scudo/standalone/linux.cpp
196

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: https://reviews.llvm.org/D138406 - I appreciate if folks here help reviewing/merging.

Can you please rebase this one?

compiler-rt/lib/scudo/standalone/combined.h
703

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
compiler-rt/lib/scudo/standalone/combined.h
703

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:

2022-12-15T09:02:48-08:00
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
compiler-rt/lib/scudo/standalone/combined.h
171

zero initialized value will work the same

694

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();

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

 }
 
}
700

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".

703

Then please just switch to strong

718–725
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
741

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

757

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

764

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

compiler-rt/lib/scudo/standalone/combined.h
169

remove {}

353–354

redundant ()

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

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

239

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

compiler-rt/lib/scudo/standalone/linux.cpp
231

remove {}

232

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

compiler-rt/lib/scudo/standalone/combined.h
169

get from RssChecker

358

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

compiler-rt/lib/scudo/standalone/linux.cpp
218

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.

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

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

compiler-rt/lib/scudo/standalone/linux.cpp
218

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.