Re: [PATCH RFC v2 3/6] mm/memcg: pass the id itself instead of memcg for putting ID

[Muchun Song <muchun.song@linux.dev>]

On 2026/9/1 16:58, Bingfang Guo via B4 Relay wrote:
> From: Bingfang Guo <bingfangguo@tencent.com>
>
> Swap uncharge knows the memcg only by its private id, and the id can
> outlive the memcg it used to belong to after we rebind memcgid to
> objcgs.  Make mem_cgroup_private_id_put() take the id and resolve the
> memcg containing the refcount internally, and keep the underlying
> __mem_cgroup_private_id_put() for the offline path that still holds a
> memcg pointer.
>
> In the uncharge path, the memcg pointer will have to be read from the
> xarray twice, but we'll fix that later by returning the memcg from the
> put path, so the uncharge path can obtain a reference in the same step.
>
> Signed-off-by: Bingfang Guo <bingfangguo@tencent.com>
> ---
>   mm/memcontrol.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ecb4fb07d7735..048c9bb0fad79 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4038,7 +4038,7 @@ static void mem_cgroup_private_id_remove(struct mem_cgroup *memcg)
>   	}
>   }
>   
> -static inline void mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned int n)
> +static void __mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned int n)
>   {
>   	if (refcount_sub_and_test(n, &memcg->id.ref)) {
>   		mem_cgroup_private_id_remove(memcg);
> @@ -4048,9 +4048,19 @@ static inline void mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned
>   	}
>   }
>   
> +static void mem_cgroup_private_id_put(unsigned short id, unsigned int n)
> +{
> +	struct mem_cgroup *memcg;
> +
> +	rcu_read_lock();
> +	memcg = mem_cgroup_from_private_id(id);
> +	__mem_cgroup_private_id_put(memcg, n);
> +	rcu_read_unlock();
> +}

To me, the direction of this change makes sense. At the same
time, I would also suggest applying a similar change to the get
function, for example: change mem_cgroup_private_id_get_online
to:

     unsigned short mem_cgroup_private_id_get(struct mem_cgroup *memcg,
                                              unsigned int n);

Have it return a private ID. That way, I think the two interfaces
look much more symmetrical overall.

Based on this change, it should be easier to implement the
direction of my last suggestion in Patch 5.

Muchun,
Thanks.

> +
>   static void mem_cgroup_private_id_kill(struct mem_cgroup *memcg)
>   {
> -	mem_cgroup_private_id_put(memcg, 1);
> +	__mem_cgroup_private_id_put(memcg, 1);
>   }
>   
>   struct mem_cgroup *mem_cgroup_private_id_get_online(struct mem_cgroup *memcg, unsigned int n)
> @@ -5815,9 +5825,10 @@ void __mem_cgroup_uncharge_swap(unsigned short id, unsigned int nr_pages)
>   				page_counter_uncharge(&memcg->swap, nr_pages);
>   		}
>   		mod_memcg_state(memcg, MEMCG_SWAP, -nr_pages);
> -		mem_cgroup_private_id_put(memcg, nr_pages);
> +		mem_cgroup_private_id_put(id, nr_pages);
>   	}
>   	rcu_read_unlock();
> +
>   }
>   
>   long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)
>