Re: [PATCH] mm/list_lru: don't copy stale shrinker id from non-memcg-aware shrinkers

[Qinyun Tan <qinyuntan@linux.alibaba.com>]

Hi Michal,

On 9/3/26 1:17 AM, Michal Koutný wrote:
> Hello Qinyun.
> 
> On Tue, Sep 01, 2026 at 07:51:04PM +0800, Qinyun Tan <qinyuntan@linux.alibaba.com> wrote:
>> With cgroup.memory=nokmem, shrinker_memcg_alloc() fails with -ENOSYS
>> for shrinkers without SHRINKER_NONSLAB, and shrinker_alloc() falls
>> back to a non-memcg-aware shrinker.  On this fallback path,
>> shrinker->id is never assigned and keeps 0 from kzalloc(), which is a
>> valid id belonging to whichever memcg-aware shrinker registers first.
>>
>> __list_lru_init() copies shrinker->id unconditionally, so every
>> list_lru backed by such a fallback shrinker (thp-deferred_split,
>> zswap-shrinker, workingset shadow nodes, superblock lrus, ...) ends
>> up with lru->shrinker_id == 0 instead of -1.
> 
> <del>thp-deferred_split shrinek has SHRINKER_NONSLAB so, the id should be
> assigned (at least I see it in thp_shrinker_init()).</del> I was looking
> at 6.12 kernel.
> 
>> Under nokmem the list_lru collapses to the shared per-node lists, but
>> __list_lru_add() still calls set_shrinker_bit() against the memcg of
>> the added object.  Most list_lru users are unaffected because their
>> objects resolve to a NULL memcg without kmem accounting, but the THP
>> deferred split queue holds user folios, which are charged regardless
>> of nokmem.  Since no memcg-aware shrinker can register under nokmem,
> 
> Not sure I understand here, the SHRINKER_NONSLAB are excluded and should
> still register for per-memcg info.
>
You're right, that sentence is imprecise - SHRINKER_NONSLAB shrinkers
do register per-memcg under nokmem.  What I meant is that on a system
where no NONSLAB shrinker happens to register, shrinker_nr_max stays 0.

>> shrinker_nr_max stays 0 and every memcg's shrinker_info has
>> map_nr_max == 0, so the first folio added by khugepaged triggers on
>> every boot:
>>
>>   WARNING: mm/shrinker.c:212 at set_shrinker_bit+0x99/0xa0
>>
>> On systems where a SHRINKER_NONSLAB shrinker (btrfs, xfs) did register
>> and expand the maps, there is no warning; instead bit 0 is set
>> spuriously for an unrelated shrinker.
> 
> Could it be that you see this issue because of a shrinker which isn't
> marked as SHRINKER_NONSLAB?
> 
> I'd even go as far as pointing at
> fafaeceb89a5e ("mm: switch deferred split shrinker to list_lru")
> which removed the flag from the the thp-deferred_split shrinker.
> 
> IOW, the proper fix should be addition of SHRINKER_NONSLAB so that
> per-memcg maps are properly allocated.
> 
> Thanks,
> Michal

Yes, good catch.  The flag was originally introduced for this very
shrinker by 0a432dcbeb32 ("mm: shrinker: make shrinker not depend on
memcg kmem"), and your 03375203e1da8 changelog even cites
deferred_split_shrinker as the NONSLAB example that still needs
per-memcg data.  fafaeceb89a5e dropped the flag without mentioning it
in the changelog, so this looks like an unintended loss.

That said, I see the flag restoration and this patch as complementary
rather than either-or: other MEMCG_AWARE-but-not-NONSLAB shrinkers
(superblock, workingset shadow nodes) still take the fallback path
under nokmem by design, and __list_lru_init() copying an unassigned
id there is a landmine independent of the deferred split case.  So I
think this patch is still wanted as the generic fix, with the flag
restoration on top for THP.

(zswap might deserve the same NONSLAB treatment as THP, btw - its
entries resolve the memcg from the folio's objcg, which is available
regardless of nokmem.)

Would you like me to send a separate patch restoring SHRINKER_NONSLAB
in thp_shrinker_init() (Fixes: fafaeceb89a5e)?  Happy to do so if
that works for you.

Andrew, since the patch is already in mm-new, could you please update
this changelog sentence

	"Since no memcg-aware shrinker can register under nokmem,"
to
	"On a system where no SHRINKER_NONSLAB shrinker registers,"

per Michal's comment above?  Or I can send a v2 if you prefer.

Thanks,
Qinyun Tan