[PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Waiman Long]

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.

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 */
+
 	protected = min(usage, setting);
 	/*
 	 * If all cgroups at this level combined claim and use more
-- 
2.31.1

Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Michal Hocko]

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?

> 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.

>  	protected = min(usage, setting);
>  	/*
>  	 * If all cgroups at this level combined claim and use more
> -- 
> 2.31.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Waiman Long]

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

Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Tejun Heo]

On Tue, Oct 11, 2022 at 01:00:22PM -0400, Waiman Long wrote:
> 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?

Wouldn't it make sense to fix the test? With recursive_prot on, the cgroup
actually is under low protection and it seems like the correct behavior is
to report the low events accordingly.

Thanks.

-- 
tejun

Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Waiman Long]

On 10/11/22 13:04, Tejun Heo wrote:
> On Tue, Oct 11, 2022 at 01:00:22PM -0400, Waiman Long wrote:
>> 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?
> Wouldn't it make sense to fix the test? With recursive_prot on, the cgroup
> actually is under low protection and it seems like the correct behavior is
> to report the low events accordingly.

Yes, that is another possible way of looking at that problem. Will talk 
to our QE people of doing that.

Thanks,
Longman

Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Michal Hocko]

On Tue 11-10-22 07:04:32, Tejun Heo wrote:
> On Tue, Oct 11, 2022 at 01:00:22PM -0400, Waiman Long wrote:
> > 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?
> 
> Wouldn't it make sense to fix the test? With recursive_prot on, the cgroup
> actually is under low protection and it seems like the correct behavior is
> to report the low events accordingly.

Agreed, the semantic makes sense and it seems to be just the test that
is not aware of it.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/memcontrol: Don't increase effective low/min if no protection needed

[Michal Koutný]

On Tue, Oct 11, 2022 at 07:04:32AM -1000, Tejun Heo <tj@kernel.org> wrote:
> Wouldn't it make sense to fix the test? With recursive_prot on, the cgroup
> actually is under low protection and it seems like the correct behavior is
> to report the low events accordingly.

It depends whether the there is a residual protection that the
memory.low=0 sibling can use (with memory_recursiveprot).

In the discussed LTP test, there should be no residual protection that
would justify the apparently misreported memory.low events. I.e. the
test is correct, the failure points to a subtle issue with distributing
residual protection among siblings.

Been there, (haven't) done that:
1) https://bugzilla.suse.com/show_bug.cgi?id=1196298
2) https://lore.kernel.org/r/20220325103118.GC2828@blackbody.suse.cz/

HTH,
Michal