linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
@ 2020-09-29 19:29 Joel Fernandes (Google)
  2020-09-29 19:29 ` [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design Joel Fernandes (Google)
  2020-10-02  4:39 ` [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already Paul E. McKenney
  0 siblings, 2 replies; 13+ messages in thread
From: Joel Fernandes (Google) @ 2020-09-29 19:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Paul E . McKenney, Neeraj Upadhyay, Jonathan Corbet,
	Josh Triplett, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
	Mauro Carvalho Chehab, Randy Dunlap, rcu, Steven Rostedt,
	Will Deacon

Currently, rcu_cpu_starting() checks to see if the RCU core expects a
quiescent state from the incoming CPU.  However, the current interaction
between RCU quiescent-state reporting and CPU-hotplug operations should
mean that the incoming CPU never needs to report a quiescent state.
First, the outgoing CPU reports a quiescent state if needed.  Second,
the race where the CPU is leaving just as RCU is initializing a new
grace period is handled by an explicit check for this condition.  Third,
the CPU's leaf rcu_node structure's ->lock serializes these checks.

This means that if rcu_cpu_starting() ever feels the need to report
a quiescent state, then there is a bug somewhere in the CPU hotplug
code or the RCU grace-period handling code.  This commit therefore
adds a WARN_ON_ONCE() to bring that bug to everyone's attention.

Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
Suggested-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 55d3700dd1e7..5efe0a98ea45 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4119,7 +4119,9 @@ void rcu_cpu_starting(unsigned int cpu)
 	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
 	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
 	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
-	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
+
+	/* An incoming CPU should never be blocking a grace period. */
+	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
 		rcu_disable_urgency_upon_qs(rdp);
 		/* Report QS -after- changing ->qsmaskinitnext! */
 		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
-- 
2.28.0.709.gb0816b6eb0-goog


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

* [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design
  2020-09-29 19:29 [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already Joel Fernandes (Google)
@ 2020-09-29 19:29 ` Joel Fernandes (Google)
  2020-09-29 19:32   ` Joel Fernandes
  2020-10-02  4:39 ` [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already Paul E. McKenney
  1 sibling, 1 reply; 13+ messages in thread
From: Joel Fernandes (Google) @ 2020-09-29 19:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Jonathan Corbet, Josh Triplett, Lai Jiangshan, linux-doc,
	Mathieu Desnoyers, Mauro Carvalho Chehab, Neeraj Upadhyay,
	Paul E. McKenney, Randy Dunlap, rcu, Steven Rostedt, Will Deacon

RCU's hotplug design will help understand the requirements an RCU
implementation needs to fullfill, such as dead-lock avoidance.

The rcu_barrier() section of the "Hotplug CPU" section already talks
about deadlocks, however the description of what else can deadlock other
than rcu_barrier is rather incomplete.

This commit therefore continues the section by describing how RCU's
design handles CPU hotplug in a deadlock-free way.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 .../RCU/Design/Requirements/Requirements.rst  | 30 +++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
index 1ae79a10a8de..e0413aa989dd 100644
--- a/Documentation/RCU/Design/Requirements/Requirements.rst
+++ b/Documentation/RCU/Design/Requirements/Requirements.rst
@@ -1929,8 +1929,10 @@ The Linux-kernel CPU-hotplug implementation has notifiers that are used
 to allow the various kernel subsystems (including RCU) to respond
 appropriately to a given CPU-hotplug operation. Most RCU operations may
 be invoked from CPU-hotplug notifiers, including even synchronous
-grace-period operations such as ``synchronize_rcu()`` and
-``synchronize_rcu_expedited()``.
+grace-period operations such as. However, the synchronous variants
+(``synchronize_rcu()`` and ``synchronize_rcu_expedited()``) should not
+from notifiers that execute via ``stop_machine()`` -- specifically those
+between the ``CPUHP_AP_OFFLINE`` and ``CPUHP_AP_ONLINE`` states.
 
 However, all-callback-wait operations such as ``rcu_barrier()`` are also
 not supported, due to the fact that there are phases of CPU-hotplug
@@ -1940,6 +1942,30 @@ deadlock. Furthermore, ``rcu_barrier()`` blocks CPU-hotplug operations
 during its execution, which results in another type of deadlock when
 invoked from a CPU-hotplug notifier.
 
+Also, RCU's implementation avoids serious deadlocks which could occur due to
+interaction between hotplug, timers and grace period processing. It does so by
+maintaining its own books of every CPU's hotplug state, independent of
+the existing general-purpose CPU masks and by reporting quiescent states
+explictly when an online CPU is going down. Due to this design, the force
+quiescent state loop (FQS) is not required to report quiescent states for
+offline CPUs, like it does for idle CPUs, but it does splat if offline CPUs are
+stalling the RCU grace period for too long.
+
+For an offline CPU, the quiescent state will be reported in either of:
+1. During CPU offlining, using RCU's hotplug notifier (``rcu_report_dead()``).
+2. During grace period initialization (``rcu_gp_init()``) if it detected a race
+   with CPU offlining, or a race with a task unblocking on a node which
+   previously had all of its CPUs offlined.
+
+The CPU onlining path (``rcu_cpu_starting()``) does not need to report a
+quiescent state for an offline CPU; in fact it would trigger a warning if a
+quiescent state was not already reported for that CPU.
+
+During the checking/modification of RCU's hotplug bookkeeping, the
+corresponding CPU's leaf node lock is held. This avoids race conditions between
+RCU's hotplug notifier hooks, grace period initialization code and the FQS loop
+which can concurrently refer to or modify the bookkeeping.
+
 Scheduler and RCU
 ~~~~~~~~~~~~~~~~~
 
-- 
2.28.0.709.gb0816b6eb0-goog


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

* Re: [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design
  2020-09-29 19:29 ` [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design Joel Fernandes (Google)
@ 2020-09-29 19:32   ` Joel Fernandes
  2020-10-02 19:34     ` Paul E. McKenney
  0 siblings, 1 reply; 13+ messages in thread
From: Joel Fernandes @ 2020-09-29 19:32 UTC (permalink / raw)
  To: linux-kernel, Paul E. McKenney
  Cc: Jonathan Corbet, Josh Triplett, Lai Jiangshan, linux-doc,
	Mathieu Desnoyers, Mauro Carvalho Chehab, Neeraj Upadhyay,
	Randy Dunlap, rcu, Steven Rostedt, Will Deacon

Hi Paul,

On Tue, Sep 29, 2020 at 03:29:28PM -0400, Joel Fernandes (Google) wrote:
> RCU's hotplug design will help understand the requirements an RCU
> implementation needs to fullfill, such as dead-lock avoidance.
> 
> The rcu_barrier() section of the "Hotplug CPU" section already talks
> about deadlocks, however the description of what else can deadlock other
> than rcu_barrier is rather incomplete.
> 
> This commit therefore continues the section by describing how RCU's
> design handles CPU hotplug in a deadlock-free way.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  .../RCU/Design/Requirements/Requirements.rst  | 30 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
> index 1ae79a10a8de..e0413aa989dd 100644
> --- a/Documentation/RCU/Design/Requirements/Requirements.rst
> +++ b/Documentation/RCU/Design/Requirements/Requirements.rst
> @@ -1929,8 +1929,10 @@ The Linux-kernel CPU-hotplug implementation has notifiers that are used
>  to allow the various kernel subsystems (including RCU) to respond
>  appropriately to a given CPU-hotplug operation. Most RCU operations may
>  be invoked from CPU-hotplug notifiers, including even synchronous
> -grace-period operations such as ``synchronize_rcu()`` and
> -``synchronize_rcu_expedited()``.
> +grace-period operations such as. However, the synchronous variants
> +(``synchronize_rcu()`` and ``synchronize_rcu_expedited()``) should not
> +from notifiers that execute via ``stop_machine()`` -- specifically those

The "should not from notifiers" should be "should not be used from
notifiers" here. Sorry and hope you can fix it up.

thanks,

 - Joel



> +between the ``CPUHP_AP_OFFLINE`` and ``CPUHP_AP_ONLINE`` states.
>  
>  However, all-callback-wait operations such as ``rcu_barrier()`` are also
>  not supported, due to the fact that there are phases of CPU-hotplug
> @@ -1940,6 +1942,30 @@ deadlock. Furthermore, ``rcu_barrier()`` blocks CPU-hotplug operations
>  during its execution, which results in another type of deadlock when
>  invoked from a CPU-hotplug notifier.
>  
> +Also, RCU's implementation avoids serious deadlocks which could occur due to
> +interaction between hotplug, timers and grace period processing. It does so by
> +maintaining its own books of every CPU's hotplug state, independent of
> +the existing general-purpose CPU masks and by reporting quiescent states
> +explictly when an online CPU is going down. Due to this design, the force
> +quiescent state loop (FQS) is not required to report quiescent states for
> +offline CPUs, like it does for idle CPUs, but it does splat if offline CPUs are
> +stalling the RCU grace period for too long.
> +
> +For an offline CPU, the quiescent state will be reported in either of:
> +1. During CPU offlining, using RCU's hotplug notifier (``rcu_report_dead()``).
> +2. During grace period initialization (``rcu_gp_init()``) if it detected a race
> +   with CPU offlining, or a race with a task unblocking on a node which
> +   previously had all of its CPUs offlined.
> +
> +The CPU onlining path (``rcu_cpu_starting()``) does not need to report a
> +quiescent state for an offline CPU; in fact it would trigger a warning if a
> +quiescent state was not already reported for that CPU.
> +
> +During the checking/modification of RCU's hotplug bookkeeping, the
> +corresponding CPU's leaf node lock is held. This avoids race conditions between
> +RCU's hotplug notifier hooks, grace period initialization code and the FQS loop
> +which can concurrently refer to or modify the bookkeeping.
> +
>  Scheduler and RCU
>  ~~~~~~~~~~~~~~~~~
>  
> -- 
> 2.28.0.709.gb0816b6eb0-goog
> 

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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-09-29 19:29 [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already Joel Fernandes (Google)
  2020-09-29 19:29 ` [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design Joel Fernandes (Google)
@ 2020-10-02  4:39 ` Paul E. McKenney
  1 sibling, 0 replies; 13+ messages in thread
From: Paul E. McKenney @ 2020-10-02  4:39 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Neeraj Upadhyay, Jonathan Corbet, Josh Triplett,
	Lai Jiangshan, linux-doc, Mathieu Desnoyers,
	Mauro Carvalho Chehab, Randy Dunlap, rcu, Steven Rostedt,
	Will Deacon

On Tue, Sep 29, 2020 at 03:29:27PM -0400, Joel Fernandes (Google) wrote:
> Currently, rcu_cpu_starting() checks to see if the RCU core expects a
> quiescent state from the incoming CPU.  However, the current interaction
> between RCU quiescent-state reporting and CPU-hotplug operations should
> mean that the incoming CPU never needs to report a quiescent state.
> First, the outgoing CPU reports a quiescent state if needed.  Second,
> the race where the CPU is leaving just as RCU is initializing a new
> grace period is handled by an explicit check for this condition.  Third,
> the CPU's leaf rcu_node structure's ->lock serializes these checks.
> 
> This means that if rcu_cpu_starting() ever feels the need to report
> a quiescent state, then there is a bug somewhere in the CPU hotplug
> code or the RCU grace-period handling code.  This commit therefore
> adds a WARN_ON_ONCE() to bring that bug to everyone's attention.
> 
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
> Suggested-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Queued for testing and further review, thank you!

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 55d3700dd1e7..5efe0a98ea45 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4119,7 +4119,9 @@ void rcu_cpu_starting(unsigned int cpu)
>  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
>  	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>  	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> -	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
> +
> +	/* An incoming CPU should never be blocking a grace period. */
> +	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
>  		rcu_disable_urgency_upon_qs(rdp);
>  		/* Report QS -after- changing ->qsmaskinitnext! */
>  		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
> -- 
> 2.28.0.709.gb0816b6eb0-goog
> 

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

* Re: [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design
  2020-09-29 19:32   ` Joel Fernandes
@ 2020-10-02 19:34     ` Paul E. McKenney
  2020-10-02 23:14       ` Joel Fernandes
  0 siblings, 1 reply; 13+ messages in thread
From: Paul E. McKenney @ 2020-10-02 19:34 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: linux-kernel, Jonathan Corbet, Josh Triplett, Lai Jiangshan,
	linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab,
	Neeraj Upadhyay, Randy Dunlap, rcu, Steven Rostedt, Will Deacon

On Tue, Sep 29, 2020 at 03:32:48PM -0400, Joel Fernandes wrote:
> Hi Paul,
> 
> On Tue, Sep 29, 2020 at 03:29:28PM -0400, Joel Fernandes (Google) wrote:
> > RCU's hotplug design will help understand the requirements an RCU
> > implementation needs to fullfill, such as dead-lock avoidance.
> > 
> > The rcu_barrier() section of the "Hotplug CPU" section already talks
> > about deadlocks, however the description of what else can deadlock other
> > than rcu_barrier is rather incomplete.
> > 
> > This commit therefore continues the section by describing how RCU's
> > design handles CPU hotplug in a deadlock-free way.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  .../RCU/Design/Requirements/Requirements.rst  | 30 +++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
> > index 1ae79a10a8de..e0413aa989dd 100644
> > --- a/Documentation/RCU/Design/Requirements/Requirements.rst
> > +++ b/Documentation/RCU/Design/Requirements/Requirements.rst
> > @@ -1929,8 +1929,10 @@ The Linux-kernel CPU-hotplug implementation has notifiers that are used
> >  to allow the various kernel subsystems (including RCU) to respond
> >  appropriately to a given CPU-hotplug operation. Most RCU operations may
> >  be invoked from CPU-hotplug notifiers, including even synchronous
> > -grace-period operations such as ``synchronize_rcu()`` and
> > -``synchronize_rcu_expedited()``.
> > +grace-period operations such as. However, the synchronous variants
> > +(``synchronize_rcu()`` and ``synchronize_rcu_expedited()``) should not
> > +from notifiers that execute via ``stop_machine()`` -- specifically those
> 
> The "should not from notifiers" should be "should not be used from
> notifiers" here. Sorry and hope you can fix it up.

Thank you, and queued for further review.  How does the below look
for a general fixup?

						Thanx, Paul

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

commit a93716177eeac726037828b28e6b1a45e828688a
Author: Joel Fernandes (Google) <joel@joelfernandes.org>
Date:   Tue Sep 29 15:29:28 2020 -0400

    docs: Update RCU's hotplug requirements with a bit about design
    
    The rcu_barrier() section of the "Hotplug CPU" section discusses
    deadlocks, however the description of deadlocks other than those involving
    rcu_barrier() is rather incomplete.
    
    This commit therefore continues the section by describing how RCU's
    design handles CPU hotplug in a deadlock-free way.
    
    Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
index 1ae79a1..98557fe 100644
--- a/Documentation/RCU/Design/Requirements/Requirements.rst
+++ b/Documentation/RCU/Design/Requirements/Requirements.rst
@@ -1929,16 +1929,45 @@ The Linux-kernel CPU-hotplug implementation has notifiers that are used
 to allow the various kernel subsystems (including RCU) to respond
 appropriately to a given CPU-hotplug operation. Most RCU operations may
 be invoked from CPU-hotplug notifiers, including even synchronous
-grace-period operations such as ``synchronize_rcu()`` and
-``synchronize_rcu_expedited()``.
-
-However, all-callback-wait operations such as ``rcu_barrier()`` are also
-not supported, due to the fact that there are phases of CPU-hotplug
-operations where the outgoing CPU's callbacks will not be invoked until
-after the CPU-hotplug operation ends, which could also result in
-deadlock. Furthermore, ``rcu_barrier()`` blocks CPU-hotplug operations
-during its execution, which results in another type of deadlock when
-invoked from a CPU-hotplug notifier.
+grace-period operations such as (``synchronize_rcu()`` and
+``synchronize_rcu_expedited()``).  However, these synchronous operations
+do block and therefore cannot be invoked from notifiers that execute via
+``stop_machine()``, specifically those between the ``CPUHP_AP_OFFLINE``
+and ``CPUHP_AP_ONLINE`` states.
+
+In addition, all-callback-wait operations such as ``rcu_barrier()`` may
+not be invoked from any CPU-hotplug notifier.  This restriction is due
+to the fact that there are phases of CPU-hotplug operations where the
+outgoing CPU's callbacks will not be invoked until after the CPU-hotplug
+operation ends, which could also result in deadlock. Furthermore,
+``rcu_barrier()`` blocks CPU-hotplug operations during its execution,
+which results in another type of deadlock when invoked from a CPU-hotplug
+notifier.
+
+Finally, RCU must avoid deadlocks due to interaction between hotplug,
+timers and grace period processing. It does so by maintaining its own set
+of books that duplicate the centrally maintained ``cpu_online_mask``,
+and also by reporting quiescent states explictly when a CPU goes
+offline.  This explicit reporting of quiescent states avoids any need
+for the force-quiescent-state loop (FQS) to report quiescent states for
+offline CPUs.  However, as a debugging measure, the FQS loop does splat
+if offline CPUs block an RCU grace period for too long.
+
+An offline CPU's quiescent state will be reported either:
+1.  As the CPU goes offline using RCU's hotplug notifier (``rcu_report_dead()``).
+2.  When grace period initialization (``rcu_gp_init()``) detects a
+    race either with CPU offlining or with a task unblocking on a leaf
+    ``rcu_node`` structure whose CPUs are all offline.
+
+The CPU-online path (``rcu_cpu_starting()``) should never need to report
+a quiescent state for an offline CPU.  However, as a debugging measure,
+it does emit a warning if a quiescent state was not already reported
+for that CPU.
+
+During the checking/modification of RCU's hotplug bookkeeping, the
+corresponding CPU's leaf node lock is held. This avoids race conditions
+between RCU's hotplug notifier hooks, the grace period initialization
+code, and the FQS loop, all of which refer to or modify this bookkeeping.
 
 Scheduler and RCU
 ~~~~~~~~~~~~~~~~~

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

* Re: [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design
  2020-10-02 19:34     ` Paul E. McKenney
@ 2020-10-02 23:14       ` Joel Fernandes
  0 siblings, 0 replies; 13+ messages in thread
From: Joel Fernandes @ 2020-10-02 23:14 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: LKML, Jonathan Corbet, Josh Triplett, Lai Jiangshan,
	open list:DOCUMENTATION, Mathieu Desnoyers,
	Mauro Carvalho Chehab, Neeraj Upadhyay, Randy Dunlap, rcu,
	Steven Rostedt, Will Deacon

On Fri, Oct 2, 2020 at 3:34 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Tue, Sep 29, 2020 at 03:32:48PM -0400, Joel Fernandes wrote:
> > Hi Paul,
> >
> > On Tue, Sep 29, 2020 at 03:29:28PM -0400, Joel Fernandes (Google) wrote:
> > > RCU's hotplug design will help understand the requirements an RCU
> > > implementation needs to fullfill, such as dead-lock avoidance.
> > >
> > > The rcu_barrier() section of the "Hotplug CPU" section already talks
> > > about deadlocks, however the description of what else can deadlock other
> > > than rcu_barrier is rather incomplete.
> > >
> > > This commit therefore continues the section by describing how RCU's
> > > design handles CPU hotplug in a deadlock-free way.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > >  .../RCU/Design/Requirements/Requirements.rst  | 30 +++++++++++++++++--
> > >  1 file changed, 28 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
> > > index 1ae79a10a8de..e0413aa989dd 100644
> > > --- a/Documentation/RCU/Design/Requirements/Requirements.rst
> > > +++ b/Documentation/RCU/Design/Requirements/Requirements.rst
> > > @@ -1929,8 +1929,10 @@ The Linux-kernel CPU-hotplug implementation has notifiers that are used
> > >  to allow the various kernel subsystems (including RCU) to respond
> > >  appropriately to a given CPU-hotplug operation. Most RCU operations may
> > >  be invoked from CPU-hotplug notifiers, including even synchronous
> > > -grace-period operations such as ``synchronize_rcu()`` and
> > > -``synchronize_rcu_expedited()``.
> > > +grace-period operations such as. However, the synchronous variants
> > > +(``synchronize_rcu()`` and ``synchronize_rcu_expedited()``) should not
> > > +from notifiers that execute via ``stop_machine()`` -- specifically those
> >
> > The "should not from notifiers" should be "should not be used from
> > notifiers" here. Sorry and hope you can fix it up.
>
> Thank you, and queued for further review.  How does the below look
> for a general fixup?

Looks great, thanks!

 -Joel



>
>                                                 Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit a93716177eeac726037828b28e6b1a45e828688a
> Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> Date:   Tue Sep 29 15:29:28 2020 -0400
>
>     docs: Update RCU's hotplug requirements with a bit about design
>
>     The rcu_barrier() section of the "Hotplug CPU" section discusses
>     deadlocks, however the description of deadlocks other than those involving
>     rcu_barrier() is rather incomplete.
>
>     This commit therefore continues the section by describing how RCU's
>     design handles CPU hotplug in a deadlock-free way.
>
>     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
> diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
> index 1ae79a1..98557fe 100644
> --- a/Documentation/RCU/Design/Requirements/Requirements.rst
> +++ b/Documentation/RCU/Design/Requirements/Requirements.rst
> @@ -1929,16 +1929,45 @@ The Linux-kernel CPU-hotplug implementation has notifiers that are used
>  to allow the various kernel subsystems (including RCU) to respond
>  appropriately to a given CPU-hotplug operation. Most RCU operations may
>  be invoked from CPU-hotplug notifiers, including even synchronous
> -grace-period operations such as ``synchronize_rcu()`` and
> -``synchronize_rcu_expedited()``.
> -
> -However, all-callback-wait operations such as ``rcu_barrier()`` are also
> -not supported, due to the fact that there are phases of CPU-hotplug
> -operations where the outgoing CPU's callbacks will not be invoked until
> -after the CPU-hotplug operation ends, which could also result in
> -deadlock. Furthermore, ``rcu_barrier()`` blocks CPU-hotplug operations
> -during its execution, which results in another type of deadlock when
> -invoked from a CPU-hotplug notifier.
> +grace-period operations such as (``synchronize_rcu()`` and
> +``synchronize_rcu_expedited()``).  However, these synchronous operations
> +do block and therefore cannot be invoked from notifiers that execute via
> +``stop_machine()``, specifically those between the ``CPUHP_AP_OFFLINE``
> +and ``CPUHP_AP_ONLINE`` states.
> +
> +In addition, all-callback-wait operations such as ``rcu_barrier()`` may
> +not be invoked from any CPU-hotplug notifier.  This restriction is due
> +to the fact that there are phases of CPU-hotplug operations where the
> +outgoing CPU's callbacks will not be invoked until after the CPU-hotplug
> +operation ends, which could also result in deadlock. Furthermore,
> +``rcu_barrier()`` blocks CPU-hotplug operations during its execution,
> +which results in another type of deadlock when invoked from a CPU-hotplug
> +notifier.
> +
> +Finally, RCU must avoid deadlocks due to interaction between hotplug,
> +timers and grace period processing. It does so by maintaining its own set
> +of books that duplicate the centrally maintained ``cpu_online_mask``,
> +and also by reporting quiescent states explictly when a CPU goes
> +offline.  This explicit reporting of quiescent states avoids any need
> +for the force-quiescent-state loop (FQS) to report quiescent states for
> +offline CPUs.  However, as a debugging measure, the FQS loop does splat
> +if offline CPUs block an RCU grace period for too long.
> +
> +An offline CPU's quiescent state will be reported either:
> +1.  As the CPU goes offline using RCU's hotplug notifier (``rcu_report_dead()``).
> +2.  When grace period initialization (``rcu_gp_init()``) detects a
> +    race either with CPU offlining or with a task unblocking on a leaf
> +    ``rcu_node`` structure whose CPUs are all offline.
> +
> +The CPU-online path (``rcu_cpu_starting()``) should never need to report
> +a quiescent state for an offline CPU.  However, as a debugging measure,
> +it does emit a warning if a quiescent state was not already reported
> +for that CPU.
> +
> +During the checking/modification of RCU's hotplug bookkeeping, the
> +corresponding CPU's leaf node lock is held. This avoids race conditions
> +between RCU's hotplug notifier hooks, the grace period initialization
> +code, and the FQS loop, all of which refer to or modify this bookkeeping.
>
>  Scheduler and RCU
>  ~~~~~~~~~~~~~~~~~

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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-08-07 15:37       ` Joel Fernandes
@ 2020-08-07 15:45         ` Joel Fernandes
  0 siblings, 0 replies; 13+ messages in thread
From: Joel Fernandes @ 2020-08-07 15:45 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

On Fri, Aug 07, 2020 at 11:37:32AM -0400, Joel Fernandes wrote:
> Hi Paul,
> 
> On Thu, Jul 30, 2020 at 08:48:25PM -0700, Paul E. McKenney wrote:
> [...]
> > > And I could make the comment here as:
> > > 	/*
> > > 	 * Delete QS reporting from here, by June 2021, if the warning does not
> > >  	 * fire. Leave the warning indefinitely. Check RCU design requirements
> > > 	 * in Documentation/RCU/ about CPU hotplug requirements.
> > > 	 */
> > 
> > Rather than decide for our future selves, could we please just suggest
> > reviewing this on June 2021?  Or, given enterprise distro schedules,
> > 2024.  :-/
> 
> I am replacing it with the following, let me know if any objections, thanks:
> 
> +        * XXX: The following rcu_report_qs_rnp() is redundant. If the below
> +        * warning does not fire, consider replacing it with the "else" block,
> +        * by June 2021 or so. The rationale for this is as follows: The CPU
> +        * onlining path does not need to report QS for an offline CPU. Either
> +        * the QS should have reported during CPU offlining, or during
> +        * rcu_gp_init() if it detected a race with either CPU offlining or
> +        * task unblocking on previously offlined CPUs. To avoid deadlocks
> +        * related to waiting on timers or cpu hotplug locks, only those paths
> +        * do the QS reporting for offline CPUs.

And you did mention you still want the warn-on indefinitely, so I'll document
that in the comment as well.

Now it looks like:

        /*
         * XXX: The following rcu_report_qs_rnp() is redundant. If the below
         * warning does not fire, consider replacing it with the "else" block,
         * by June 2021 or so (while keeping the warning). The rationale for
         * this is as follows: The CPU onlining path does not need to report QS
         * for an offline CPU. Either the QS should have reported during CPU
         * offlining, or during rcu_gp_init() if it detected a race with either
         * CPU offlining or task unblocking on a node with all of its CPUs
         * previously offlined.  To avoid deadlocks related to waiting on
         * timers or cpu hotplug locks, only these paths do the QS reporting
         * for offline CPUs making the following reporting redundant.
         */

thanks,

 - Joel

> 
> thanks,
> 
>  - Joel
> 
> > 
> > 							Thanx, Paul
> > 
> > > I will post my v3 with changes to the requirements document.
> > > 
> > > Let me know any other comments, thanks,
> > > 
> > >  - Joel
> > > 

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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-07-31  3:48     ` Paul E. McKenney
@ 2020-08-07 15:37       ` Joel Fernandes
  2020-08-07 15:45         ` Joel Fernandes
  0 siblings, 1 reply; 13+ messages in thread
From: Joel Fernandes @ 2020-08-07 15:37 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

Hi Paul,

On Thu, Jul 30, 2020 at 08:48:25PM -0700, Paul E. McKenney wrote:
[...]
> > And I could make the comment here as:
> > 	/*
> > 	 * Delete QS reporting from here, by June 2021, if the warning does not
> >  	 * fire. Leave the warning indefinitely. Check RCU design requirements
> > 	 * in Documentation/RCU/ about CPU hotplug requirements.
> > 	 */
> 
> Rather than decide for our future selves, could we please just suggest
> reviewing this on June 2021?  Or, given enterprise distro schedules,
> 2024.  :-/

I am replacing it with the following, let me know if any objections, thanks:

+        * XXX: The following rcu_report_qs_rnp() is redundant. If the below
+        * warning does not fire, consider replacing it with the "else" block,
+        * by June 2021 or so. The rationale for this is as follows: The CPU
+        * onlining path does not need to report QS for an offline CPU. Either
+        * the QS should have reported during CPU offlining, or during
+        * rcu_gp_init() if it detected a race with either CPU offlining or
+        * task unblocking on previously offlined CPUs. To avoid deadlocks
+        * related to waiting on timers or cpu hotplug locks, only those paths
+        * do the QS reporting for offline CPUs.

thanks,

 - Joel

> 
> 							Thanx, Paul
> 
> > I will post my v3 with changes to the requirements document.
> > 
> > Let me know any other comments, thanks,
> > 
> >  - Joel
> > 

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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-07-31  1:42   ` Joel Fernandes
@ 2020-07-31  3:48     ` Paul E. McKenney
  2020-08-07 15:37       ` Joel Fernandes
  0 siblings, 1 reply; 13+ messages in thread
From: Paul E. McKenney @ 2020-07-31  3:48 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: linux-kernel, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

On Thu, Jul 30, 2020 at 09:42:22PM -0400, Joel Fernandes wrote:
> On Thu, Jul 30, 2020 at 09:21:59AM -0700, Paul E. McKenney wrote:
> > On Wed, Jul 29, 2020 at 11:02:20PM -0400, Joel Fernandes (Google) wrote:
> > > Add a warning if CPU being onlined did not report QS already. This is to
> > > simplify the code in the CPU onlining path and also to make clear about
> > > where QS is reported. The act of QS reporting in CPU onlining path is
> > > is likely unnecessary as shown by code reading and testing with
> > > rcutorture's TREE03 and hotplug parameters.
> > 
> > How about something like this for the commit log?
> > 
> > ------------------------------------------------------------------------
> > 
> > Currently, rcu_cpu_starting() checks to see if the RCU core expects a
> > quiescent state from the incoming CPU.  However, the current interaction
> > between RCU quiescent-state reporting and CPU-hotplug operations should
> > mean that the incoming CPU never needs to report a quiescent state.
> > First, the outgoing CPU reports a quiescent state if needed.  Second,
> > the race where the CPU is leaving just as RCU is initializing a new
> > grace period is handled by an explicit check for this condition.  Third,
> > the CPU's leaf rcu_node structure's ->lock serializes these checks.
> > 
> > This means that if rcu_cpu_starting() ever feels the need to report
> > a quiescent state, then there is a bug somewhere in the CPU hotplug
> > code or the RCU grace-period handling code.  This commit therefore
> > adds a WARN_ON_ONCE() to bring that bug to everyone's attention.
> > 
> > ------------------------------------------------------------------------
> > 
> > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
> > > Suggested-by: Paul E. McKenney <paulmck@kernel.org>
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > 
> > > ---
> > >  kernel/rcu/tree.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 65e1b5e92319..1e51962b565b 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3996,7 +3996,19 @@ void rcu_cpu_starting(unsigned int cpu)
> > >  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
> > >  	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> > >  	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> > > -	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
> > > +
> > > +	/*
> > > +	 * Delete QS reporting from here, by June 2021, if warning does not
> > > +	 * fire. Let us make the rules for reporting QS for an offline CPUs
> > > +	 * more explicit. The CPU onlining path does not need to report QS for
> > > +	 * an offline CPU. Either the QS should have reported during CPU
> > > +	 * offlining, or during rcu_gp_init() if it detected a race with either
> > > +	 * CPU offlining or task unblocking on previously offlined CPUs. Note
> > > +	 * that the FQS loop also does not report QS for an offline CPU any
> > > +	 * longer (unless it splats due to an offline CPU blocking the GP for
> > > +	 * too long).
> > > +	 */
> > 
> > Let's leave at least the WARN_ON_ONCE() indefinitely.  If you don't
> > believe me, remove this code in your local tree, have someone give you
> > several branches, some with bugs injected, and then try to figure out
> > which have the bugs and then try to find those bugs.
> > 
> > This is not a fastpath, so the overhead of the check is not a concern.
> > Believe me, the difficulty of bug location without this check is a very
> > real concern!  ;-)
> > 
> > On the other hand, I fully agree with the benefits of documenting the
> > design rules.  But is this really the best place to do that from the
> > viewpoint of someone who is trying to figure out how RCU works?
> 
> I can move this comment to: "Hotplug CPU" section in
> Documentation/RCU/Design/Requirements/Requirements.rst

That sounds like a better place for it, very good!

> And I could make the comment here as:
> 	/*
> 	 * Delete QS reporting from here, by June 2021, if the warning does not
>  	 * fire. Leave the warning indefinitely. Check RCU design requirements
> 	 * in Documentation/RCU/ about CPU hotplug requirements.
> 	 */

Rather than decide for our future selves, could we please just suggest
reviewing this on June 2021?  Or, given enterprise distro schedules,
2024.  :-/

							Thanx, Paul

> I will post my v3 with changes to the requirements document.
> 
> Let me know any other comments, thanks,
> 
>  - Joel
> 

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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-07-30 16:21 ` Paul E. McKenney
  2020-07-31  1:08   ` Joel Fernandes
@ 2020-07-31  1:42   ` Joel Fernandes
  2020-07-31  3:48     ` Paul E. McKenney
  1 sibling, 1 reply; 13+ messages in thread
From: Joel Fernandes @ 2020-07-31  1:42 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

On Thu, Jul 30, 2020 at 09:21:59AM -0700, Paul E. McKenney wrote:
> On Wed, Jul 29, 2020 at 11:02:20PM -0400, Joel Fernandes (Google) wrote:
> > Add a warning if CPU being onlined did not report QS already. This is to
> > simplify the code in the CPU onlining path and also to make clear about
> > where QS is reported. The act of QS reporting in CPU onlining path is
> > is likely unnecessary as shown by code reading and testing with
> > rcutorture's TREE03 and hotplug parameters.
> 
> How about something like this for the commit log?
> 
> ------------------------------------------------------------------------
> 
> Currently, rcu_cpu_starting() checks to see if the RCU core expects a
> quiescent state from the incoming CPU.  However, the current interaction
> between RCU quiescent-state reporting and CPU-hotplug operations should
> mean that the incoming CPU never needs to report a quiescent state.
> First, the outgoing CPU reports a quiescent state if needed.  Second,
> the race where the CPU is leaving just as RCU is initializing a new
> grace period is handled by an explicit check for this condition.  Third,
> the CPU's leaf rcu_node structure's ->lock serializes these checks.
> 
> This means that if rcu_cpu_starting() ever feels the need to report
> a quiescent state, then there is a bug somewhere in the CPU hotplug
> code or the RCU grace-period handling code.  This commit therefore
> adds a WARN_ON_ONCE() to bring that bug to everyone's attention.
> 
> ------------------------------------------------------------------------
> 
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
> > Suggested-by: Paul E. McKenney <paulmck@kernel.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > ---
> >  kernel/rcu/tree.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 65e1b5e92319..1e51962b565b 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3996,7 +3996,19 @@ void rcu_cpu_starting(unsigned int cpu)
> >  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
> >  	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> >  	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> > -	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
> > +
> > +	/*
> > +	 * Delete QS reporting from here, by June 2021, if warning does not
> > +	 * fire. Let us make the rules for reporting QS for an offline CPUs
> > +	 * more explicit. The CPU onlining path does not need to report QS for
> > +	 * an offline CPU. Either the QS should have reported during CPU
> > +	 * offlining, or during rcu_gp_init() if it detected a race with either
> > +	 * CPU offlining or task unblocking on previously offlined CPUs. Note
> > +	 * that the FQS loop also does not report QS for an offline CPU any
> > +	 * longer (unless it splats due to an offline CPU blocking the GP for
> > +	 * too long).
> > +	 */
> 
> Let's leave at least the WARN_ON_ONCE() indefinitely.  If you don't
> believe me, remove this code in your local tree, have someone give you
> several branches, some with bugs injected, and then try to figure out
> which have the bugs and then try to find those bugs.
> 
> This is not a fastpath, so the overhead of the check is not a concern.
> Believe me, the difficulty of bug location without this check is a very
> real concern!  ;-)
> 
> On the other hand, I fully agree with the benefits of documenting the
> design rules.  But is this really the best place to do that from the
> viewpoint of someone who is trying to figure out how RCU works?

I can move this comment to: "Hotplug CPU" section in
Documentation/RCU/Design/Requirements/Requirements.rst

And I could make the comment here as:
	/*
	 * Delete QS reporting from here, by June 2021, if the warning does not
 	 * fire. Leave the warning indefinitely. Check RCU design requirements
	 * in Documentation/RCU/ about CPU hotplug requirements.
	 */

I will post my v3 with changes to the requirements document.

Let me know any other comments, thanks,

 - Joel


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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-07-30 16:21 ` Paul E. McKenney
@ 2020-07-31  1:08   ` Joel Fernandes
  2020-07-31  1:42   ` Joel Fernandes
  1 sibling, 0 replies; 13+ messages in thread
From: Joel Fernandes @ 2020-07-31  1:08 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

On Thu, Jul 30, 2020 at 09:21:59AM -0700, Paul E. McKenney wrote:
> On Wed, Jul 29, 2020 at 11:02:20PM -0400, Joel Fernandes (Google) wrote:
> > Add a warning if CPU being onlined did not report QS already. This is to
> > simplify the code in the CPU onlining path and also to make clear about
> > where QS is reported. The act of QS reporting in CPU onlining path is
> > is likely unnecessary as shown by code reading and testing with
> > rcutorture's TREE03 and hotplug parameters.
> 
> How about something like this for the commit log?
> 
> ------------------------------------------------------------------------
> 
> Currently, rcu_cpu_starting() checks to see if the RCU core expects a
> quiescent state from the incoming CPU.  However, the current interaction
> between RCU quiescent-state reporting and CPU-hotplug operations should
> mean that the incoming CPU never needs to report a quiescent state.
> First, the outgoing CPU reports a quiescent state if needed.  Second,
> the race where the CPU is leaving just as RCU is initializing a new
> grace period is handled by an explicit check for this condition.  Third,
> the CPU's leaf rcu_node structure's ->lock serializes these checks.
> 
> This means that if rcu_cpu_starting() ever feels the need to report
> a quiescent state, then there is a bug somewhere in the CPU hotplug
> code or the RCU grace-period handling code.  This commit therefore
> adds a WARN_ON_ONCE() to bring that bug to everyone's attention.

Wow, this is so well written. I certainly need to up my writing game a bitt.
But yes, this is flawless explanation and really will help people reading it
in the future.

I will make the change to my tree for the next revision.

thanks,

 - Joel


> ------------------------------------------------------------------------
> 
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
> > Suggested-by: Paul E. McKenney <paulmck@kernel.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > ---
> >  kernel/rcu/tree.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 65e1b5e92319..1e51962b565b 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3996,7 +3996,19 @@ void rcu_cpu_starting(unsigned int cpu)
> >  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
> >  	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> >  	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> > -	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
> > +
> > +	/*
> > +	 * Delete QS reporting from here, by June 2021, if warning does not
> > +	 * fire. Let us make the rules for reporting QS for an offline CPUs
> > +	 * more explicit. The CPU onlining path does not need to report QS for
> > +	 * an offline CPU. Either the QS should have reported during CPU
> > +	 * offlining, or during rcu_gp_init() if it detected a race with either
> > +	 * CPU offlining or task unblocking on previously offlined CPUs. Note
> > +	 * that the FQS loop also does not report QS for an offline CPU any
> > +	 * longer (unless it splats due to an offline CPU blocking the GP for
> > +	 * too long).
> > +	 */
> 
> Let's leave at least the WARN_ON_ONCE() indefinitely.  If you don't
> believe me, remove this code in your local tree, have someone give you
> several branches, some with bugs injected, and then try to figure out
> which have the bugs and then try to find those bugs.
> 
> This is not a fastpath, so the overhead of the check is not a concern.
> Believe me, the difficulty of bug location without this check is a very
> real concern!  ;-)
> 
> On the other hand, I fully agree with the benefits of documenting the
> design rules.  But is this really the best place to do that from the
> viewpoint of someone who is trying to figure out how RCU works?
> 
> 							Thanx, Paul
> 
> > +	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
> >  		rcu_disable_urgency_upon_qs(rdp);
> >  		/* Report QS -after- changing ->qsmaskinitnext! */
> >  		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
> > -- 
> > 2.28.0.rc0.142.g3c755180ce-goog
> > 

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

* Re: [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
  2020-07-30  3:02 Joel Fernandes (Google)
@ 2020-07-30 16:21 ` Paul E. McKenney
  2020-07-31  1:08   ` Joel Fernandes
  2020-07-31  1:42   ` Joel Fernandes
  0 siblings, 2 replies; 13+ messages in thread
From: Paul E. McKenney @ 2020-07-30 16:21 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

On Wed, Jul 29, 2020 at 11:02:20PM -0400, Joel Fernandes (Google) wrote:
> Add a warning if CPU being onlined did not report QS already. This is to
> simplify the code in the CPU onlining path and also to make clear about
> where QS is reported. The act of QS reporting in CPU onlining path is
> is likely unnecessary as shown by code reading and testing with
> rcutorture's TREE03 and hotplug parameters.

How about something like this for the commit log?

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

Currently, rcu_cpu_starting() checks to see if the RCU core expects a
quiescent state from the incoming CPU.  However, the current interaction
between RCU quiescent-state reporting and CPU-hotplug operations should
mean that the incoming CPU never needs to report a quiescent state.
First, the outgoing CPU reports a quiescent state if needed.  Second,
the race where the CPU is leaving just as RCU is initializing a new
grace period is handled by an explicit check for this condition.  Third,
the CPU's leaf rcu_node structure's ->lock serializes these checks.

This means that if rcu_cpu_starting() ever feels the need to report
a quiescent state, then there is a bug somewhere in the CPU hotplug
code or the RCU grace-period handling code.  This commit therefore
adds a WARN_ON_ONCE() to bring that bug to everyone's attention.

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

> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
> Suggested-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> ---
>  kernel/rcu/tree.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 65e1b5e92319..1e51962b565b 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3996,7 +3996,19 @@ void rcu_cpu_starting(unsigned int cpu)
>  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
>  	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>  	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> -	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
> +
> +	/*
> +	 * Delete QS reporting from here, by June 2021, if warning does not
> +	 * fire. Let us make the rules for reporting QS for an offline CPUs
> +	 * more explicit. The CPU onlining path does not need to report QS for
> +	 * an offline CPU. Either the QS should have reported during CPU
> +	 * offlining, or during rcu_gp_init() if it detected a race with either
> +	 * CPU offlining or task unblocking on previously offlined CPUs. Note
> +	 * that the FQS loop also does not report QS for an offline CPU any
> +	 * longer (unless it splats due to an offline CPU blocking the GP for
> +	 * too long).
> +	 */

Let's leave at least the WARN_ON_ONCE() indefinitely.  If you don't
believe me, remove this code in your local tree, have someone give you
several branches, some with bugs injected, and then try to figure out
which have the bugs and then try to find those bugs.

This is not a fastpath, so the overhead of the check is not a concern.
Believe me, the difficulty of bug location without this check is a very
real concern!  ;-)

On the other hand, I fully agree with the benefits of documenting the
design rules.  But is this really the best place to do that from the
viewpoint of someone who is trying to figure out how RCU works?

							Thanx, Paul

> +	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
>  		rcu_disable_urgency_upon_qs(rdp);
>  		/* Report QS -after- changing ->qsmaskinitnext! */
>  		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 

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

* [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already
@ 2020-07-30  3:02 Joel Fernandes (Google)
  2020-07-30 16:21 ` Paul E. McKenney
  0 siblings, 1 reply; 13+ messages in thread
From: Joel Fernandes (Google) @ 2020-07-30  3:02 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Paul E . McKenney, Neeraj Upadhyay, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt

Add a warning if CPU being onlined did not report QS already. This is to
simplify the code in the CPU onlining path and also to make clear about
where QS is reported. The act of QS reporting in CPU onlining path is
is likely unnecessary as shown by code reading and testing with
rcutorture's TREE03 and hotplug parameters.

Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Neeraj Upadhyay <neeraju@codeaurora.org>
Suggested-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

---
 kernel/rcu/tree.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 65e1b5e92319..1e51962b565b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3996,7 +3996,19 @@ void rcu_cpu_starting(unsigned int cpu)
 	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
 	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
 	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
-	if (rnp->qsmask & mask) { /* RCU waiting on incoming CPU? */
+
+	/*
+	 * Delete QS reporting from here, by June 2021, if warning does not
+	 * fire. Let us make the rules for reporting QS for an offline CPUs
+	 * more explicit. The CPU onlining path does not need to report QS for
+	 * an offline CPU. Either the QS should have reported during CPU
+	 * offlining, or during rcu_gp_init() if it detected a race with either
+	 * CPU offlining or task unblocking on previously offlined CPUs. Note
+	 * that the FQS loop also does not report QS for an offline CPU any
+	 * longer (unless it splats due to an offline CPU blocking the GP for
+	 * too long).
+	 */
+	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
 		rcu_disable_urgency_upon_qs(rdp);
 		/* Report QS -after- changing ->qsmaskinitnext! */
 		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
-- 
2.28.0.rc0.142.g3c755180ce-goog


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

end of thread, other threads:[~2020-10-02 23:15 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 19:29 [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already Joel Fernandes (Google)
2020-09-29 19:29 ` [PATCH 2/2] docs: Update RCU's hotplug requirements with a bit about design Joel Fernandes (Google)
2020-09-29 19:32   ` Joel Fernandes
2020-10-02 19:34     ` Paul E. McKenney
2020-10-02 23:14       ` Joel Fernandes
2020-10-02  4:39 ` [PATCH 1/2] rcu/tree: Add a warning if CPU being onlined did not report QS already Paul E. McKenney
  -- strict thread matches above, loose matches on Subject: below --
2020-07-30  3:02 Joel Fernandes (Google)
2020-07-30 16:21 ` Paul E. McKenney
2020-07-31  1:08   ` Joel Fernandes
2020-07-31  1:42   ` Joel Fernandes
2020-07-31  3:48     ` Paul E. McKenney
2020-08-07 15:37       ` Joel Fernandes
2020-08-07 15:45         ` Joel Fernandes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).