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

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

Hi Yosry,

On 2/25/26 10:58 PM, Yosry Ahmed wrote:
>> @@ -473,6 +493,29 @@ unsigned long lruvec_page_state_local(struct lruvec *lruvec,
>>          return x;
>>   }
>>
>> +#ifdef CONFIG_MEMCG_V1
>> +void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
>> +                                      struct mem_cgroup *parent, int idx)
>> +{
>> +       int i = memcg_stats_index(idx);
>> +       int nid;
>> +
>> +       if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
>> +               return;
>> +
>> +       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]));
>> +       }
>> +}
> 
> Did you measure the impact of making state_local atomic on the flush

No, because this is not a performance-critical path, I did not perform
performance testing. :(

> 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.

Got it. Do you have any test cases that previously identified this
problem?

> 
> 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()?

Thank you for providing optimization ideas; I will take a closer look.