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

[Qi Zheng <qi.zheng@linux.dev>]

On 2/27/26 11:11 AM, Qi Zheng wrote:
> Hi Yosry,
> 
> On 2/26/26 11:16 PM, 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.
>>
>> 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:
> 
> Thank you so much for doing this! I'm willing to squash and test.
> 
> And I found some issues with the diff:
> 
>>
>> 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);
> 
> We can't use mod_memcg_lruvec_state() here, because child memcg has
> already been set CSS_DYING. So in mod_memcg_lruvec_state(), we will
> get parent memcg.
> 
> It seems we need to reimplement a function or add a parameter to
> mod_memcg_lruvec_state() to solve the problem. What do you think?

Since child memcg is about to disappear, perhaps we can just add value
to parent memcg without handling the child memcg. Make sense?

> 
>> +               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);
> 
> Same as mod_memcg_lruvec_state().
> 
> Thanks,
> Qi
> 
>> +       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,
>