Re: [PATCH v2 1/2] memcg: Don't generate low/min events if either low/min or elow/emin is 0

[Waiman Long]

On 4/4/25 2:26 PM, Michal Koutný wrote:
> Hello Waiman.
>
> On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long <longman@xxxxxxxxxx> wrote:
> > 1) memory.low is set to 0, but low events can still be triggered and
> >     so the cgroup may have a non-zero low event count. I doubt users are
> >     looking for that as they didn't set memory.low at all.
> I agree with this reasoning, been there [1] but fix ain't easy (also
> consensus of whether such an event should count or not and whether
> reclaim should happen or not). (See also [2] where I had tried other
> approaches that _didn't_ work.)
>
> > 2) memory.low is set to a non-zero value but the cgroup has no task in
> >     it so that it has an effective low value of 0.
> There maybe page cache remaining in the cgroup even with not present
> task inside it.

For the test_memcontrol case, a cgroup is created but no task has 
already been moved into it. So the memory usage is 0. I agree that if a 
task has ever lived in the cgroup, the usage will not be 0. In that case 
memory reclaim is certainly justified.


> >     Again it may have a non-zero low event count if memory reclaim
> >     happens. This is probably not a result expected by the users and it
> >     is really doubtful that users will check an empty cgroup with no
> >     task in it and expecting some non-zero event counts.
> Well, if memory.current > 0, some reclaim events can be justified and
> thus expected (e.g. by me).
>
> > The simple and naive fix of changing the operator to ">", however,
> > changes the memory reclaim behavior which can lead to other failures
> > as low events are needed to facilitate memory reclaim.  So we can't do
> > that without some relatively riskier changes in memory reclaim.
> >
> > Another simpler alternative is to avoid reporting below_low failure
> > if either memory.low or its effective equivalent is 0 which is done
> > by this patch specifically for the two failed use cases above.
> Admittedly, I haven't seen any complaints from real world about these
> events except for this test (which was ported from selftests to LTP
> too).
>
> > With this patch applied, the test_memcg_low sub-test finishes
> > successfully without failure in most cases.
> I'd say the simplest solution to make the test pass without figuring out
> what semantics of low events should be correct is not to check the
> memory.events:low at all with memory_recursiveprot (this is what was
> done in the cloned LTP test).

Another alternative is to modify the test to allow non-zero event count 
even if low is not set.

Cheers,
Longman

> Michal
>
> [1] https://lore.kernel.org/all/20220322182248.29121-1-mkoutny@xxxxxxxx/
> [2] https://bugzilla.suse.com/show_bug.cgi?id=1196298