[PATCH] D40038: [scudo] Soft and hard RSS limit checks

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:318
+      return atomic_load_relaxed(&RssLimitExceeded);
+    const uptr CurrentRssMb = GetRSS() >> 20;
+    if (HardRssLimitMb && HardRssLimitMb < CurrentRssMb) {
----------------
alekseyshl wrote:
> cryptoad wrote:
> > cryptoad wrote:
> > > alekseyshl wrote:
> > > > How fast is it? It might read a file, right?
> > > It either reads `/proc/self/statm`, or falls back to `getrusage` if statm is not allowed.
> > > So yes, not the fastest of code paths.
> > > I am not sure I see a way around it though.
> > Also the common flag is called `can_use_proc_maps_statm`, which is misdirecting, I think it really should be `can_use_proc_self_statm`.
> Do not fall back to reading proc/self/statm, for example? Reading a file might cause unpredictable delays, right? Not too great for the allocator. Maybe increase the interval to 1s?
> 
> Also the general idea does not feel right, the allocator is one of the moving parts affecting RSS, not the only part, why should it be burdened with RSS check?
> 
> But yeah, I have questions, but not answers, no good solution either.
I agree that it's not an ideal solution.

For some context, I am trying to address a problem that we are bringing to ourselves: due to the memory reservation made by the Primary, we can't enforce a sensible RLIMIT_AS for a process. Which some processes need (see `limitProcessMemory` in Android). The solution that comes to mind is to be able to constrain our RSS since we are the most likely consumer of memory. While not ideal, I don't think it's too unreasonable either.

Now for potential improvements I could code my own `getMaxRSS` that would be only `getrusage` based. That way, we skip the filesystem interaction. It would require a Fuchsia equivalent but that should be straightforward.



https://reviews.llvm.org/D40038