rcu/nocb updates v2

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

On both situations, a future call_rcu() on that rdp may be fooled and think the timer is armed when it's not, missing a deferred nocb_gp wakeup.

Case 1) is very unlikely due to timing constraint (the timer fires after 1 jiffy) but still possible in theory. Case 2) is more likely to happen. But in any case such scenario require the CPU to spend a long time within a kernel thread without exiting to idle or user space, which is a pretty exotic behaviour.

Fix this with resetting rdp->nocb_defer_wakeup everytime we disarm the timer.

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

Cc Stable <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/rcu/tree_plugin.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

kernel/rcu/tree_plugin.h +5 -2

index 2ec9d7f55f99..dd0dc66c282d 100644
@@ -1720,7 +1720,11 @@ static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
 		rcu_nocb_unlock_irqrestore(rdp, flags);
 		return false;
 	}
-	del_timer(&rdp->nocb_timer);
+
+	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
+		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+		del_timer(&rdp->nocb_timer);
+	}
 	rcu_nocb_unlock_irqrestore(rdp, flags);
 	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {

@@ -2349,7 +2353,6 @@ static bool do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 		return false;
 	}
 	ndw = READ_ONCE(rdp->nocb_defer_wakeup);
-	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
 	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));
 

Signature · 1 line

2.25.1

Quoted 9 lines · Frederic Weisbecker

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

As discussed offlist, expanding the above scenario results in this sequence of steps:

1. There are no callbacks queued for any CPU covered by CPU 0-2's

	->nocb_gp_kthread.

2. CPU 0 enqueues its first callback with interrupts disabled, and

	thus must defer awakening its ->nocb_gp_kthread.  It therefore
	queues its rcu_data structure's ->nocb_timer.

3. CPU 1, which shares the same ->nocb_gp_kthread, also enqueues a

	callback, but with interrupts enabled, allowing it to directly
	awaken the ->nocb_gp_kthread.

4. The newly awakened ->nocb_gp_kthread associates both CPU 0's
and CPU 1's callbacks with a future grace period and arranges for that grace period to be started.

5. This ->nocb_gp_kthread goes to sleep waiting for the end of this
future grace period.

6. This grace period elapses before the CPU 0's timer fires.
This is normally improbably given that the timer is set for only one jiffy, but timers can be delayed. Besides, it is possible that kernel was built with CONFIG_RCU_STRICT_GRACE_PERIOD=y.

7. The grace period ends, so rcu_gp_kthread awakens the

	->nocb_gp_kthread, which in turn awakens both CPU 0's and
	CPU 1's ->nocb_cb_kthread.

8. CPU 0's ->nocb_cb_kthread invokes its callback.

9. Note that neither kthread updated any ->nocb_timer state,

	so CPU 0's ->nocb_defer_wakeup is still set to either
	RCU_NOCB_WAKE or RCU_NOCB_WAKE_FORCE.

10. CPU 0 enqueues its second callback, again with interrupts
disabled, and thus must again defer awakening its ->nocb_gp_kthread. However, ->nocb_defer_wakeup prevents CPU 0 from queueing the timer.

So far so good, but why isn't the timer still queued from back in step 2? What am I missing here? Either way, could you please update the commit logs to tell the full story? At some later time, you might be very happy that you did. ;-)

Thanx, Paul

Quoted 58 lines

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

On both situations, a future call_rcu() on that rdp may be fooled and think the timer is armed when it's not, missing a deferred nocb_gp wakeup.

Case 1) is very unlikely due to timing constraint (the timer fires after 1 jiffy) but still possible in theory. Case 2) is more likely to happen. But in any case such scenario require the CPU to spend a long time within a kernel thread without exiting to idle or user space, which is a pretty exotic behaviour.

Fix this with resetting rdp->nocb_defer_wakeup everytime we disarm the timer.

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

Cc Stable <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/rcu/tree_plugin.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

kernel/rcu/tree_plugin.h +5 -2

index 2ec9d7f55f99..dd0dc66c282d 100644
@@ -1720,7 +1720,11 @@ static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
 		rcu_nocb_unlock_irqrestore(rdp, flags);
 		return false;
 	}
-	del_timer(&rdp->nocb_timer);
+
+	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
+		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+		del_timer(&rdp->nocb_timer);
+	}
 	rcu_nocb_unlock_irqrestore(rdp, flags);
 	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {

@@ -2349,7 +2353,6 @@ static bool do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 		return false;
 	}
 	ndw = READ_ONCE(rdp->nocb_defer_wakeup);
-	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
 	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));
 

Signature · 1 line

2.25.1

Quoted 43 lines · Paul E. McKenney

Quoted 9 lines · Frederic Weisbecker

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

As discussed offlist, expanding the above scenario results in this sequence of steps:

1. There are no callbacks queued for any CPU covered by CPU 0-2's

	->nocb_gp_kthread.

2. CPU 0 enqueues its first callback with interrupts disabled, and

	thus must defer awakening its ->nocb_gp_kthread.  It therefore
	queues its rcu_data structure's ->nocb_timer.

3. CPU 1, which shares the same ->nocb_gp_kthread, also enqueues a

	callback, but with interrupts enabled, allowing it to directly
	awaken the ->nocb_gp_kthread.

4. The newly awakened ->nocb_gp_kthread associates both CPU 0's
and CPU 1's callbacks with a future grace period and arranges for that grace period to be started.

5. This ->nocb_gp_kthread goes to sleep waiting for the end of this
future grace period.

6. This grace period elapses before the CPU 0's timer fires.
This is normally improbably given that the timer is set for only one jiffy, but timers can be delayed. Besides, it is possible that kernel was built with CONFIG_RCU_STRICT_GRACE_PERIOD=y.

7. The grace period ends, so rcu_gp_kthread awakens the

	->nocb_gp_kthread, which in turn awakens both CPU 0's and
	CPU 1's ->nocb_cb_kthread.

8. CPU 0's ->nocb_cb_kthread invokes its callback.

Quoted 8 lines

9. Note that neither kthread updated any ->nocb_timer state,

	so CPU 0's ->nocb_defer_wakeup is still set to either
	RCU_NOCB_WAKE or RCU_NOCB_WAKE_FORCE.

10. CPU 0 enqueues its second callback, again with interrupts
disabled, and thus must again defer awakening its ->nocb_gp_kthread. However, ->nocb_defer_wakeup prevents CPU 0 from queueing the timer.

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 0 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.

	It does so with calling __wake_nocb_gp() which also cancels the
	pending timer that got queued in step 2. But that doesn't reset
	CPU 0's ->nocb_defer_wakeup which is still set to RCU_NOCB_WAKE.
	So CPU 0's ->nocb_defer_wakeup and CPU 0's ->nocb_timer are now
	desynchronized.

11. ->nocb_gp_kthread associates the callback queued in 10 with a new
grace period, arrange for it to start and sleeps on it.

12. The grace period ends, ->nocb_gp_kthread awakens and wakes up

	CPU 0's ->nocb_cb_kthread which invokes the callback queued in 10.

13. CPU 0 enqueues its third callback, this time with interrupts

	disabled so it tries to queue a deferred wakeup. However
	->nocb_defer_wakeup has a stalled RCU_NOCB_WAKE value which prevents
	the CPU 0's ->nocb_timer, that got cancelled in 10, from being armed.

14. CPU 0 has its pending callback and it may go unnoticed until
some other CPU ever wakes up ->nocb_gp_kthread or CPU 0 ever calls an explicit deferred wake up caller like idle entry.

I hope I'm not missing something this time...

Thanks.

Quoted 66 lines

So far so good, but why isn't the timer still queued from back in step 2? What am I missing here? Either way, could you please update the commit logs to tell the full story? At some later time, you might be very happy that you did. ;-)

Thanx, Paul

Quoted 58 lines

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

On both situations, a future call_rcu() on that rdp may be fooled and think the timer is armed when it's not, missing a deferred nocb_gp wakeup.

Case 1) is very unlikely due to timing constraint (the timer fires after 1 jiffy) but still possible in theory. Case 2) is more likely to happen. But in any case such scenario require the CPU to spend a long time within a kernel thread without exiting to idle or user space, which is a pretty exotic behaviour.

Fix this with resetting rdp->nocb_defer_wakeup everytime we disarm the timer.

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

Cc Stable <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/rcu/tree_plugin.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

kernel/rcu/tree_plugin.h +5 -2

