On Wed, Jun 17, 2020 at 07:01:00AM -0700, Paul E. McKenney wrote: > On Wed, Jun 17, 2020 at 02:39:54AM -0400, Joel Fernandes wrote: > > 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. > > Ah, I see. What are you doing to compute the number of callbacks? Here is my segcb trace patch, I added a rcu_segcblist_countseq() function: https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?id=794cba82fe8b5c72df46d4f2548957db36317d39 > > > 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? > > You are quite correct that empty segments should not be assigned > grace-period numbers. Or at least that any such grace-period numbers > should not be taken very seriously. Thanks. This is what the test diff I shared below does, basically return false from _accelerate() if all the segments assigned gp_seq(s) are empty. > > ---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; > > } > > Looks like this needs a focused test program. ;-) > > Please see attached for an outdated test userspace program. It probably > needs help to check for this additional case. And probably also other > changes to account for three years of change. > > This test program works by copying files from the designated Linux > source tree, then building them into a userspace torture test. Thanks, I will give it a try. I also added a warning in the trace patch I linked above, at the end of _accelerate() to make sure NEXT segment was always merged: + /* + * Make sure the NEXT list is always empty after an acceleration. + */ + WARN_ON_ONCE(!rcu_segcblist_restempty(rsclp, RCU_NEXT_READY_TAIL)); thanks, - Joel
