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

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 9/25/26 12:57, Harry Yoo wrote:
> 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?

Would have to check if anyone could find that to be unexpected.
Seems safer to continue with the sheaf to satisfy the request at hand even
if not full.

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

True!

> 
>> 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**"?

I mean we can still get to a situation where we're freeing during allocation
and making all more complex. But at least it doesn't go through sheaves so
the freeing is rather straightforward, no gotchas like
__pcs_replace_full_main() allocating a sheaf below.

>> 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?

Remains to be seen :)

>> >      -> 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@kernel.org>
>> > 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@linutronix.de
>> 
>> 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!

I want it to be! We did want to stop advertising particular LLMs (now done
in the kernel docs, IIRC) but also document how it was used as there's a
large spectrum of possibilities (not sure if that made it to the docs but it
was discussed definitely).

>> > Signed-off-by: Harry Yoo (Meta) <harry@kernel.org>
>> > ---
>> > 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.
>