On Thu, Nov 17, 2022 at 01:39:52AM +0000, Zhang, Qiang1 wrote: > 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); > >Consider that srcu_might_be_idle() renders a rough estimation, this is >not necessary but better to have. > >Also could you pack this pattern in a inline function if my patch is >accepted [1]? > Hi, Pingfan patch already exists https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=1d0f495f69001f0f121aa1f60f114a94b1c052ad Thanks Zqiang >Thanks, > > Pingfan > >[1]: https://lore.kernel.org/rcu/20221114175640.GA410504@paulmck-ThinkPad-P17-Gen-1/T/#m6534975507c2abca497a94d81c7abbfea1d0978d > 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 > >
