Stable Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs
       [not found] <20200214235536.GA13364@paulmck-ThinkPad-P72>
@ 2020-02-14 23:56 ` paulmck
  2020-02-25 10:24   ` Boqun Feng
  0 siblings, 1 reply; 6+ messages in thread
From: paulmck @ 2020-02-14 23:56 UTC (permalink / raw)
  To: rcu
  Cc: linux-kernel, kernel-team, mingo, jiangshanlai, dipankar, akpm,
	mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
	edumazet, fweisbec, oleg, joel, Paul E. McKenney, # 5 . 5 . x

From: "Paul E. McKenney" <paulmck@kernel.org>

Currently, rcu_barrier() ignores offline CPUs,  However, it is possible
for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
must wait for those callbacks.  This commit therefore makes rcu_barrier()
directly invoke the rcu_barrier_func() with interrupts disabled for such
CPUs.  This requires passing the CPU number into this function so that
it can entrain the rcu_barrier() callback onto the correct CPU's callback
list, given that the code must instead execute on the current CPU.

While in the area, this commit fixes a bug where the first CPU's callback
might have been invoked before rcu_segcblist_entrain() returned, which
would also result in an early wakeup.

Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: <stable@vger.kernel.org> # 5.5.x
---
 include/trace/events/rcu.h |  1 +
 kernel/rcu/tree.c          | 32 ++++++++++++++++++++------------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
index 5e49b06..d56d54c 100644
--- a/include/trace/events/rcu.h
+++ b/include/trace/events/rcu.h
@@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
  *	"Begin": rcu_barrier() started.
  *	"EarlyExit": rcu_barrier() piggybacked, thus early exit.
  *	"Inc1": rcu_barrier() piggyback check counter incremented.
+ *	"OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
  *	"OnlineQ": rcu_barrier() found online CPU with callbacks.
  *	"OnlineNQ": rcu_barrier() found online CPU, no callbacks.
  *	"IRQ": An rcu_barrier_callback() callback posted on remote CPU.
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d15041f..160643e 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
 /*
  * Called with preemption disabled, and from cross-cpu IRQ context.
  */
