[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200618221119.GX2723@paulmck-ThinkPad-P72>
Date: Thu, 18 Jun 2020 15:11:19 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: "Joel Fernandes (Google)" <joel@...lfernandes.org>
Cc: linux-kernel@...r.kernel.org, urezki@...il.com,
Davidlohr Bueso <dave@...olabs.net>,
Ingo Molnar <mingo@...hat.com>,
Josh Triplett <josh@...htriplett.org>,
Lai Jiangshan <jiangshanlai@...il.com>,
Marco Elver <elver@...gle.com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
rcu@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH 1/7] rcu/segcblist: Prevent useless GP start if no CBs to
accelerate
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@...il.com
> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> ---
> 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
>
Powered by blists - more mailing lists