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 <[email protected]>
> 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 <[email protected]>
> ---
Looks good to me,
Reviewed-by: Harry Yoo <[email protected]>
with a nit:
> +bool __kfree_rcu_sheaf(struct kmem_cache *s, void *obj)
> +{
> + struct slub_percpu_sheaves *pcs;
> + struct slab_sheaf *rcu_sheaf;
> +
> + if (!local_trylock(&s->cpu_sheaves->lock))
> + goto fail;
> +
> + pcs = this_cpu_ptr(s->cpu_sheaves);
> +
> + if (unlikely(!pcs->rcu_free)) {
> +
> + struct slab_sheaf *empty;
> + struct node_barn *barn;
> +
> + if (pcs->spare && pcs->spare->size == 0) {
> + pcs->rcu_free = pcs->spare;
> + pcs->spare = NULL;
> + goto do_free;
> + }
> +
> + barn = get_barn(s);
> +
> + empty = barn_get_empty_sheaf(barn);
> +
> + if (empty) {
> + pcs->rcu_free = empty;
> + goto do_free;
> + }
> +
> + local_unlock(&s->cpu_sheaves->lock);
> +
> + empty = alloc_empty_sheaf(s, GFP_NOWAIT);
> +
> + if (!empty)
> + goto fail;
> +
> + if (!local_trylock(&s->cpu_sheaves->lock)) {
> + barn_put_empty_sheaf(barn, empty);
> + goto fail;
> + }
> +
> + pcs = this_cpu_ptr(s->cpu_sheaves);
> +
> + if (unlikely(pcs->rcu_free))
> + barn_put_empty_sheaf(barn, empty);
> + else
> + pcs->rcu_free = empty;
> + }
> +
> +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()?
--
Cheers,
Harry / Hyeonggon
> + local_unlock(&s->cpu_sheaves->lock);
> +
> + stat(s, FREE_RCU_SHEAF);
> + return true;
> +
> +fail:
> + stat(s, FREE_RCU_SHEAF_FAIL);
> + return false;
> +}
