Re: [PATCH 1/7] rcu/segcblist: Prevent useless GP start if no CBs to accelerate

From: Paul E. McKenney
Date: Thu Jun 18 2020 - 18:11:22 EST


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
>