Re: [PATCH RFC v2 rcu] Fix get_state_synchronize_rcu_full() GP-start detection

[Frederic Weisbecker <frederic@kernel.org>]

Le Fri, Jan 24, 2025 at 04:01:55PM -0800, Paul E. McKenney a écrit :
> On Sat, Jan 25, 2025 at 12:03:58AM +0100, Frederic Weisbecker wrote:
> > Le Fri, Dec 13, 2024 at 11:49:49AM -0800, Paul E. McKenney a écrit :
> > > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > > index 2f9c9272cd486..d2a91f705a4ab 100644
> > > --- a/kernel/rcu/rcu.h
> > > +++ b/kernel/rcu/rcu.h
> > > @@ -162,7 +162,7 @@ static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
> > >  {
> > >  	unsigned long cur_s = READ_ONCE(*sp);
> > >  
> > > -	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (2 * RCU_SEQ_STATE_MASK + 1));
> > > +	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (3 * RCU_SEQ_STATE_MASK + 1));
> > 
> > This might need a comment.
> 
> Good point!  Would you like to propose one?

Ok.

> 
> > The way I understand it is that rcu_state.gp_seq might be seen started while
> > root_rnp->gp_seq is not. So rcu_seq_snap() on the started rcu_state.gp_seq
> > may return maximum 2 full GPs ahead of root_rnp->gp_seq. And therefore it takes below
> > 2 GPs to safely deduce we wrapped around.
> 
> Exactly!
> 
> > Should it be ULONG_CMP_LT(cur_s, s - (2 * (RCU_SEQ_STATE_MASK + 1))) ?
> 
> Quite possibly.  I freely admit that I allowed a bit of slop because
> time was of the essence (holidays and all that) and also it does not
> hurt much to lose a couple of counts out of a 2^32 cycle, to say nothing
> of the common-case 2^64 cycle.  It would not hurt to be exact, but it
> would be necessary to convince ourselves that we were not off by one in
> the wrong direction.
> 
> I would be happy to see a patch, as long as it was sufficiently
> convincing.

I'm not so much concerned about being exact but rather about making
sure we still understand what we did within one year. We can leave one
more grace period than what we expect out of paranoia but, the most
important is that we comment about what we expect and why. Let me
prepare a patch for that.

In the meantime for your patch:

   Reviewed-by: Frederic Weisbecker <frederic@kernel.org>


> 
> > Or am I missing something?
> 
> Not that I can see.  So the answer is probably "yes".  ;-)

Thanks! :-)

> 
> 							Thanx, Paul