index 2ec9d7f55f99..dd0dc66c282d 100644
@@ -1720,7 +1720,11 @@ static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
 		rcu_nocb_unlock_irqrestore(rdp, flags);
 		return false;
 	}
-	del_timer(&rdp->nocb_timer);
+
+	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
+		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+		del_timer(&rdp->nocb_timer);
+	}
 	rcu_nocb_unlock_irqrestore(rdp, flags);
 	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {

@@ -2349,7 +2353,6 @@ static bool do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 		return false;
 	}
 	ndw = READ_ONCE(rdp->nocb_defer_wakeup);
-	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
 	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));
 

Signature · 1 line

2.25.1

Quoted 22 lines · Frederic Weisbecker

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 0 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.

	It does so with calling __wake_nocb_gp() which also cancels the
	pending timer that got queued in step 2. But that doesn't reset
	CPU 0's ->nocb_defer_wakeup which is still set to RCU_NOCB_WAKE.
	So CPU 0's ->nocb_defer_wakeup and CPU 0's ->nocb_timer are now
	desynchronized.

11. ->nocb_gp_kthread associates the callback queued in 10 with a new
grace period, arrange for it to start and sleeps on it.

12. The grace period ends, ->nocb_gp_kthread awakens and wakes up

	CPU 0's ->nocb_cb_kthread which invokes the callback queued in 10.

13. CPU 0 enqueues its third callback, this time with interrupts

	disabled so it tries to queue a deferred wakeup. However
	->nocb_defer_wakeup has a stalled RCU_NOCB_WAKE value which prevents
	the CPU 0's ->nocb_timer, that got cancelled in 10, from being armed.

Quoted 5 lines

14. CPU 0 has its pending callback and it may go unnoticed until
some other CPU ever wakes up ->nocb_gp_kthread or CPU 0 ever calls an explicit deferred wake up caller like idle entry.

I hope I'm not missing something this time...

Thank you, that does sound plausible. I guess I can see how rcutorture might have missed this one!

Quoted 27 lines · Paul E. McKenney

Quoted 26 lines · Frederic Weisbecker

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 0 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.

	It does so with calling __wake_nocb_gp() which also cancels the
	pending timer that got queued in step 2. But that doesn't reset
	CPU 0's ->nocb_defer_wakeup which is still set to RCU_NOCB_WAKE.
	So CPU 0's ->nocb_defer_wakeup and CPU 0's ->nocb_timer are now
	desynchronized.

11. ->nocb_gp_kthread associates the callback queued in 10 with a new
grace period, arrange for it to start and sleeps on it.

12. The grace period ends, ->nocb_gp_kthread awakens and wakes up

	CPU 0's ->nocb_cb_kthread which invokes the callback queued in 10.

13. CPU 0 enqueues its third callback, this time with interrupts

	disabled so it tries to queue a deferred wakeup. However
	->nocb_defer_wakeup has a stalled RCU_NOCB_WAKE value which prevents
	the CPU 0's ->nocb_timer, that got cancelled in 10, from being armed.

14. CPU 0 has its pending callback and it may go unnoticed until
some other CPU ever wakes up ->nocb_gp_kthread or CPU 0 ever calls an explicit deferred wake up caller like idle entry.

Quoted 4 lines

Quoted 1 line

I hope I'm not missing something this time...

Thank you, that does sound plausible. I guess I can see how rcutorture might have missed this one!

I must admit it requires a lot of stars to be aligned :-)

Quoted 11 lines · Paul E. McKenney

Quoted 9 lines · Frederic Weisbecker

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

Quoted 2 lines

As discussed offlist, expanding the above scenario results in this sequence of steps:

Quoted 2 lines

1. There are no callbacks queued for any CPU covered by CPU 0-2's

	->nocb_gp_kthread.

Quoted 3 lines

2. CPU 1 enqueues its first callback with interrupts disabled, and

	thus must defer awakening its ->nocb_gp_kthread.  It therefore
	queues its rcu_data structure's ->nocb_timer.

Quoted 11 lines

3. CPU 2, which shares the same ->nocb_gp_kthread, also enqueues a

	callback, but with interrupts enabled, allowing it to directly
	awaken the ->nocb_gp_kthread.

4. The newly awakened ->nocb_gp_kthread associates both CPU 1's
and CPU 2's callbacks with a future grace period and arranges for that grace period to be started.

5. This ->nocb_gp_kthread goes to sleep waiting for the end of this
future grace period.

Quoted 8 lines

6. This grace period elapses before the CPU 1's timer fires.
This is normally improbably given that the timer is set for only one jiffy, but timers can be delayed. Besides, it is possible that kernel was built with CONFIG_RCU_STRICT_GRACE_PERIOD=y.

7. The grace period ends, so rcu_gp_kthread awakens the

	->nocb_gp_kthread, which in turn awakens both CPU 1's and
	CPU 2's ->nocb_cb_kthread.

Quoted 9 lines

8. CPU 1's ->nocb_cb_kthread invokes its callback.

9. Note that neither kthread updated any ->nocb_timer state,

	so CPU 1's ->nocb_defer_wakeup is still set to RCU_NOCB_WAKE.

10. CPU 1 enqueues its second callback, again with interrupts
disabled, and thus must again defer awakening its ->nocb_gp_kthread. However, ->nocb_defer_wakeup prevents CPU 1 from queueing the timer.

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 1 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.
It does so with calling __wake_nocb_gp() which also cancels the

	pending timer that got queued in step 2. But that doesn't reset
	CPU 1's ->nocb_defer_wakeup which is still set to RCU_NOCB_WAKE.
	So CPU 1's ->nocb_defer_wakeup and CPU 1's ->nocb_timer are now
	desynchronized.

11. ->nocb_gp_kthread associates the callback queued in 10 with a new
grace period, arrange for it to start and sleeps on it.

12. The grace period ends, ->nocb_gp_kthread awakens and wakes up

	CPU 1's ->nocb_cb_kthread which invokes the callback queued in 10.

13. CPU 1 enqueues its third callback, this time with interrupts

	disabled so it tries to queue a deferred wakeup. However
	->nocb_defer_wakeup has a stalled RCU_NOCB_WAKE value which prevents
	the CPU 1's ->nocb_timer, that got cancelled in 10, from being armed.

14. CPU 1 has its pending callback and it may go unnoticed until

	some other CPU ever wakes up ->nocb_gp_kthread or CPU 1 ever calls
	an explicit deferred wake up caller like idle entry.

I hope I'm not missing something this time...

Quoted 9 lines

So far so good, but why isn't the timer still queued from back in step 2? What am I missing here? Either way, could you please update the commit logs to tell the full story? At some later time, you might be very happy that you did. ;-)

Quoted 4 lines

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

Quoted 53 lines

Quoted 53 lines

On both situations, a future call_rcu() on that rdp may be fooled and think the timer is armed when it's not, missing a deferred nocb_gp wakeup.

Case 1) is very unlikely due to timing constraint (the timer fires after 1 jiffy) but still possible in theory. Case 2) is more likely to happen. But in any case such scenario require the CPU to spend a long time within a kernel thread without exiting to idle or user space, which is a pretty exotic behaviour.

Fix this with resetting rdp->nocb_defer_wakeup everytime we disarm the timer.

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

Cc Stable <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/rcu/tree_plugin.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

kernel/rcu/tree_plugin.h +5 -2

index 2ec9d7f55f99..dd0dc66c282d 100644
@@ -1720,7 +1720,11 @@ static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
 		rcu_nocb_unlock_irqrestore(rdp, flags);
 		return false;
 	}
-	del_timer(&rdp->nocb_timer);
+
+	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
+		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+		del_timer(&rdp->nocb_timer);
+	}
 	rcu_nocb_unlock_irqrestore(rdp, flags);
 	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {

@@ -2349,7 +2353,6 @@ static bool do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 		return false;
 	}
 	ndw = READ_ONCE(rdp->nocb_defer_wakeup);
-	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
 	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));
 

Signature · 1 line

2.25.1

Quoted 12 lines · Frederic Weisbecker

Quoted 11 lines · Paul E. McKenney

Quoted 9 lines · Frederic Weisbecker

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

Quoted 2 lines

Quoted 2 lines

As discussed offlist, expanding the above scenario results in this sequence of steps:

I renumbered the CPUs, since the ->nocb_gp_kthread would normally be associated with CPU 0. If the first CPU to enqueue a callback was also CPU 0, nocb_gp_wait() might clear that CPU's ->nocb_defer_wakeup, which would prevent this scenario from playing out. (But admittedly only if some other CPU handled by this same ->nocb_gp_kthread used its bypass.)

