Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <defdb421-342e-ebcb-d7f0-005559dd1e0d@redhat.com>
Date:   Tue, 11 Oct 2022 13:00:22 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no
 protection needed
Content-Language: en-US
To:     Michal Hocko <mhocko@suse.com>
Cc:     Johannes Weiner <hannes@cmpxchg.org>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Shakeel Butt <shakeelb@google.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
        linux-mm@kvack.org, Tejun Heo <tj@kernel.org>,
        Chris Down <chris@chrisdown.name>
References: <20221011143015.1152968-1-longman@redhat.com>
 <Y0WOPZxWSnUjzZ8e@dhcp22.suse.cz>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <Y0WOPZxWSnUjzZ8e@dhcp22.suse.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/11/22 11:39, Michal Hocko wrote:
> On Tue 11-10-22 10:30:15, Waiman Long wrote:
>> Since commit bc50bcc6e00b ("mm: memcontrol: clean up and document
>> effective low/min calculations"), the effective low/min protections can
>> be non-zero even if the corresponding memory.low/min values are 0. That
>> can surprise users to see MEMCG_LOW events even when the memory.low
>> value is not set. One example is the LTP's memcontrol04 test which fails
>> because it detects some MEMCG_LOW events for a cgroup with a memory.min
>> value of 0.
> Is this with memory_recursiveprot mount option?
Yes, the memory_recursiveprot mount option is indeed turned on.
>
>> Fix this by updating effective_protection() to not returning a non-zero
>> low/min protection values if the corresponding memory.low/min values
>> or those of its parent are 0.
>>
>> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   mm/memcontrol.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index b69979c9ced5..893d4d5e518a 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -6660,6 +6660,9 @@ static unsigned long effective_protection(unsigned long usage,
>>   	unsigned long protected;
>>   	unsigned long ep;
>>   
>> +	if (!setting || !parent_effective)
>> +		return 0UL;	/* No protection is needed */
>> +
> This will break the above memory_recursiveprot AFAICS.

You are right about that. An alternative way to address this issue is to 
disable memory low event when memory.low isn't set. An user who want to 
track memory.low event has to set it to a non-zero value. Would that be 
acceptable?

Cheers,
Longman