Re: [PATCH v2 1/4] mm: memcontrol: drop kmemcg_id and use the memcg ID for list_lru indexing

[Johannes Weiner]

On Mon, Sep 07, 2026 at 07:01:08PM +0800, Qinyun Tan wrote:
> kmemcg_id is a copy of the memcg ID assigned in memcg_online_kmem(),
> and is only used as the list_lru xarray index.  With
> cgroup.memory=nokmem the assignment never happens, so every memcg
> resolves to the per-node lists.  The next patch needs the index to
> work under nokmem as well, so drop the copy and use the memcg ID.
> 
> The ID works just as well as the copy did: root and NULL still
> return -1 and use the per-node lists, and the ID is only released
> after the list_lru reparenting, so a stale or recycled ID can never
> reach a live list_lru entry.
> 
> The early return of memcg_offline_kmem() under nokmem is dropped as
> well, so the reparenting also covers lrus that stay memcg aware
> without kmem accounting.
> 
> Signed-off-by: Qinyun Tan <qinyuntan@xxxxxxxxxxxxxxxxx>
> ---
>  include/linux/memcontrol.h |  8 +++++---
>  mm/list_lru.c              | 10 +++++-----
>  mm/memcontrol.c            |  6 ------
>  3 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index fdf4812e1d818..edeb287978934 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -254,7 +254,6 @@ struct mem_cgroup {
>  #if BITS_PER_LONG < 64
>  	seqlock_t		socket_pressure_seqlock;
>  #endif
> -	int kmemcg_id;
>  
>  #ifdef CONFIG_CGROUP_WRITEBACK
>  	struct list_head cgwb_list;
> @@ -1775,12 +1774,15 @@ static inline void memcg_kmem_uncharge_page(struct page *page, int order)
>  }
>  
>  /*
> - * A helper for accessing memcg's kmem_id, used for getting
> + * A helper for accessing the memcg ID, used for getting
>   * corresponding LRU lists.
>   */
>  static inline int memcg_kmem_id(struct mem_cgroup *memcg)
>  {
> -	return memcg ? memcg->kmemcg_id : -1;
> +	if (!memcg || mem_cgroup_is_root(memcg))
> +		return -1;
> +
> +	return memcg->id.id;
>  }

This is a private ID with lifetime only guaranteed for online
groups. Reparenting happens right before it dies at offlining right
now, but this is not a great dependency to have.

Use mem_cgroup_id() instead and just get rid of that helper.

>  struct mem_cgroup *mem_cgroup_from_virt(void *p);
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index a4522ca93ebcb..6fd4e9af84396 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -502,7 +502,7 @@ static void memcg_reparent_list_lru_one(struct list_lru *lru, int nid,
>  					struct list_lru_one *src,
>  					struct mem_cgroup *dst_memcg)
>  {
> -	int dst_idx = dst_memcg->kmemcg_id;
> +	int dst_idx = memcg_kmem_id(dst_memcg);
>  	struct list_lru_one *dst;
>  
>  	spin_lock_irq(&src->lock);
> @@ -536,7 +536,7 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
>  		 * allocating a new mlru since CSS_DYING is already set for this
>  		 * memcg a rcu grace period ago.
>  		 */
> -		mlru = xa_load(&lru->xa, memcg->kmemcg_id);
> +		mlru = xa_load(&lru->xa, memcg_kmem_id(memcg));
>  		if (!mlru)
>  			continue;
>  
> @@ -551,7 +551,7 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
>  		for_each_node(i)
>  			memcg_reparent_list_lru_one(lru, i, &mlru->node[i], parent);
>  
> -		xa_erase_irq(&lru->xa, memcg->kmemcg_id);
> +		xa_erase_irq(&lru->xa, memcg_kmem_id(memcg));
>  
>  		/*
>  		 * Here all list_lrus corresponding to the cgroup are guaranteed
> @@ -566,7 +566,7 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
>  static inline bool memcg_list_lru_allocated(struct mem_cgroup *memcg,
>  					    struct list_lru *lru)
>  {
> -	int idx = memcg->kmemcg_id;
> +	int idx = memcg_kmem_id(memcg);
>  
>  	return idx < 0 || xa_load(&lru->xa, idx);
>  }
> @@ -602,7 +602,7 @@ static int __memcg_list_lru_alloc(struct mem_cgroup *memcg,
>  			if (!mlru)
>  				return -ENOMEM;
>  		}
> -		xas_set(&xas, pos->kmemcg_id);
> +		xas_set(&xas, memcg_kmem_id(pos));
>  		do {
>  			xas_lock_irqsave(&xas, flags);
>  			if (!xas_load(&xas) && !css_is_dying(&pos->css)) {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7ce50bccf1264..619d4c1f2e8f2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3780,17 +3780,12 @@ static void memcg_online_kmem(struct mem_cgroup *memcg)
>  		return;
>  
>  	static_branch_enable(&memcg_kmem_online_key);
> -
> -	memcg->kmemcg_id = memcg->id.id;
>  }
>  
>  static void memcg_offline_kmem(struct mem_cgroup *memcg)
>  {
>  	struct mem_cgroup *parent;
>  
> -	if (mem_cgroup_kmem_disabled())
> -		return;
> -
>  	if (unlikely(mem_cgroup_is_root(memcg)))
>  		return;

Both of these functions do very little now and the asymmetry you're
adding on the mem_cgroup_kmem_disabled() check looks odd. Please just
inline them into mem_cgroup_css_online()/offline():

onlining:
	if (!mem_cgroup_kmem_disabled() && likely(!mem_cgroup_is_root()))
		static_branch_enable(&memcg_kmem_online_key);

offlining:
	memcg_reparent_list_lrus(memcg, parent);

The root check is unnecessary because roots are not destroyed. But if
you'd rather not make that change here, keep the root check, and leave
its removal to a separate cleanup patch, that's fine too.