Re: [PATCH] mm: shmem: make unused huge shrinker memcg aware

[Andrew Morton]

On Tue, 14 Jul 2026 11:19:18 +0800 Qi Zheng <qi.zheng@xxxxxxxxx> wrote:

> From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> 
> The shmem unused huge shrinker keeps a per-superblock list of inodes whose
> tail huge folio extends beyond i_size. Since that list is not memcg aware,
> reclaim triggered by one memcg can scan inodes from the whole superblock
> and split shmem huge folios charged to unrelated memcgs.
> 
> Convert the shrink list to a memcg-aware list_lru. Queue each inode on the
> list_lru sublist matching the memcg and node of the current tail huge
> folio, so non-root memcg reclaim only walks candidates charged to the
> reclaiming memcg. Global reclaim, root memcg reclaim and shmem quota
> reclaim keep global semantics.
> 
> The list_lru still tracks inodes while the actual split target is the
> current tail huge folio, so validate the folio memcg/node during scan. If
> the folio no longer matches the reclaim context or splitting cannot
> proceed, requeue the inode according to the current tail folio; if the
> inode is no longer shrinkable, drop the scan entry.
> 
> This can be tested with the shrinker debugfs interface by allocating 32
> tmpfs tail THPs in each of two memcgs, then scanning the sb-tmpfs shrinker
> with memcg A's cgroup id:
>
>                before A scan        after A scan
>   base         A=64M, B=64M         A=0,  B=0
>   patched      A=64M, B=64M         A=0,  B=64M

Sashiko said a thing:
	https://sashiko.dev/#/patchset/20260714031918.308-1-qi.zheng@xxxxxxxxx

> This patch is based on next-20260701 because it doesn't include this patch:
> 
> https://lore.kernel.org/all/20260609123047.1948242-1-usama.arif@xxxxxxxxx/
>
> Later on, Usama will consider moving this restriction down into the fs callback.

I'm not understanding.  Was Usama's above patch ever included in
anything?

> -	unsigned long shrinklist_len; /* Length of shrinklist */
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	struct list_lru shrinklist; /* List of shrinkable inodes */
> +#endif
>  	struct shmem_quota_limits qlimits; /* Default quota limits */
>  	struct simple_xattr_cache xa_cache;
>  };
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b06c1ae2f50c3..a17150e6107c6 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -724,50 +724,252 @@ static const char *shmem_format_huge(int huge)
>  }
>  #endif
>  
> +static inline struct mem_cgroup *shmem_get_and_clear_memcg(struct shmem_inode_info *info)
> +{
> +	struct mem_cgroup *memcg = info->shrinklist_memcg;
> +
> +	info->shrinklist_memcg = NULL;
> +	info->shrinklist_nid = -1;
> +
> +	return memcg;
> +}

Explicit inlining shouldn't be needed - gcc will figure it out.

> +
> +static void shmem_unused_huge_add(struct inode *inode, struct folio *folio,
> +				  gfp_t gfp)
> +{
> +	struct shmem_inode_info *info = SHMEM_I(inode);
> +	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
> +	int nid = folio_nid(folio);
> +	struct mem_cgroup *memcg = NULL, *old_memcg = NULL;
> +
> +	memcg = shmem_unused_huge_alloc_lru(sbinfo, folio, gfp);
> +	if (IS_ERR(memcg))
> +		return;
> +
> +	spin_lock(&info->lock);
> +	if (!list_empty(&info->shrinklist)) {
> +		if (info->shrinklist_nid == nid &&
> +		    info->shrinklist_memcg == memcg)
> +			goto unlock;
> +
> +		list_lru_del(&sbinfo->shrinklist, &info->shrinklist,
> +			     info->shrinklist_nid, info->shrinklist_memcg);
> +		old_memcg = shmem_get_and_clear_memcg(info);
> +	}
> +
> +	list_lru_add(&sbinfo->shrinklist, &info->shrinklist, nid, memcg);
> +	info->shrinklist_memcg = memcg;
> +	info->shrinklist_nid = nid;

Totally pointless style thing: it's nice to fully initialize the object
before adding it to the list.

> +	memcg = NULL;
> +unlock:
> +	spin_unlock(&info->lock);
> +	mem_cgroup_put(old_memcg);
> +	mem_cgroup_put(memcg);
> +}
>
> ..,
>
> +static enum lru_status shmem_unused_huge_isolate(struct list_head *item,
> +						 struct list_lru_one *lru,
> +						 void *arg)
> +{
> +	struct shmem_unused_huge_scan *scan = arg;
> +	struct shmem_inode_info *info;
> +	struct inode *inode;
> +	struct mem_cgroup *memcg = NULL;
> +
> +	info = list_entry(item, struct shmem_inode_info, shrinklist);
> +	if (!spin_trylock(&info->lock))
> +		return LRU_SKIP;

Why the trylock?  Please add comment explaining its use.


> +	if (info->shrinklist_isolated) {
> +		spin_unlock(&info->lock);
> +		return LRU_SKIP;
> +	}
> +
> +	inode = igrab(&info->vfs_inode);
> +	if (!inode) {
> +		/*
> +		 * This inode is being evicted, just drop its stale active LRU
> +		 * entry.
> +		 */
> +		list_lru_isolate(lru, item);
> +		memcg = shmem_get_and_clear_memcg(info);
> +		spin_unlock(&info->lock);
> +		mem_cgroup_put(memcg);
> +		return LRU_REMOVED;
> +	}
> +
> +	info->shrinklist_isolated = true;
> +	list_lru_isolate(lru, item);
> +	memcg = shmem_get_and_clear_memcg(info);
> +	list_add_tail(&info->shrinklist_scan, &scan->list);
> +	spin_unlock(&info->lock);
> +	mem_cgroup_put(memcg);
> +
> +	return LRU_REMOVED;
> +}
> +
> +static bool is_shmem_unused_huge_readded(struct inode *inode)
> +{
> +	struct shmem_inode_info *info = SHMEM_I(inode);
> +	bool readded;
> +
> +	spin_lock(&info->lock);
> +	readded = info->shrinklist_isolated && !list_empty(&info->shrinklist);
> +	spin_unlock(&info->lock);
> +
> +	return readded;
> +}

The return value is out of date as soon as the lock is dropped?  A
comment explaining why this is OK would be helpful.

> +static bool is_shmem_unused_huge_match(struct folio *folio,
> +				       struct shrink_control *sc)
> +{
> +	struct mem_cgroup *memcg = NULL;
> +	bool match;
> +
> +	/*
> +	 * Only non-root memcg reclaim needs to match the folio charge against
> +	 * sc->memcg. Skip the folio memcg check for the following cases:
> +	 * 1. shmem quota reclaim (sc == NULL)
> +	 * 2. global shrinker reclaim
> +	 * 3. root memcg reclaim
> +	 */
> +	if (!sc || !sc->memcg || mem_cgroup_is_root(sc->memcg))
> +		return true;
> +
> +	if (folio_nid(folio) != sc->nid)
> +		return false;
> +
> +	memcg = get_mem_cgroup_from_folio(folio);
> +	match = memcg == sc->memcg;
> +	mem_cgroup_put(memcg);
> +
> +	return match;
> +}
>
> ...
>

Generally speaking, the code seems a bit thin on comments explaining
why functions exist, why the code is doing a partucular thing, etc.