On 9/19/25 08:47, Harry Yoo wrote:
> On Thu, Sep 18, 2025 at 10:09:34AM +0200, Vlastimil Babka wrote:
>> On 9/17/25 16:14, Vlastimil Babka wrote:
>> > On 9/17/25 15:34, Harry Yoo wrote:
>> >> On Wed, Sep 17, 2025 at 03:21:31PM +0200, Vlastimil Babka wrote:
>> >>> On 9/17/25 15:07, Harry Yoo wrote:
>> >>> > On Wed, Sep 17, 2025 at 02:05:49PM +0200, Vlastimil Babka wrote:
>> >>> >> On 9/17/25 13:32, Harry Yoo wrote:
>> >>> >> > On Wed, Sep 17, 2025 at 11:55:10AM +0200, Vlastimil Babka wrote:
>> >>> >> >> On 9/17/25 10:30, Harry Yoo wrote:
>> >>> >> >> > On Wed, Sep 10, 2025 at 10:01:06AM +0200, Vlastimil Babka wrote:
>> >>> >> >> >> + sfw->skip = true;
>> >>> >> >> >> + continue;
>> >>> >> >> >> + }
>> >>> >> >> >>
>> >>> >> >> >> + INIT_WORK(&sfw->work, flush_rcu_sheaf);
>> >>> >> >> >> + sfw->skip = false;
>> >>> >> >> >> + sfw->s = s;
>> >>> >> >> >> + queue_work_on(cpu, flushwq, &sfw->work);
>> >>> >> >> >> + flushed = true;
>> >>> >> >> >> + }
>> >>> >> >> >> +
>> >>> >> >> >> + for_each_online_cpu(cpu) {
>> >>> >> >> >> + sfw = &per_cpu(slub_flush, cpu);
>> >>> >> >> >> + if (sfw->skip)
>> >>> >> >> >> + continue;
>> >>> >> >> >> + flush_work(&sfw->work);
>> >>> >> >> >> + }
>> >>> >> >> >> +
>> >>> >> >> >> + mutex_unlock(&flush_lock);
>> >>> >> >> >> + }
>> >>> >> >> >> +
>> >>> >> >> >> + mutex_unlock(&slab_mutex);
>> >>> >> >> >> + cpus_read_unlock();
>> >>> >> >> >> +
>> >>> >> >> >> + if (flushed)
>> >>> >> >> >> + rcu_barrier();
>> >>> >> >> >
>> >>> >> >> > I think we need to call rcu_barrier() even if flushed == false?
>> >>> >> >> >
>> >>> >> >> > Maybe a kvfree_rcu()'d object was already waiting for the rcu
>> >>> >> >> > callback to
>> >>> >> >> > be processed before flush_all_rcu_sheaves() is called, and
>> >>> >> >> > in flush_all_rcu_sheaves() we skipped all (cache, cpu) pairs,
>> >>> >> >> > so flushed == false but the rcu callback isn't processed yet
>> >>> >> >> > by the end of the function?
>> >>> >> >> >
>> >>> >> >> > That sounds like a very unlikely to happen in a realistic
>> >>> >> >> > scenario,
>> >>> >> >> > but still possible...
>> >>> >> >>
>> >>> >> >> Yes also good point, will flush unconditionally.
>> >>> >> >>
>> >>> >> >> Maybe in __kfree_rcu_sheaf() I should also move the call_rcu(...)
>> >>> >> >> before
>> >>> >> >> local_unlock().
>> >>> >> >>
>> >>> >> >> So we don't end up seeing a NULL pcs->rcu_free in
>> >>> >> >> flush_all_rcu_sheaves() because __kfree_rcu_sheaf() already set it
>> >>> >> >> to NULL,
>> >>> >> >> but didn't yet do the call_rcu() as it got preempted after
>> >>> >> >> local_unlock().
>> >>> >> >
>> >>> >> > Makes sense to me.
>> >>> >
>> >>> > Wait, I'm confused.
>> >>> >
>> >>> > I think the caller of kvfree_rcu_barrier() should make sure that it's
>> >>> > invoked
>> >>> > only after a kvfree_rcu(X, rhs) call has returned, if the caller
>> >>> > expects
>> >>> > the object X to be freed before kvfree_rcu_barrier() returns?
>> >>>
>> >>> Hmm, the caller of kvfree_rcu(X, rhs) might have returned without
>> >>> filling up
>> >>> the rcu_sheaf fully and thus without submitting it to call_rcu(), then
>> >>> migrated to another cpu. Then it calls kvfree_rcu_barrier() while another
>> >>> unrelated kvfree_rcu(X, rhs) call on the previous cpu is for the same
>> >>> kmem_cache (kvfree_rcu_barrier() is not only for cache destruction),
>> >>> fills
>> >>> up the rcu_sheaf fully and is about to call_rcu() on it. And since that
>> >>> sheaf also contains the object X, we should make sure that is flushed.
>> >>
>> >> I was going to say "but we queue and wait for the flushing work to
>> >> complete, so the sheaf containing object X should be flushed?"
>> >>
>> >> But nah, that's true only if we see pcs->rcu_free != NULL in
>> >> flush_all_rcu_sheaves().
>> >>
>> >> You are right...
>> >>
>> >> Hmm, maybe it's simpler to fix this by never skipping queueing the work
>> >> even when pcs->rcu_sheaf == NULL?
>> >
>> > I guess it's simpler, yeah.
>>
>> So what about this? The unconditional queueing should cover all races with
>> __kfree_rcu_sheaf() so there's just unconditional rcu_barrier() in the end.
>>
>> From 0722b29fa1625b31c05d659d1d988ec882247b38 Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka <vba...@suse.cz>
>> Date: Wed, 3 Sep 2025 14:59:46 +0200
>> Subject: [PATCH] slab: add sheaf support for batching kfree_rcu() operations
>>
>> Extend the sheaf infrastructure for more efficient kfree_rcu() handling.
>> For caches with sheaves, on each cpu maintain a rcu_free sheaf in
>> addition to main and spare sheaves.
>>
>> kfree_rcu() operations will try to put objects on this sheaf. Once full,
>> the sheaf is detached and submitted to call_rcu() with a handler that
>> will try to put it in the barn, or flush to slab pages using bulk free,
>> when the barn is full. Then a new empty sheaf must be obtained to put
>> more objects there.
>>
>> It's possible that no free sheaves are available to use for a new
>> rcu_free sheaf, and the allocation in kfree_rcu() context can only use
>> GFP_NOWAIT and thus may fail. In that case, fall back to the existing
>> kfree_rcu() implementation.
>>
>> Expected advantages:
>> - batching the kfree_rcu() operations, that could eventually replace the
>> existing batching
>> - sheaves can be reused for allocations via barn instead of being
>> flushed to slabs, which is more efficient
>> - this includes cases where only some cpus are allowed to process rcu
>> callbacks (Android)
>>
>> Possible disadvantage:
>> - objects might be waiting for more than their grace period (it is
>> determined by the last object freed into the sheaf), increasing memory
>> usage - but the existing batching does that too.
>>
>> Only implement this for CONFIG_KVFREE_RCU_BATCHED as the tiny
>> implementation favors smaller memory footprint over performance.
>>
>> Also for now skip the usage of rcu sheaf for CONFIG_PREEMPT_RT as the
>> contexts where kfree_rcu() is called might not be compatible with taking
>> a barn spinlock or a GFP_NOWAIT allocation of a new sheaf taking a
>> spinlock - the current kfree_rcu() implementation avoids doing that.
>>
>> Teach kvfree_rcu_barrier() to flush all rcu_free sheaves from all caches
>> that have them. This is not a cheap operation, but the barrier usage is
>> rare - currently kmem_cache_destroy() or on module unload.
>>
>> Add CONFIG_SLUB_STATS counters free_rcu_sheaf and free_rcu_sheaf_fail to
>> count how many kfree_rcu() used the rcu_free sheaf successfully and how
>> many had to fall back to the existing implementation.
>>
>> Signed-off-by: Vlastimil Babka <vba...@suse.cz>
>> ---
>
> Looks good to me,
> Reviewed-by: Harry Yoo <harry....@oracle.com>
Thanks.
>> +do_free:
>> +
>> + rcu_sheaf = pcs->rcu_free;
>> +
>> + rcu_sheaf->objects[rcu_sheaf->size++] = obj;
>> +
>> + if (likely(rcu_sheaf->size < s->sheaf_capacity))
>> + rcu_sheaf = NULL;
>> + else
>> + pcs->rcu_free = NULL;
>> +
>> + /*
>> + * we flush before local_unlock to make sure a racing
>> + * flush_all_rcu_sheaves() doesn't miss this sheaf
>> + */
>> + if (rcu_sheaf)
>> + call_rcu(&rcu_sheaf->rcu_head, rcu_free_sheaf);
>
> nit: now we don't have to put this inside local_lock()~local_unlock()?
I think we still need to? AFAICS I wrote before is still true:
The caller of kvfree_rcu(X, rhs) might have returned without filling up
the rcu_sheaf fully and thus without submitting it to call_rcu(), then
migrated to another cpu. Then it calls kvfree_rcu_barrier() while another
unrelated kvfree_rcu(X, rhs) call on the previous cpu is for the same
kmem_cache (kvfree_rcu_barrier() is not only for cache destruction), fills
up the rcu_sheaf fully and is about to call_rcu() on it.
If it can local_unlock() before doing the call_rcu(), it can local_unlock(),
get preempted, and our flush worqueue handler will only see there's no
rcu_free sheaf and do nothing.
If if must call_rcu() before local_unlock(), our flush workqueue handler
will not execute on the cpu until it performs the call_rcu() and
local_unlock(), because it can't preempt that section (!RT) or will have to
wait doing local_lock() in flush_rcu_sheaf() (RT) - here it's important it
takes the lock unconditionally.
Or am I missing something?
