On Tue, Jan 21, 2025 at 03:14:16PM +0100, Uladzislau Rezki wrote:
> On Tue, Jan 21, 2025 at 02:49:13PM +0100, Vlastimil Babka wrote:
> > On 1/21/25 2:33 PM, Uladzislau Rezki wrote:
> > > On Mon, Jan 20, 2025 at 11:06:13PM +0100, Vlastimil Babka wrote:
> > >> On 12/16/24 17:46, Paul E. McKenney wrote:
> > >>> On Mon, Dec 16, 2024 at 04:55:06PM +0100, Uladzislau Rezki wrote:
> > >>>> On Mon, Dec 16, 2024 at 04:44:41PM +0100, Vlastimil Babka wrote:
> > >>>>> On 12/16/24 16:41, Uladzislau Rezki wrote:
> > >>>>>> On Mon, Dec 16, 2024 at 03:20:44PM +0100, Vlastimil Babka wrote:
> > >>>>>>> On 12/16/24 12:03, Uladzislau Rezki wrote:
> > >>>>>>>> On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote:
> > >>>>>>>>
> > >>>>>>>>> Also how about a followup patch moving the rcu-tiny implementation of
> > >>>>>>>>> kvfree_call_rcu()?
> > >>>>>>>>>
> > >>>>>>>> As, Paul already noted, it would make sense. Or just remove a tiny
> > >>>>>>>> implementation.
> > >>>>>>>
> > >>>>>>> AFAICS tiny rcu is for !SMP systems. Do they benefit from the "full"
> > >>>>>>> implementation with all the batching etc or would that be unnecessary overhead?
> > >>>>>>>
> > >>>>>> Yes, it is for a really small systems with low amount of memory. I see
> > >>>>>> only one overhead it is about driving objects in pages. For a small
> > >>>>>> system it can be critical because we allocate.
> > >>>>>>
> > >>>>>> From the other hand, for a tiny variant we can modify the normal variant
> > >>>>>> by bypassing batching logic, thus do not consume memory(for Tiny case)
> > >>>>>> i.e. merge it to a normal kvfree_rcu() path.
> > >>>>>
> > >>>>> Maybe we could change it to use CONFIG_SLUB_TINY as that has similar use
> > >>>>> case (less memory usage on low memory system, tradeoff for worse performance).
> > >>>>>
> > >>>> Yep, i also was thinking about that without saying it :)
> > >>>
> > >>> Works for me as well!
> > >>
> > >> Hi, so I tried looking at this. First I just moved the code to slab as seen
> > >> in the top-most commit here [1]. Hope the non-inlined __kvfree_call_rcu() is
> > >> not a show-stopper here.
> > >>
> > >> Then I wanted to switch the #ifdefs from CONFIG_TINY_RCU to CONFIG_SLUB_TINY
> > >> to control whether we use the full blown batching implementation or the
> > >> simple call_rcu() implmentation, and realized it's not straightforward and
> > >> reveals there are still some subtle dependencies of kvfree_rcu() on RCU
> > >> internals :)
> > >>
> > >> Problem 1: !CONFIG_SLUB_TINY with CONFIG_TINY_RCU
> > >>
> > >> AFAICS the batching implementation includes kfree_rcu_scheduler_running()
> > >> which is called from rcu_set_runtime_mode() but only on TREE_RCU. Perhaps
> > >> there are other facilities the batching implementation needs that only
> > >> exists in the TREE_RCU implementation
> > >>
> > >> Possible solution: batching implementation depends on both !CONFIG_SLUB_TINY
> > >> and !CONFIG_TINY_RCU. I think it makes sense as both !SMP systems and small
> > >> memory systems are fine with the simple implementation.
> > >>
> > >> Problem 2: CONFIG_TREE_RCU with !CONFIG_SLUB_TINY
> > >>
> > >> AFAICS I can't just make the simple implementation do call_rcu() on
> > >> CONFIG_TREE_RCU, because call_rcu() no longer knows how to handle the fake
> > >> callback (__is_kvfree_rcu_offset()) - I see how rcu_reclaim_tiny() does that
> > >> but no such equivalent exists in TREE_RCU. Am I right?
> > >>
> > >> Possible solution: teach TREE_RCU callback invocation to handle
> > >> __is_kvfree_rcu_offset() again, perhaps hide that branch behind #ifndef
> > >> CONFIG_SLUB_TINY to avoid overhead if the batching implementation is used.
> > >> Downside: we visibly demonstrate how kvfree_rcu() is not purely a slab thing
> > >> but RCU has to special case it still.
> > >>
> > >> Possible solution 2: instead of the special offset handling, SLUB provides a
> > >> callback function, which will determine pointer to the object from the
> > >> pointer to a middle of it without knowing the rcu_head offset.
> > >> Downside: this will have some overhead, but SLUB_TINY is not meant to be
> > >> performant anyway so we might not care.
> > >> Upside: we can remove __is_kvfree_rcu_offset() from TINY_RCU as well
> > >>
> > >> Thoughts?
> > >>
> > > For the call_rcu() and to be able to reclaim over it we need to patch the
> > > tree.c(please note TINY already works):
> > >
> > > <snip>
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index b1f883fcd918..ab24229dfa73 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -2559,13 +2559,19 @@ static void rcu_do_batch(struct rcu_data *rdp)
> > > debug_rcu_head_unqueue(rhp);
> > >
> > > rcu_lock_acquire(&rcu_callback_map);
> > > - trace_rcu_invoke_callback(rcu_state.name, rhp);
> > >
> > > f = rhp->func;
> > > - debug_rcu_head_callback(rhp);
> > > - WRITE_ONCE(rhp->func, (rcu_callback_t)0L);
> > > - f(rhp);
> > >
> > > + if (__is_kvfree_rcu_offset((unsigned long) f)) {
> > > + trace_rcu_invoke_kvfree_callback("", rhp, (unsigned long) f);
> > > + kvfree((void *) rhp - (unsigned long) f);
> > > + } else {
> > > + trace_rcu_invoke_callback(rcu_state.name, rhp);
> > > + debug_rcu_head_callback(rhp);
> > > + WRITE_ONCE(rhp->func, (rcu_callback_t)0L);
> > > + f(rhp);
> > > + }
> > > rcu_lock_release(&rcu_callback_map);
> >
> > Right so that's the first Possible solution, but without the #ifdef. So
> > there's an overhead of checking __is_kvfree_rcu_offset() even if the
> > batching is done in slab and this function is never called with an offset.
> >
> Or fulfilling a missing functionality? TREE is broken in that sense
> whereas a TINY handles it without any issues.
>
> It can be called for SLUB_TINY option, just call_rcu() instead of
> batching layer. And yes, kvfree_rcu_barrier() switches to rcu_barrier().
Would this make sense?
if (IS_ENABLED(CONFIG_TINY_RCU) && __is_kvfree_rcu_offset((unsigned long) f)) {
Just to be repetitive, other alternatives include:
1. Take advantage of SLOB being no longer with us.
2. Get rid of Tiny RCU's special casing of kfree_rcu(), and then
eliminate the above "if" statement in favor of its "else" clause.
3. Make Tiny RCU implement a trivial version of kfree_rcu() that
passes a list through RCU.
I don't have strong feelings, and am happy to defer to your guys'
decision.
> > After coming up with Possible solution 2, I've started liking the idea
> > more as RCU could then forget about the __is_kvfree_rcu_offset()
> > "callbacks" completely, and the performant case of TREE_RCU + batching
> > would be unaffected.
> >
> I doubt it is a performance issue :)
Me neither, especially with IS_ENABLED().
> > I'm speculating perhaps if there was not CONFIG_SLOB in the past, the
> > __is_kvfree_rcu_offset() would never exist in the first place? SLAB and
> > SLUB both can determine start of the object from a pointer to the middle
> > of it, while SLOB couldn't.
> >
> We needed just to reclaim over RCU. So, i do not know. Paul probably
> knows more then me :)
In the absence of SLOB, yes, I would hope that I would have thought of
determining the start of the object from a pointer to the middle of it.
Or that someone would have pointed that out during review. But I honestly
do not remember. ;-)
> > > /*
> > > <snip>
> > >
> > > Mixing up CONFIG_SLUB_TINY with CONFIG_TINY_RCU in the slab_common.c
> > > should be avoided, i.e. if we can, we should eliminate a dependency on
> > > TREE_RCU or TINY_RCU in a slab. As much as possible.
> > >
> > > So, it requires a more closer look for sure :)
> >
> > That requires solving Problem 1 above, but question is if it's worth the
> > trouble. Systems running TINY_RCU are unlikely to benefit from the batching?
> >
> > But sure there's also possibility to hide these dependencies in KConfig,
> > so the slab code would only consider a single (for example) #ifdef
> > CONFIG_KVFREE_RCU_BATCHING that would be set automatically depending on
> > TREE_RCU and !SLUB_TINY.
> >
> It is for small systems. We can use TINY or !SMP. We covered this AFAIR
> that a single CPU system should not go with batching:
>
> #ifdef SLUB_TINY || !SMP || TINY_RCU
>
> or:
>
> config TINY_RCU
> bool
> default y if !PREEMPT_RCU && !SMP
> + select SLUB_TINY
>
>
> Paul, more input?
I will say that Tiny RCU used to get much more focus from its users
10-15 years ago than it does now. So one approach is to implement
the simplest option, and add any needed complexity back in when and if
people complain. ;-)
Thanx, Paul
