| Message ID | 20250404012435.656045-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] memcg: Don't generate low/min events if either low/min or elow/emin is 0 | expand |
Hello, On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long wrote: ... > The simple and naive fix of changing the operator to ">", however, > changes the memory reclaim behavior which can lead to other failures > as low events are needed to facilitate memory reclaim. So we can't do > that without some relatively riskier changes in memory reclaim. I'm doubtful using ">" would change reclaim behavior in a meaningful way and that'd be more straightforward. What do mm people think? Thanks.
On 4/4/25 1:12 PM, Tejun Heo wrote: > Hello, > > On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long wrote: > ... >> The simple and naive fix of changing the operator to ">", however, >> changes the memory reclaim behavior which can lead to other failures >> as low events are needed to facilitate memory reclaim. So we can't do >> that without some relatively riskier changes in memory reclaim. > I'm doubtful using ">" would change reclaim behavior in a meaningful way and > that'd be more straightforward. What do mm people think? I haven't looked deeply into why that is the case, but test_memcg_low/min tests had other failures when I made this change. Cheers, Longman > > Thanks. >
On Fri, Apr 04, 2025 at 01:25:33PM -0400, Waiman Long wrote: > > On 4/4/25 1:12 PM, Tejun Heo wrote: > > Hello, > > > > On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long wrote: > > ... > >> The simple and naive fix of changing the operator to ">", however, > >> changes the memory reclaim behavior which can lead to other failures > >> as low events are needed to facilitate memory reclaim. So we can't do > >> that without some relatively riskier changes in memory reclaim. > > I'm doubtful using ">" would change reclaim behavior in a meaningful way and > > that'd be more straightforward. What do mm people think? The knob documentation uses "within low" and "above low" to distinguish whether you are protected or not, so at least from a code clarity pov, >= makes more sense to me: if your protection is N and you use exactly N, you're considered protected. That also means that by definition an empty cgroup is protected. It's not in excess of its protection. The test result isn't wrong. The real weirdness is issuing a "low reclaim" event when no reclaim is going to happen*. The patch effectively special cases "empty means in excess" to avoid the event and fall through to reclaim, which then does nothing as a result of its own scan target calculations. That seems convoluted. Why not skip empty cgroups before running inapplicable checks? diff --git a/mm/vmscan.c b/mm/vmscan.c index b620d74b0f66..260ab238ec22 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -5963,6 +5963,9 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) mem_cgroup_calculate_protection(target_memcg, memcg); + if (!mem_cgroup_usage(memcg, false)) + continue; + if (mem_cgroup_below_min(target_memcg, memcg)) { /* * Hard protection. > I haven't looked deeply into why that is the case, but > test_memcg_low/min tests had other failures when I made this change. It surprises me as well that it makes any practical difference. * 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.
Hello Waiman. On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long <longman@redhat.com> wrote: > 1) memory.low is set to 0, but low events can still be triggered and > so the cgroup may have a non-zero low event count. I doubt users are > looking for that as they didn't set memory.low at all. I agree with this reasoning, been there [1] but fix ain't easy (also consensus of whether such an event should count or not and whether reclaim should happen or not). (See also [2] where I had tried other approaches that _didn't_ work.) > 2) memory.low is set to a non-zero value but the cgroup has no task in > it so that it has an effective low value of 0. There maybe page cache remaining in the cgroup even with not present task inside it. > Again it may have a non-zero low event count if memory reclaim > happens. This is probably not a result expected by the users and it > is really doubtful that users will check an empty cgroup with no > task in it and expecting some non-zero event counts. Well, if memory.current > 0, some reclaim events can be justified and thus expected (e.g. by me). > The simple and naive fix of changing the operator to ">", however, > changes the memory reclaim behavior which can lead to other failures > as low events are needed to facilitate memory reclaim. So we can't do > that without some relatively riskier changes in memory reclaim. > > Another simpler alternative is to avoid reporting below_low failure > if either memory.low or its effective equivalent is 0 which is done > by this patch specifically for the two failed use cases above. Admittedly, I haven't seen any complaints from real world about these events except for this test (which was ported from selftests to LTP too). > With this patch applied, the test_memcg_low sub-test finishes > successfully without failure in most cases. I'd say the simplest solution to make the test pass without figuring out what semantics of low events should be correct is not to check the memory.events:low at all with memory_recursiveprot (this is what was done in the cloned LTP test). Michal [1] https://lore.kernel.org/all/20220322182248.29121-1-mkoutny@suse.com/ [2] https://bugzilla.suse.com/show_bug.cgi?id=1196298
On 4/4/25 2:13 PM, Johannes Weiner wrote: > On Fri, Apr 04, 2025 at 01:25:33PM -0400, Waiman Long wrote: >> On 4/4/25 1:12 PM, Tejun Heo wrote: >>> Hello, >>> >>> On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long wrote: >>> ... >>>> The simple and naive fix of changing the operator to ">", however, >>>> changes the memory reclaim behavior which can lead to other failures >>>> as low events are needed to facilitate memory reclaim. So we can't do >>>> that without some relatively riskier changes in memory reclaim. >>> I'm doubtful using ">" would change reclaim behavior in a meaningful way and >>> that'd be more straightforward. What do mm people think? > The knob documentation uses "within low" and "above low" to > distinguish whether you are protected or not, so at least from a code > clarity pov, >= makes more sense to me: if your protection is N and > you use exactly N, you're considered protected. > > That also means that by definition an empty cgroup is protected. It's > not in excess of its protection. The test result isn't wrong. > > The real weirdness is issuing a "low reclaim" event when no reclaim is > going to happen*. > > The patch effectively special cases "empty means in excess" to avoid > the event and fall through to reclaim, which then does nothing as a > result of its own scan target calculations. That seems convoluted. > > Why not skip empty cgroups before running inapplicable checks? > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index b620d74b0f66..260ab238ec22 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -5963,6 +5963,9 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) > > mem_cgroup_calculate_protection(target_memcg, memcg); > > + if (!mem_cgroup_usage(memcg, false)) > + continue; > + > if (mem_cgroup_below_min(target_memcg, memcg)) { > /* > * Hard protection. Yes, that should take care of the memcg with no task case. > >> I haven't looked deeply into why that is the case, but >> test_memcg_low/min tests had other failures when I made this change. > It surprises me as well that it makes any practical difference. I looked at it again and failure is the same expected memory.current check in test_memcontrol. If I remove the equal sign, I got errors like: values_close: child 0 = 8339456, 29MB = 30408704 failed with err = 21 not ok 1 test_memcg_min So the test is expecting memory.current to have around 29MB, but it got a lot less (~8MB) in this case. Before removing the equality sign, I usually got about 25 MB and above for child 0. That is a pretty big change in behavior, so I didn't make it. > > * 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 diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 91721c8862c3..48a8bfa7d337 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -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); } void mem_cgroup_commit_charge(struct folio *folio, struct mem_cgroup *memcg); diff --git a/mm/vmscan.c b/mm/vmscan.c index 77d015d5db0c..e8c1838c7962 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -4827,7 +4827,8 @@ static int shrink_one(struct lruvec *lruvec, struct scan_> if (READ_ONCE(lruvec->lrugen.seg) != MEMCG_LRU_TAIL) return MEMCG_LRU_TAIL; - memcg_memory_event(memcg, MEMCG_LOW); + if (memcg->memory.low) + memcg_memory_event(memcg, MEMCG_LOW); } success = try_to_shrink_lruvec(lruvec, sc); @@ -5902,6 +5903,9 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct s> mem_cgroup_calculate_protection(target_memcg, memcg); + if (!mem_cgroup_usage(memcg, false)) + continue; + if (mem_cgroup_below_min(target_memcg, memcg)) { /* * Hard protection. @@ -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); } reclaimed = sc->nr_reclaimed; Cheers, Longman
On 4/4/25 2:26 PM, Michal Koutný wrote: > Hello Waiman. > > On Thu, Apr 03, 2025 at 09:24:34PM -0400, Waiman Long <longman@redhat.com> wrote: >> 1) memory.low is set to 0, but low events can still be triggered and >> so the cgroup may have a non-zero low event count. I doubt users are >> looking for that as they didn't set memory.low at all. > I agree with this reasoning, been there [1] but fix ain't easy (also > consensus of whether such an event should count or not and whether > reclaim should happen or not). (See also [2] where I had tried other > approaches that _didn't_ work.) > >> 2) memory.low is set to a non-zero value but the cgroup has no task in >> it so that it has an effective low value of 0. > There maybe page cache remaining in the cgroup even with not present > task inside it. For the test_memcontrol case, a cgroup is created but no task has already been moved into it. So the memory usage is 0. I agree that if a task has ever lived in the cgroup, the usage will not be 0. In that case memory reclaim is certainly justified. >> Again it may have a non-zero low event count if memory reclaim >> happens. This is probably not a result expected by the users and it >> is really doubtful that users will check an empty cgroup with no >> task in it and expecting some non-zero event counts. > Well, if memory.current > 0, some reclaim events can be justified and > thus expected (e.g. by me). > >> The simple and naive fix of changing the operator to ">", however, >> changes the memory reclaim behavior which can lead to other failures >> as low events are needed to facilitate memory reclaim. So we can't do >> that without some relatively riskier changes in memory reclaim. >> >> Another simpler alternative is to avoid reporting below_low failure >> if either memory.low or its effective equivalent is 0 which is done >> by this patch specifically for the two failed use cases above. > Admittedly, I haven't seen any complaints from real world about these > events except for this test (which was ported from selftests to LTP > too). > >> With this patch applied, the test_memcg_low sub-test finishes >> successfully without failure in most cases. > I'd say the simplest solution to make the test pass without figuring out > what semantics of low events should be correct is not to check the > memory.events:low at all with memory_recursiveprot (this is what was > done in the cloned LTP test). Another alternative is to modify the test to allow non-zero event count even if low is not set. Cheers, Longman > > Michal > > [1] https://lore.kernel.org/all/20220322182248.29121-1-mkoutny@suse.com/ > [2] https://bugzilla.suse.com/show_bug.cgi?id=1196298
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? > @@ -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? > @@ -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.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 53364526d877..4d4a1f159eaa 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -601,21 +601,31 @@ static inline bool mem_cgroup_unprotected(struct mem_cgroup *target, 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); + if (!elow || !READ_ONCE(memcg->memory.low)) + return false; + + return 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); + if (!emin || !READ_ONCE(memcg->memory.min)) + return false; + + return page_counter_read(&memcg->memory) <= emin; } int __mem_cgroup_charge(struct folio *folio, struct mm_struct *mm, gfp_t gfp);
The test_memcontrol selftest consistently fails its test_memcg_low sub-test due to the fact that two of its test child cgroups which have a memmory.low of 0 or an effective memory.low of 0 still have low events generated for them since mem_cgroup_below_low() use the ">=" operator when comparing to elow. The two failed use cases are as follows: 1) memory.low is set to 0, but low events can still be triggered and so the cgroup may have a non-zero low event count. I doubt users are looking for that as they didn't set memory.low at all. 2) memory.low is set to a non-zero value but the cgroup has no task in it so that it has an effective low value of 0. Again it may have a non-zero low event count if memory reclaim happens. This is probably not a result expected by the users and it is really doubtful that users will check an empty cgroup with no task in it and expecting some non-zero event counts. The simple and naive fix of changing the operator to ">", however, changes the memory reclaim behavior which can lead to other failures as low events are needed to facilitate memory reclaim. So we can't do that without some relatively riskier changes in memory reclaim. Another simpler alternative is to avoid reporting below_low failure if either memory.low or its effective equivalent is 0 which is done by this patch specifically for the two failed use cases above. With this patch applied, the test_memcg_low sub-test finishes successfully without failure in most cases. Though both test_memcg_low and test_memcg_min sub-tests may still fail occasionally if the memory.current values fall outside of the expected ranges. To be consistent, similar change is appled to mem_cgroup_below_min() as to avoid the two failed use cases above with low replaced by min. Signed-off-by: Waiman Long <longman@redhat.com> --- include/linux/memcontrol.h | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)