On Sun, Nov 20, 2022 at 11:40:14AM +0800, Pingfan Liu wrote: > At present, snp->srcu_have_cbs[idx] is updated by either > srcu_funnel_gp_start() or srcu_gp_end(). > > But as the code changes, now, srcu_funnel_gp_start() is called with srcu > read lock held. And its input parameter s, s = rcu_seq_snap(&ssp->srcu_gp_seq), > whose counter field always proceeds that of the srcu_gp_seq by one or two. > > If the seq snap only proceeds by one, the state machine should be in > state SRCU_STATE_IDLE, the held srcu read lock will prevent the state > machine from moving ahead. So when srcu_gp_end() updates > snp->srcu_have_cbs[idx], the idx must be the past idx for > srcu_funnel_gp_start() that is cared by nobody. > > So removing the unnecessary updating in srcu_gp_end(). This looks plausible, good eyes! But is there a debug check that could verify that this is unnecessary? Logical reasoning is all well and good, but the actual system always wins arguments. ;-) Thanx, Paul > Test info: > Running "tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 10h --configs 18*SRCU-P" > without any failure. > > 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 | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 7df59fc8073e..c54d6c04751f 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -783,8 +783,6 @@ static void srcu_gp_end(struct srcu_struct *ssp) > last_lvl = snp >= ssp->level[rcu_num_lvls - 1]; > if (last_lvl) > cbs = ss_state < SRCU_SIZE_BIG || snp->srcu_have_cbs[idx] == gpseq; > - snp->srcu_have_cbs[idx] = gpseq; > - rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1); > sgsne = snp->srcu_gp_seq_needed_exp; > if (srcu_invl_snp_seq(sgsne) || ULONG_CMP_LT(sgsne, gpseq)) > WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq); > -- > 2.31.1 >
