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

[Qi Zheng <qi.zheng@linux.dev>]

Hi Andrew,

On 7/15/26 12:32 PM, Andrew Morton wrote:
> On Tue, 14 Jul 2026 11:19:18 +0800 Qi Zheng <qi.zheng@linux.dev> wrote:
> 
>> From: Qi Zheng <zhengqi.arch@bytedance.com>
>>
>> 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@linux.dev

Got it. I noticed that too. shrinklist_memcg and shrinklist_isolated
require explicit initalization. I'll fix this in the next version once
the following patch is merged into linux-next:

https://lore.kernel.org/all/20260714101454.1202449-1-usama.arif@linux.dev/

> 
>> This patch is based on next-20260701 because it doesn't include this patch:
>>
>> https://lore.kernel.org/all/20260609123047.1948242-1-usama.arif@linux.dev/
>>
>> 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?

It's included in linux-next, e.g. next-20260710.

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

Got it.

> 
>> +
>> +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.

Got it.

> 
>> +	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.

This is to prevent an ABBA deadlock.

The shmem add/delete/requeue paths take info->lock first and then call
list_lru_add()/list_lru_del(), which acquire the list_lru lock. But the
current path takes the list_lru lock first, hence the use of trylock
here.

Will add a comment explaining this.


> 
> 
>> +	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.

Yes, the value is only a snapshot. The helper is a conservative race
detector for the case where the shrinker has an inode on the local scan
list and a concurrent fault has already queued the same inode back on
the active list_lru. If we observe that, the caller drops the scan entry
and lets the active list_lru entry represent the current tail folio.

Will add a comment explaining this.

> 
>> +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.

Will add more comments in the next version.

Thanks,
Qi

>