Re: [PATCH 2/4] rcu: No need to reset the poll request flag before completion

["Paul E. McKenney" <paulmck@kernel.org>]

On Wed, Mar 30, 2022 at 01:27:52PM +0200, Frederic Weisbecker wrote:
> On Wed, Mar 16, 2022 at 03:42:53PM +0100, Frederic Weisbecker wrote:
> > The flag allowing to requeue the polling work is reset before the
> > polling even starts. However there is no point in having two competing
> > polling on the same grace period. Just reset the flag once we have
> > completed the grace period only.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Uladzislau Rezki <uladzislau.rezki@sony.com>
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/tree_exp.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index b6fd857f34ba..763ec35546ed 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -911,7 +911,6 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
> >  
> >  	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> >  	s = rnp->exp_seq_poll_rq;
> > -	rnp->exp_seq_poll_rq |= 0x1;
> 
> On a second (or actually twentieth) thought, this patch and all those following
> make wrapping issues more likely:
> 
> * Before this patch, wrapping occuring *after* the 0x1 is set on the beginning
>   of the workqueue is fine. The last vulnerable wrapping scenario is when
>   the wrapping happens before we reach the beginning of the workqueue
>   execution that sets the 0x1, so the work may happen not to be queued.
> 
> 
> * After this patch, wrapping occuring *before* the GP completion in the
>   workqueue will be ignored and fail. Still unlikely, but less unlikely than
>   before this patch.
> 
> So please revert this series. Only the first patch "rcu: Remove needless polling
> work requeue for further waiter" still seem to make sense.

I know that twentieth-thought feeling!

I reverted the following commits, and will remove the original and the
reversion of each on my next rebase:

26632dde0c40 ("rcu: No need to reset the poll request flag before completion")
b889e463d447 ("rcu: Perform early sequence fetch for polling locklessly")
11eccc01200f ("rcu: Name internal polling flag")

Would it make sense to apply rcu_seq_done_exact(), perhaps as follows?
Or is there some reason this would cause problems?

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index b6fd857f34ba..bd47fce0e08c 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -992,7 +992,7 @@ bool poll_state_synchronize_rcu_expedited(unsigned long oldstate)
 	WARN_ON_ONCE(!(oldstate & RCU_GET_STATE_FROM_EXPEDITED));
 	if (oldstate & RCU_GET_STATE_USE_NORMAL)
 		return poll_state_synchronize_rcu(oldstate & ~RCU_GET_STATE_BAD_FOR_NORMAL);
-	if (!rcu_exp_gp_seq_done(oldstate & ~RCU_SEQ_STATE_MASK))
+	if (!rcu_seq_done_exact(&rcu_state.expedited_sequence, oldstate & ~RCU_SEQ_STATE_MASK))
 		return false;
 	smp_mb(); /* Ensure GP ends before subsequent accesses. */
 	return true;