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

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

On 3/3/26 10:56 PM, Yosry Ahmed wrote:
> On Tue, Mar 03, 2026 at 11:08:56AM +0800, Qi Zheng wrote:
>> Hi Yosry,
> [..]
>>>
>>> I don't think we should end up with two copies of
>>> __mod_memcg_state/mod_memcg_state() and
>>> __mod_memcg_lruvec_state/mod_memcg_lruvec_state(). I meant to refactor
>>> mod_memcg_state() to call __mod_memcg_state(), where the latter does
>>> not call get_non_dying_memcg_{start/end}(). Same for
>>> mod_memcg_lruvec_state().
>>
>> Okay, like the following? But this would require modifications to
>> [PATCH v5 31/32]. If there are no problems, I will send the updated
>> patch to [PATCH v5 29/32] and [PATCH v5 31/32].
> 
> I cannot apply the diff, seems a bit corrupted.
> 
> But ideally, instead of a @reparent argument, we just have
> __mod_memcg_lruvec_state() and __mod_memcg_state() do the work without
> getting parent of dead memcgs, and then mod_memcg_lruvec_state() and
> mod_memcg_state() just call them after get_non_dying_memcg_start().
> 
> What about this (untested), it should apply on top of 'mm: memcontrol:
> eliminate the problem of dying memory cgroup for LRU folios' in mm-new,
> so maybe it needs to be broken down across different patches:

It looks feasible. I will apply and test it. If there are no problems,
I will send the update patch.

Thanks,
Qi

> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 753d76e96cc67..f0d55e1f9c49a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -527,7 +527,7 @@ unsigned long lruvec_page_state_local(struct lruvec *lruvec,
>   }
>   
>   #ifdef CONFIG_MEMCG_V1
> -static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
> +static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
>   				     enum node_stat_item idx, int val);
>   
>   void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
> @@ -536,16 +536,17 @@ void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
>   	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));
>   		unsigned long value = lruvec_page_state_local(child_lruvec, idx);
> +		struct mem_cgroup_per_node *child_pn, *parent_pn;
>   
> -		__mod_memcg_lruvec_state(child_lruvec, idx, -value);
> -		__mod_memcg_lruvec_state(parent_lruvec, idx, value);
> +		child_pn = container_of(child_lruvec, struct mem_cgroup_per_node, lruvec);
> +		parent_pn = container_of(parent_lruvec, struct mem_cgroup_per_node, lruvec);
> +
> +		__mod_memcg_lruvec_state(child_pn, idx, -value);
> +		__mod_memcg_lruvec_state(parent_pn, idx, value);
>   	}
>   }
>   #endif
> @@ -831,39 +832,42 @@ static inline void get_non_dying_memcg_end(void)
>   }
>   #endif
>   
> -/**
> - * mod_memcg_state - update cgroup memory statistics
> - * @memcg: the memory cgroup
> - * @idx: the stat item - can be enum memcg_stat_item or enum node_stat_item
> - * @val: delta to add to the counter, can be negative
> - */
> -void mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
> -		       int val)
> +static void __mod_memcg_state(struct mem_cgroup *memcg,
> +			      enum memcg_stat_item idx, int val)
>   {
>   	int i = memcg_stats_index(idx);
>   	int cpu;
>   
> -	if (mem_cgroup_disabled())
> -		return;
> -
>   	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
>   		return;
>   
>   	cpu = get_cpu();
>   
> -	memcg = get_non_dying_memcg_start(memcg);
> -
>   	this_cpu_add(memcg->vmstats_percpu->state[i], val);
>   	val = memcg_state_val_in_pages(idx, val);
>   	memcg_rstat_updated(memcg, val, cpu);
> -
> -	get_non_dying_memcg_end();
> -
>   	trace_mod_memcg_state(memcg, idx, val);
>   
>   	put_cpu();
>   }
>   
> +/**
> + * mod_memcg_state - update cgroup memory statistics
> + * @memcg: the memory cgroup
> + * @idx: the stat item - can be enum memcg_stat_item or enum node_stat_item
> + * @val: delta to add to the counter, can be negative
> + */
> +void mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
> +		     int val)
> +{
> +	if (mem_cgroup_disabled())
> +		return;
> +
> +	memcg = get_non_dying_memcg_start(memcg);
> +	__mod_memcg_state(memcg, idx, val);
> +	get_non_dying_memcg_end();
> +}
> +
>   #ifdef CONFIG_MEMCG_V1
>   /* idx can be of type enum memcg_stat_item or node_stat_item. */
>   unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
> @@ -882,35 +886,26 @@ unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
>   	return x;
>   }
>   
> -static void __mod_memcg_state(struct mem_cgroup *memcg,
> -			      enum memcg_stat_item idx, int val)
> +void reparent_memcg_state_local(struct mem_cgroup *memcg,
> +				struct mem_cgroup *parent, int idx)
>   {
>   	int i = memcg_stats_index(idx);
> -	int cpu;
> -
> -	if (mem_cgroup_disabled())
> -		return;
> -
> -	cpu = get_cpu();
> -
> -	this_cpu_add(memcg->vmstats_percpu->state[i], val);
> -	val = memcg_state_val_in_pages(idx, val);
> -	memcg_rstat_updated(memcg, val, cpu);
> -	trace_mod_memcg_state(memcg, idx, val);
> +	unsigned long value = memcg_page_state_local(memcg, idx);
>   
> -	put_cpu();
> +	__mod_memcg_state(memcg, idx, -value);
> +	__mod_memcg_state(parent, idx, value);
>   }
> +#endif
>   
> -static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
> +static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
>   				     enum node_stat_item idx, int val)
>   {
> -	struct mem_cgroup_per_node *pn;
> -	struct mem_cgroup *memcg;
> +	struct mem_cgroup *memcg = pn->memcg;
>   	int i = memcg_stats_index(idx);
>   	int cpu;
>   
> -	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> -	memcg = pn->memcg;
> +	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
> +		return;
>   
>   	cpu = get_cpu();
>   
> @@ -927,20 +922,6 @@ static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
>   	put_cpu();
>   }
>   
> -void reparent_memcg_state_local(struct mem_cgroup *memcg,
> -				struct mem_cgroup *parent, int idx)
> -{
> -	int i = memcg_stats_index(idx);
> -	unsigned long value = memcg_page_state_local(memcg, idx);
> -
> -	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
> -		return;
> -
> -	__mod_memcg_state(memcg, idx, -value);
> -	__mod_memcg_state(parent, idx, value);
> -}
> -#endif
> -
>   static void mod_memcg_lruvec_state(struct lruvec *lruvec,
>   				     enum node_stat_item idx,
>   				     int val)
> @@ -948,32 +929,13 @@ static void mod_memcg_lruvec_state(struct lruvec *lruvec,
>   	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>   	struct mem_cgroup_per_node *pn;
>   	struct mem_cgroup *memcg;
> -	int i = memcg_stats_index(idx);
> -	int cpu;
> -
> -	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
> -		return;
>   
>   	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> -	memcg = pn->memcg;
> -
> -	cpu = get_cpu();
> -
> -	memcg = get_non_dying_memcg_start(memcg);
> +	memcg = get_non_dying_memcg_start(pn->memcg);
>   	pn = memcg->nodeinfo[pgdat->node_id];
> -
> -	/* Update memcg */
> -	this_cpu_add(memcg->vmstats_percpu->state[i], val);
> -	/* Update lruvec */
> -	this_cpu_add(pn->lruvec_stats_percpu->state[i], val);
> -	val = memcg_state_val_in_pages(idx, val);
> -	memcg_rstat_updated(memcg, val, cpu);
> -
> +	__mod_memcg_lruvec_state(pn, idx, val);
>   	get_non_dying_memcg_end();
>   
> -	trace_mod_memcg_lruvec_state(memcg, idx, val);
> -
> -	put_cpu();
>   }
>   
>   /