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.
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'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.
> /*
> <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.
> --
> Uladzislau Rezki
