From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-177.mta0.migadu.com (out-177.mta0.migadu.com [91.218.175.177]) (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 046F521CC71 for ; Wed, 15 Jul 2026 06:49:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784098179; cv=none; b=WS51bIhdt+0a7EmuT+XzlVhfdXY7Nb6t0+CAR3QHFgEdvBdli2Jcvfe68JiytUjzTyyPu4kjssWhuoyYXGaSEyrjhrZBScKr8C+ykfpC+OIoGfttSajkiXBuXPZ3NROgqHq3SkIqZRqEXN1M+3yUxj4Bd6WrLwas/fSI8KmqXYY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784098179; c=relaxed/simple; bh=oFJr13a8TZ/6lEBcypNo2NG6XLzSvSiYb/yK8muCPbA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ofZ9PdWYDnwFmdKdvw+66vr2H64C4RAT9eAiazOcfve7VRsz4lDeocFwmoqF98YGs4XZHhCsCjwY5IrixPViqwKokJn5CVDEA/6PRioOsiv9d2JIKhcr7elyAZOwaCyNDHSoTTzp0ClnLy42kxm7plPtTw53O7bPup/gRoTTd8E= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=s1gdms2L; arc=none smtp.client-ip=91.218.175.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="s1gdms2L" Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1784098175; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VTiWiCvkaxhWbbTXxutJ9/xRtBOZlE2lyth+xPWUM7k=; b=s1gdms2LzEnGLQ2nRil8fpx1bBzcxflCDRf1EhGxkQHJWQPfkuGWxSPC9ZU785f0Rnc4eC ECt/gLxl99TTA1iJT3QNbuaC5cwlVSvu8xM10MmEi41lx0FXaUdHSk1YVaqRHQshZqdNOp /U5eBnVwMbKyrYzT49UeVY3fmbuMmUc= Date: Wed, 15 Jul 2026 14:49:28 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH] mm: shmem: make unused huge shrinker memcg aware To: Andrew Morton Cc: hughd@google.com, baolin.wang@linux.alibaba.com, usama.arif@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Qi Zheng References: <20260714031918.308-1-qi.zheng@linux.dev> <20260714213223.9a03d78f4aba252c512e01b9@linux-foundation.org> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Qi Zheng In-Reply-To: <20260714213223.9a03d78f4aba252c512e01b9@linux-foundation.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT Hi Andrew, On 7/15/26 12:32 PM, Andrew Morton wrote: > On Tue, 14 Jul 2026 11:19:18 +0800 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 > > 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 >