Re: [PATCH v5 29/32] mm: memcontrol: prepare for reparenting non-hierarchical stats

[Shakeel Butt <shakeel.butt@linux.dev>]

On Thu, Feb 26, 2026 at 07:16:50AM -0800, Yosry Ahmed wrote:
> > > Did you measure the impact of making state_local atomic on the flush
> > > path? It's a slow path but we've seen pain from it being too slow
> > > before, because it extends the critical section of the rstat flush
> > > lock.
> >
> > Qi, please measure the impact on flushing and if no impact then no need to do
> > anything as I don't want anymore churn in this series.
> >
> > >
> > > Can we keep this non-atomic and use mod_memcg_lruvec_state() here? It
> > > will update the stat on the local counter and it will be added to
> > > state_local in the flush path when needed. We can even force another
> > > flush in reparent_state_local () after reparenting is completed, if we
> > > want to avoid leaving a potentially large stat update pending, as it
> > > can be missed by mem_cgroup_flush_stats_ratelimited().
> > >
> > > Same for reparent_memcg_state_local(), we can probably use mod_memcg_state()?
> >
> > Yosry, do you mind sending the patch you are thinking about over this series?
> 
> Honestly, I'd rather squash it into this patch if possible. It avoids
> churn in the history (switch to atomics and back), and is arguably
> simpler than checking for regressions in the flush path.

Yup, let's squash it into the original patch. Please add your sign-off tag.

> 
> What I have in mind is the diff below (build tested only). Qi, would
> you be able to test this? It applies directly on this patch in mm-new:

Qi, please squash this diff into the patch and test. You might need to change
the subsequent patches. Once you are done with testing, you can post the diffs
for those in reply to those patches and we will ask Andrew to squash into
orinigal ones.

