Re: [PATCH 3/4] memcg: manipulate memcg private ID references by ID

[Bingfang Guo]

On Fri, Sep 18, 2026 at 11:14:03AM +0800, Shakeel Butt wrote:
> On Fri, Sep 18, 2026 at 05:18:42PM +0800, Bingfang Guo via B4 Relay wrote:
> > From: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
> > 
> > This is a preparatory work for moving memcgid from memcg to objcg.
> > 
> > Swap entries retain a private ID rather than a memcg pointer. Once
> > private ID references are moved to objcgs, the ID can also outlive the
> > memcg to which it was originally assigned.  So it's better to make the
> > get and put functions accept the ID itself instead of the memcg.
> > 
> > Rename mem_cgroup_private_id_get_online() to
> > mem_cgroup_private_id_get(), and make it return the ID only.  If the
> > memcg is already dying, the dying memcg will still be used for charging
> > and stats accounting in v2 swap charging path. But they are hierarchical
> > and will be reparented after offlining so it doesn't matter.
> > 
> > Make mem_cgroup_private_id_put() take the ID and resolve the reference
> > holder internally. Convert swap uncharge and charge rollback to release
> > the reference using that ID.  This introduces an extra xarray lookup for
> > now, which will be removed in the final patch.
> > 
> > Separate the online-state reference release into
> > mem_cgroup_private_id_kill(). The offline path already has the memcg
> > pointer and can call the underlying put helper directly.
> > 
> > Signed-off-by: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
> > ---
> >  mm/memcontrol-v1.c |  7 +++----
> >  mm/memcontrol-v1.h |  3 +--
> >  mm/memcontrol.c    | 32 +++++++++++++++++++++++---------
> >  3 files changed, 27 insertions(+), 15 deletions(-)
> > 
> > diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> > index ed015fdd95123..b7f2868885071 100644
> > --- a/mm/memcontrol-v1.c
> > +++ b/mm/memcontrol-v1.c
> > @@ -268,7 +268,7 @@ void memcg1_commit_charge(struct folio *folio, struct mem_cgroup *memcg)
> >   */
> >  void __memcg1_swapout(struct folio *folio, struct swap_cluster_info *ci)
> >  {
> > -	struct mem_cgroup *memcg, *swap_memcg;
> > +	struct mem_cgroup *memcg;
> >  	struct obj_cgroup *objcg;
> >  	unsigned int nr_entries;
> >  	unsigned short private_id;
> > @@ -298,9 +298,8 @@ void __memcg1_swapout(struct folio *folio, struct swap_cluster_info *ci)
> >  	 * if the ID refers to the root memcg.
> >  	 */
> >  	nr_entries = folio_nr_pages(folio);
> > -	swap_memcg = mem_cgroup_private_id_get_online(memcg, nr_entries);
> > -	private_id = mem_cgroup_private_id(swap_memcg);
> > -	mod_memcg_state(swap_memcg, MEMCG_SWAP, nr_entries);
> > +	private_id = mem_cgroup_private_id_get(memcg, nr_entries);
> > +	mod_memcg_state(memcg, MEMCG_SWAP, nr_entries);
> >  
> >  	__swap_cgroup_set(ci, swp_cluster_offset(folio->swap), nr_entries, private_id);
> >  
> > diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
> > index 23be2512702dc..281425273ea97 100644
> > --- a/mm/memcontrol-v1.h
> > +++ b/mm/memcontrol-v1.h
> > @@ -27,8 +27,7 @@ static inline bool mem_cgroup_private_id_is_root(unsigned short id)
> >  	return id == mem_cgroup_private_id(root_mem_cgroup);
> >  }
> >  
> > -struct mem_cgroup *mem_cgroup_private_id_get_online(struct mem_cgroup *memcg,
> > -						    unsigned int n);
> > +unsigned short mem_cgroup_private_id_get(struct mem_cgroup *memcg, unsigned int n);
> >  
> >  void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
> >  				       struct mem_cgroup *parent, int idx);
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index bfe53e4392f09..ed44b3e7ac938 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -4082,7 +4082,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->private_id_ref)) {
> >  		mem_cgroup_private_id_remove(memcg);
> > @@ -4092,7 +4092,22 @@ static inline void mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned
> >  	}
> >  }
> >  
> > -struct mem_cgroup *mem_cgroup_private_id_get_online(struct mem_cgroup *memcg, unsigned int n)
> > +static inline void mem_cgroup_private_id_put(unsigned short id, unsigned int n)
> > +{
> > +	struct mem_cgroup *memcg;
> > +
> > +	rcu_read_lock();
> 
> Use lockdep_assert_in_rcu_read_lock() here instead of taking rcu as both callers
> already taking rcu read lock.
> 

Thanks for pointing out this.

Agreed.  Both two callers are already holding the rcu lock so
taking the lock here is unnecessary. So I will drop the
rcu_read_lock() and use that in the next version!

My concern is that: mem_cgroup_private_id_put() looks like a
universal put function, requiring rcu held (which is true today)
is not that obvious to the users.  So I think adding a short kdoc
comment to make it clear later might be a good idea.