Quoted 3 lines

Quoted 3 lines

1. There are no callbacks queued for any CPU covered by CPU 0-2's

	->nocb_gp_kthread.

And ->nocb_gp_kthread is associated with CPU 0.

Quoted 3 lines

Quoted 3 lines

2. CPU 1 enqueues its first callback with interrupts disabled, and

	thus must defer awakening its ->nocb_gp_kthread.  It therefore
	queues its rcu_data structure's ->nocb_timer.

At this point, CPU 1's rdp->nocb_defer_wakeup is RCU_NOCB_WAKE.

Quoted 3 lines

Quoted 3 lines

7. The grace period ends, so rcu_gp_kthread awakens the

	->nocb_gp_kthread, which in turn awakens both CPU 1's and
	CPU 2's ->nocb_cb_kthread.

And then ->nocb_cb_kthread sleeps waiting for more callbacks.

Quoted 6 lines

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 1 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.
It does so with calling __wake_nocb_gp() which also cancels the

wake_nocb_gp() in current -rcu, correct?

Quoted 5 lines

Quoted 5 lines

So far so good, but why isn't the timer still queued from back in step 2? What am I missing here? Either way, could you please update the commit logs to tell the full story? At some later time, you might be very happy that you did. ;-)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

Would like to similarly expand this one, or would you prefer to rest your case on Case 1) above?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

Quoted 13 lines · Paul E. McKenney

Quoted 12 lines · Frederic Weisbecker

Quoted 11 lines · Paul E. McKenney

Quoted 9 lines · Frederic Weisbecker

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

As discussed offlist, expanding the above scenario results in this sequence of steps:

I renumbered the CPUs, since the ->nocb_gp_kthread would normally be associated with CPU 0. If the first CPU to enqueue a callback was also CPU 0, nocb_gp_wait() might clear that CPU's ->nocb_defer_wakeup, which would prevent this scenario from playing out. (But admittedly only if some other CPU handled by this same ->nocb_gp_kthread used its bypass.)

Ok good point.

Quoted 11 lines

Quoted 3 lines

Quoted 3 lines

1. There are no callbacks queued for any CPU covered by CPU 0-2's

	->nocb_gp_kthread.

And ->nocb_gp_kthread is associated with CPU 0.

Quoted 3 lines

Quoted 3 lines

2. CPU 1 enqueues its first callback with interrupts disabled, and

	thus must defer awakening its ->nocb_gp_kthread.  It therefore
	queues its rcu_data structure's ->nocb_timer.

At this point, CPU 1's rdp->nocb_defer_wakeup is RCU_NOCB_WAKE.

Right.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

7. The grace period ends, so rcu_gp_kthread awakens the

	->nocb_gp_kthread, which in turn awakens both CPU 1's and
	CPU 2's ->nocb_cb_kthread.

And then ->nocb_cb_kthread sleeps waiting for more callbacks.

Yep

Quoted 8 lines

Quoted 6 lines

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 1 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.
It does so with calling __wake_nocb_gp() which also cancels the

wake_nocb_gp() in current -rcu, correct?

Heh, right.

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

So far so good, but why isn't the timer still queued from back in step 2? What am I missing here? Either way, could you please update the commit logs to tell the full story? At some later time, you might be very happy that you did. ;-)

Quoted 4 lines

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

Quoted 2 lines

Would like to similarly expand this one, or would you prefer to rest your case on Case 1) above?

I was about to say that we can skip that one, the changelog will already be big enough but the "Fixes:" tag refers to the second scenario, since it's the oldest vulnerable commit AFAICS.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

On Tue, Mar 02, 2021 at 01:34:44PM +0100, Frederic Weisbecker wrote:

OK, how about if I queue a temporary commit (shown below) that just calls out the first scenario so that I can start testing, and you get me more detail on the second scenario? I can then update the commit.

Quoted 5 lines · Paul E. McKenney

On Tue, Mar 02, 2021 at 01:34:44PM +0100, Frederic Weisbecker wrote:

OK, how about if I queue a temporary commit (shown below) that just calls out the first scenario so that I can start testing, and you get me more detail on the second scenario? I can then update the commit.

Sure, meanwhile here is an attempt for a nocb_bypass_timer based scenario, it's overly hairy and perhaps I picture more power in the hands of callbacks advancing on nocb_cb_wait() than it really has:

0. CPU 0's ->nocb_cb_kthread just called rcu_do_batch() and
executed all the ready callbacks. Its segcblist is now entirely empty. It's preempted while calling local_bh_enable().

1. A new callback is enqueued on CPU 0 with IRQs enabled. So
the ->nocb_gp_kthread for CPU 0-2's is awaken. Then a storm of callbacks enqueue follows on CPU 0 and even reaches the bypass queue. Note that ->nocb_gp_kthread is also associated with CPU 0.

2. CPU 0 queues one last bypass callback.

3. The ->nocb_gp_kthread wakes up and associates a grace period
with the whole queue of regular callbacks on CPU 0. It also tries to flush the bypass queue of CPU 0 but the bypass lock is contended due to the concurrent enqueuing on the previous step 2, so the flush fails.

4. This ->nocb_gp_kthread arms its ->nocb_bypass_timer and goes
to sleep waiting for the end of this future grace period.

5. This grace period elapses before the ->nocb_bypass_timer timer
fires. This is normally improbably given that the timer is set for only two jiffies, but timers can be delayed. Besides, it is possible that kernel was built with CONFIG_RCU_STRICT_GRACE_PERIOD=y.

6. The grace period ends, so rcu_gp_kthread awakens the
->nocb_gp_kthread but it doesn't get a chance to run on a CPU before a while.

7. CPU 0's ->nocb_cb_kthread get back to the CPU after its preemption.
As it notices the new completed grace period, it advances the callbacks and executes them. Then it gets preempted again on local_bh_enabled().

8. A new callback enqueue on CPU 0 flushes itself the bypass queue

            because CPU 0's ->nocb_nobypass_count < nocb_nobypass_lim_per_jiffy.

9. CPUs from other ->nocb_gp_kthread groups (above CPU 2) initiate and

            elapse a few grace periods. CPU 0's ->nocb_gp_kthread still hasn't
            got an opportunity to run on a CPU and its ->nocb_bypass_timer still
            hasn't fired.

10. CPU 0's ->nocb_cb_kthread wakes up from preemption. It notices the
new grace periods that have elapsed, advance all the callbacks and executes them. Then it goes to sleep waiting for invocable callbacks.

11. CPU 0 enqueues a new callback with interrupts disabled, and

            defers awakening its ->nocb_gp_kthread even though ->nocb_gp_sleep
            is actually false. It therefore queues its rcu_data structure's
            ->nocb_timer. At this point, CPU 0's rdp->nocb_defer_wakeup is
            RCU_NOCB_WAKE.

12. The ->nocb_bypass_timer finally fires! It doesn't wake up

            ->nocb_gp_kthread because it's actually awaken already.
            But it cancels CPU 0's ->nocb_timer armed at 11. Yet it doesn't
            re-initialize CPU 0's ->nocb_defer_wakeup which stays with the
            stale RCU_NOCB_WAKE value. So CPU 0's->nocb_defer_wakeup and
            its ->nocb_timer are now desynchronized.

13. The ->nocb_gp_kthread finally runs. It cancels the ->nocb_bypass_timer
which has already fired. It sees the new callback on CPU 0 and associate it with a new grace period then sleep on it.

14. The grace period elapses, rcu_gp_kthread wakes up ->nocb_gb_kthread

            which wakes up CPU 0's->nocb_cb_kthread which runs the callback.
            Both ->nocb_gp_kthread and CPU 0's->nocb_cb_kthread now wait for new
            callbacks.

15. CPU 0 enqueues another callback, again with interrupts

            disabled so it must queue a timer for a deferred wakeup. However
            the value of its ->nocb_defer_wakeup is RCU_NOCB_WAKE which
            incorrectly indicates that a timer is already queued.  Instead,
            CPU 0's ->nocb_timer was cancelled in 12.  CPU 0 therefore fails
            to queue the ->nocb_timer.

16. CPU 0 has its pending callback and it may go unnoticed until
some other CPU ever wakes up ->nocb_gp_kthread or CPU 0 ever calls an explicit deferred wakeup, for example, during idle entry.

Thanks.

Quoted 11 lines · Frederic Weisbecker

