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

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

Hi Baolin,

On 7/17/26 8:50 PM, Baolin Wang wrote:
> 
> 
> 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().

That also sounds workable.

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

I will look into it.

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

Got it.

Thanks,
Qi

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