-static void rcu_barrier_func(void *unused)
+static void rcu_barrier_func(void *cpu_in)
 {
-	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
+	uintptr_t cpu = (uintptr_t)cpu_in;
+	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
 
 	rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
 	rdp->barrier_head.func = rcu_barrier_callback;
@@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
  */
 void rcu_barrier(void)
 {
-	int cpu;
+	uintptr_t cpu;
 	struct rcu_data *rdp;
 	unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
 
@@ -3150,13 +3151,14 @@ void rcu_barrier(void)
 	rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
 
 	/*
-	 * Initialize the count to one rather than to zero in order to
-	 * avoid a too-soon return to zero in case of a short grace period
-	 * (or preemption of this task).  Exclude CPU-hotplug operations
-	 * to ensure that no offline CPU has callbacks queued.
+	 * Initialize the count to two rather than to zero in order
+	 * to avoid a too-soon return to zero in case of an immediate
+	 * invocation of the just-enqueued callback (or preemption of
+	 * this task).  Exclude CPU-hotplug operations to ensure that no
+	 * offline non-offloaded CPU has callbacks queued.
 	 */
 	init_completion(&rcu_state.barrier_completion);
-	atomic_set(&rcu_state.barrier_cpu_count, 1);
+	atomic_set(&rcu_state.barrier_cpu_count, 2);
 	get_online_cpus();
 
 	/*
@@ -3166,13 +3168,19 @@ void rcu_barrier(void)
 	 */
 	for_each_possible_cpu(cpu) {
 		rdp = per_cpu_ptr(&rcu_data, cpu);
-		if (!cpu_online(cpu) &&
+		if (cpu_is_offline(cpu) &&
 		    !rcu_segcblist_is_offloaded(&rdp->cblist))
 			continue;
-		if (rcu_segcblist_n_cbs(&rdp->cblist)) {
+		if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
 			rcu_barrier_trace(TPS("OnlineQ"), cpu,
 					  rcu_state.barrier_sequence);
-			smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
+			smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
+		} else if (cpu_is_offline(cpu)) {
+			rcu_barrier_trace(TPS("OfflineNoCBQ"), cpu,
+					  rcu_state.barrier_sequence);
+			local_irq_disable();
+			rcu_barrier_func((void *)cpu);
+			local_irq_enable();
 		} else {
 			rcu_barrier_trace(TPS("OnlineNQ"), cpu,
 					  rcu_state.barrier_sequence);
@@ -3184,7 +3192,7 @@ void rcu_barrier(void)
 	 * Now that we have an rcu_barrier_callback() callback on each
 	 * CPU, and thus each counted, remove the initial count.
 	 */
-	if (atomic_dec_and_test(&rcu_state.barrier_cpu_count))
+	if (atomic_sub_and_test(2, &rcu_state.barrier_cpu_count))
 		complete(&rcu_state.barrier_completion);
 
 	/* Wait for all rcu_barrier_callback() callbacks to be invoked. */
-- 
2.9.5


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs
  2020-02-14 23:56 ` [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs paulmck
@ 2020-02-25 10:24   ` Boqun Feng
  2020-02-26  3:14     ` Paul E. McKenney
  0 siblings, 1 reply; 6+ messages in thread
From: Boqun Feng @ 2020-02-25 10:24 UTC (permalink / raw)
  To: paulmck
  Cc: rcu, linux-kernel, kernel-team, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
	edumazet, fweisbec, oleg, joel, # 5 . 5 . x

Hi Paul,

On Fri, Feb 14, 2020 at 03:56:07PM -0800, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> Currently, rcu_barrier() ignores offline CPUs,  However, it is possible
> for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
> must wait for those callbacks.  This commit therefore makes rcu_barrier()
> directly invoke the rcu_barrier_func() with interrupts disabled for such
> CPUs.  This requires passing the CPU number into this function so that
> it can entrain the rcu_barrier() callback onto the correct CPU's callback
> list, given that the code must instead execute on the current CPU.
> 
> While in the area, this commit fixes a bug where the first CPU's callback
> might have been invoked before rcu_segcblist_entrain() returned, which
> would also result in an early wakeup.
> 
> Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: <stable@vger.kernel.org> # 5.5.x
> ---
>  include/trace/events/rcu.h |  1 +
>  kernel/rcu/tree.c          | 32 ++++++++++++++++++++------------
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> index 5e49b06..d56d54c 100644
> --- a/include/trace/events/rcu.h
> +++ b/include/trace/events/rcu.h
> @@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
>   *	"Begin": rcu_barrier() started.
>   *	"EarlyExit": rcu_barrier() piggybacked, thus early exit.
>   *	"Inc1": rcu_barrier() piggyback check counter incremented.
> + *	"OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
>   *	"OnlineQ": rcu_barrier() found online CPU with callbacks.
>   *	"OnlineNQ": rcu_barrier() found online CPU, no callbacks.
>   *	"IRQ": An rcu_barrier_callback() callback posted on remote CPU.
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d15041f..160643e 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
>  /*
>   * Called with preemption disabled, and from cross-cpu IRQ context.
>   */
> -static void rcu_barrier_func(void *unused)
> +static void rcu_barrier_func(void *cpu_in)
>  {
> -	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
> +	uintptr_t cpu = (uintptr_t)cpu_in;
> +	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
>  
>  	rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
>  	rdp->barrier_head.func = rcu_barrier_callback;
> @@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
>   */
>  void rcu_barrier(void)
>  {
> -	int cpu;
> +	uintptr_t cpu;
>  	struct rcu_data *rdp;
>  	unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
>  
> @@ -3150,13 +3151,14 @@ void rcu_barrier(void)
>  	rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
>  
>  	/*
> -	 * Initialize the count to one rather than to zero in order to
> -	 * avoid a too-soon return to zero in case of a short grace period
> -	 * (or preemption of this task).  Exclude CPU-hotplug operations
> -	 * to ensure that no offline CPU has callbacks queued.
> +	 * Initialize the count to two rather than to zero in order
> +	 * to avoid a too-soon return to zero in case of an immediate
> +	 * invocation of the just-enqueued callback (or preemption of
> +	 * this task).  Exclude CPU-hotplug operations to ensure that no
> +	 * offline non-offloaded CPU has callbacks queued.
>  	 */
>  	init_completion(&rcu_state.barrier_completion);
> -	atomic_set(&rcu_state.barrier_cpu_count, 1);
> +	atomic_set(&rcu_state.barrier_cpu_count, 2);
>  	get_online_cpus();
>  
>  	/*
> @@ -3166,13 +3168,19 @@ void rcu_barrier(void)
>  	 */
>  	for_each_possible_cpu(cpu) {
>  		rdp = per_cpu_ptr(&rcu_data, cpu);
> -		if (!cpu_online(cpu) &&
> +		if (cpu_is_offline(cpu) &&
>  		    !rcu_segcblist_is_offloaded(&rdp->cblist))
>  			continue;
> -		if (rcu_segcblist_n_cbs(&rdp->cblist)) {
> +		if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
>  			rcu_barrier_trace(TPS("OnlineQ"), cpu,
>  					  rcu_state.barrier_sequence);
> -			smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
> +			smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
> +		} else if (cpu_is_offline(cpu)) {

I wonder whether this should be:

		  else if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_is_offline(cpu))

? Because I think we only want to queue the barrier call back if there
are callbacks for a particular CPU. Am I missing something subtle?

Regards,
Boqun

> +			rcu_barrier_trace(TPS("OfflineNoCBQ"), cpu,
> +					  rcu_state.barrier_sequence);
> +			local_irq_disable();
> +			rcu_barrier_func((void *)cpu);
> +			local_irq_enable();
>  		} else {
>  			rcu_barrier_trace(TPS("OnlineNQ"), cpu,
>  					  rcu_state.barrier_sequence);
> @@ -3184,7 +3192,7 @@ void rcu_barrier(void)
>  	 * Now that we have an rcu_barrier_callback() callback on each
>  	 * CPU, and thus each counted, remove the initial count.
>  	 */
> -	if (atomic_dec_and_test(&rcu_state.barrier_cpu_count))
> +	if (atomic_sub_and_test(2, &rcu_state.barrier_cpu_count))
>  		complete(&rcu_state.barrier_completion);
>  
>  	/* Wait for all rcu_barrier_callback() callbacks to be invoked. */
> -- 
> 2.9.5
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs
  2020-02-25 10:24   ` Boqun Feng
@ 2020-02-26  3:14     ` Paul E. McKenney
  2020-02-26  4:18       ` Paul E. McKenney
  2020-02-26  6:14       ` Boqun Feng
  0 siblings, 2 replies; 6+ messages in thread
From: Paul E. McKenney @ 2020-02-26  3:14 UTC (permalink / raw)
  To: Boqun Feng
  Cc: rcu, linux-kernel, kernel-team, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
	edumazet, fweisbec, oleg, joel, # 5 . 5 . x

On Tue, Feb 25, 2020 at 06:24:36PM +0800, Boqun Feng wrote:
> Hi Paul,
> 
> On Fri, Feb 14, 2020 at 03:56:07PM -0800, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > Currently, rcu_barrier() ignores offline CPUs,  However, it is possible
> > for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
> > must wait for those callbacks.  This commit therefore makes rcu_barrier()
> > directly invoke the rcu_barrier_func() with interrupts disabled for such
> > CPUs.  This requires passing the CPU number into this function so that
> > it can entrain the rcu_barrier() callback onto the correct CPU's callback
> > list, given that the code must instead execute on the current CPU.
> > 
> > While in the area, this commit fixes a bug where the first CPU's callback
> > might have been invoked before rcu_segcblist_entrain() returned, which
> > would also result in an early wakeup.
> > 
> > Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: <stable@vger.kernel.org> # 5.5.x
> > ---
> >  include/trace/events/rcu.h |  1 +
> >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++------------
> >  2 files changed, 21 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> > index 5e49b06..d56d54c 100644
> > --- a/include/trace/events/rcu.h
> > +++ b/include/trace/events/rcu.h
> > @@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
> >   *	"Begin": rcu_barrier() started.
> >   *	"EarlyExit": rcu_barrier() piggybacked, thus early exit.
> >   *	"Inc1": rcu_barrier() piggyback check counter incremented.
> > + *	"OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
> >   *	"OnlineQ": rcu_barrier() found online CPU with callbacks.
> >   *	"OnlineNQ": rcu_barrier() found online CPU, no callbacks.
> >   *	"IRQ": An rcu_barrier_callback() callback posted on remote CPU.
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index d15041f..160643e 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
> >  /*
> >   * Called with preemption disabled, and from cross-cpu IRQ context.
> >   */
> > -static void rcu_barrier_func(void *unused)
> > +static void rcu_barrier_func(void *cpu_in)
> >  {
> > -	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
> > +	uintptr_t cpu = (uintptr_t)cpu_in;
> > +	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> >  
> >  	rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
> >  	rdp->barrier_head.func = rcu_barrier_callback;
> > @@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
> >   */
> >  void rcu_barrier(void)
> >  {
> > -	int cpu;
> > +	uintptr_t cpu;
> >  	struct rcu_data *rdp;
> >  	unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
> >  
> > @@ -3150,13 +3151,14 @@ void rcu_barrier(void)
> >  	rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
> >  
> >  	/*
> > -	 * Initialize the count to one rather than to zero in order to
> > -	 * avoid a too-soon return to zero in case of a short grace period
> > -	 * (or preemption of this task).  Exclude CPU-hotplug operations
> > -	 * to ensure that no offline CPU has callbacks queued.
> > +	 * Initialize the count to two rather than to zero in order
> > +	 * to avoid a too-soon return to zero in case of an immediate
> > +	 * invocation of the just-enqueued callback (or preemption of
> > +	 * this task).  Exclude CPU-hotplug operations to ensure that no
> > +	 * offline non-offloaded CPU has callbacks queued.
> >  	 */
> >  	init_completion(&rcu_state.barrier_completion);
> > -	atomic_set(&rcu_state.barrier_cpu_count, 1);
> > +	atomic_set(&rcu_state.barrier_cpu_count, 2);
> >  	get_online_cpus();
> >  
> >  	/*
> > @@ -3166,13 +3168,19 @@ void rcu_barrier(void)
> >  	 */
> >  	for_each_possible_cpu(cpu) {
> >  		rdp = per_cpu_ptr(&rcu_data, cpu);
> > -		if (!cpu_online(cpu) &&
> > +		if (cpu_is_offline(cpu) &&
> >  		    !rcu_segcblist_is_offloaded(&rdp->cblist))
> >  			continue;
> > -		if (rcu_segcblist_n_cbs(&rdp->cblist)) {
> > +		if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
> >  			rcu_barrier_trace(TPS("OnlineQ"), cpu,
> >  					  rcu_state.barrier_sequence);
> > -			smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
> > +			smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
> > +		} else if (cpu_is_offline(cpu)) {
> 
> I wonder whether this should be:
> 
> 		  else if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_is_offline(cpu))
> 
> ? Because I think we only want to queue the barrier call back if there
> are callbacks for a particular CPU. Am I missing something subtle?

I don't believe that you are missing anything at all!

Thank you very much -- this bug would not have shown up in any validation
setup that I am aware of.  ;-)

							Thanx, Paul

> Regards,
> Boqun
> 
> > +			rcu_barrier_trace(TPS("OfflineNoCBQ"), cpu,
> > +					  rcu_state.barrier_sequence);
> > +			local_irq_disable();
> > +			rcu_barrier_func((void *)cpu);
> > +			local_irq_enable();
> >  		} else {
> >  			rcu_barrier_trace(TPS("OnlineNQ"), cpu,
> >  					  rcu_state.barrier_sequence);
> > @@ -3184,7 +3192,7 @@ void rcu_barrier(void)
> >  	 * Now that we have an rcu_barrier_callback() callback on each
> >  	 * CPU, and thus each counted, remove the initial count.
> >  	 */
> > -	if (atomic_dec_and_test(&rcu_state.barrier_cpu_count))
> > +	if (atomic_sub_and_test(2, &rcu_state.barrier_cpu_count))
> >  		complete(&rcu_state.barrier_completion);
> >  
> >  	/* Wait for all rcu_barrier_callback() callbacks to be invoked. */
> > -- 
> > 2.9.5
> > 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs
  2020-02-26  3:14     ` Paul E. McKenney
@ 2020-02-26  4:18       ` Paul E. McKenney
  2020-02-26  6:14       ` Boqun Feng
  1 sibling, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2020-02-26  4:18 UTC (permalink / raw)
  To: Boqun Feng
  Cc: rcu, linux-kernel, kernel-team, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
	edumazet, fweisbec, oleg, joel, # 5 . 5 . x

On Tue, Feb 25, 2020 at 07:14:55PM -0800, Paul E. McKenney wrote:
> On Tue, Feb 25, 2020 at 06:24:36PM +0800, Boqun Feng wrote:
> > Hi Paul,
> > 
> > On Fri, Feb 14, 2020 at 03:56:07PM -0800, paulmck@kernel.org wrote:
> > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > 
> > > Currently, rcu_barrier() ignores offline CPUs,  However, it is possible
> > > for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
> > > must wait for those callbacks.  This commit therefore makes rcu_barrier()
> > > directly invoke the rcu_barrier_func() with interrupts disabled for such
> > > CPUs.  This requires passing the CPU number into this function so that
> > > it can entrain the rcu_barrier() callback onto the correct CPU's callback
> > > list, given that the code must instead execute on the current CPU.
> > > 
> > > While in the area, this commit fixes a bug where the first CPU's callback
> > > might have been invoked before rcu_segcblist_entrain() returned, which
> > > would also result in an early wakeup.
> > > 
> > > Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: <stable@vger.kernel.org> # 5.5.x
> > > ---
> > >  include/trace/events/rcu.h |  1 +
> > >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++------------
> > >  2 files changed, 21 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> > > index 5e49b06..d56d54c 100644
> > > --- a/include/trace/events/rcu.h
> > > +++ b/include/trace/events/rcu.h
> > > @@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
> > >   *	"Begin": rcu_barrier() started.
> > >   *	"EarlyExit": rcu_barrier() piggybacked, thus early exit.
> > >   *	"Inc1": rcu_barrier() piggyback check counter incremented.
> > > + *	"OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
> > >   *	"OnlineQ": rcu_barrier() found online CPU with callbacks.
> > >   *	"OnlineNQ": rcu_barrier() found online CPU, no callbacks.
> > >   *	"IRQ": An rcu_barrier_callback() callback posted on remote CPU.
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index d15041f..160643e 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
> > >  /*
> > >   * Called with preemption disabled, and from cross-cpu IRQ context.
> > >   */
> > > -static void rcu_barrier_func(void *unused)
> > > +static void rcu_barrier_func(void *cpu_in)
> > >  {
> > > -	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
> > > +	uintptr_t cpu = (uintptr_t)cpu_in;
> > > +	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > >  
> > >  	rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
> > >  	rdp->barrier_head.func = rcu_barrier_callback;
> > > @@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
> > >   */
> > >  void rcu_barrier(void)
> > >  {
> > > -	int cpu;
> > > +	uintptr_t cpu;
> > >  	struct rcu_data *rdp;
> > >  	unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
> > >  
> > > @@ -3150,13 +3151,14 @@ void rcu_barrier(void)
> > >  	rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
> > >  
> > >  	/*
> > > -	 * Initialize the count to one rather than to zero in order to
> > > -	 * avoid a too-soon return to zero in case of a short grace period
> > > -	 * (or preemption of this task).  Exclude CPU-hotplug operations
> > > -	 * to ensure that no offline CPU has callbacks queued.
> > > +	 * Initialize the count to two rather than to zero in order
> > > +	 * to avoid a too-soon return to zero in case of an immediate
> > > +	 * invocation of the just-enqueued callback (or preemption of
> > > +	 * this task).  Exclude CPU-hotplug operations to ensure that no
> > > +	 * offline non-offloaded CPU has callbacks queued.
> > >  	 */
> > >  	init_completion(&rcu_state.barrier_completion);
> > > -	atomic_set(&rcu_state.barrier_cpu_count, 1);
> > > +	atomic_set(&rcu_state.barrier_cpu_count, 2);
> > >  	get_online_cpus();
> > >  
> > >  	/*
> > > @@ -3166,13 +3168,19 @@ void rcu_barrier(void)
> > >  	 */
> > >  	for_each_possible_cpu(cpu) {
> > >  		rdp = per_cpu_ptr(&rcu_data, cpu);
> > > -		if (!cpu_online(cpu) &&
> > > +		if (cpu_is_offline(cpu) &&
> > >  		    !rcu_segcblist_is_offloaded(&rdp->cblist))
> > >  			continue;
> > > -		if (rcu_segcblist_n_cbs(&rdp->cblist)) {
> > > +		if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
> > >  			rcu_barrier_trace(TPS("OnlineQ"), cpu,
> > >  					  rcu_state.barrier_sequence);
> > > -			smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
> > > +			smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
> > > +		} else if (cpu_is_offline(cpu)) {
> > 
> > I wonder whether this should be:
> > 
> > 		  else if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_is_offline(cpu))
> > 
> > ? Because I think we only want to queue the barrier call back if there
> > are callbacks for a particular CPU. Am I missing something subtle?
> 
> I don't believe that you are missing anything at all!
> 
> Thank you very much -- this bug would not have shown up in any validation
> setup that I am aware of.  ;-)

And with additional adjustment to make tracing accurate.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs
  2020-02-26  3:14     ` Paul E. McKenney
  2020-02-26  4:18       ` Paul E. McKenney
@ 2020-02-26  6:14       ` Boqun Feng
  2020-02-26 15:02         ` Paul E. McKenney
  1 sibling, 1 reply; 6+ messages in thread
From: Boqun Feng @ 2020-02-26  6:14 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: rcu, linux-kernel, kernel-team, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
	edumazet, fweisbec, oleg, joel, # 5 . 5 . x

On Tue, Feb 25, 2020 at 07:14:55PM -0800, Paul E. McKenney wrote:
> On Tue, Feb 25, 2020 at 06:24:36PM +0800, Boqun Feng wrote:
> > Hi Paul,
> > 
> > On Fri, Feb 14, 2020 at 03:56:07PM -0800, paulmck@kernel.org wrote:
> > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > 
> > > Currently, rcu_barrier() ignores offline CPUs,  However, it is possible
> > > for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
> > > must wait for those callbacks.  This commit therefore makes rcu_barrier()
> > > directly invoke the rcu_barrier_func() with interrupts disabled for such
> > > CPUs.  This requires passing the CPU number into this function so that
> > > it can entrain the rcu_barrier() callback onto the correct CPU's callback
> > > list, given that the code must instead execute on the current CPU.
> > > 
> > > While in the area, this commit fixes a bug where the first CPU's callback
> > > might have been invoked before rcu_segcblist_entrain() returned, which
> > > would also result in an early wakeup.
> > > 
> > > Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: <stable@vger.kernel.org> # 5.5.x
> > > ---
> > >  include/trace/events/rcu.h |  1 +
> > >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++------------
> > >  2 files changed, 21 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> > > index 5e49b06..d56d54c 100644
> > > --- a/include/trace/events/rcu.h
> > > +++ b/include/trace/events/rcu.h
> > > @@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
> > >   *	"Begin": rcu_barrier() started.
> > >   *	"EarlyExit": rcu_barrier() piggybacked, thus early exit.
> > >   *	"Inc1": rcu_barrier() piggyback check counter incremented.
> > > + *	"OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
> > >   *	"OnlineQ": rcu_barrier() found online CPU with callbacks.
> > >   *	"OnlineNQ": rcu_barrier() found online CPU, no callbacks.
> > >   *	"IRQ": An rcu_barrier_callback() callback posted on remote CPU.
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index d15041f..160643e 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
> > >  /*
> > >   * Called with preemption disabled, and from cross-cpu IRQ context.
> > >   */
> > > -static void rcu_barrier_func(void *unused)
> > > +static void rcu_barrier_func(void *cpu_in)
> > >  {
> > > -	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
> > > +	uintptr_t cpu = (uintptr_t)cpu_in;
> > > +	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > >  
> > >  	rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
> > >  	rdp->barrier_head.func = rcu_barrier_callback;
> > > @@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
> > >   */
> > >  void rcu_barrier(void)
> > >  {
> > > -	int cpu;
> > > +	uintptr_t cpu;
> > >  	struct rcu_data *rdp;
> > >  	unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
> > >  
> > > @@ -3150,13 +3151,14 @@ void rcu_barrier(void)
> > >  	rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
> > >  
> > >  	/*
> > > -	 * Initialize the count to one rather than to zero in order to
> > > -	 * avoid a too-soon return to zero in case of a short grace period
> > > -	 * (or preemption of this task).  Exclude CPU-hotplug operations
> > > -	 * to ensure that no offline CPU has callbacks queued.
> > > +	 * Initialize the count to two rather than to zero in order
> > > +	 * to avoid a too-soon return to zero in case of an immediate
> > > +	 * invocation of the just-enqueued callback (or preemption of
> > > +	 * this task).  Exclude CPU-hotplug operations to ensure that no
> > > +	 * offline non-offloaded CPU has callbacks queued.
> > >  	 */
> > >  	init_completion(&rcu_state.barrier_completion);
> > > -	atomic_set(&rcu_state.barrier_cpu_count, 1);
> > > +	atomic_set(&rcu_state.barrier_cpu_count, 2);
> > >  	get_online_cpus();
> > >  
> > >  	/*
> > > @@ -3166,13 +3168,19 @@ void rcu_barrier(void)
> > >  	 */
> > >  	for_each_possible_cpu(cpu) {
> > >  		rdp = per_cpu_ptr(&rcu_data, cpu);
> > > -		if (!cpu_online(cpu) &&
> > > +		if (cpu_is_offline(cpu) &&
> > >  		    !rcu_segcblist_is_offloaded(&rdp->cblist))
> > >  			continue;
> > > -		if (rcu_segcblist_n_cbs(&rdp->cblist)) {
> > > +		if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
> > >  			rcu_barrier_trace(TPS("OnlineQ"), cpu,
> > >  					  rcu_state.barrier_sequence);
> > > -			smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
> > > +			smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
> > > +		} else if (cpu_is_offline(cpu)) {
> > 
> > I wonder whether this should be:
> > 
> > 		  else if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_is_offline(cpu))
> > 
> > ? Because I think we only want to queue the barrier call back if there
> > are callbacks for a particular CPU. Am I missing something subtle?
> 
> I don't believe that you are missing anything at all!
> 
> Thank you very much -- this bug would not have shown up in any validation
> setup that I am aware of.  ;-)
> 
> 							Thanx, Paul
> 
> > Regards,
> > Boqun
> > 
> > > +			rcu_barrier_trace(TPS("OfflineNoCBQ"), cpu,
> > > +					  rcu_state.barrier_sequence);
> > > +			local_irq_disable();
> > > +			rcu_barrier_func((void *)cpu);
> > > +			local_irq_enable();

Another (interesting) thing I found here is that we actually don't need
the irq-off section to call rcu_barrier_func() in this branch. Because
the target CPU is offlined, so only the cblist is only accessed at two
places, IIUC, one is the rcuo kthread and one is here (in
rcu_barrier()), and both places are in the process context rather than
irq context, so irq-off is not required to prevent the deadlock.

But yes, I know, if we drop the local_irq_disable/enable() pair here,
it will make lockdep very unhappy ;-)

Regards,
Boqun

> > >  		} else {
> > >  			rcu_barrier_trace(TPS("OnlineNQ"), cpu,
> > >  					  rcu_state.barrier_sequence);
> > > @@ -3184,7 +3192,7 @@ void rcu_barrier(void)
> > >  	 * Now that we have an rcu_barrier_callback() callback on each
> > >  	 * CPU, and thus each counted, remove the initial count.
> > >  	 */
> > > -	if (atomic_dec_and_test(&rcu_state.barrier_cpu_count))
> > > +	if (atomic_sub_and_test(2, &rcu_state.barrier_cpu_count))
> > >  		complete(&rcu_state.barrier_completion);
> > >  
> > >  	/* Wait for all rcu_barrier_callback() callbacks to be invoked. */
> > > -- 
> > > 2.9.5
> > > 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs
  2020-02-26  6:14       ` Boqun Feng
@ 2020-02-26 15:02         ` Paul E. McKenney
  0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2020-02-26 15:02 UTC (permalink / raw)
  To: Boqun Feng
  Cc: rcu, linux-kernel, kernel-team, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
	edumazet, fweisbec, oleg, joel, # 5 . 5 . x

On Wed, Feb 26, 2020 at 02:14:30PM +0800, Boqun Feng wrote:
> On Tue, Feb 25, 2020 at 07:14:55PM -0800, Paul E. McKenney wrote:
> > On Tue, Feb 25, 2020 at 06:24:36PM +0800, Boqun Feng wrote:
> > > Hi Paul,
> > > 
> > > On Fri, Feb 14, 2020 at 03:56:07PM -0800, paulmck@kernel.org wrote:
> > > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > > 
> > > > Currently, rcu_barrier() ignores offline CPUs,  However, it is possible
> > > > for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
> > > > must wait for those callbacks.  This commit therefore makes rcu_barrier()
> > > > directly invoke the rcu_barrier_func() with interrupts disabled for such
> > > > CPUs.  This requires passing the CPU number into this function so that
> > > > it can entrain the rcu_barrier() callback onto the correct CPU's callback
> > > > list, given that the code must instead execute on the current CPU.
> > > > 
> > > > While in the area, this commit fixes a bug where the first CPU's callback
> > > > might have been invoked before rcu_segcblist_entrain() returned, which
> > > > would also result in an early wakeup.
> > > > 
> > > > Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > Cc: <stable@vger.kernel.org> # 5.5.x
> > > > ---
> > > >  include/trace/events/rcu.h |  1 +
> > > >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++------------
> > > >  2 files changed, 21 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> > > > index 5e49b06..d56d54c 100644
> > > > --- a/include/trace/events/rcu.h
> > > > +++ b/include/trace/events/rcu.h
> > > > @@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
> > > >   *	"Begin": rcu_barrier() started.
> > > >   *	"EarlyExit": rcu_barrier() piggybacked, thus early exit.
> > > >   *	"Inc1": rcu_barrier() piggyback check counter incremented.
> > > > + *	"OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
> > > >   *	"OnlineQ": rcu_barrier() found online CPU with callbacks.
> > > >   *	"OnlineNQ": rcu_barrier() found online CPU, no callbacks.
> > > >   *	"IRQ": An rcu_barrier_callback() callback posted on remote CPU.
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index d15041f..160643e 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
> > > >  /*
> > > >   * Called with preemption disabled, and from cross-cpu IRQ context.
> > > >   */
> > > > -static void rcu_barrier_func(void *unused)
> > > > +static void rcu_barrier_func(void *cpu_in)
> > > >  {
> > > > -	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
> > > > +	uintptr_t cpu = (uintptr_t)cpu_in;
> > > > +	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > > >  
> > > >  	rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
> > > >  	rdp->barrier_head.func = rcu_barrier_callback;
> > > > @@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
> > > >   */
> > > >  void rcu_barrier(void)
> > > >  {
> > > > -	int cpu;
> > > > +	uintptr_t cpu;
> > > >  	struct rcu_data *rdp;
> > > >  	unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
> > > >  
> > > > @@ -3150,13 +3151,14 @@ void rcu_barrier(void)
> > > >  	rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
> > > >  
> > > >  	/*
> > > > -	 * Initialize the count to one rather than to zero in order to
> > > > -	 * avoid a too-soon return to zero in case of a short grace period
> > > > -	 * (or preemption of this task).  Exclude CPU-hotplug operations
> > > > -	 * to ensure that no offline CPU has callbacks queued.
> > > > +	 * Initialize the count to two rather than to zero in order
> > > > +	 * to avoid a too-soon return to zero in case of an immediate
> > > > +	 * invocation of the just-enqueued callback (or preemption of
> > > > +	 * this task).  Exclude CPU-hotplug operations to ensure that no
> > > > +	 * offline non-offloaded CPU has callbacks queued.
> > > >  	 */
> > > >  	init_completion(&rcu_state.barrier_completion);
> > > > -	atomic_set(&rcu_state.barrier_cpu_count, 1);
> > > > +	atomic_set(&rcu_state.barrier_cpu_count, 2);
> > > >  	get_online_cpus();
> > > >  
> > > >  	/*
> > > > @@ -3166,13 +3168,19 @@ void rcu_barrier(void)
> > > >  	 */
> > > >  	for_each_possible_cpu(cpu) {
> > > >  		rdp = per_cpu_ptr(&rcu_data, cpu);
> > > > -		if (!cpu_online(cpu) &&
> > > > +		if (cpu_is_offline(cpu) &&
> > > >  		    !rcu_segcblist_is_offloaded(&rdp->cblist))
> > > >  			continue;
> > > > -		if (rcu_segcblist_n_cbs(&rdp->cblist)) {
> > > > +		if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
> > > >  			rcu_barrier_trace(TPS("OnlineQ"), cpu,
> > > >  					  rcu_state.barrier_sequence);
> > > > -			smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
> > > > +			smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
> > > > +		} else if (cpu_is_offline(cpu)) {
> > > 
> > > I wonder whether this should be:
> > > 
> > > 		  else if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_is_offline(cpu))
> > > 
> > > ? Because I think we only want to queue the barrier call back if there
> > > are callbacks for a particular CPU. Am I missing something subtle?
> > 
> > I don't believe that you are missing anything at all!
> > 
> > Thank you very much -- this bug would not have shown up in any validation
> > setup that I am aware of.  ;-)
> > 
> > 							Thanx, Paul
> > 
> > > Regards,
> > > Boqun
> > > 
> > > > +			rcu_barrier_trace(TPS("OfflineNoCBQ"), cpu,
> > > > +					  rcu_state.barrier_sequence);
> > > > +			local_irq_disable();
> > > > +			rcu_barrier_func((void *)cpu);
> > > > +			local_irq_enable();
> 
> Another (interesting) thing I found here is that we actually don't need
> the irq-off section to call rcu_barrier_func() in this branch. Because
> the target CPU is offlined, so only the cblist is only accessed at two
> places, IIUC, one is the rcuo kthread and one is here (in
> rcu_barrier()), and both places are in the process context rather than
> irq context, so irq-off is not required to prevent the deadlock.
> 
> But yes, I know, if we drop the local_irq_disable/enable() pair here,
> it will make lockdep very unhappy ;-)

And acquiring ->nocb_lock with interrupts enabled would be rather scary.
And probably would be an accident waiting to happen.  So I am happy to
disable interrupts on this path, given that it should be infrequent,
only being executed for a short time after a no-CBs CPU goes offline.

Much nicer to let lockdep do its thing than to have to second-guess it
on every change that involves acquiring ->nocb_lock in an interrupt
handler!  ;-)

							Thanx, Paul

> Regards,
> Boqun
> 
> > > >  		} else {
> > > >  			rcu_barrier_trace(TPS("OnlineNQ"), cpu,
> > > >  					  rcu_state.barrier_sequence);
> > > > @@ -3184,7 +3192,7 @@ void rcu_barrier(void)
> > > >  	 * Now that we have an rcu_barrier_callback() callback on each
> > > >  	 * CPU, and thus each counted, remove the initial count.
> > > >  	 */
> > > > -	if (atomic_dec_and_test(&rcu_state.barrier_cpu_count))
> > > > +	if (atomic_sub_and_test(2, &rcu_state.barrier_cpu_count))
> > > >  		complete(&rcu_state.barrier_completion);
> > > >  
> > > >  	/* Wait for all rcu_barrier_callback() callbacks to be invoked. */
> > > > -- 
> > > > 2.9.5
> > > > 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200214235536.GA13364@paulmck-ThinkPad-P72>
2020-02-14 23:56 ` [PATCH tip/core/rcu 30/30] rcu: Make rcu_barrier() account for offline no-CBs CPUs paulmck
2020-02-25 10:24   ` Boqun Feng
2020-02-26  3:14     ` Paul E. McKenney
2020-02-26  4:18       ` Paul E. McKenney
2020-02-26  6:14       ` Boqun Feng
2020-02-26 15:02         ` Paul E. McKenney

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org
	public-inbox-index stable

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git