Re: [PATCH RFC v1 1/2] rcu/tree: Add basic support for kfree_rcu batching

[Joel Fernandes]

On Sun, Aug 11, 2019 at 04:35:04PM -0700, Paul E. McKenney wrote:
> On Sat, Aug 10, 2019 at 10:26:58PM -0400, Joel Fernandes wrote:
> > On Sat, Aug 10, 2019 at 11:24:46AM -0700, Paul E. McKenney wrote:
> > > On Sat, Aug 10, 2019 at 12:20:37AM -0400, Joel Fernandes wrote:
> > > > On Fri, Aug 09, 2019 at 08:38:14PM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Aug 09, 2019 at 10:42:32PM -0400, Joel Fernandes wrote:
> > > > > > On Wed, Aug 07, 2019 at 10:52:15AM -0700, Paul E. McKenney wrote:
> > > > > > [snip] 
> > > > > > > > > > @@ -3459,6 +3645,8 @@ void __init rcu_init(void)
> > > > > > > > > >  {
> > > > > > > > > >  	int cpu;
> > > > > > > > > >  
> > > > > > > > > > +	kfree_rcu_batch_init();
> > > > > > > > > 
> > > > > > > > > What happens if someone does a kfree_rcu() before this point?  It looks
> > > > > > > > > like it should work, but have you tested it?
> > > > > > > > > 
> > > > > > > > > >  	rcu_early_boot_tests();
> > > > > > > > > 
> > > > > > > > > For example, by testing it in rcu_early_boot_tests() and moving the
> > > > > > > > > call to kfree_rcu_batch_init() here.
> > > > > > > > 
> > > > > > > > I have not tried to do the kfree_rcu() this early. I will try it out.
> > > > > > > 
> > > > > > > Yeah, well, call_rcu() this early came as a surprise to me back in the
> > > > > > > day, so...  ;-)
> > > > > > 
> > > > > > I actually did get surprised as well!
> > > > > > 
> > > > > > It appears the timers are not fully initialized so the really early
> > > > > > kfree_rcu() call from rcu_init() does cause a splat about an initialized
> > > > > > timer spinlock (even though future kfree_rcu()s and the system are working
> > > > > > fine all the way into the torture tests).
> > > > > > 
> > > > > > I think to resolve this, we can just not do batching until early_initcall,
> > > > > > during which I have an initialization function which switches batching on.
> > > > > > >From that point it is safe.
> > > > > 
> > > > > Just go ahead and batch, but don't bother with the timer until
> > > > > after single-threaded boot is done.  For example, you could check
> > > > > rcu_scheduler_active similar to how sync_rcu_exp_select_cpus() does.
> > > > > (See kernel/rcu/tree_exp.h.)
> > > > 
> > > > Cool, that works nicely and I tested it. Actually I made it such that we
> > > > don't need to batch even, before the scheduler is up. I don't see any benefit
> > > > of that unless we can see a kfree_rcu() flood happening that early at boot
> > > > which seems highly doubtful as a real world case.
> > > 
> > > The benefit is removing the kfree_rcu() special cases from the innards
> > > of RCU, for example, in rcu_do_batch().  Another benefit is removing the
> > > current restriction on the position of the rcu_head structure within the
> > > enclosing data structure.
> > > 
> > > So it would be good to avoid the current kfree_rcu() special casing within
> > > RCU itself.
> > > 
> > > Or are you using some trick that avoids both the batching and the current
> > > kfree_rcu() special casing?
> > 
> > Oh. I see what you mean. Would it be Ok with you to have that be a follow up
> > patch?  I am not getting rid (yet) of the special casing in rcu_do_batch in
> > this patch, but can do that in another patch.
> 
> I am OK having that in another patch, and I will be looking over yours
> and Byungchul's two patches tomorrow.  If they look OK, I will queue them.

Ok, some of the code comments are stale as Byungchul pointed, allow me to fix
them and then you can look at v3 directly, to save you the time.

> However, I won't send them upstream without a follow-on patch that gets
> rid of the kfree_rcu() special casing within rcu_do_batch() and perhaps
> elsewhere.  This follow-on patch would of course also need to change rcuperf
> appropriately.

Sounds good.

> > For now I am just doing something like the following in kfree_call_rcu(). I
> > was almost about to hit send on the v1 and I have been testing this a lot so
> > I'll post it anyway; and we can discuss more about this point on that.
> > 
> > +void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > +{
> > +       unsigned long flags;
> > +       struct kfree_rcu_cpu *krcp;
> > +       bool monitor_todo;
> > +
> > +       /* kfree_call_rcu() batching requires timers to be up. If the scheduler
> > +        * is not yet up, just skip batching and do non-batched kfree_call_rcu().
> > +        */
> > +       if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
> > +               return kfree_call_rcu_nobatch(head, func);
> > +
> 
> As a stopgap until the follow-on patch, this looks fine.

Cool, thanks!

- Joel