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

[Bingfang Guo <bfguo@icloud.com>]

On Sat, Sep 05, 2026 at 06:01:44PM +0800, Muchun Song wrote:
> 
> 
> 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.

Sure, I will take that.

Again, thanks for your suggestions and reviews! It's very
inspiring to me.

Regards,
Bingfang

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