On Thu, Jun 18, 2020 at 04:29:49PM -0400, Joel Fernandes (Google) wrote: First, this looks like a very nice optimization, thank you! > rcu_segcblist_accelerate() returns true if a GP is to be > started/requested and false if not. During tracing, I found that it is > asking that GPs be requested s/GPs/unnecessary GPs/? Plus "." at end of the sentence. > The exact flow seems to be something like: > 1. Callbacks are queued on CPU A - into the NEXT list. > 2. softirq runs on CPU A, accelerate all CBs from NEXT->WAIT and request a GP X. > 3. GP thread wakes up on another CPU, starts the GP X and requests QS from CPU A. > 4. CPU A's softirq runs again presumably because of #3. Yes, that is one reason RCU softirq might run again. > 5. CPU A's softirq now does acceleration again, this time no CBs are > accelerated since last attempt, but it still requests GP X+1 which > could be useless. I can't think of a case where this request helps. How about: "but it still unnecessarily requests GP X+1"? > The fix is, prevent the useless GP start if we detect no CBs are there > to accelerate. > > With this, we have the following improvement in short runs of > rcutorture (5 seconds each): > +----+-------------------+-------------------+ > | # | Number of GPs | Number of Wakeups | > +====+=========+=========+=========+=========+ > | 1 | With | Without | With | Without | > +----+---------+---------+---------+---------+ > | 2 | 75 | 89 | 113 | 119 | > +----+---------+---------+---------+---------+ > | 3 | 62 | 91 | 105 | 123 | > +----+---------+---------+---------+---------+ > | 4 | 60 | 79 | 98 | 110 | > +----+---------+---------+---------+---------+ > | 5 | 63 | 79 | 99 | 112 | > +----+---------+---------+---------+---------+ > | 6 | 57 | 89 | 96 | 123 | > +----+---------+---------+---------+---------+ > | 7 | 64 | 85 | 97 | 118 | > +----+---------+---------+---------+---------+ > | 8 | 58 | 83 | 98 | 113 | > +----+---------+---------+---------+---------+ > | 9 | 57 | 77 | 89 | 104 | > +----+---------+---------+---------+---------+ > | 10 | 66 | 82 | 98 | 119 | > +----+---------+---------+---------+---------+ > | 11 | 52 | 82 | 83 | 117 | > +----+---------+---------+---------+---------+ So the reductions in wakeups ranges from 5% to 40%, with almost a 20% overall reduction in wakeups across all the runs. That should be of some use to someone. ;-) I do run rcutorture quite a bit, but is there a more real-world benchmark that could be tried? > Cc: urezki@xxxxxxxxx > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx> > --- > kernel/rcu/rcu_segcblist.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c > index 9a0f66133b4b3..4782cf17bf4f9 100644 > --- a/kernel/rcu/rcu_segcblist.c > +++ b/kernel/rcu/rcu_segcblist.c > @@ -475,8 +475,15 @@ bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq) > * Also advance to the oldest segment of callbacks whose > * ->gp_seq[] completion is at or after that passed in via "seq", > * skipping any empty segments. > + * > + * Note that "i" is the youngest segment of the list after which > + * any older segments than "i" would not be mutated or assigned > + * GPs. For example, if i == WAIT_TAIL, then neither WAIT_TAIL, > + * nor DONE_TAIL will be touched. Only CBs in NEXT_TAIL will be > + * merged with NEXT_READY_TAIL and the GP numbers of both of > + * them would be updated. In this case, only the GP number of NEXT_READY_TAIL would be updated, correct? Or am I missing something subtle in the loop just past the end of this patch? Thanx, Paul > */ > - if (++i >= RCU_NEXT_TAIL) > + if (rcu_segcblist_restempty(rsclp, i) || ++i >= RCU_NEXT_TAIL) > return false; > > /* > -- > 2.27.0.111.gc72c7da667-goog >