Quoted 5 lines · Paul E. McKenney

On Tue, Mar 02, 2021 at 01:34:44PM +0100, Frederic Weisbecker wrote:

OK, how about if I queue a temporary commit (shown below) that just calls out the first scenario so that I can start testing, and you get me more detail on the second scenario? I can then update the commit.

Sure, meanwhile here is an attempt for a nocb_bypass_timer based scenario, it's overly hairy and perhaps I picture more power in the hands of callbacks advancing on nocb_cb_wait() than it really has:

Thank you very much!

I must defer looking through this in detail until I am more awake, but I do very much like the fine-grained exposition.

Thanx, Paul

Quoted 46 lines

0. CPU 0's ->nocb_cb_kthread just called rcu_do_batch() and
executed all the ready callbacks. Its segcblist is now entirely empty. It's preempted while calling local_bh_enable().

1. A new callback is enqueued on CPU 0 with IRQs enabled. So
the ->nocb_gp_kthread for CPU 0-2's is awaken. Then a storm of callbacks enqueue follows on CPU 0 and even reaches the bypass queue. Note that ->nocb_gp_kthread is also associated with CPU 0.

2. CPU 0 queues one last bypass callback.

3. The ->nocb_gp_kthread wakes up and associates a grace period
with the whole queue of regular callbacks on CPU 0. It also tries to flush the bypass queue of CPU 0 but the bypass lock is contended due to the concurrent enqueuing on the previous step 2, so the flush fails.

4. This ->nocb_gp_kthread arms its ->nocb_bypass_timer and goes
to sleep waiting for the end of this future grace period.

5. This grace period elapses before the ->nocb_bypass_timer timer
fires. This is normally improbably given that the timer is set for only two jiffies, but timers can be delayed. Besides, it is possible that kernel was built with CONFIG_RCU_STRICT_GRACE_PERIOD=y.

6. The grace period ends, so rcu_gp_kthread awakens the
->nocb_gp_kthread but it doesn't get a chance to run on a CPU before a while.

7. CPU 0's ->nocb_cb_kthread get back to the CPU after its preemption.
As it notices the new completed grace period, it advances the callbacks and executes them. Then it gets preempted again on local_bh_enabled().

8. A new callback enqueue on CPU 0 flushes itself the bypass queue

            because CPU 0's ->nocb_nobypass_count < nocb_nobypass_lim_per_jiffy.

9. CPUs from other ->nocb_gp_kthread groups (above CPU 2) initiate and

            elapse a few grace periods. CPU 0's ->nocb_gp_kthread still hasn't
            got an opportunity to run on a CPU and its ->nocb_bypass_timer still
            hasn't fired.

10. CPU 0's ->nocb_cb_kthread wakes up from preemption. It notices the
new grace periods that have elapsed, advance all the callbacks and executes them. Then it goes to sleep waiting for invocable callbacks.

Quoted 68 lines · Frederic Weisbecker

Quoted 13 lines · Paul E. McKenney

Quoted 12 lines · Frederic Weisbecker

Quoted 11 lines · Paul E. McKenney

Quoted 9 lines · Frederic Weisbecker

Two situations can cause a missed nocb timer rearm:

1) rdp(CPU A) queues its nocb timer. The grace period elapses before the timer get a chance to fire. The nocb_gp kthread is awaken by rdp(CPU B). The nocb_cb kthread for rdp(CPU A) is awaken and process the callbacks, again before the nocb_timer for CPU A get a chance to fire. rdp(CPU A) queues a callback and wakes up nocb_gp kthread, cancelling the pending nocb_timer without resetting the corresponding nocb_defer_wakeup.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

As discussed offlist, expanding the above scenario results in this sequence of steps:

I renumbered the CPUs, since the ->nocb_gp_kthread would normally be associated with CPU 0. If the first CPU to enqueue a callback was also CPU 0, nocb_gp_wait() might clear that CPU's ->nocb_defer_wakeup, which would prevent this scenario from playing out. (But admittedly only if some other CPU handled by this same ->nocb_gp_kthread used its bypass.)

Ok good point.

Quoted 11 lines

Quoted 3 lines

Quoted 3 lines

1. There are no callbacks queued for any CPU covered by CPU 0-2's

	->nocb_gp_kthread.

And ->nocb_gp_kthread is associated with CPU 0.

Quoted 3 lines

Quoted 3 lines

2. CPU 1 enqueues its first callback with interrupts disabled, and

	thus must defer awakening its ->nocb_gp_kthread.  It therefore
	queues its rcu_data structure's ->nocb_timer.

At this point, CPU 1's rdp->nocb_defer_wakeup is RCU_NOCB_WAKE.

Right.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

7. The grace period ends, so rcu_gp_kthread awakens the

	->nocb_gp_kthread, which in turn awakens both CPU 1's and
	CPU 2's ->nocb_cb_kthread.

And then ->nocb_cb_kthread sleeps waiting for more callbacks.

Yep

Quoted 8 lines

Quoted 6 lines

I managed to recollect some pieces of my brain. So keep the above but let's change the point 10:

10. CPU 1 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread.
It does so with calling __wake_nocb_gp() which also cancels the

wake_nocb_gp() in current -rcu, correct?

Heh, right.

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

So far so good, but why isn't the timer still queued from back in step 2? What am I missing here? Either way, could you please update the commit logs to tell the full story? At some later time, you might be very happy that you did. ;-)

Quoted 4 lines

2) The "nocb_bypass_timer" ends up calling wake_nocb_gp() which deletes the pending "nocb_timer" (note they are not the same timers) for the given rdp without resetting the matching state stored in nocb_defer wakeup.

Quoted 8 lines

Quoted 2 lines

Would like to similarly expand this one, or would you prefer to rest your case on Case 1) above?

I was about to say that we can skip that one, the changelog will already be big enough but the "Fixes:" tag refers to the second scenario, since it's the oldest vulnerable commit AFAICS.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

OK, how about if I queue a temporary commit (shown below) that just calls out the first scenario so that I can start testing, and you get me more detail on the second scenario? I can then update the commit.

Thanx, Paul

------------------------------------------------------------------------

commit 302fd54b9ae98f678624cbf9bf7a4ca88455a8f9

Author: Frederic Weisbecker <[email protected]>
Date:   Tue Feb 23 01:09:59 2021 +0100

rcu/nocb: Fix missed nocb_timer requeue

This sequence of events can lead to a failure to requeue a CPU's ->nocb_timer:

1. There are no callbacks queued for any CPU covered by CPU 0-2's
->nocb_gp_kthread. Note that ->nocb_gp_kthread is associated with CPU 0.

2. CPU 1 enqueues its first callback with interrupts disabled, and
thus must defer awakening its ->nocb_gp_kthread. It therefore queues its rcu_data structure's ->nocb_timer. At this point, CPU 1's rdp->nocb_defer_wakeup is RCU_NOCB_WAKE.

3. CPU 2, which shares the same ->nocb_gp_kthread, also enqueues a
callback, but with interrupts enabled, allowing it to directly awaken the ->nocb_gp_kthread.

4. The newly awakened ->nocb_gp_kthread associates both CPU 1's
and CPU 2's callbacks with a future grace period and arranges for that grace period to be started.

5. This ->nocb_gp_kthread goes to sleep waiting for the end of this
future grace period.

6. This grace period elapses before the CPU 1's timer fires.
This is normally improbably given that the timer is set for only one jiffy, but timers can be delayed. Besides, it is possible that kernel was built with CONFIG_RCU_STRICT_GRACE_PERIOD=y.

7. The grace period ends, so rcu_gp_kthread awakens the
->nocb_gp_kthread, which in turn awakens both CPU 1's and CPU 2's ->nocb_cb_kthread. Then ->nocb_gb_kthread sleeps waiting for more newly queued callbacks.

8. CPU 1's ->nocb_cb_kthread invokes its callback, then sleeps
waiting for more invocable callbacks.

9. Note that neither kthread updated any ->nocb_timer state,
so CPU 1's ->nocb_defer_wakeup is still set to RCU_NOCB_WAKE.

10. CPU 1 enqueues its second callback, this time with interrupts
enabled so it can wake directly ->nocb_gp_kthread. It does so with calling wake_nocb_gp() which also cancels the pending timer that got queued in step 2. But that doesn't reset CPU 1's ->nocb_defer_wakeup which is still set to RCU_NOCB_WAKE. So CPU 1's ->nocb_defer_wakeup and its ->nocb_timer are now desynchronized.