The diff looks good to me though.
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d82dbfcc28057..404565e80cbf3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -234,11 +234,18 @@ static inline void reparent_state_local(struct
> mem_cgroup *memcg, struct mem_cgr
>         if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>                 return;
> 
> +       /*
> +        * Reparent stats exposed non-hierarchically. Flush @memcg's
> stats first to
> +        * read its stats accurately , and conservatively flush @parent's stats
> +        * after reparenting to avoid hiding a potentially large stat update
> +        * (e.g. from callers of mem_cgroup_flush_stats_ratelimited()).
> +        */
>         __mem_cgroup_flush_stats(memcg, true);
> 
> -       /* The following counts are all non-hierarchical and need to
> be reparented. */
>         reparent_memcg1_state_local(memcg, parent);
>         reparent_memcg1_lruvec_state_local(memcg, parent);
> +
> +       __mem_cgroup_flush_stats(parent, true);
>  }
>  #else
>  static inline void reparent_state_local(struct mem_cgroup *memcg,
> struct mem_cgroup *parent)
> @@ -442,7 +449,7 @@ struct lruvec_stats {
>         long state[NR_MEMCG_NODE_STAT_ITEMS];
> 
>         /* Non-hierarchical (CPU aggregated) state */
> -       atomic_long_t state_local[NR_MEMCG_NODE_STAT_ITEMS];
> +       long state_local[NR_MEMCG_NODE_STAT_ITEMS];
> 
>         /* Pending child counts during tree propagation */
>         long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
> @@ -485,7 +492,7 @@ unsigned long lruvec_page_state_local(struct lruvec *lruvec,
>                 return 0;
> 
>         pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> -       x = atomic_long_read(&(pn->lruvec_stats->state_local[i]));
> +       x = READ_ONCE(pn->lruvec_stats->state_local[i]);
>  #ifdef CONFIG_SMP
>         if (x < 0)
>                 x = 0;
> @@ -493,6 +500,10 @@ unsigned long lruvec_page_state_local(struct
> lruvec *lruvec,
>         return x;
>  }
> 
> +static void mod_memcg_lruvec_state(struct lruvec *lruvec,
> +                                  enum node_stat_item idx,
> +                                  int val);
> +
>  #ifdef CONFIG_MEMCG_V1
>  void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
>                                        struct mem_cgroup *parent, int idx)
> @@ -506,12 +517,10 @@ void reparent_memcg_lruvec_state_local(struct
> mem_cgroup *memcg,
>         for_each_node(nid) {
>                 struct lruvec *child_lruvec = mem_cgroup_lruvec(memcg,
> NODE_DATA(nid));
>                 struct lruvec *parent_lruvec =
> mem_cgroup_lruvec(parent, NODE_DATA(nid));
> -               struct mem_cgroup_per_node *parent_pn;
>                 unsigned long value =
> lruvec_page_state_local(child_lruvec, idx);
> 
> -               parent_pn = container_of(parent_lruvec, struct
> mem_cgroup_per_node, lruvec);
> -
> -               atomic_long_add(value,
> &(parent_pn->lruvec_stats->state_local[i]));
> +               mod_memcg_lruvec_state(child_lruvec, idx, -value);
> +               mod_memcg_lruvec_state(parent_lruvec, idx, value);
>         }
>  }
>  #endif
> @@ -598,7 +607,7 @@ struct memcg_vmstats {
>         unsigned long           events[NR_MEMCG_EVENTS];
> 
>         /* Non-hierarchical (CPU aggregated) page state & events */
> -       atomic_long_t           state_local[MEMCG_VMSTAT_SIZE];
> +       long                    state_local[MEMCG_VMSTAT_SIZE];
>         unsigned long           events_local[NR_MEMCG_EVENTS];
> 
>         /* Pending child counts during tree propagation */
> @@ -835,7 +844,7 @@ unsigned long memcg_page_state_local(struct
> mem_cgroup *memcg, int idx)
>         if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n",
> __func__, idx))
>                 return 0;
> 
> -       x = atomic_long_read(&(memcg->vmstats->state_local[i]));
> +       x = READ_ONCE(memcg->vmstats->state_local[i]);
>  #ifdef CONFIG_SMP
>         if (x < 0)
>                 x = 0;
> @@ -852,7 +861,8 @@ void reparent_memcg_state_local(struct mem_cgroup *memcg,
>         if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n",
> __func__, idx))
>                 return;
> 
> -       atomic_long_add(value, &(parent->vmstats->state_local[i]));
> +       mod_memcg_state(memcg, idx, -value);
> +       mod_memcg_state(parent, idx, value);
>  }
>  #endif
> 
> @@ -4174,8 +4184,6 @@ struct aggregate_control {
>         long *aggregate;
>         /* pointer to the non-hierarchichal (CPU aggregated) counters */
>         long *local;
> -       /* pointer to the atomic non-hierarchichal (CPU aggregated) counters */
> -       atomic_long_t *alocal;
>         /* pointer to the pending child counters during tree propagation */
>         long *pending;
>         /* pointer to the parent's pending counters, could be NULL */
> @@ -4213,12 +4221,8 @@ static void mem_cgroup_stat_aggregate(struct
> aggregate_control *ac)
>                 }
> 
>                 /* Aggregate counts on this level and propagate upwards */
> -               if (delta_cpu) {
> -                       if (ac->local)
> -                               ac->local[i] += delta_cpu;
> -                       else if (ac->alocal)
> -                               atomic_long_add(delta_cpu, &(ac->alocal[i]));
> -               }
> +               if (delta_cpu)
> +                       ac->local[i] += delta_cpu;
> 
>                 if (delta) {
>                         ac->aggregate[i] += delta;
> @@ -4289,8 +4293,7 @@ static void mem_cgroup_css_rstat_flush(struct
> cgroup_subsys_state *css, int cpu)
> 
>         ac = (struct aggregate_control) {
>                 .aggregate = memcg->vmstats->state,
> -               .local = NULL,
> -               .alocal = memcg->vmstats->state_local,
> +               .local = memcg->vmstats->state_local,
>                 .pending = memcg->vmstats->state_pending,
>                 .ppending = parent ? parent->vmstats->state_pending : NULL,
>                 .cstat = statc->state,
> @@ -4323,8 +4326,7 @@ static void mem_cgroup_css_rstat_flush(struct
> cgroup_subsys_state *css, int cpu)
> 
>                 ac = (struct aggregate_control) {
>                         .aggregate = lstats->state,
> -                       .local = NULL,
> -                       .alocal = lstats->state_local,
> +                       .local = lstats->state_local,
>                         .pending = lstats->state_pending,
>                         .ppending = plstats ? plstats->state_pending : NULL,
>                         .cstat = lstatc->state,