On Wed, Nov 16, 2022 at 09:56:26AM +0800, Pingfan Liu wrote:
> The pair of segcblist operations: rcu_segcblist_advance() and
> rcu_segcblist_accelerate() in srcu_gp_start() is needless from two
> perspectives:
>
> -1. As a part of the SRCU state machine, it should take care of either
> all sda or none. But here it only takes care of a single sda.
>
>I am not so sure that I agree.
>
>Taking care of all srcu_node structures' callbacks would be extremely
>expensive, with the expense increasing with the number of CPUs. However,
>the cost of taking care of the current CPU's callbacks is quite cheap,
>at least in the case where the srcu_struct structure's ->srcu_size_state
>field is equal to SRCU_SIZE_BIG.
>
>But are you seeing performance issues with that code on real hardware
>running real workloads when the srcu_struct structure's ->srcu_size_state
>field does not equal SRCU_SIZE_BIG? My guess is "no" given that this
>code has already paid the cost of acquiring the srcu_struct structure's
>->lock, but I figured I should ask. The opinion of real hardware running
>real workloads beats any of our opinions, after all. ;-)
>
>If you are seeing real slowdowns, then one option is to decrease the
>default value of big_cpu_lim from 128 to some appropriate smaller value.
>Maybe 16 or 32?
>
>Alternatively, the code in srcu_gp_start() that this patch removes might
>be executed only when ssp->srcu_size_state is equal to SRCU_SIZE_BIG.
>But this approach would likely still have performance issues due to the
>acquisition of srcu_struct structure's ->lock, right?
>
>Also, if you are seeing real slowdowns, please place that information
>in the commit log.
>
>Thoughts?
> -2. From the viewpoint of the callback, at the entrance,
> srcu_gp_start_if_needed() has called that pair operations and attached
> it with gp_seq. At the exit, srcu_invoke_callbacks() calls that pair
> again to extract the done callbacks. So the harvesting of the callback
> is not affected by the call to that pair in srcu_gp_start().
>
> But because the updating of RCU_DONE_TAIL by srcu_invoke_callbacks() may
> have some delay than by srcu_gp_end()->srcu_gp_start(), the removal may
> cause srcu_might_be_idle() not to be real time. To compensate that,
> supplement that pair just before the calling to rcu_segcblist_pend_cbs()
> in srcu_might_be_idle().
>
>I agree that srcu_might_be_idle() is not a bad place to add such a check,
>especially given that it has other reasons to acquire the srcu_data
>structure's ->lock. Except that this misses both call_srcu() and
>synchronize_srcu_expedited(), so that if a workload invokes call_srcu()
>and/or synchronize_srcu_expedited() on a given srcu_struct structure,
>but never synchronize)srcu(), the opportunity to advance that CPU's
>callbacks at the beginning of a grace period will always be lost.
>
Maybe at least we need the following check:
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 6af031200580..ce878b246d07 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1099,7 +1099,10 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
check_init_srcu_struct(ssp);
/* If the local srcu_data structure has callbacks, not idle. */
- sdp = raw_cpu_ptr(ssp->sda);
+ if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
+ sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
+ else
+ sdp = raw_cpu_ptr(ssp->sda);
spin_lock_irqsave_rcu_node(sdp, flags);
if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
spin_unlock_irqrestore_rcu_node(sdp, flags);
thoughts?
Thanx, Zqiang
>Alternatively, if we have checks in both synchronize_srcu() and
>srcu_gp_start_if_needed(), we end up duplicating the work in the case
>where synchronize_srcu() is invoked. This is also not particularly good.
>
>Again, thoughts?
>
> Thanx, Paul
>
> Test info:
> torture test passed using the following command against commit 094226ad94f4 ("Linux 6.1-rc5")
> tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P
>
> Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
> Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> To: rcu@xxxxxxxxxxxxxxx
> ---
> kernel/rcu/srcutree.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 725c82bb0a6a..36ba18967133 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -659,21 +659,10 @@ EXPORT_SYMBOL_GPL(__srcu_read_unlock);
> */
> static void srcu_gp_start(struct srcu_struct *ssp)
> {
> - struct srcu_data *sdp;
> int state;
>
> - if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
> - sdp = per_cpu_ptr(ssp->sda, 0);
> - else
> - sdp = this_cpu_ptr(ssp->sda);
> lockdep_assert_held(&ACCESS_PRIVATE(ssp, lock));
> WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
> - spin_lock_rcu_node(sdp); /* Interrupts already disabled. */
> - rcu_segcblist_advance(&sdp->srcu_cblist,
> - rcu_seq_current(&ssp->srcu_gp_seq));
> - (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> - rcu_seq_snap(&ssp->srcu_gp_seq));
> - spin_unlock_rcu_node(sdp); /* Interrupts remain disabled. */
> WRITE_ONCE(ssp->srcu_gp_start, jiffies);
> WRITE_ONCE(ssp->srcu_n_exp_nodelay, 0);
> smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
> @@ -1037,6 +1026,10 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
> /* If the local srcu_data structure has callbacks, not idle. */
> sdp = raw_cpu_ptr(ssp->sda);
> spin_lock_irqsave_rcu_node(sdp, flags);
> + rcu_segcblist_advance(&sdp->srcu_cblist,
> + rcu_seq_current(&ssp->srcu_gp_seq));
> + (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> + rcu_seq_snap(&ssp->srcu_gp_seq));
> if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
> spin_unlock_irqrestore_rcu_node(sdp, flags);
> return false; /* Callbacks already present, so not idle. */
> --
> 2.31.1
>
