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

[Qinyun Tan]

Hi, Michal

On 9/3/26 5:20 PM, Michal Koutný wrote:
> On Thu, Sep 03, 2026 at 12:06:53PM +0800, Qinyun Tan <qinyuntan@xxxxxxxxxxxxxxxxx> wrote:
>> 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.
> 
> And ideally, with cgroup.memory=nokmem no slab-based code should call
> set_shrinker_bit() with whatever shrinker_id.
> 

Agreed, and with this patch that holds: slab-based lrus fall back to
shrinker_id == -1 and set_shrinker_bit() is never reached.

>> That said, I see the flag restoration and this patch as complementary
>> rather than either-or: 
> 
> Yes, it fixes your immediate issue and it is a prudent approach in
> list_lru_init(). OTOH, with this fixup, we won't be able to learn about
> shrinkers that are missing SHRINKER_NONSLAB.
>

True. Though the old behavior wasn't a reliable canary either: the
warning only fired while shrinker_nr_max was still 0. Once any
SHRINKER_NONSLAB shrinker had expanded the maps, a missing flag showed
up as a silently mis-set bit 0 rather than a warning. With your audit
below covering the current tree, I think losing that accidental signal
is an acceptable trade-off.
 
> 
>> 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.)
> 
> _That_ is an important point.
> So that would need a fix too, and likely Fixes: b5ba474f3f518 ("zswap:
> shrink zswap pool based on memory pressure").
> 

Will do.

> (I'd have preferred if there weren't Fixes: 03375203e1da8 which honors
> the SHRINKER_NONSLAB semantics and it only uncovered pre-existing
> missing flags. Or keep that Fixes: but explain the relation to the other
> commits. (To make it clear to backporters.))
> 

Fair point.  I'd propose retargeting the tag to

  Fixes: fafaeceb89a5e ("mm: switch deferred split shrinker to list_lru")

as that is the commit that made the problem user-visible: it put a
list_lru holding charged user folios behind a fallback shrinker, and
the boot-time warning is reproducible from v7.2 on.  Does that work
for you?

Andrew, thanks for the sentence fixup yesterday.  If Michal agrees,
could you please also retarget the Fixes: tag:

  -Fixes: 03375203e1da8 ("mm: do not allocate shrinker info with cgroup.memory=nokmem")
  +Fixes: fafaeceb89a5e ("mm: switch deferred split shrinker to list_lru")

>> 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.
> 
> SHRINKER_MEMCG_AWARE are due to be checked for correct marking with
> SHRINKER_NONSLAB. I can see that it is missing in: thp-deferred_split,
> zswap-shrinker in the v7.3-rc1. (The shrinkers in nfs42xattr.c seem to
> be slab-based so they're correct.)
> 

Thanks for auditing the whole tree.  I'll send a two-patch series
restoring/adding SHRINKER_NONSLAB for thp-deferred_split
(Fixes: fafaeceb89a5e) and zswap-shrinker (Fixes: b5ba474f3f518).

> The shrinkers mentioned in your original commit message:  workingset
> shadow nodes, superblock lrus are both slab-based, so systemd with only
> those shouldn't be affected by this bug.
> 

Right - they were listed only as lrus that end up carrying the bogus
id 0, not as triggers; as the changelog notes, their objects resolve
to a NULL memcg under nokmem so set_shrinker_bit() is never called for
them.  The only actual trigger is thp-deferred_split.

> Thanks,
> Michal


Thanks,

Qinyun Tan