| Message ID | 20221011143015.1152968-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcontrol: Don't increase effective low/min if no protection needed | expand |
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
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
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.
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
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.
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
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
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(+)