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. */
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.
- 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) &&
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();@@ -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
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.
- 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) &&
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();@@ -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
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.
- 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) &&
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
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.
- 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) &&
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, PaulRegards, 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
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.
- 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) &&
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, PaulRegards, 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
linux-stable-mirror@lists.linaro.org
-
Boqun Feng -
Paul E. McKenney -
paulmck＠kernel.org