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

[Shakeel Butt]

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.