Re: [PATCH v3 1/3] mm: memcg: redirect stats updates of dying memcgs for all hierarchies

[Hui Zhu]

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Redirecting dying memcg stat updates to the parent on cgroup v2 without reparenting the base stats causes permanent negative stat leaks and underflows.
> --
>
> commit 106b19246ac20191c0af6a41927414095cb5d019
> Author: Hui Zhu <zhuhui@xxxxxxxxxx>
>
> mm: memcg: redirect stats updates of dying memcgs for all hierarchies
>
> This patch removes the cgroup v1 restriction on redirecting dying memcg stat
> updates to the closest non-dying ancestor. It unconditionally applies this
> redirection across all hierarchies in an effort to fix workingset shadow node
> budget calculations under MGLRU.
>
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 9a65d7148c227..64eb59bba61cf 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> [ ... ]
>
> > -static inline struct mem_cgroup *get_non_dying_memcg_start(struct mem_cgroup *memcg,
> > -							   bool *rcu_locked)
> > +static inline struct mem_cgroup *
> > +get_non_dying_memcg_start(struct mem_cgroup *memcg, bool *rcu_locked)
> >   {
> > -	/* Rebinding can cause this value to be changed at runtime */
> > -	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> > -		*rcu_locked = false;
> > -		return memcg;
> > -	}
> > -
> >   	rcu_read_lock();
> >   	*rcu_locked = true;
> >   
> > @@ -870,22 +864,8 @@ static inline struct mem_cgroup *get_non_dying_memcg_start(struct mem_cgroup *me
> [Severity: High]
> Does this redirection cause permanent stat underflows in the ancestor under
> cgroup v2?
>
> In cgroup v2, base local stats are explicitly not reparented during offlining.
> Looking at reparent_state_local():
>
>      if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>          return;
>
> Because of this early return, the dying memcg retains its positive base counts.
>
> However, with this patch, when resources previously charged to the dying memcg
> are freed later on, the negative delta applied in mod_memcg_state() is
> redirected up to the parent.
>
> Since the non-dying ancestor never received the positive base counts but now
> receives the negative uncharge updates, its local stats (like NR_FILE_PAGES,
> MEMCG_SOCK, and LRU counts) will permanently underflow.
>
> Because lruvec_page_state_local() clamps negative values to zero, wouldn't
> this severe underflow mask the ancestor's own legitimate positive stats,
> ultimately breaking the workingset shadow node budget calculation and causing
> the exact premature reclaim and thrashing this patch aims to resolve?
The observation is correct: on cgroup v2 reparent_state_local() returns
early, so the dying memcg keeps its positive base counts while the
negative uncharge deltas land on the ancestor, and the ancestor's
state_local can permanently underflow and get clamped to zero.

One clarification on attribution, though: this underflow is not
introduced by the redirection.  Once memcg_reparent_objcgs() rewrites
objcg->memcg to the parent, folio_memcg() of the reparented folios
already returns the parent, so the freeing path applies the negative
deltas to the parent's lruvec directly, with or without this patch.
The redirection only matters during the short window between
css_offline() and the objcg reparenting.  So the missing base reparent
is a pre-existing gap that this series re-exposes rather than creates.

That said, I agree it needs to be handled.  I'm preparing a follow-up
patch that also reparents the non-hierarchical lruvec state_locals
(the ones count_shadow_nodes() reads) on cgroup v2, mirroring what v1
already does.  Will fold it into the next version.

Best,
Hui