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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 7/15/26 5:55 PM, Qi Zheng wrote:
> Hi Baolin,

Sorry for late reply.

> On 7/15/26 5:37 PM, Baolin Wang wrote:
>> Hi Qi,
>>
>> On 7/14/26 11:19 AM, Qi Zheng 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
>>>
>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>>> ---
>>
>> Thanks for your patch. This work has been on my TODO list for a while :)
>>
>> Overall it looks good. I'll take a closer look and run some tests.
> 
> Thanks!
> 
>>
>>> 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.
>>>
>>>   include/linux/shmem_fs.h |  14 +-
>>>   mm/shmem.c               | 379 ++++++++++++++++++++++++++++++---------
>>>   2 files changed, 310 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>>> index e729b9b0e38d4..0b14de1e890a9 100644
>>> --- a/include/linux/shmem_fs.h
>>> +++ b/include/linux/shmem_fs.h
>>> @@ -11,6 +11,7 @@
>>>   #include <linux/fs_parser.h>
>>>   #include <linux/userfaultfd_k.h>
>>>   #include <linux/bits.h>
>>> +#include <linux/list_lru.h>
>>>   struct swap_iocb;
>>> @@ -56,6 +57,13 @@ struct shmem_inode_info {
>>>       struct dquot __rcu    *i_dquot[MAXQUOTAS];
>>>   #endif
>>>       struct inode        vfs_inode;
>>> +
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>> +    struct list_head    shrinklist_scan;
>>
>> Why introduce another list_head member? Why can't we reuse 
>> 'shrinklist' in this structure? In other words, if we isolate this 
>> inode from list_lru, then 'inode_info->shrinklist' can be reused to 
>> link it into the temporary list, right?
> 
> After list_lru isolation, the shrinker drops the list_lru lock and
> processes the inode on a local scan list. During that time, shmem
> faults can make the inode's current tail huge folio change, and
> shmem_unused_huge_add() may need to queue the same inode again on
> the active list_lru, possibly under a different memcg/node sublist.
> 
> If we reused info->shrinklist for the temporary scan list, then while 
> the inode is being scanned:
> 
> 1. info->shrinklist would be non-empty because it is on the local scan
>     list, so shmem_unused_huge_add() could not tell that the inode is no
>     longer on the active list_lru.
> 2. Re-adding it to list_lru with the same list_head would corrupt the
>     local scan list.

Right. Just some preliminary thoughts:

If the current inode needs to be re-added to the list_lru, it means the 
inode's i_size has changed. If 'info->shrinklist' is on the temporary 
list at this point, we can follow the same logic as the original 
'info->shrinklist' handling and simply skip the re-adding to list_lru. 
Instead, we can let shmem_unused_huge_shrink() get the end folio via 
filemap_get_entry(i_size), and then check the memcg and nid — if they 
differ from the 'sc', we can re-add the inode back to the list_lru in 
shmem_unused_huge_shrink().

Of course, there may be other races that need to be considered as well.

In any case, we should try to avoid adding too many members to the 
'shmem_inode_info' struct, since when there are many tmpfs files this 
could introduce noticeable memory overhead.

> 3. Skipping the add would lose the new current tail huge folio if the
>     scan later drops the old scan entry instead of requeueing it.
> 
> So isolation removes the inode from the active list_lru and leaves
> info->shrinklist empty. A concurrent fault can safely add the inode back
> to the active list_lru with the current folio's memcg/node, while the
> shrinker still owns the old scan entry through shrinklist_scan. The scan
> loop then detects that re-add and drops the stale scan entry, leaving
> the active entry for a later scan.
> 
>>
>>> +    struct mem_cgroup    *shrinklist_memcg;
>>> +    int            shrinklist_nid;
>>> +    bool            shrinklist_isolated;
>>
>> Do we need an extra bool to indicate whether it's isolated? Can't we 
>> just use 'list_empty(inode_info->shrinklist)'? I may have missed some 
>> details.
> 
> list_empty(info->shrinklist) cannot be used for this. That list node 
> tracks active list_lru membership only. But you are right that the
> explicit bool is not necessary, we can use list_empty(&info- 
>  >shrinklist_scan) instead.
> 
> Thanks,
> Qi
> 
>