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

[Qi Zheng]

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