From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-124.freemail.mail.aliyun.com (out30-124.freemail.mail.aliyun.com [115.124.30.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C970D72623 for ; Wed, 29 Jul 2026 03:30:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=115.124.30.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785295808; cv=none; b=La5rr9X2yPf+LZ7LFFjJL9lXsi8P6HRnhLF75USW9yovcpVkadqSzWfCrMiIbar3kTGWekJCLjwiUzJv3eS6LtKFwQEIsBYpHZhyKw9znCXzXJqi9sXXcRB/7nb0l+NfW4WmyKeffdAlfM3gTepfHcE0GktRypLvhukItSMLjho= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785295808; c=relaxed/simple; bh=9/+V2+4NzHp/hcNIbricgrpkzUD2w6AweDVc6vZguNY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=eNCStoYXc/li1tUzoPSgsXxBS0goKB9TspCQ4bDfnnaPtpuEF2sbcZ4nwpaVb9nwkJZiNDL9Si+x8geaegsh+dC55ouPXr0M+ftCTW9frw9jwCz+DGfXj/jo7jwY26XEJa1RrhRWjdov0fT0p0Vo01+KzEZbXQ+YA4tXRa7eO6A= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com; spf=pass smtp.mailfrom=linux.alibaba.com; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b=Ct6S3ir3; arc=none smtp.client-ip=115.124.30.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b="Ct6S3ir3" DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1785295795; h=Message-ID:Date:MIME-Version:Subject:To:From:Content-Type; bh=YhbXbLaVW+Q38TBhdKjf7H1d46R7yaH2q6zPCnlMtVA=; b=Ct6S3ir37REFbOivGOBCcFWBs65hG0pY3Nvf4kp8L74Kn16+F3dsvzMStdTCVRCBRqUNG2jqPFv/MURqbZSpL2YK2PoBGym2W+tf0A6vY1JR5cq3BpsKKdDIe6WJp3u9S8fNZBxOxaYuKblXG9/Az0htDVFXqDlpv1f6lQ8yvQg= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R641e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=maildocker-contentspam033045133197;MF=baolin.wang@linux.alibaba.com;NM=1;PH=DS;RN=7;SR=0;TI=SMTPD_---0X80oAqF_1785295793; Received: from 30.74.144.130(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0X80oAqF_1785295793 cluster:ay36) by smtp.aliyun-inc.com; Wed, 29 Jul 2026 11:29:54 +0800 Message-ID: Date: Wed, 29 Jul 2026 11:29:53 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] mm: shmem: make unused huge shrinker memcg aware To: Qi Zheng , hughd@google.com, akpm@linux-foundation.org, usama.arif@linux.dev Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Qi Zheng References: <889581093179462979dbfb5458620fcca531787b.1784621804.git.zhengqi.arch@bytedance.com> <313c4dc7-9d03-40da-b6a0-170a774010b3@linux.alibaba.com> From: Baolin Wang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 7/28/26 3:12 PM, Qi Zheng wrote: > Hi Baolin, > > On 7/27/26 1:11 PM, Baolin Wang wrote: >> Hi Qi, >> >> (Sorry for the late reply due to my vacation) > > No worries, and thank you for your response! > >> >> On 7/21/26 4:34 PM, Qi Zheng wrote: >>> From: Qi Zheng >>> >>> 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 >>> --- >>>   include/linux/shmem_fs.h |  12 +- >>>   mm/shmem.c               | 378 +++++++++++++++++++++++++++++---------- >>>   2 files changed, 294 insertions(+), 96 deletions(-) >>> >>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h >>> index 5663dff53186e..aeb59901a840c 100644 >>> --- a/include/linux/shmem_fs.h >>> +++ b/include/linux/shmem_fs.h >>> @@ -11,6 +11,7 @@ >>>   #include >>>   #include >>>   #include >>> +#include >>>   /* inode in-kernel data */ >>> @@ -54,6 +55,11 @@ struct shmem_inode_info { >>>       struct dquot __rcu    *i_dquot[MAXQUOTAS]; >>>   #endif >>>       struct inode        vfs_inode; >>> + >>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE >>> +    struct mem_cgroup    *shrinklist_memcg; >>> +    int            shrinklist_nid; >>> +#endif >>>   }; >> >> Thanks. This looks better now.:) Some comments below. >> >>>   #define SHMEM_FL_USER_VISIBLE        (FS_FL_USER_VISIBLE | >>> FS_CASEFOLD_FL) >>> @@ -83,9 +89,9 @@ struct shmem_sb_info { >>>       ino_t next_ino;            /* The next per-sb inode number to >>> use */ >>>       ino_t __percpu *ino_batch;  /* The next per-cpu inode number to >>> use */ >>>       struct mempolicy *mpol;     /* default memory policy for >>> mappings */ >>> -    spinlock_t shrinklist_lock;   /* Protects shrinklist */ >>> -    struct list_head shrinklist;  /* List of shinkable inodes */ >>> -    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 20c24d92da430..8bf6dd850f85b 100644 >>> --- a/mm/shmem.c >>> +++ b/mm/shmem.c >>> @@ -724,51 +724,242 @@ static const char *shmem_format_huge(int huge) >>>   } >>>   #endif >>> +static bool is_shmem_unused_huge_isolated(struct shmem_inode_info >>> *info) >>> +{ >>> + >>> +    return info->shrinklist_nid == -1; >>> +} >>> + >>> +static void set_shmem_unused_huge_isolated(struct shmem_inode_info >>> *info) >>> +{ >>> +    info->shrinklist_nid = -1; >>> +} >>> + >>> +static struct mem_cgroup *shmem_get_and_clear_memcg(struct >>> shmem_inode_info *info) >>> +{ >>> +    struct mem_cgroup *memcg = info->shrinklist_memcg; >>> + >>> +    info->shrinklist_memcg = NULL; >>> + >>> +    return memcg; >>> +} >>> + >>> +#ifdef CONFIG_MEMCG >>> +static struct mem_cgroup * >>> +shmem_unused_huge_alloc_lru(struct shmem_sb_info *sbinfo, struct >>> folio *folio, >>> +                gfp_t gfp) >>> +{ >>> +    struct mem_cgroup *memcg; >>> +    int ret; >>> + >>> +    memcg = get_mem_cgroup_from_folio(folio); >>> +    if (!memcg) >>> +        return NULL; >> >> It looks like we should return an error here instead of NULL? since >> callers use IS_ERR() to check the return value of this function. > > Returning NULL here is intentional - it is not an error case. > > The get_mem_cgroup_from_folio() returns NULL only when > mem_cgroup_disabled() is true. In that case the list_lru is not > memcg-aware, so use the global (root) list: > > list_lru_add() /* memcg == NULL */ > --> lock_list_lru_of_memcg() >     --> list_lru_from_memcg_idx() /* memcg_kmem_id(NULL) == -1 */ >         --> return &lru->node[nid].lru OK. I see. Thanks. [snip] >>>   static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info >>> *sbinfo, >>>           struct shrink_control *sc, unsigned long nr_to_free) >>>   { >>> -    LIST_HEAD(list), *pos, *next; >>> +    struct shmem_unused_huge_scan scan; >>>       struct inode *inode; >>>       struct shmem_inode_info *info; >>>       struct folio *folio; >>> -    unsigned long batch = sc ? sc->nr_to_scan : 128; >>> +    struct list_head *pos, *next; >>>       unsigned long split = 0, freed = 0; >>> -    if (list_empty(&sbinfo->shrinklist)) >>> -        return SHRINK_STOP; >>> - >>> -    spin_lock(&sbinfo->shrinklist_lock); >>> -    list_for_each_safe(pos, next, &sbinfo->shrinklist) { >>> -        info = list_entry(pos, struct shmem_inode_info, shrinklist); >>> - >>> -        /* pin the inode */ >>> -        inode = igrab(&info->vfs_inode); >>> - >>> -        /* inode is about to be evicted */ >>> -        if (!inode) { >>> -            list_del_init(&info->shrinklist); >>> -            goto next; >>> -        } >>> - >>> -        list_move(&info->shrinklist, &list); >>> -next: >>> -        sbinfo->shrinklist_len--; >>> -        if (!--batch) >>> -            break; >>> -    } >>> -    spin_unlock(&sbinfo->shrinklist_lock); >>> +    INIT_LIST_HEAD(&scan.list); >>> +    scan.sc = sc; >>> +    if (sc) >>> +        list_lru_shrink_walk(&sbinfo->shrinklist, sc, >>> +                     shmem_unused_huge_isolate, &scan); >>> +    else >>> +        list_lru_walk(&sbinfo->shrinklist, shmem_unused_huge_isolate, >>> +                  &scan, 128); >>> -    list_for_each_safe(pos, next, &list) { >>> -        pgoff_t next, end; >>> +    list_for_each_safe(pos, next, &scan.list) { >>> +        pgoff_t folio_end, end; >>>           loff_t i_size; >>>           int ret; >>>           info = list_entry(pos, struct shmem_inode_info, shrinklist); >>>           inode = &info->vfs_inode; >>> -        if (nr_to_free && freed >= nr_to_free) >>> -            goto move_back; >>> - >> >> Why move this logic? If this fixes anything, then a separate fix patch >> would be clearer. > > After converting the shmem huge shrinker to be memcg-aware, we need to > acquire the folio first to determine which sublist it should be returned > to. OK. Now the requeue needs to acquire the folio. >>>           i_size = i_size_read(inode); >>>           folio = filemap_get_entry(inode->i_mapping, i_size / >>> PAGE_SIZE); >>>           if (!folio || xa_is_value(folio)) >>> @@ -781,13 +972,25 @@ static unsigned long >>> shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo, >>>           } >>>           /* Check if there is anything to gain from splitting */ >>> -        next = folio_next_index(folio); >>> +        folio_end = folio_next_index(folio); >>>           end = shmem_fallocend(inode, DIV_ROUND_UP(i_size, PAGE_SIZE)); >>> -        if (end <= folio->index || end >= next) { >>> +        if (end <= folio->index || end >= folio_end) { >>>               folio_put(folio); >>>               goto drop; >>>           } >>> +        if (!is_shmem_unused_huge_match(folio, scan.sc)) { >>> +            shmem_unused_huge_requeue(inode, folio); >>> +            folio_put(folio); >> >> Maybe keeping the 'move_back' label would be clearer? since you've >> added several places to requeue the inode. > > Agree, will do. > >> >>> +            goto put; >>> +        } >>> + >>> +        if (nr_to_free && freed >= nr_to_free) { >>> +            shmem_unused_huge_requeue(inode, folio); >>> +            folio_put(folio); >>> +            goto put; >>> +        } >>> + >>>           /* >>>            * Move the inode on the list back to shrinklist if we failed >>>            * to lock the page at this time. >>> @@ -796,34 +999,33 @@ static unsigned long >>> shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo, >>>            * reclaim path. >>>            */ >>>           if (!folio_trylock(folio)) { >>> +            shmem_unused_huge_requeue(inode, folio); >>> +            folio_put(folio); >>> +            goto put; >>> +        } >>> + >>> +        if (!is_shmem_unused_huge_match(folio, scan.sc)) { >>> +            folio_unlock(folio); >>> +            shmem_unused_huge_requeue(inode, folio); >>>               folio_put(folio); >>> -            goto move_back; >>> +            goto put; >>>           } >>>           ret = split_folio(folio); >>>           folio_unlock(folio); >>> -        folio_put(folio); >>>           /* If split failed move the inode on the list back to >>> shrinklist */ >>> -        if (ret) >>> -            goto move_back; >>> +        if (ret) { >>> +            shmem_unused_huge_requeue(inode, folio); >>> +            folio_put(folio); >>> +            goto put; >>> +        } >>> -        freed += next - end; >>> +        freed += folio_end - end; >>>           split++; >>> +        folio_put(folio); >>>   drop: >>> -        list_del_init(&info->shrinklist); >>> -        goto put; >>> -move_back: >>> -        /* >>> -         * Make sure the inode is either on the global list or deleted >>> -         * from any local list before iput() since it could be deleted >>> -         * in another thread once we put the inode (then the local list >>> -         * is corrupted). >>> -         */ >>> -        spin_lock(&sbinfo->shrinklist_lock); >>> -        list_move(&info->shrinklist, &sbinfo->shrinklist); >>> -        sbinfo->shrinklist_len++; >>> -        spin_unlock(&sbinfo->shrinklist_lock); >>> +        shmem_unused_huge_drop(inode); >>>   put: >>>           iput(inode); >>>       } >>> @@ -836,7 +1038,7 @@ static long shmem_unused_huge_scan(struct >>> super_block *sb, >>>   { >>>       struct shmem_sb_info *sbinfo = SHMEM_SB(sb); >>> -    if (!READ_ONCE(sbinfo->shrinklist_len)) >>> +    if (!list_lru_shrink_count(&sbinfo->shrinklist, sc)) >>>           return SHRINK_STOP; >>>       return shmem_unused_huge_shrink(sbinfo, sc, 0); >>> @@ -847,21 +1049,21 @@ static long shmem_unused_huge_count(struct >>> super_block *sb, >>>   { >>>       struct shmem_sb_info *sbinfo = SHMEM_SB(sb); >>> -    /* >>> -     * The per-superblock shrinklist is filesystem-global and does not >>> -     * honour sc->memcg, so it is only meaningful on the global >>> (kswapd or >>> -     * root direct reclaim) shrink path. Skip the per-memcg >>> iterations of >>> -     * shrink_slab_memcg() to avoid queueing duplicate global work. >>> -     */ >>> -    if (!mem_cgroup_shrink_is_root(sc)) >>> -        return 0; >>> - >>> -    return READ_ONCE(sbinfo->shrinklist_len); >>> +    return list_lru_shrink_count(&sbinfo->shrinklist, sc); >>>   } >>>   #else /* !CONFIG_TRANSPARENT_HUGEPAGE */ >>>   #define shmem_huge SHMEM_HUGE_DENY >>> +static void shmem_unused_huge_add(struct inode *inode, struct folio >>> *folio, >>> +                  gfp_t gfp) >>> +{ >>> +} >>> + >>> +static void shmem_unused_huge_del(struct inode *inode) >>> +{ >>> +} >>> + >>>   static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info >>> *sbinfo, >>>           struct shrink_control *sc, unsigned long nr_to_free) >>>   { >>> @@ -1417,14 +1619,7 @@ static void shmem_evict_inode(struct inode >>> *inode) >>>           inode->i_size = 0; >>>           mapping_set_exiting(inode->i_mapping); >>>           shmem_truncate_range(inode, 0, (loff_t)-1); >>> -        if (!list_empty(&info->shrinklist)) { >>> -            spin_lock(&sbinfo->shrinklist_lock); >>> -            if (!list_empty(&info->shrinklist)) { >>> -                list_del_init(&info->shrinklist); >>> -                sbinfo->shrinklist_len--; >>> -            } >>> -            spin_unlock(&sbinfo->shrinklist_lock); >>> -        } >>> +        shmem_unused_huge_del(inode); >>>           while (!list_empty(&info->swaplist)) { >>>               /* Wait while shmem_unuse() is scanning this inode... */ >>>               wait_var_event(&info->stop_eviction, >>> @@ -2532,28 +2727,6 @@ static int shmem_get_folio_gfp(struct inode >>> *inode, pgoff_t index, >>>   alloced: >>>       alloced = true; >>> -    if (folio_test_large(folio) && >>> -        DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE) < >>> -                    folio_next_index(folio)) { >>> -        struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb); >>> -        struct shmem_inode_info *info = SHMEM_I(inode); >>> -        /* >>> -         * Part of the large folio is beyond i_size: subject >>> -         * to shrink under memory pressure. >>> -         */ >>> -        spin_lock(&sbinfo->shrinklist_lock); >>> -        /* >>> -         * _careful to defend against unlocked access to >>> -         * ->shrink_list in shmem_unused_huge_shrink() >>> -         */ >>> -        if (list_empty_careful(&info->shrinklist)) { >>> -            list_add_tail(&info->shrinklist, >>> -                      &sbinfo->shrinklist); >>> -            sbinfo->shrinklist_len++; >>> -        } >>> -        spin_unlock(&sbinfo->shrinklist_lock); >>> -    } >> >> Why move this additional logic down? > > Because under the following condition: > >         if (sgp <= SGP_CACHE && >         ((loff_t)index << PAGE_SHIFT) >= i_size_read(inode)) { >         error = -EINVAL; >         goto unlock; >     } > > we will jump to unlock and remove the folio from page cache. > > With the new memcg-aware design, shmem_unused_huge_add() takes a memcg > reference. If the folio is then removed by the error path, the inode > sits on the shrinker list holding a stale memcg reference and pointing > at an i_size that no longer matches the folio. > > So we should ensure the inode is only queued when the folio is fully set > up and about to be returned successfully. Right. But I think this deserves a separate preparation patch with above explanation, moving the original shrinklist addition logic to after all checks are completed.