Re: CPU trying to start a GP when no CBs were assigned new GP numbers

[Joel Fernandes <joel@joelfernandes.org>]

On Tue, Jun 16, 2020 at 09:06:14PM -0700, Paul E. McKenney wrote:
> On Tue, Jun 16, 2020 at 08:24:37PM -0400, Joel Fernandes wrote:
> > Hi,
> > I am seeing something a bit strange with RCU where it is trying to
> > start a GP twice from a CPU even though no new CB was queued on that
> > CPU. It is quite possible that I'm missing something. Anyway, I wrote
> > a patch to add some tracing when CBs are queued into the segcb. I am
> > planning to post this trace patch later.
> > 
> > The trace in the link below shows CPU2 queuing around 5 CBs, which
> > then gets accelerated at 5.192123. The GP thread running on CPU3
> > starts a new GP. Now the CPU2 softirq runs again (roughly 1ms after
> > the previous acceleration). The softirq runs probably because the GP
> > thread is expecting a QS report from CPU 2.  When the CPU2's softirq
> > runs though, it does an acceleration again which triggers a second new
> > GP start. This seems a bit unnecessary AFAICS - because the need for
> > GP *832 was already recorded which is all CPU2 should really be caring
> > about right?
> > 
> > Here is the trace: https://pastebin.com/raw/AYGzu1g4
> 
> Assuming that the WAIT= and NEXT_READY= numbers are grace-period numbers,

In the trace there are 2 numbers for WAIT and NEXT_READY each, number of
callbacks and gp numbers. Sorry, should have clarified that.

> this trace is expected behavior for two sets of callbacks, one that
> arrived at CPU 2 by time 5.192121 and another that arrived between then
> and time 5.193131.

There is just 1 set of callbacks.
> 
> So I have to ask...  What tells you that no callbacks arrived at CPU 2
> during this interval?

Because there is no rcu_callback tracepoint fired in the interim. The number
of callbacks that I'm tracing also confirm this.

> 
> On the other hand, if CPU 2 is offloaded, what you might be seeing is
> the delayed drain of callbacks from the bypass.

Sorry should have clarified it was not offloaded.

I dug more deeper and noticed that during acceleration, it is possible that
the gp_seq numbers of empty segments are updated. In this case,
rcu_segcblist_accelerate() still returns true resulting in starting of a new
future GP. The below patch cures it, but I'm not sure if it introduces other
issues. In light testing, it appears working. WDYT?

---8<-----------------------

diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
index 5f4fd3b8777ca..ebdba1d95f629 100644
--- a/kernel/rcu/rcu_segcblist.c
+++ b/kernel/rcu/rcu_segcblist.c
@@ -446,7 +478,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
  */
 bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
 {
-	int i;
+	int i, oldest_seg;
 
 	WARN_ON_ONCE(!rcu_segcblist_is_enabled(rsclp));
 	if (rcu_segcblist_restempty(rsclp, RCU_DONE_TAIL))
@@ -465,6 +497,9 @@ bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
 		    ULONG_CMP_LT(rsclp->gp_seq[i], seq))
 			break;
 
+	/* The oldest segment after which everything later is merged. */
+	oldest_seg = i;
+
 	/*
 	 * If all the segments contain callbacks that correspond to
 	 * earlier grace-period sequence numbers than "seq", leave.
@@ -488,10 +523,19 @@ bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
 	 * where there were no pending callbacks in the rcu_segcblist
 	 * structure other than in the RCU_NEXT_TAIL segment.
 	 */
 	for (; i < RCU_NEXT_TAIL; i++) {
 		WRITE_ONCE(rsclp->tails[i], rsclp->tails[RCU_NEXT_TAIL]);
 		rsclp->gp_seq[i] = seq;
 	}
+
+	/*
+	 * If all segments after oldest_seg were empty, then new GP numbers
+	 * were assigned to empty segments. In this case, no need to start
+	 * those future GPs.
+	 */
+	if (rcu_segcblist_restempty(rsclp, oldest_seg))
+		return false;
+
 	return true;
 }