11. ->nocb_gp_kthread associates the callback queued in 10 with a new
grace period, arranges for that grace period to start and sleeps waiting for it to complete.

12. The grace period ends, rcu_gp_kthread awakens ->nocb_gp_kthread,
which in turn wakes up CPU 1's ->nocb_cb_kthread which then invokes the callback queued in 10.

13. CPU 1 enqueues its third callback, this time with interrupts
disabled so it must queue a timer for a deferred wakeup. However the value of its ->nocb_defer_wakeup is RCU_NOCB_WAKE which incorrectly indicates that a timer is already queued. Instead, CPU 1's ->nocb_timer was cancelled in 10. CPU 1 therefore fails to queue the ->nocb_timer.

14. CPU 1 has its pending callback and it may go unnoticed until
some other CPU ever wakes up ->nocb_gp_kthread or CPU 1 ever calls an explicit deferred wakeup, for example, during idle entry.

This commit fixes this bug by resetting rdp->nocb_defer_wakeup everytime we delete the ->nocb_timer.

Fixes d1b222c6be1f (rcu/nocb: Add bypass callback queueing)

Cc Stable <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcu/tree_plugin.h +5 -1

index 44746d8..429491d 100644
@@ -1721,7 +1721,11 @@ static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
  		rcu_nocb_unlock_irqrestore(rdp, flags);
  		return false;
  	}
-	del_timer(&rdp->nocb_timer);
+
+	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
+		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+		del_timer(&rdp->nocb_timer);
+	}
  	rcu_nocb_unlock_irqrestore(rdp, flags);
  	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
  	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {

Currently each offline rdp has its own nocb_timer armed when the nocb_gp wakeup must be deferred. This layout has many drawbacks, compared to a solution based on a single timer per rdp group:

* There are a lot of timers to maintain.

* The per-rdp ->nocb_lock must be held to queue and cancel the timer and this lock can already be quite contended.

* One timer firing doesn't cancel the other timers in the same group:
- These other timers can thus cause spurious wakeups
- Each rdp that queued a timer must lock both ->nocb_lock and then

    ->nocb_gp_lock upon exit from the kernel to idle/user/guest mode.

* We can't cancel all of them if we detect an unflushed bypass in nocb_gp_wait(). In fact currently we only ever cancel the nocb_timer of the leader group.

* The leader group's nocb_timer is cancelled without locking ->nocb_lock in nocb_gp_wait(). This currently appears to be safe but is an accident waiting to happen.

* Since the timer acquires ->nocb_lock, it requires extra care in the NOCB (de-)offloading process, requiring that it be either enabled or disabled and flushed.

This commit instead uses the rcuog kthread's CPU's ->nocb_timer instead. It is protected by nocb_gp_lock, which is _way_ less contended and remains so even after this change. As a matter of fact, the nocb_timer almost never fires and the deferred wakeup is mostly carried out upon idle/user/guest entry. Now the early check performed at this point in do_nocb_deferred_wakeup() is done on rdp_gp->nocb_defer_wakeup, which is of course racy. However, this raciness is harmless because we only need the guarantee that the timer is queued if we were the last one to queue it. Any other situation (another CPU has queued it and we either see it or not) is fine.

This solves all the issues listed above.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

---
 kernel/rcu/tree.h        |   1 -
 kernel/rcu/tree_plugin.h | 142 +++++++++++++++++++++------------------
 2 files changed, 78 insertions(+), 65 deletions(-)

kernel/rcu/tree.h +0 -1

index 71821d59d95c..b280a843bd2c 100644
@@ -257,7 +257,6 @@ struct rcu_data {
 };
 
 /* Values for nocb_defer_wakeup field in struct rcu_data. */
-#define RCU_NOCB_WAKE_OFF	-1
 #define RCU_NOCB_WAKE_NOT	0
 #define RCU_NOCB_WAKE		1
 #define RCU_NOCB_WAKE_FORCE	2

kernel/rcu/tree_plugin.h +5 -21

index 587df271d640..847636d3e93d 100644
@@ -33,10 +33,6 @@ static inline bool rcu_current_is_nocb_kthread(struct rcu_data *rdp)
 	return false;
 }
 
-static inline bool rcu_running_nocb_timer(struct rcu_data *rdp)
-{
-	return (timer_curr_running(&rdp->nocb_timer) && !in_irq());
-}
 #else
 static inline int rcu_lockdep_is_held_nocb(struct rcu_data *rdp)
 {

@@ -48,11 +44,6 @@ static inline bool rcu_current_is_nocb_kthread(struct rcu_data *rdp)
 	return false;
 }
 
-static inline bool rcu_running_nocb_timer(struct rcu_data *rdp)
-{
-	return false;
-}
-
 #endif /* #ifdef CONFIG_RCU_NOCB_CPU */
 
 static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)

@@ -72,8 +63,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
 		  rcu_lockdep_is_held_nocb(rdp) ||
 		  (rdp == this_cpu_ptr(&rcu_data) &&
 		   !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
-		  rcu_current_is_nocb_kthread(rdp) ||
-		  rcu_running_nocb_timer(rdp)),
+		  rcu_current_is_nocb_kthread(rdp)),
 		"Unsafe read of RCU_NOCB offloaded state"
 	);
 

@@ -1702,43 +1692,50 @@ bool rcu_is_nocb_cpu(int cpu)
 	return false;
 }
 
-/*
- * Kick the GP kthread for this NOCB group.  Caller holds ->nocb_lock
- * and this function releases it.
- */
-static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
-			 unsigned long flags)
-	__releases(rdp->nocb_lock)
+static bool __wake_nocb_gp(struct rcu_data *rdp_gp,
+			   struct rcu_data *rdp,
+			   bool force, unsigned long flags)
+	__releases(rdp_gp->nocb_gp_lock)
 {
 	bool needwake = false;
-	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
 
-	lockdep_assert_held(&rdp->nocb_lock);
 	if (!READ_ONCE(rdp_gp->nocb_gp_kthread)) {
-		rcu_nocb_unlock_irqrestore(rdp, flags);

kernel/rcu/tree_plugin.h +40 -15

+		WRITE_ONCE(rdp_gp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+		del_timer(&rdp_gp->nocb_timer);
 	}
-	rcu_nocb_unlock_irqrestore(rdp, flags);
-	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
+
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {
 		WRITE_ONCE(rdp_gp->nocb_gp_sleep, false);
 		needwake = true;
+	}
+	raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
+	if (needwake) {
 		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DoWake"));
-	}
-	raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
-	if (needwake)
 		wake_up_process(rdp_gp->nocb_gp_kthread);
+	}
 
 	return needwake;
 }
 
+/*
+ * Kick the GP kthread for this NOCB group.
+ */
+static bool wake_nocb_gp(struct rcu_data *rdp, bool force)
+{
+	unsigned long flags;
+	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
+
+	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
+	return __wake_nocb_gp(rdp_gp, rdp, force, flags);
+}
+
 /*
  * Arrange to wake the GP kthread for this NOCB group at some future
  * time when it is safe to do so.

@@ -1746,12 +1743,18 @@ static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
 static void wake_nocb_gp_defer(struct rcu_data *rdp, int waketype,
 			       const char *reason)
 {
-	if (rdp->nocb_defer_wakeup == RCU_NOCB_WAKE_OFF)
-		return;
-	if (rdp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT)
-		mod_timer(&rdp->nocb_timer, jiffies + 1);
-	if (rdp->nocb_defer_wakeup < waketype)
-		WRITE_ONCE(rdp->nocb_defer_wakeup, waketype);
+	unsigned long flags;
+	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
+
+	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
+
+	if (rdp_gp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT)
+		mod_timer(&rdp_gp->nocb_timer, jiffies + 1);
+	if (rdp_gp->nocb_defer_wakeup < waketype)
+		WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
+
+	raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
+
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, reason);
 }
 

@@ -1978,13 +1981,14 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
 		rdp->qlen_last_fqs_check = len;
 		if (!irqs_disabled_flags(flags)) {
 			/* ... if queue was empty ... */
-			wake_nocb_gp(rdp, false, flags);
+			rcu_nocb_unlock_irqrestore(rdp, flags);
+			wake_nocb_gp(rdp, false);
 			trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
 					    TPS("WakeEmpty"));
 		} else {
+			rcu_nocb_unlock_irqrestore(rdp, flags);
 			wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE,
 					   TPS("WakeEmptyIsDeferred"));
