On 4/14/25 2:10 PM, Johannes Weiner wrote:
> On Mon, Apr 14, 2025 at 08:01:42PM +0200, Michal Koutný wrote:
>> On Mon, Apr 14, 2025 at 12:47:21PM -0400, Johannes Weiner<hannes@cmpxchg.org> wrote:
>>> It's not a functional change to the protection semantics or the
>>> reclaim behavior.
>> Yes, that's how I understand it, therefore I'm wondering what does it
>> change.
>>
>> If this is taken:
>>                 if (!mem_cgroup_usage(memcg, false))
>>                         continue;
>>
>> this would've been taken too:
>>                  if (mem_cgroup_below_min(target_memcg, memcg))
>>                          continue;
>> (unless target_memcg == memcg but that's not interesting for the events
>> here)
> D'oh.
>
>>> The problem is if we go into low_reclaim and encounter an empty group,
>>> we'll issue "low-protected group is being reclaimed" events,
>> How can this happen when
>> 	page_counter_read(&memcg->memory) <= memcg->memory.emin
>> ? (I.e. in this case 0 <= emin and emin >= 0.)
>>
>>> which is kind of absurd (nothing will be reclaimed) and thus confusing
>>> to users (I didn't even configure any protection!)
>> Yes.
>>   
>>> I suggested, instead of redefining the protection definitions for that
>>> special case, to bypass all the checks and the scan count calculations
>>> when we already know the group is empty and none of this applies.
>>>
>>> https://lore.kernel.org/linux-mm/20250404181308.GA300138@cmpxchg.org/
>> Is this non-functional change to make shrink_node_memcgs() robust
>> against possible future redefinitions of mem_cgroup_below_*()?
> No, this was really just aimed to stop low events on empty groups.
>
> But as you rightfully point out, they should not get past the min
> check in the first place. So something seems missing here.

I think I saw some low events in the !usage case was because my original 
patch was to remove the '=' from mem_cgroup_below_low() and 
mem_cgroup_below_min() which made it past the mem_cgroup_below_min() 
check. Without touching mem_cgroup_below_min/low(), the addition of 
mem_cgroup_usage() in shrink_node_memcgs() is probably redundant. I can 
remove it from the patch.

Thanks for the detailed review.

Cheers,
Longman