Re: [patch 59/66] rcu: Convert rcutree to hotplug state machine

[Paul E. McKenney]

On Thu, Aug 18, 2016 at 07:35:35PM +0200, Sebastian Andrzej Siewior wrote:
> On 2016-07-11 11:38:28 [-0700], Paul E. McKenney wrote:
> > commit da7095f39456dd0f28fa21697f2f976a61bc6d0a
> > Author: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > Date:   Thu Jun 30 13:58:26 2016 -0700
> > 
> >     rcu: Exact CPU-online tracking for RCU
> â
> 
> I don't pretend that I know what I going on here. I have just one simple
> question :)

That is what they all say!  ;-)

> >     Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > 
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -882,6 +882,7 @@ void notify_cpu_starting(unsigned int cpu)
> >  	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
> >  	enum cpuhp_state target = min((int)st->target, CPUHP_AP_ONLINE);
> >  
> > +	rcu_cpu_starting(cpu);  /* All CPU_STARTING notifiers can use RCU. */
> >  	while (st->state < target) {
> >  		struct cpuhp_step *step;
> 
> What happens if something post CPUHP_AP_ONLINE fails and we do a
> rollback to 0? Do we need to revert / undo rcu_cpu_starting() doing?

Yes, by calling rcu_cleanup_dying_idle_cpu().

But please note that rcu_cpu_starting() is invoked from the CPU itself
during the onlining process.  Is it really possible to fail beyond
that point?

> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 3121242b8579..5e7c1d6a6108 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3793,8 +3793,6 @@ rcu_init_percpu_data(int cpu, struct rcu_state *rsp)
> >  	rnp = rdp->mynode;
> >  	mask = rdp->grpmask;
> >  	raw_spin_lock_rcu_node(rnp);		/* irqs already disabled. */
> > -	rnp->qsmaskinitnext |= mask;
> > -	rnp->expmaskinitnext |= mask;
> >  	if (!rdp->beenonline)
> >  		WRITE_ONCE(rsp->ncpus, READ_ONCE(rsp->ncpus) + 1);
> >  	rdp->beenonline = true;	 /* We have now been online. */
> > @@ -4211,8 +4235,10 @@ void __init rcu_init(void)
> >  	 */
> >  	cpu_notifier(rcu_cpu_notify, 0);
> >  	pm_notifier(rcu_pm_notify, 0);
> > -	for_each_online_cpu(cpu)
> > +	for_each_online_cpu(cpu) {
> >  		rcu_cpu_notify(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
> > +		rcu_cpu_starting(cpu);
> > +	}
> 
> and looking at this from x86-64 then at this point I have CPU0 online
> and all other are down (or not yet up). So this is invoked for one CPU
> only. And later via hotplug callbacks for the other CPUs.

Yes, that is the current situation.  The reason for the loop is in
case someone clever decides to online other CPUs before RCU initializes
itself.  No idea how anyone would make that sort of thing work, but I
have learned not to underestimate the creativity of the fast-boot guys.

And please do not invoke rcu_cpu_starting() before rcu_init(), at least
not without some careful inspection and likely reworking of rcu_init()
and the things that it calls.

							Thanx, Paul

> >  }
> >  
> >  #include "tree_exp.h"
> 
> Sebastian
>