Re: [PATCH 2/2] srcu: Remove needless updating of srcu_have_cbs in srcu_gp_end()

[Pingfan Liu <kernelfans@gmail.com>]

On Mon, Nov 21, 2022 at 05:19:16PM -0800, Paul E. McKenney wrote:
> 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.  ;-)
> 

Agree. Reasoning may miss some ground and render a wrong result.

But it is a little hard to demonstrate the past idx is not accessed. I
will go on to figure a way out.


Thanks,

	Pingfan

> 							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@gmail.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > To: rcu@vger.kernel.org
> > ---
> >  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
> >