-			rcu_nocb_unlock_irqrestore(rdp, flags);
 		}
 	} else if (len > rdp->qlen_last_fqs_check + qhimark) {
 		/* ... or if many callbacks queued. */

@@ -1999,10 +2003,14 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
 		smp_mb(); /* Enqueue before timer_pending(). */
 		if ((rdp->nocb_cb_sleep ||
 		     !rcu_segcblist_ready_cbs(&rdp->cblist)) &&
-		    !timer_pending(&rdp->nocb_bypass_timer))
+		    !timer_pending(&rdp->nocb_bypass_timer)) {
+			rcu_nocb_unlock_irqrestore(rdp, flags);
 			wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_FORCE,
 					   TPS("WakeOvfIsDeferred"));
-		rcu_nocb_unlock_irqrestore(rdp, flags);
+		} else {
+			rcu_nocb_unlock_irqrestore(rdp, flags);
+			trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("WakeNot"));
+		}
 	} else {
 		rcu_nocb_unlock_irqrestore(rdp, flags);
 		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("WakeNot"));

kernel/rcu/tree_plugin.h +1 -1

+
 		// Advance callbacks if helpful and low contention.
 		needwake_gp = false;
 		if (!rcu_segcblist_restempty(&rdp->cblist,

@@ -2178,11 +2182,18 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
 	my_rdp->nocb_gp_bypass = bypass;
 	my_rdp->nocb_gp_gp = needwait_gp;
 	my_rdp->nocb_gp_seq = needwait_gp ? wait_gp_seq : 0;
-	if (bypass && !rcu_nocb_poll) {

kernel/rcu/tree_plugin.h +13 -5

+		if (!rcu_nocb_poll) {
+			// At least one child with non-empty ->nocb_bypass, so set
+			// timer in order to avoid stranding its callbacks.
+			mod_timer(&my_rdp->nocb_bypass_timer, j + 2);
+		}
 		raw_spin_unlock_irqrestore(&my_rdp->nocb_gp_lock, flags);
 	}
 	if (rcu_nocb_poll) {

@@ -2356,15 +2367,18 @@ static bool do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 {
 	unsigned long flags;
 	int ndw;
+	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
 	int ret;
 
-	rcu_nocb_lock_irqsave(rdp, flags);
-	if (!rcu_nocb_need_deferred_wakeup(rdp)) {
-		rcu_nocb_unlock_irqrestore(rdp, flags);
+	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
+
+	if (!rcu_nocb_need_deferred_wakeup(rdp_gp)) {
+		raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
 		return false;
 	}
-	ndw = READ_ONCE(rdp->nocb_defer_wakeup);
-	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
+
+	ndw = rdp_gp->nocb_defer_wakeup;
+	ret = __wake_nocb_gp(rdp_gp, rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));
 
 	return ret;

kernel/rcu/tree_plugin.h +9 -10

+
+	if (rcu_nocb_need_deferred_wakeup(rdp->nocb_gp_rdp))
 		return do_nocb_deferred_wakeup_common(rdp);
 	return false;
 }

@@ -2454,17 +2471,15 @@ static long rcu_nocb_rdp_deoffload(void *arg)
 	swait_event_exclusive(rdp->nocb_state_wq,
 			      !rcu_segcblist_test_flags(cblist, SEGCBLIST_KTHREAD_CB |
 							SEGCBLIST_KTHREAD_GP));
-	rcu_nocb_lock_irqsave(rdp, flags);
-	/* Make sure nocb timer won't stay around */
-	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_OFF);
-	rcu_nocb_unlock_irqrestore(rdp, flags);
-	del_timer_sync(&rdp->nocb_timer);
-
 	/*
-	 * Theoretically we could set SEGCBLIST_SOFTIRQ_ONLY with CB unlocked
-	 * and IRQs disabled but let's be paranoid.
+	 * Lock one last time to acquire latest callback updates from kthreads
+	 * so we can later handle callbacks locally without locking.
 	 */
 	rcu_nocb_lock_irqsave(rdp, flags);
+	/*
+	 * Theoretically we could set SEGCBLIST_SOFTIRQ_ONLY after the nocb
+	 * lock is released but how about being paranoid for once?
+	 */
 	rcu_segcblist_set_flags(cblist, SEGCBLIST_SOFTIRQ_ONLY);
 	/*
 	 * With SEGCBLIST_SOFTIRQ_ONLY, we can't use

@@ -2528,8 +2543,7 @@ static long rcu_nocb_rdp_offload(void *arg)
 	 * SEGCBLIST_SOFTIRQ_ONLY mode.
 	 */
 	raw_spin_lock_irqsave(&rdp->nocb_lock, flags);
-	/* Re-enable nocb timer */
-	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+
 	/*
 	 * We didn't take the nocb lock while working on the
 	 * rdp->cblist in SEGCBLIST_SOFTIRQ_ONLY mode.

Signature · 1 line

2.25.1

The first question is of course: Did you try this with lockdep enabled? ;-)

Quoted 22 lines

kernel/rcu/tree_plugin.h +4 -10

@@ -1702,43 +1692,50 @@ bool rcu_is_nocb_cpu(int cpu)
 	return false;
 }
 
-/*
- * Kick the GP kthread for this NOCB group.  Caller holds ->nocb_lock
- * and this function releases it.
- */
-static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
-			 unsigned long flags)
-	__releases(rdp->nocb_lock)
+static bool __wake_nocb_gp(struct rcu_data *rdp_gp,
+			   struct rcu_data *rdp,
+			   bool force, unsigned long flags)
+	__releases(rdp_gp->nocb_gp_lock)
 {
 	bool needwake = false;
-	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
 
-	lockdep_assert_held(&rdp->nocb_lock);
 	if (!READ_ONCE(rdp_gp->nocb_gp_kthread)) {
-		rcu_nocb_unlock_irqrestore(rdp, flags);

kernel/rcu/tree_plugin.h bool rcu_is_nocb_cpu(int cpu) · line 1709

Quoted 10 lines

kernel/rcu/tree_plugin.h +2 -3

+		raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
 		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
 				    TPS("AlreadyAwake"));
 		return false;
 	}
 
-	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
-		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
-		del_timer(&rdp->nocb_timer);
+	if (rdp_gp->nocb_defer_wakeup > RCU_NOCB_WAKE_NOT) {

So there are no longer any data races involving ->nocb_defer_wakeup?

(Yes, I could fire up KCSAN, but my KCSAN-capable system is otherwise occupied for several more hours.)

Quoted 14 lines

kernel/rcu/tree_plugin.h +2 -4

+
 		// Advance callbacks if helpful and low contention.
 		needwake_gp = false;
 		if (!rcu_segcblist_restempty(&rdp->cblist,

@@ -2178,11 +2182,18 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
 	my_rdp->nocb_gp_bypass = bypass;
 	my_rdp->nocb_gp_gp = needwait_gp;
 	my_rdp->nocb_gp_seq = needwait_gp ? wait_gp_seq : 0;
-	if (bypass && !rcu_nocb_poll) {
-		// At least one child with non-empty ->nocb_bypass, so set
-		// timer in order to avoid stranding its callbacks.
+	if (bypass) {
 		raw_spin_lock_irqsave(&my_rdp->nocb_gp_lock, flags);
-		mod_timer(&my_rdp->nocb_bypass_timer, j + 2);

kernel/rcu/tree_plugin.h static void nocb_gp_wait(struct rcu_data *my_rdp) · line 2191

Quoted 5 lines

kernel/rcu/tree_plugin.h +5 -0

+		// Avoid race with first bypass CB.
+		if (my_rdp->nocb_defer_wakeup > RCU_NOCB_WAKE_NOT) {
+			WRITE_ONCE(my_rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+			del_timer(&my_rdp->nocb_timer);
+		}

Given that the timer does not get queued if rcu_nocb_poll, why not move the above "if" statement under the one following?

Quoted 11 lines

kernel/rcu/tree_plugin.h +5 -0

+		if (!rcu_nocb_poll) {
+			// At least one child with non-empty ->nocb_bypass, so set
+			// timer in order to avoid stranding its callbacks.
+			mod_timer(&my_rdp->nocb_bypass_timer, j + 2);
+		}
 		raw_spin_unlock_irqrestore(&my_rdp->nocb_gp_lock, flags);
 	}
 	if (rcu_nocb_poll) {

@@ -2385,7 +2399,10 @@ static void do_nocb_deferred_wakeup_timer(struct timer_list *t)
  */

kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_timer(struct timer_list *t) · line 2403

Quoted 5 lines

kernel/rcu/tree_plugin.h +2 -1

 static bool do_nocb_deferred_wakeup(struct rcu_data *rdp)
 {
-	if (rcu_nocb_need_deferred_wakeup(rdp))
+	if (!rdp->nocb_gp_rdp)
+		return false;

This check was not necessary previously because each CPU used its own rdp, correct?

The theory is that this early return is taken only during boot, and that the spawning of the kthreads will act as an implicit wakeup?

Quoted 1 line · Paul E. McKenney

The first question is of course: Did you try this with lockdep enabled? ;-)

Yep I always do. But I may miss some configs on my testings. I usually test at least TREE01 on x86 and arm64.

Quoted 35 lines

Quoted 22 lines

kernel/rcu/tree_plugin.h +4 -10

@@ -1702,43 +1692,50 @@ bool rcu_is_nocb_cpu(int cpu)
 	return false;
 }
 
-/*
- * Kick the GP kthread for this NOCB group.  Caller holds ->nocb_lock
- * and this function releases it.
- */
-static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
-			 unsigned long flags)
-	__releases(rdp->nocb_lock)
+static bool __wake_nocb_gp(struct rcu_data *rdp_gp,
+			   struct rcu_data *rdp,
+			   bool force, unsigned long flags)
+	__releases(rdp_gp->nocb_gp_lock)
 {
 	bool needwake = false;
-	struct rcu_data *rdp_gp = rdp->nocb_gp_rdp;
 
-	lockdep_assert_held(&rdp->nocb_lock);
 	if (!READ_ONCE(rdp_gp->nocb_gp_kthread)) {
-		rcu_nocb_unlock_irqrestore(rdp, flags);

kernel/rcu/tree_plugin.h bool rcu_is_nocb_cpu(int cpu) · line 1709

Quoted 10 lines

kernel/rcu/tree_plugin.h +2 -3

+		raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
 		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
 				    TPS("AlreadyAwake"));
 		return false;
 	}
 
-	if (READ_ONCE(rdp->nocb_defer_wakeup) > RCU_NOCB_WAKE_NOT) {
-		WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
-		del_timer(&rdp->nocb_timer);
+	if (rdp_gp->nocb_defer_wakeup > RCU_NOCB_WAKE_NOT) {

So there are no longer any data races involving ->nocb_defer_wakeup?

Quoted 2 lines

(Yes, I could fire up KCSAN, but my KCSAN-capable system is otherwise occupied for several more hours.)

To be more specific, there is no more unlocked write to the timer (queue/cancel) and its nocb_defer_wakeup matching state. And there is only one (on purpose) racy reader of ->nocb_defer_wakeup which is the non-timer deferred wakeup.

So the writes to the timer keep their WRITE_ONCE() and only the reader in do_nocb_deferred_wakeup() keeps its READ_ONCE(). Other readers are protected by the ->nocb_gp_lock.

Quoted 14 lines

Quoted 14 lines

kernel/rcu/tree_plugin.h +2 -4

+
 		// Advance callbacks if helpful and low contention.
 		needwake_gp = false;
 		if (!rcu_segcblist_restempty(&rdp->cblist,

@@ -2178,11 +2182,18 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
 	my_rdp->nocb_gp_bypass = bypass;
 	my_rdp->nocb_gp_gp = needwait_gp;
 	my_rdp->nocb_gp_seq = needwait_gp ? wait_gp_seq : 0;
-	if (bypass && !rcu_nocb_poll) {
-		// At least one child with non-empty ->nocb_bypass, so set
-		// timer in order to avoid stranding its callbacks.
+	if (bypass) {
 		raw_spin_lock_irqsave(&my_rdp->nocb_gp_lock, flags);
-		mod_timer(&my_rdp->nocb_bypass_timer, j + 2);

Quoted 8 lines

kernel/rcu/tree_plugin.h static void nocb_gp_wait(struct rcu_data *my_rdp) · line 2191

Quoted 5 lines

kernel/rcu/tree_plugin.h +5 -0

+		// Avoid race with first bypass CB.
+		if (my_rdp->nocb_defer_wakeup > RCU_NOCB_WAKE_NOT) {
+			WRITE_ONCE(my_rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+			del_timer(&my_rdp->nocb_timer);
+		}

Given that the timer does not get queued if rcu_nocb_poll, why not move the above "if" statement under the one following?

It's done later in the set.

Quoted 11 lines

Quoted 11 lines

kernel/rcu/tree_plugin.h +5 -0

+		if (!rcu_nocb_poll) {
+			// At least one child with non-empty ->nocb_bypass, so set
+			// timer in order to avoid stranding its callbacks.
+			mod_timer(&my_rdp->nocb_bypass_timer, j + 2);
+		}
 		raw_spin_unlock_irqrestore(&my_rdp->nocb_gp_lock, flags);
 	}
 	if (rcu_nocb_poll) {

@@ -2385,7 +2399,10 @@ static void do_nocb_deferred_wakeup_timer(struct timer_list *t)
  */

Quoted 8 lines

kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_timer(struct timer_list *t) · line 2403

Quoted 5 lines

kernel/rcu/tree_plugin.h +2 -1

 static bool do_nocb_deferred_wakeup(struct rcu_data *rdp)
 {
-	if (rcu_nocb_need_deferred_wakeup(rdp))
+	if (!rdp->nocb_gp_rdp)
+		return false;

This check was not necessary previously because each CPU used its own rdp, correct?

Exactly!

Quoted 2 lines

The theory is that this early return is taken only during boot, and that the spawning of the kthreads will act as an implicit wakeup?

You guessed right! That probably deserve a comment.

A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() is going to check again the bypass state and rearm the bypass timer if necessary.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

---
 kernel/rcu/tree_plugin.h | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/tree_plugin.h +2 -0

index b62ad79bbda5..9da67b0d3997 100644
@@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp,
 		del_timer(&rdp_gp->nocb_timer);
 	}
 
+	del_timer(&rdp_gp->nocb_bypass_timer);
+
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {
 		WRITE_ONCE(rdp_gp->nocb_gp_sleep, false);
 		needwake = true;

Signature · 1 line

2.25.1

Quoted 10 lines · Frederic Weisbecker

A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() is going to check again the bypass state and rearm the bypass timer if necessary.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Give that you delete this code a couple of patches later in this series, why not just leave it out entirely? ;-)

Thanx, Paul

Quoted 12 lines

---
 kernel/rcu/tree_plugin.h | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/tree_plugin.h +0 -0

index b62ad79bbda5..9da67b0d3997 100644
@@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp,
 		del_timer(&rdp_gp->nocb_timer);
 	}
 

kernel/rcu/tree_plugin.h static bool __wake_nocb_gp(struct rcu_data *rdp_gp, · line 1718

Quoted 5 lines

kernel/rcu/tree_plugin.h +2 -0

+	del_timer(&rdp_gp->nocb_bypass_timer);
+
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {
 		WRITE_ONCE(rdp_gp->nocb_gp_sleep, false);
 		needwake = true;

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Frederic Weisbecker

A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() is going to check again the bypass state and rearm the bypass timer if necessary.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Quoted 2 lines

Give that you delete this code a couple of patches later in this series, why not just leave it out entirely? ;-)

It's not exactly deleted later, it's rather merged within the "del_timer(&rdp_gp->nocb_timer)".

The purpose of that patch is to make it clear that we explicitly cancel the nocb_bypass_timer here before we do it implicitly later with the merge of nocb_bypass_timer into nocb_timer.

We could drop that patch, the resulting code in the end of the patchset will be the same of course but the behaviour detail described here might slip out of the reviewers attention :-)

Quoted 15 lines

Thanx, Paul

Quoted 12 lines

---
 kernel/rcu/tree_plugin.h | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/tree_plugin.h +0 -0

index b62ad79bbda5..9da67b0d3997 100644
@@ -1711,6 +1711,8 @@ static bool __wake_nocb_gp(struct rcu_data *rdp_gp,
 		del_timer(&rdp_gp->nocb_timer);
 	}
 

Quoted 5 lines

kernel/rcu/tree_plugin.h static bool __wake_nocb_gp(struct rcu_data *rdp_gp, · line 1718

Quoted 5 lines

kernel/rcu/tree_plugin.h +2 -0

+	del_timer(&rdp_gp->nocb_bypass_timer);
+
 	if (force || READ_ONCE(rdp_gp->nocb_gp_sleep)) {
 		WRITE_ONCE(rdp_gp->nocb_gp_sleep, false);
 		needwake = true;

Thanks.

Quoted 23 lines · Frederic Weisbecker

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Frederic Weisbecker

A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() is going to check again the bypass state and rearm the bypass timer if necessary.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Quoted 2 lines

Give that you delete this code a couple of patches later in this series, why not just leave it out entirely? ;-)

It's not exactly deleted later, it's rather merged within the "del_timer(&rdp_gp->nocb_timer)".

The purpose of that patch is to make it clear that we explicitly cancel the nocb_bypass_timer here before we do it implicitly later with the merge of nocb_bypass_timer into nocb_timer.

Quoted 4 lines

We could drop that patch, the resulting code in the end of the patchset will be the same of course but the behaviour detail described here might slip out of the reviewers attention :-)

How about merging the timers first and adding those small improvements later? i.e. move patch #12 #13 right after #7 (IIUC, #7 is the last requirement you need for merging timers)

, and then patch #8~#11 just follow

Just my 2 cents. The overall patchset looks good to me ;-)

Feel free to add

Reviewed-by Boqun Feng <[email protected]>

Quoted 24 lines · Boqun Feng

Quoted 23 lines · Frederic Weisbecker

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Frederic Weisbecker

A NOCB-gp wake up can safely delete the nocb_bypass_timer. nocb_gp_wait() is going to check again the bypass state and rearm the bypass timer if necessary.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

Quoted 2 lines

Give that you delete this code a couple of patches later in this series, why not just leave it out entirely? ;-)

It's not exactly deleted later, it's rather merged within the "del_timer(&rdp_gp->nocb_timer)".

The purpose of that patch is to make it clear that we explicitly cancel the nocb_bypass_timer here before we do it implicitly later with the merge of nocb_bypass_timer into nocb_timer.

Quoted 8 lines

Quoted 4 lines

We could drop that patch, the resulting code in the end of the patchset will be the same of course but the behaviour detail described here might slip out of the reviewers attention :-)

How about merging the timers first and adding those small improvements later? i.e. move patch #12 #13 right after #7 (IIUC, #7 is the last requirement you need for merging timers)

Hmm, nope, patches 9 and 10 are actually preparation work for timers merge. In fact they could even be skipped and timers could be merged directly but I wanted the unified behaviour to be fully explicit for reviewers through those incremental changes before merging the timers together.

Quoted 1 line

, and then patch #8~#11 just follow

Patch 8 really need to stay where it is because it is an important limitation on nocb de-offloading that can be removed right after patch 7 (which itself removes the sole reason for rdp leader to remain nocb) and it doesn't depend on the timers unification that comes after.

Quoted 1 line

Quoted 5 lines

Just my 2 cents. The overall patchset looks good to me ;-)

Feel free to add

Reviewed-by Boqun Feng <[email protected]>

Thanks a lot for checking that!

No need to disarm the nocb_timer if rcu_nocb is polling because it shouldn't be armed either.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

---
 kernel/rcu/tree_plugin.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

kernel/rcu/tree_plugin.h +7 -7

index 9da67b0d3997..d8b50ff40e4b 100644
@@ -2186,18 +2186,18 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
 	my_rdp->nocb_gp_gp = needwait_gp;
 	my_rdp->nocb_gp_seq = needwait_gp ? wait_gp_seq : 0;
 	if (bypass) {
-		raw_spin_lock_irqsave(&my_rdp->nocb_gp_lock, flags);
-		// Avoid race with first bypass CB.
-		if (my_rdp->nocb_defer_wakeup > RCU_NOCB_WAKE_NOT) {
-			WRITE_ONCE(my_rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
-			del_timer(&my_rdp->nocb_timer);
-		}
 		if (!rcu_nocb_poll) {
+			raw_spin_lock_irqsave(&my_rdp->nocb_gp_lock, flags);
+			// Avoid race with first bypass CB.
+			if (my_rdp->nocb_defer_wakeup > RCU_NOCB_WAKE_NOT) {
+				WRITE_ONCE(my_rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
+				del_timer(&my_rdp->nocb_timer);
+			}
 			// At least one child with non-empty ->nocb_bypass, so set
 			// timer in order to avoid stranding its callbacks.
 			mod_timer(&my_rdp->nocb_bypass_timer, j + 2);
+			raw_spin_unlock_irqrestore(&my_rdp->nocb_gp_lock, flags);
 		}
-		raw_spin_unlock_irqrestore(&my_rdp->nocb_gp_lock, flags);
 	}
 	if (rcu_nocb_poll) {
 		/* Polling, so trace if first poll in the series. */

Signature · 1 line

2.25.1

Quoted 9 lines · Frederic Weisbecker

No need to disarm the nocb_timer if rcu_nocb is polling because it shouldn't be armed either.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc Neeraj Upadhyay <[email protected]>

Cc Boqun Feng <[email protected]>

OK, so it does make sense to move that del_timer() under the following "if" statement, then. ;-)

Provide a way to tune the deferred wakeup level we want to perform from a safe wakeup point. Currently those sites are:

* nocb_timer
* user/idle/guest entry
* CPU down
* softirq/rcuc

All of these sites perform the wake up for both RCU_NOCB_WAKE and RCU_NOCB_WAKE_FORCE.

In order to merge nocb_timer and nocb_bypass_timer together, we plan to add a new RCU_NOCB_WAKE_BYPASS that really want to be deferred until a timer fires so that we don't wake up the NOCB-gp kthread too early.

To prepare for that, integrate a wake up level/limit that a callsite is deemed to perform.

Quoted 11 lines · Frederic Weisbecker

Provide a way to tune the deferred wakeup level we want to perform from a safe wakeup point. Currently those sites are:

* nocb_timer
* user/idle/guest entry
* CPU down
* softirq/rcuc

All of these sites perform the wake up for both RCU_NOCB_WAKE and RCU_NOCB_WAKE_FORCE.

Quoted 6 lines

In order to merge nocb_timer and nocb_bypass_timer together, we plan to add a new RCU_NOCB_WAKE_BYPASS that really want to be deferred until a timer fires so that we don't wake up the NOCB-gp kthread too early.

To prepare for that, integrate a wake up level/limit that a callsite is deemed to perform.

This appears to need the following in order to build for non-NOCB configurations. I will fold it in and am retesting.

Thanx, Paul

------------------------------------------------------------------------

commit 55f59dd75a11455cf558fd387fbf9011017dcc8a

Author: Paul E. McKenney <[email protected]>
Date:   Mon Mar 15 20:00:34 2021 -0700

squash! rcu/nocb: Prepare for fine-grained deferred wakeup

[ paulmck: Fix non-NOCB rcu_nocb_need_deferred_wakeup() definition. ]

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcu/tree_plugin.h +0 -0

index 0cc7f68..dfb048e 100644

Quoted 43 lines · Paul E. McKenney

Quoted 11 lines · Frederic Weisbecker

Provide a way to tune the deferred wakeup level we want to perform from a safe wakeup point. Currently those sites are:

* nocb_timer
* user/idle/guest entry
* CPU down
* softirq/rcuc

All of these sites perform the wake up for both RCU_NOCB_WAKE and RCU_NOCB_WAKE_FORCE.

Quoted 6 lines

In order to merge nocb_timer and nocb_bypass_timer together, we plan to add a new RCU_NOCB_WAKE_BYPASS that really want to be deferred until a timer fires so that we don't wake up the NOCB-gp kthread too early.

To prepare for that, integrate a wake up level/limit that a callsite is deemed to perform.

This appears to need the following in order to build for non-NOCB configurations. I will fold it in and am retesting.

Thanx, Paul

------------------------------------------------------------------------

commit 55f59dd75a11455cf558fd387fbf9011017dcc8a

Author: Paul E. McKenney <[email protected]>
Date:   Mon Mar 15 20:00:34 2021 -0700

squash! rcu/nocb: Prepare for fine-grained deferred wakeup

[ paulmck: Fix non-NOCB rcu_nocb_need_deferred_wakeup() definition. ]

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcu/tree_plugin.h +0 -0

index 0cc7f68..dfb048e 100644
@@ -2926,7 +2926,7 @@ static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp)
 {
 }
 

kernel/rcu/tree_plugin.h static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp) · line 2932

Quoted 5 lines

kernel/rcu/tree_plugin.h +1 -1

-static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp)
+static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp, int level)
 {
 	return false;
 }

Oops thanks, I missed that.