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

[Waiman Long <llong@redhat.com>]

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@redhat.com> 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@suse.com/
> [2] https://bugzilla.suse.com/show_bug.cgi?id=1196298