Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1465685 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch 59/66] rcu: Convert rcutree to hotplug state machine "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-19 03:00 +0200
    Re: [patch 59/66] rcu: Convert rcutree to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-08-19 22:20 +0200
      Re: [patch 59/66] rcu: Convert rcutree to hotplug state machine "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-19 23:20 +0200

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

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@linux.vnet.ibm.com>
> > 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@linux.vnet.ibm.com>
> > 
> > --- 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
> 

[toc] | [next] | [standalone]

#1466655

On 2016-08-18 11:30:13 [-0700], Paul E. McKenney wrote:
> > > --- 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().

Thank you for that :)
 
> 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?

"sure". We enter here at CPUHP_BRINGUP_CPU. The next states until
(approx) CPUHP_AP_ONLINE are currently defined as "can't fail". As you
see in notify_cpu_starting() the return code of cpuhp_invoke_callback()
isn't checked and there is no rollback.
Later, CPUHP_AP_SMPBOOT_THREADS could fail (not in current but from here
on we no longer the return code). At this point we would return to where
we started (CPUHP_OFFLINE is most cases). During the rollback we get to
rcu_cleanup_dying_idle_cpu() via rcu_report_dead() so that is fine.

> > > 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.

doubt this would work. start_kernel() is invoked from the boot CPU.
Other CPUs are usually down because the scheduler wasn't up yet (so they
can't idle in their idle thread nor could they be marked "online" in the
CPU mask). Later (rest_init() -> kernel_init() ->
kernel_init_freeable()) there is smp_init() which boots the additional
CPUs.

> 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.

I did not intend to. I was thinking about moving it to
CPUHP_AP_RCUTREE_DYING but since the teardown method does not match it
makes no sense.
Thank you for clarifying the counterpart of rcu_cpu_starting().

> 							Thanx, Paul

Sebastian

[toc] | [prev] | [next] | [standalone]

#1466690

On Fri, Aug 19, 2016 at 10:12:00PM +0200, Sebastian Andrzej Siewior wrote:
> On 2016-08-18 11:30:13 [-0700], Paul E. McKenney wrote:
> > > > --- 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().
> 
> Thank you for that :)

No problem!  ;-)

> > 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?
> 
> "sure". We enter here at CPUHP_BRINGUP_CPU. The next states until
> (approx) CPUHP_AP_ONLINE are currently defined as "can't fail". As you
> see in notify_cpu_starting() the return code of cpuhp_invoke_callback()
> isn't checked and there is no rollback.
> Later, CPUHP_AP_SMPBOOT_THREADS could fail (not in current but from here
> on we no longer the return code). At this point we would return to where
> we started (CPUHP_OFFLINE is most cases). During the rollback we get to
> rcu_cleanup_dying_idle_cpu() via rcu_report_dead() so that is fine.

OK, so any later failure looks to RCU like the CPU came online, then
immediately went offline.  Works for me!

> > > > 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.
> 
> doubt this would work. start_kernel() is invoked from the boot CPU.
> Other CPUs are usually down because the scheduler wasn't up yet (so they
> can't idle in their idle thread nor could they be marked "online" in the
> CPU mask). Later (rest_init() -> kernel_init() ->
> kernel_init_freeable()) there is smp_init() which boots the additional
> CPUs.

Just as there is currently an early boot implementation of printk(),
some crazy person might well create an early boot implementation of the
scheduler that had fixed per-CPU tasks for which blocking is forbidden.
I understand that this is crazy, but much else has been considered crazy
not long before inclusion.  PREEMPT_RT, for but one example.  ;-)

> > 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.
> 
> I did not intend to. I was thinking about moving it to
> CPUHP_AP_RCUTREE_DYING but since the teardown method does not match it
> makes no sense.

Whew!!!  ;-)

> Thank you for clarifying the counterpart of rcu_cpu_starting().

Again, no problem!

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web