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

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

sashiko left two review comments about this [1], but they look like 
false positives.

[1]. 
https://sashiko.dev/#/patchset/cover.1784621804.git.zhengqi.arch%40bytedance.com

 > > +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;
 > Does this strict equality check fail when a memcg is offlined 
(reparented)?
 > When a memcg is offlined (e.g., a container exits), its list_lru 
items are
 > reparented to its online parent. During memory pressure, the parent 
memcg runs
 > its shrinker, making sc->memcg the parent. However, the folio remains 
charged
 > to the offline child memcg.
 > Consequently, get_mem_cgroup_from_folio() returns the offline child 
memcg, and
 > this strict equality check fails.

No, get_mem_cgroup_from_folio() should returns parent memcg, not the
child memcg.

 > > +static void shmem_unused_huge_requeue(struct inode *inode, struct 
folio *folio)
 > > +{
 > > +	struct shmem_inode_info *info = SHMEM_I(inode);
 > > +	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
 > > +	struct mem_cgroup *memcg;
 > > +	int nid = folio_nid(folio);
 > > +
 > > +	memcg = shmem_unused_huge_alloc_lru(sbinfo, folio, GFP_NOWAIT);
 > > +	if (IS_ERR(memcg))
 > > +		goto drop;
 > > +
 > > +	spin_lock(&info->lock);
 > > +	/* Requeue the inode to shrinklist */
 > > +	list_del_init(&info->shrinklist);
 > > +	list_lru_add(&sbinfo->shrinklist, &info->shrinklist, nid, memcg);
 > > +	info->shrinklist_memcg = memcg;
 > Can this cause a livelock under memory pressure?
 > Because the memcg match failed in is_shmem_unused_huge_match(),
 > shmem_unused_huge_shrink() calls this function to place the inode 
back on the
 > list. This attempts to requeue it using the offline child memcg.
 > list_lru_add() transparently routes it back to the parent's list, but
 > info->shrinklist_memcg records the child memcg. In the next shrinker 
pass, the
 > match fails again, creating an infinite requeue loop during reclaim.

No, shmem_unused_huge_alloc_lru also returns the parent memcg:

shmem_unused_huge_alloc_lru
--> get_mem_cgroup_from_folio
     --> folio_memcg(folio)
         --> obj_cgroup_memcg(folio_objcg(folio))
             --> objcg->memcg  /* reparented to the parent memcg */