Re: [PATCH v4 1/5] rcu/tree: Add a warning if CPU being onlined did not report QS already

[Paul E. McKenney]

On Mon, Aug 10, 2020 at 03:25:54PM -0400, Joel Fernandes wrote:
> On Mon, Aug 10, 2020 at 10:57:17AM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 10, 2020 at 01:39:31PM -0400, Joel Fernandes wrote:
> > > On Mon, Aug 10, 2020 at 08:46:54AM -0700, Paul E. McKenney wrote:
> > > > On Fri, Aug 07, 2020 at 01:07:18PM -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@xxxxxxxxxx>
> > > > > Cc: Neeraj Upadhyay <neeraju@xxxxxxxxxxxxxx>
> > > > > Suggested-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  kernel/rcu/tree.c | 9 ++++++++-
> > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index 65e1b5e92319..a49fa3b60faa 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -3996,7 +3996,14 @@ 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? */
> > > > > +
> > > > > +	/*
> > > > > +	 * 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). Refer to RCU's
> > > > > +	 * Requirements documentation for the rationale.
> > > > 
> > > > Let's suppose that this change is made, and further that in a year or
> > > > two the "if" statement below is replaced with its "else" block.
> > > > 
> > > > Now let's suppose that (some years after that) a hard-to-trigger bug
> > > > makes its way into RCU's CPU-hotplug code that would have resulted in
> > > > the WARN_ON_ONCE() triggering, but that this bug turns out to be not so
> > > > hard to trigger in certain large production environments.
> > > > 
> > > > Let's suppose further that you have moved on to where you are responsible
> > > > for one of these large production environments.  How would this
> > > > hypothetical RCU/CPU-hotplug bug manifest?
> > > 
> > > It could manifest as an RCU stall (after the warning triggers) since RCU
> > > would wait forever.
> > > 
> > > Were you thinking it is not worth doing this? I thought we wanted to remove
> > > the reundant rcu_report_qs_rnp here to solidify everyone's understanding of
> > > the code and fail early if there's something misunderstood (since such
> > > misunderstanding could mean there are other hidden bugs somewhere). The
> > > counter-argument to that being, making the code robust is more important for
> > > the large production failure scenario where failures are costly.
> > 
> > The benefits of removing code that is in theory redundant was my thought
> > at one point, but sleeping on this several times since has made me much
> > less favorable to this change.  And perhaps my experiences with my new
> > employer have affected my views on this as well.  You never know!  ;-)
> 
> Can we just keep the warning then, and delete the comments to revisit?
> 
> IMHO a comment saying this rcu_report_qs_rnp() is not necessary here but is
> done anyway, would be quite useful to a code reader, (with appropriate
> comments to point to RCU requirements section and the added warning) :-)

Agreed, the warning does make sense.

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> > 
> > 							Thanx, Paul
> > 
> > > thanks,
> > > 
> > >  - Joel
> > > 
> > > 
> > > > 							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.236.gb10cc79966-goog
> > > > >