Re: [PATCH slab/for-next-fixes] mm/slab: do not wake up kswapd in __kfree_rcu_sheaf()

[Harry Yoo]

On Thu, Sep 24, 2026 at 10:43:01AM +0200, Vlastimil Babka (SUSE) wrote:
> On 9/22/26 13:56, Harry Yoo (Meta) wrote:
> > Fix this by always avoiding waking up kswapd in __kfree_rcu_sheaf().
> > Note that there are two paths that might wake up kswapd:
> > 
> >   1) __kfree_rcu_sheaf()
> >      // __GFP_KSWAPD_RECLAIM might wake up kswapd
> >      -> alloc_empty_sheaf(GFP_NOWAIT)
> > 
> >   2) __kfree_rcu_sheaf()
> >      // Let's say __kfree_rcu_sheaf() doesn't pass GFP_NOWAIT
> >      -> alloc_empty_sheaf(__GFP_NOWARN)
> >      -> kmalloc_flags()
> >      -> slab_alloc_node()
> >      -> alloc_from_pcs()
> >      -> __pcs_replace_empty_main()
> >      // Free a sheaf in an allocation path when the sheaf becomes empty
> >      // and refilling the sheaf fails
> 
> So that's this
> 
>                 /*
>                  * we must be very low on memory so don't bother
>                  * with the barn
>                  */
>                 sheaf_flush_unused(s, empty);
>                 free_empty_sheaf(s, empty);
> 
> Now I wonder if we should just use the barn then, lol. We either took the
> empty sheaf from there, or there was none, so we don't risk overfilling it
> with free sheaves.

Well when refill_sheaf() fails, it's not guaranteed to be empty.
Might have to put a partial sheaf to full list?

... well, now we have sheaf_partial though :-)

> Well but I guess sheaf_flush_unused() could end up in freeing paths anyway.
> But that's a bulk free which doesn't involve sheaves at least.

What do you mean by "**sheaf_flush_unused()** could end up in freeing path"
but "that's a bulk free which doesn't involve **sheaves**"?

> We could also distinguish which callers of refill_sheaf() can continue with
> a partially refilled sheaf. This one likely can so we'd not have to be
> flushing, ever?

Can we do that without adding too much complexity?

> >      -> free_empty_sheaf()
> >      -> slab_free()
> >      -> free_to_pcs()
> >      -> __pcs_replace_full_main()
> >      // However free path always assumes it's safe to wake up kswapd
> >      -> alloc_empty_sheaf(GFP_NOWAIT)
> 
> Would be great to avoid all this from kfree_rcu().

Yeah.

> > Drop __GFP_KSWAPD_RECLAIM in both cases. Note that the kfree_rcu() is
> > not the only user of free_to_pcs() path, but it should be fixed as it
> > can be invoked under pi_lock.
> > 
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Closes: https://sashiko.dev/#/message/20260831-b4-kfree_rcu_hotfix-v1-1-4f0fb882638b%40kernel.org
> > Fixes: ec66e0d59952 ("slab: add sheaf support for batching kfree_rcu() operations")
> > Link: https://lore.kernel.org/linux-mm/20260831143500.x-saxdAs@xxxxxxxxxxxxx
> 
> Fixed up per your reply.

Ack.

> > Assisted-by: LLM
> 
> Changed to (per below)
> 
> Assisted-by: LLM # dicovery and verification

Ack.

Didn't know adding # blablah after Assisted-by: was a thing!

> > Signed-off-by: Harry Yoo (Meta) <harry@xxxxxxxxxx>
> > ---
> > The discovery and verification (w/ a modified kernel) of the bug was
> > assisted by LLMs.
> > 
> > More speicifically, the first path was pointed out by Sashiko, and
> > the second path was discovered by LLM while reviewing the commit with
> > review-prompts [1].
> > 
> > Harry Yoo reviewed those findings and manually crafted the patch based
> > on that.
> > 
> > [1] https://github.com/masoncl/review-prompts
> > 
> > I believe the right direction to address this issue is to make
> > kfree_nolock() work in any context and replace it with kfree_rcu()
> > in the scheduler. However for now it won't work under pi_lock,
> > and resolving that would be a longer journey.
> 
> Indeed.
> 
> > Address this issue by dropping __GFP_KSWAPD_RECLAIM, for now.
> 
> Applied to mm/slab.git slab/for-next-fixes, thanks!

Thanks!

> But still could discuss a better solution per above.

-- 
Cheers,
Harry / Hyeonggon