Re: [PATCH v4 3/5] rcu/tree: Make FQS complaining about offline CPU more aggressive

[Paul E. McKenney]

On Fri, Aug 07, 2020 at 01:07:20PM -0400, Joel Fernandes (Google) wrote:
> The FQS loop detecting that an offline CPU has not yet reported a
> quiescent state, is a serious problem. The current interaction between
> RCU quiescent-state reporting and CPU-hotplug operations means that the
> FQS loop should never find that an offline CPU (by RCU's understanding
> of what is offline or not) has not yet reported a quiescent state.
> 
> First, the outgoing CPU explicitly reports a quiescent state if needed
> in rcu_report_dead().  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 in rcu_gp_init().  Third, the CPU's leaf rcu_node
> structure's ->lock serializes these modifications and checks.
> 
> At the moment, the FQS loop detections this anomaly but only reports it
> after a second has elapsed.
> 
> This commit therefore makes the warning more aggressive such that we fail
> immediately when the FQS loop scan happens and bring the problem to
> everyone's attention.
> 
> Light testing with TREE03 and hotplug shows no warnings. Converted the
> warning as well to WARN_ON_ONCE() to reduce log spam.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>

Looks good, queued, thank you!

I did a bit of editing as shown below.  Please let me know if I messed
anything up.

							Thanx, Paul

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

commit 61b82b349d0089120a9705240ece6ecf2b176fd5
Author: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
Date:   Fri Aug 7 13:07:20 2020 -0400

    rcu: Make FQS more aggressive in complaining about offline CPUs
    
    The RCU grace-period kthread's force-quiescent state (FQS) loop should
    never see an offline CPU that has not yet reported a quiescent state.
    After all, the offline CPU should have reported a quiescent state
    during the CPU-offline process, or, failing that, by rcu_gp_init()
    if it ran concurrently with either the CPU going offline or the last
    task on a leaf rcu_node structure exiting its RCU read-side critical
    section while all CPUs corresponding to that structure are offline.
    The FQS loop should therefore complain if it does see an offline CPU
    that has not yet reported a quiescent state.
    
    And it does, but only once the grace period has been in force for a
    full second.  This commit therefore makes this warning more aggressive,
    so that it will trigger as soon as the condition makes its appearance.
    
    Light testing with TREE03 and hotplug shows no warnings.  This commit
    also converts the warning to WARN_ON_ONCE() in order to stave off possible
    log spam.
    
    Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 59e1943..f79827b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1215,13 +1215,28 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 		return 1;
 	}
 
-	/* If waiting too long on an offline CPU, complain. */
-	if (!(rdp->grpmask & rcu_rnp_online_cpus(rnp)) &&
-	    time_after(jiffies, rcu_state.gp_start + HZ)) {
+	/*
+	 * Complain if a CPU that is considered to be offline from RCU's
+	 * perspective has not yet reported a quiescent state.  After all,
+	 * the offline CPU should have reported a quiescent state during
+	 * the CPU-offline process, or, failing that, by rcu_gp_init()
+	 * if it ran concurrently with either the CPU going offline or the
+	 * last task on a leaf rcu_node structure exiting its RCU read-side
+	 * critical section while all CPUs corresponding to that structure
+	 * are offline.  This added warning detects bugs in any of these
+	 * code paths.
+	 *
+	 * The rcu_node structure's ->lock is held here, which excludes
+	 * the relevant portions the CPU-hotplug code, the grace-period
+	 * initialization code, and the rcu_read_unlock() code paths.
+	 *
+	 * For more detail, please refer to the "Hotplug CPU" section
+	 * of RCU's Requirements documentation.
+	 */
+	if (WARN_ON_ONCE(!(rdp->grpmask & rcu_rnp_online_cpus(rnp)))) {
 		bool onl;
 		struct rcu_node *rnp1;
 
-		WARN_ON(1);  /* Offline CPUs are supposed to report QS! */
 		pr_info("%s: grp: %d-%d level: %d ->gp_seq %ld ->completedqs %ld\n",
 			__func__, rnp->grplo, rnp->grphi, rnp->level,
 			(long)rnp->gp_seq, (long)rnp->completedqs);