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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6712624c-c798-4ccf-afc1-6dfc9efc4b5e@redhat.com>
Date: Sat, 5 Apr 2025 14:52:04 -0400
From: Waiman Long <llong@...hat.com>
To: Johannes Weiner <hannes@...xchg.org>, Waiman Long <llong@...hat.com>
Cc: Tejun Heo <tj@...nel.org>, Michal Hocko <mhocko@...nel.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Shakeel Butt <shakeel.butt@...ux.dev>, Muchun Song <muchun.song@...ux.dev>,
 Andrew Morton <akpm@...ux-foundation.org>, Michal Koutný
 <mkoutny@...e.com>, Shuah Khan <shuah@...nel.org>,
 linux-kernel@...r.kernel.org, cgroups@...r.kernel.org, linux-mm@...ck.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 1/2] memcg: Don't generate low/min events if either
 low/min or elow/emin is 0


On 4/4/25 3:38 PM, Johannes Weiner wrote:
> On Fri, Apr 04, 2025 at 02:55:35PM -0400, Waiman Long wrote:
>> On 4/4/25 2:13 PM, Johannes Weiner wrote:
>>> * Waiman points out that the weirdness is seeing low events without
>>>     having a low configured. Eh, this isn't really true with recursive
>>>     propagation; you may or may not have an elow depending on parental
>>>     configuration and sibling behavior.
>>>
>> Do you mind if we just don't update the low event count if low isn't
>> set, but leave the rest the same like
> What's the motivation for doing anything beyond the skip-on-!usage?
It is to avoid making further change. I am fine with modifying the test 
to allow low event even when low isn't set.
>> @@ -659,21 +659,25 @@ static inline bool mem_cgroup_unprotected(struct
>> mem_cgro>
>>    static inline bool mem_cgroup_below_low(struct mem_cgroup *target,
>>                                           struct mem_cgroup *memcg)
>>    {
>> +       unsigned long elow;
>> +
>>           if (mem_cgroup_unprotected(target, memcg))
>>                   return false;
>>
>> -       return READ_ONCE(memcg->memory.elow) >=
>> -               page_counter_read(&memcg->memory);
>> +       elow = READ_ONCE(memcg->memory.elow);
>> +       return elow && (page_counter_read(&memcg->memory) <= elow);
>>    }
>>
>>    static inline bool mem_cgroup_below_min(struct mem_cgroup *target,
>>                                           struct mem_cgroup *memcg)
>>    {
>> +       unsigned long emin;
>> +
>>           if (mem_cgroup_unprotected(target, memcg))
>>                   return false;
>>
>> -       return READ_ONCE(memcg->memory.emin) >=
>> -               page_counter_read(&memcg->memory);
>> +       emin = READ_ONCE(memcg->memory.emin);
>> +       return emin && (page_counter_read(&memcg->memory) <= emin);
>>    }
> This still redefines the empty case to mean excess. That's a quirk I
> would have liked to avoid. I don't see why you would need it?
OK, I will drop that.
>
>> @@ -5919,7 +5923,8 @@ static void shrink_node_memcgs(pg_data_t *pgdat,
>> struct s>
>>                                   sc->memcg_low_skipped = 1;
>>                                   continue;
>>                           }
>> -                       memcg_memory_event(memcg, MEMCG_LOW);
>> +                       if (memcg->memory.low)
>> +                               memcg_memory_event(memcg, MEMCG_LOW);
> That's not right. In setups where protection comes from the parent, no
> breaches would ever be counted.

OK. Will post a v3 to incorporate your suggestion.

Thanks,
Longman