[RFC PATCH 0/4] rcu: Expedited GP polling improvements

[RFC PATCH 0/4] rcu: Expedited GP polling improvements

[Frederic Weisbecker]

I initially wanted to shape that into the form of reviews or questions
but I thought I might as well send patches.

Be careful, it's fairly possible I misunderstood some things and those
patches are completely off the track.

At least TREE07 looks happy.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	rcu/dev

HEAD: dd1f68246e04fe03fde46cd55f1c87ea92a6c57e

Thanks,
	Frederic
---

Frederic Weisbecker (4):
      rcu: Remove needless polling work requeue for further waiter
      rcu: No need to reset the poll request flag before completion
      rcu: Perform early sequence fetch for polling locklessly
      rcu: Name internal polling flag


 kernel/rcu/rcu.h      |  5 +++++
 kernel/rcu/tree.c     |  2 +-
 kernel/rcu/tree_exp.h | 18 +++++++-----------
 3 files changed, 13 insertions(+), 12 deletions(-)

[PATCH 1/4] rcu: Remove needless polling work requeue for further waiter

[Frederic Weisbecker]

If another expedited polling site is waiting for the next grace period,
there is no need to requeue the work because it is guaranteed to be
either already queued or executing the actual polling upon completion.

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 | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index a6cb02a4d27c..b6fd857f34ba 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -919,9 +919,7 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
 		__synchronize_rcu_expedited(true);
 	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
 	s = rnp->exp_seq_poll_rq;
-	if (!(s & 0x1) && !sync_exp_work_done(s))
-		queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
-	else
+	if (!(s & 0x1) && sync_exp_work_done(s))
 		rnp->exp_seq_poll_rq |= 0x1;
 	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
 }
-- 
2.25.1

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

[Frederic Weisbecker]

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;
 	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
 	if (s & 0x1)
 		return;
-- 
2.25.1

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

[Frederic Weisbecker]

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.

Thanks.

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

[Paul E. McKenney]

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;

[PATCH 3/4] rcu: Perform early sequence fetch for polling locklessly

[Frederic Weisbecker]

The workqueue ordering guarantees that the work sees all the accesses
of the task prior to its call to the corresponding queue_work().

Therefore the sequence to poll can be retrieved locklessly.

The only downside is that it is then possible to miss the 0x1 flag set
by a prior work. But this could already happen concurrently anyway after
the exp_poll_lock is unlocked. In the worst case the slow path involving
synchronize_rcu_expedited() takes care of the situation.

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 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 763ec35546ed..c4a19c6a83cf 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -909,9 +909,7 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
 	struct rcu_node *rnp = container_of(wp, struct rcu_node, exp_poll_wq);
 	unsigned long s;
 
-	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
-	s = rnp->exp_seq_poll_rq;
-	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
+	s = READ_ONCE(rnp->exp_seq_poll_rq);
 	if (s & 0x1)
 		return;
 	while (!sync_exp_work_done(s))
@@ -919,7 +917,7 @@ 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;
 	if (!(s & 0x1) && sync_exp_work_done(s))
-		rnp->exp_seq_poll_rq |= 0x1;
+		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
 	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
 }
 
@@ -949,7 +947,7 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
 	if (rcu_init_invoked())
 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
 	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
-		rnp->exp_seq_poll_rq = s;
+		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
 		if (rcu_init_invoked())
 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
 	}
-- 
2.25.1

[PATCH 4/4] rcu: Name internal polling flag

[Frederic Weisbecker]

Give a proper self-explanatory name to the expedited grace period
internal polling flag.

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/rcu.h      | 5 +++++
 kernel/rcu/tree.c     | 2 +-
 kernel/rcu/tree_exp.h | 9 +++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index eccbdbdaa02e..8a62bb416ba4 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -30,6 +30,11 @@
 #define RCU_GET_STATE_USE_NORMAL	0x2
 #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
 
+/*
+ * Low-order bit definitions for polled grace-period internals.
+ */
+#define RCU_EXP_SEQ_POLL_DONE 0x1
+
 /*
  * Return the counter portion of a sequence number previously returned
  * by rcu_seq_snap() or rcu_seq_current().
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5da381a3cbe5..b3223b365f9f 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4679,7 +4679,7 @@ static void __init rcu_init_one(void)
 			spin_lock_init(&rnp->exp_lock);
 			mutex_init(&rnp->boost_kthread_mutex);
 			raw_spin_lock_init(&rnp->exp_poll_lock);
-			rnp->exp_seq_poll_rq = 0x1;
+			rnp->exp_seq_poll_rq = RCU_EXP_SEQ_POLL_DONE;
 			INIT_WORK(&rnp->exp_poll_wq, sync_rcu_do_polled_gp);
 		}
 	}
diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index c4a19c6a83cf..7ccb909d6355 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -910,14 +910,14 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
 	unsigned long s;
 
 	s = READ_ONCE(rnp->exp_seq_poll_rq);
-	if (s & 0x1)
+	if (s & RCU_EXP_SEQ_POLL_DONE)
 		return;
 	while (!sync_exp_work_done(s))
 		__synchronize_rcu_expedited(true);
 	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
 	s = rnp->exp_seq_poll_rq;
-	if (!(s & 0x1) && sync_exp_work_done(s))
-		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
+	if (!(s & RCU_EXP_SEQ_POLL_DONE) && sync_exp_work_done(s))
+		WRITE_ONCE(rnp->exp_seq_poll_rq, s | RCU_EXP_SEQ_POLL_DONE);
 	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
 }
 
@@ -946,7 +946,8 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
 	rnp = rdp->mynode;
 	if (rcu_init_invoked())
 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
-	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
+	if ((rnp->exp_seq_poll_rq & RCU_EXP_SEQ_POLL_DONE) ||
+	    ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
 		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
 		if (rcu_init_invoked())
 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
-- 
2.25.1

Re: [PATCH 4/4] rcu: Name internal polling flag

[Neeraj Upadhyay]

On 3/16/2022 8:12 PM, Frederic Weisbecker wrote:
> Give a proper self-explanatory name to the expedited grace period
> internal polling flag.
> 
> 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/rcu.h      | 5 +++++
>   kernel/rcu/tree.c     | 2 +-
>   kernel/rcu/tree_exp.h | 9 +++++----
>   3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index eccbdbdaa02e..8a62bb416ba4 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -30,6 +30,11 @@
>   #define RCU_GET_STATE_USE_NORMAL	0x2
>   #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
>   
> +/*
> + * Low-order bit definitions for polled grace-period internals.
> + */
> +#define RCU_EXP_SEQ_POLL_DONE 0x1

 From what I understood, this flag is intended for lifecycle management
of the ->exp_seq_poll_rq; with the flag set meaning that we need to 
re-poll, which could be used for cases, where there is long gap between 
2 polls, such that the sequence wraps around. So, maybe we can name it 
as RCU_EXP_SEQ_POLL_EXPIRED? However, my understanding could be wrong here.


Thanks
Neeraj

> +
>   /*
>    * Return the counter portion of a sequence number previously returned
>    * by rcu_seq_snap() or rcu_seq_current().
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 5da381a3cbe5..b3223b365f9f 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4679,7 +4679,7 @@ static void __init rcu_init_one(void)
>   			spin_lock_init(&rnp->exp_lock);
>   			mutex_init(&rnp->boost_kthread_mutex);
>   			raw_spin_lock_init(&rnp->exp_poll_lock);
> -			rnp->exp_seq_poll_rq = 0x1;
> +			rnp->exp_seq_poll_rq = RCU_EXP_SEQ_POLL_DONE;
>   			INIT_WORK(&rnp->exp_poll_wq, sync_rcu_do_polled_gp);
>   		}
>   	}
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index c4a19c6a83cf..7ccb909d6355 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -910,14 +910,14 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
>   	unsigned long s;
>   
>   	s = READ_ONCE(rnp->exp_seq_poll_rq);
> -	if (s & 0x1)
> +	if (s & RCU_EXP_SEQ_POLL_DONE)
>   		return;
>   	while (!sync_exp_work_done(s))
>   		__synchronize_rcu_expedited(true);
>   	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
>   	s = rnp->exp_seq_poll_rq;
> -	if (!(s & 0x1) && sync_exp_work_done(s))
> -		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
> +	if (!(s & RCU_EXP_SEQ_POLL_DONE) && sync_exp_work_done(s))
> +		WRITE_ONCE(rnp->exp_seq_poll_rq, s | RCU_EXP_SEQ_POLL_DONE);
>   	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
>   }
>   
> @@ -946,7 +946,8 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
>   	rnp = rdp->mynode;
>   	if (rcu_init_invoked())
>   		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> -	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> +	if ((rnp->exp_seq_poll_rq & RCU_EXP_SEQ_POLL_DONE) ||
> +	    ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
>   		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
>   		if (rcu_init_invoked())
>   			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);

Re: [PATCH 4/4] rcu: Name internal polling flag

[Frederic Weisbecker]

On Thu, Mar 17, 2022 at 03:12:17PM +0530, Neeraj Upadhyay wrote:
> 
> 
> On 3/16/2022 8:12 PM, Frederic Weisbecker wrote:
> > Give a proper self-explanatory name to the expedited grace period
> > internal polling flag.
> > 
> > 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/rcu.h      | 5 +++++
> >   kernel/rcu/tree.c     | 2 +-
> >   kernel/rcu/tree_exp.h | 9 +++++----
> >   3 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > index eccbdbdaa02e..8a62bb416ba4 100644
> > --- a/kernel/rcu/rcu.h
> > +++ b/kernel/rcu/rcu.h
> > @@ -30,6 +30,11 @@
> >   #define RCU_GET_STATE_USE_NORMAL	0x2
> >   #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
> > +/*
> > + * Low-order bit definitions for polled grace-period internals.
> > + */
> > +#define RCU_EXP_SEQ_POLL_DONE 0x1
> 
> From what I understood, this flag is intended for lifecycle management
> of the ->exp_seq_poll_rq; with the flag set meaning that we need to re-poll,
> which could be used for cases, where there is long gap between 2 polls, such
> that the sequence wraps around. So, maybe we can name it as
> RCU_EXP_SEQ_POLL_EXPIRED? However, my understanding could be wrong here.

It may be confusing also because this patchset slightly extends the role of this
bit.

Before the patchset, the role is indeed to deal with wrapping.
After the patchset it deals with wrapping and the polling cycle.

I would say that before the patchset, the name could be RCU_EXP_SEQ_POLLABLE,
and after the patchset it can indeed be RCU_EXP_SEQ_POLL_EXPIRED.

Re: [PATCH 4/4] rcu: Name internal polling flag

[Paul E. McKenney]

On Wed, Mar 16, 2022 at 03:42:55PM +0100, Frederic Weisbecker wrote:
> Give a proper self-explanatory name to the expedited grace period
> internal polling flag.
> 
> 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/rcu.h      | 5 +++++
>  kernel/rcu/tree.c     | 2 +-
>  kernel/rcu/tree_exp.h | 9 +++++----
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index eccbdbdaa02e..8a62bb416ba4 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -30,6 +30,11 @@
>  #define RCU_GET_STATE_USE_NORMAL	0x2
>  #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
>  
> +/*
> + * Low-order bit definitions for polled grace-period internals.
> + */
> +#define RCU_EXP_SEQ_POLL_DONE 0x1
> +
>  /*
>   * Return the counter portion of a sequence number previously returned
>   * by rcu_seq_snap() or rcu_seq_current().
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 5da381a3cbe5..b3223b365f9f 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4679,7 +4679,7 @@ static void __init rcu_init_one(void)
>  			spin_lock_init(&rnp->exp_lock);
>  			mutex_init(&rnp->boost_kthread_mutex);
>  			raw_spin_lock_init(&rnp->exp_poll_lock);
> -			rnp->exp_seq_poll_rq = 0x1;
> +			rnp->exp_seq_poll_rq = RCU_EXP_SEQ_POLL_DONE;
>  			INIT_WORK(&rnp->exp_poll_wq, sync_rcu_do_polled_gp);
>  		}
>  	}
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index c4a19c6a83cf..7ccb909d6355 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -910,14 +910,14 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
>  	unsigned long s;
>  
>  	s = READ_ONCE(rnp->exp_seq_poll_rq);
> -	if (s & 0x1)
> +	if (s & RCU_EXP_SEQ_POLL_DONE)
>  		return;
>  	while (!sync_exp_work_done(s))
>  		__synchronize_rcu_expedited(true);

One additional question.  If we re-read rnp->exp_seq_poll_rq on each pass
through the loop, wouldn't we have less trouble with counter wrap?

							Thanx, Paul

>  	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
>  	s = rnp->exp_seq_poll_rq;
> -	if (!(s & 0x1) && sync_exp_work_done(s))
> -		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
> +	if (!(s & RCU_EXP_SEQ_POLL_DONE) && sync_exp_work_done(s))
> +		WRITE_ONCE(rnp->exp_seq_poll_rq, s | RCU_EXP_SEQ_POLL_DONE);
>  	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
>  }
>  
> @@ -946,7 +946,8 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
>  	rnp = rdp->mynode;
>  	if (rcu_init_invoked())
>  		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> -	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> +	if ((rnp->exp_seq_poll_rq & RCU_EXP_SEQ_POLL_DONE) ||
> +	    ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
>  		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
>  		if (rcu_init_invoked())
>  			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> -- 
> 2.25.1
> 

Re: [PATCH 4/4] rcu: Name internal polling flag

[Frederic Weisbecker]

On Mon, Mar 21, 2022 at 07:11:07PM -0700, Paul E. McKenney wrote:
> On Wed, Mar 16, 2022 at 03:42:55PM +0100, Frederic Weisbecker wrote:
> > Give a proper self-explanatory name to the expedited grace period
> > internal polling flag.
> > 
> > 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/rcu.h      | 5 +++++
> >  kernel/rcu/tree.c     | 2 +-
> >  kernel/rcu/tree_exp.h | 9 +++++----
> >  3 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > index eccbdbdaa02e..8a62bb416ba4 100644
> > --- a/kernel/rcu/rcu.h
> > +++ b/kernel/rcu/rcu.h
> > @@ -30,6 +30,11 @@
> >  #define RCU_GET_STATE_USE_NORMAL	0x2
> >  #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
> >  
> > +/*
> > + * Low-order bit definitions for polled grace-period internals.
> > + */
> > +#define RCU_EXP_SEQ_POLL_DONE 0x1
> > +
> >  /*
> >   * Return the counter portion of a sequence number previously returned
> >   * by rcu_seq_snap() or rcu_seq_current().
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 5da381a3cbe5..b3223b365f9f 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -4679,7 +4679,7 @@ static void __init rcu_init_one(void)
> >  			spin_lock_init(&rnp->exp_lock);
> >  			mutex_init(&rnp->boost_kthread_mutex);
> >  			raw_spin_lock_init(&rnp->exp_poll_lock);
> > -			rnp->exp_seq_poll_rq = 0x1;
> > +			rnp->exp_seq_poll_rq = RCU_EXP_SEQ_POLL_DONE;
> >  			INIT_WORK(&rnp->exp_poll_wq, sync_rcu_do_polled_gp);
> >  		}
> >  	}
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index c4a19c6a83cf..7ccb909d6355 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -910,14 +910,14 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
> >  	unsigned long s;
> >  
> >  	s = READ_ONCE(rnp->exp_seq_poll_rq);
> > -	if (s & 0x1)
> > +	if (s & RCU_EXP_SEQ_POLL_DONE)
> >  		return;
> >  	while (!sync_exp_work_done(s))
> >  		__synchronize_rcu_expedited(true);
> 
> One additional question.  If we re-read rnp->exp_seq_poll_rq on each pass
> through the loop, wouldn't we have less trouble with counter wrap?

We can indeed do that, though it won't eliminate the possibility of wrapping.

> 
> 							Thanx, Paul
> 
> >  	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> >  	s = rnp->exp_seq_poll_rq;
> > -	if (!(s & 0x1) && sync_exp_work_done(s))
> > -		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
> > +	if (!(s & RCU_EXP_SEQ_POLL_DONE) && sync_exp_work_done(s))
> > +		WRITE_ONCE(rnp->exp_seq_poll_rq, s | RCU_EXP_SEQ_POLL_DONE);
> >  	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
> >  }
> >  
> > @@ -946,7 +946,8 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> >  	rnp = rdp->mynode;
> >  	if (rcu_init_invoked())
> >  		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > -	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> > +	if ((rnp->exp_seq_poll_rq & RCU_EXP_SEQ_POLL_DONE) ||
> > +	    ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> >  		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
> >  		if (rcu_init_invoked())
> >  			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> > -- 
> > 2.25.1
> > 

Re: [PATCH 4/4] rcu: Name internal polling flag

[Paul E. McKenney]

On Tue, Mar 22, 2022 at 11:32:24AM +0100, Frederic Weisbecker wrote:
> On Mon, Mar 21, 2022 at 07:11:07PM -0700, Paul E. McKenney wrote:
> > On Wed, Mar 16, 2022 at 03:42:55PM +0100, Frederic Weisbecker wrote:
> > > Give a proper self-explanatory name to the expedited grace period
> > > internal polling flag.
> > > 
> > > 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/rcu.h      | 5 +++++
> > >  kernel/rcu/tree.c     | 2 +-
> > >  kernel/rcu/tree_exp.h | 9 +++++----
> > >  3 files changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > > index eccbdbdaa02e..8a62bb416ba4 100644
> > > --- a/kernel/rcu/rcu.h
> > > +++ b/kernel/rcu/rcu.h
> > > @@ -30,6 +30,11 @@
> > >  #define RCU_GET_STATE_USE_NORMAL	0x2
> > >  #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
> > >  
> > > +/*
> > > + * Low-order bit definitions for polled grace-period internals.
> > > + */
> > > +#define RCU_EXP_SEQ_POLL_DONE 0x1
> > > +
> > >  /*
> > >   * Return the counter portion of a sequence number previously returned
> > >   * by rcu_seq_snap() or rcu_seq_current().
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 5da381a3cbe5..b3223b365f9f 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -4679,7 +4679,7 @@ static void __init rcu_init_one(void)
> > >  			spin_lock_init(&rnp->exp_lock);
> > >  			mutex_init(&rnp->boost_kthread_mutex);
> > >  			raw_spin_lock_init(&rnp->exp_poll_lock);
> > > -			rnp->exp_seq_poll_rq = 0x1;
> > > +			rnp->exp_seq_poll_rq = RCU_EXP_SEQ_POLL_DONE;
> > >  			INIT_WORK(&rnp->exp_poll_wq, sync_rcu_do_polled_gp);
> > >  		}
> > >  	}
> > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > index c4a19c6a83cf..7ccb909d6355 100644
> > > --- a/kernel/rcu/tree_exp.h
> > > +++ b/kernel/rcu/tree_exp.h
> > > @@ -910,14 +910,14 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
> > >  	unsigned long s;
> > >  
> > >  	s = READ_ONCE(rnp->exp_seq_poll_rq);
> > > -	if (s & 0x1)
> > > +	if (s & RCU_EXP_SEQ_POLL_DONE)
> > >  		return;
> > >  	while (!sync_exp_work_done(s))
> > >  		__synchronize_rcu_expedited(true);
> > 
> > One additional question.  If we re-read rnp->exp_seq_poll_rq on each pass
> > through the loop, wouldn't we have less trouble with counter wrap?
> 
> We can indeed do that, though it won't eliminate the possibility of wrapping.

True.  But in conjunction with an exact check for expired grace-period
sequence number, it reduces the maximum addtional penalty for wrapping
to two grace periods.

							Thanx, Paul

> > >  	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > >  	s = rnp->exp_seq_poll_rq;
> > > -	if (!(s & 0x1) && sync_exp_work_done(s))
> > > -		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
> > > +	if (!(s & RCU_EXP_SEQ_POLL_DONE) && sync_exp_work_done(s))
> > > +		WRITE_ONCE(rnp->exp_seq_poll_rq, s | RCU_EXP_SEQ_POLL_DONE);
> > >  	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
> > >  }
> > >  
> > > @@ -946,7 +946,8 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> > >  	rnp = rdp->mynode;
> > >  	if (rcu_init_invoked())
> > >  		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > > -	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> > > +	if ((rnp->exp_seq_poll_rq & RCU_EXP_SEQ_POLL_DONE) ||
> > > +	    ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> > >  		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
> > >  		if (rcu_init_invoked())
> > >  			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> > > -- 
> > > 2.25.1
> > > 

Re: [PATCH 4/4] rcu: Name internal polling flag

[Paul E. McKenney]

On Wed, Mar 23, 2022 at 06:04:02PM -0700, Paul E. McKenney wrote:
> On Tue, Mar 22, 2022 at 11:32:24AM +0100, Frederic Weisbecker wrote:
> > On Mon, Mar 21, 2022 at 07:11:07PM -0700, Paul E. McKenney wrote:
> > > On Wed, Mar 16, 2022 at 03:42:55PM +0100, Frederic Weisbecker wrote:
> > > > Give a proper self-explanatory name to the expedited grace period
> > > > internal polling flag.
> > > > 
> > > > 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/rcu.h      | 5 +++++
> > > >  kernel/rcu/tree.c     | 2 +-
> > > >  kernel/rcu/tree_exp.h | 9 +++++----
> > > >  3 files changed, 11 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > > > index eccbdbdaa02e..8a62bb416ba4 100644
> > > > --- a/kernel/rcu/rcu.h
> > > > +++ b/kernel/rcu/rcu.h
> > > > @@ -30,6 +30,11 @@
> > > >  #define RCU_GET_STATE_USE_NORMAL	0x2
> > > >  #define RCU_GET_STATE_BAD_FOR_NORMAL	(RCU_GET_STATE_FROM_EXPEDITED | RCU_GET_STATE_USE_NORMAL)
> > > >  
> > > > +/*
> > > > + * Low-order bit definitions for polled grace-period internals.
> > > > + */
> > > > +#define RCU_EXP_SEQ_POLL_DONE 0x1
> > > > +
> > > >  /*
> > > >   * Return the counter portion of a sequence number previously returned
> > > >   * by rcu_seq_snap() or rcu_seq_current().
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 5da381a3cbe5..b3223b365f9f 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -4679,7 +4679,7 @@ static void __init rcu_init_one(void)
> > > >  			spin_lock_init(&rnp->exp_lock);
> > > >  			mutex_init(&rnp->boost_kthread_mutex);
> > > >  			raw_spin_lock_init(&rnp->exp_poll_lock);
> > > > -			rnp->exp_seq_poll_rq = 0x1;
> > > > +			rnp->exp_seq_poll_rq = RCU_EXP_SEQ_POLL_DONE;
> > > >  			INIT_WORK(&rnp->exp_poll_wq, sync_rcu_do_polled_gp);
> > > >  		}
> > > >  	}
> > > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > > index c4a19c6a83cf..7ccb909d6355 100644
> > > > --- a/kernel/rcu/tree_exp.h
> > > > +++ b/kernel/rcu/tree_exp.h
> > > > @@ -910,14 +910,14 @@ static void sync_rcu_do_polled_gp(struct work_struct *wp)
> > > >  	unsigned long s;
> > > >  
> > > >  	s = READ_ONCE(rnp->exp_seq_poll_rq);
> > > > -	if (s & 0x1)
> > > > +	if (s & RCU_EXP_SEQ_POLL_DONE)
> > > >  		return;
> > > >  	while (!sync_exp_work_done(s))
> > > >  		__synchronize_rcu_expedited(true);
> > > 
> > > One additional question.  If we re-read rnp->exp_seq_poll_rq on each pass
> > > through the loop, wouldn't we have less trouble with counter wrap?
> > 
> > We can indeed do that, though it won't eliminate the possibility of wrapping.
> 
> True.  But in conjunction with an exact check for expired grace-period
> sequence number, it reduces the maximum addtional penalty for wrapping
> to two grace periods.

Oh, and I did finally queue this series for testing and further review.

							Thanx, Paul

> > > >  	raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > > >  	s = rnp->exp_seq_poll_rq;
> > > > -	if (!(s & 0x1) && sync_exp_work_done(s))
> > > > -		WRITE_ONCE(rnp->exp_seq_poll_rq, s | 0x1);
> > > > +	if (!(s & RCU_EXP_SEQ_POLL_DONE) && sync_exp_work_done(s))
> > > > +		WRITE_ONCE(rnp->exp_seq_poll_rq, s | RCU_EXP_SEQ_POLL_DONE);
> > > >  	raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
> > > >  }
> > > >  
> > > > @@ -946,7 +946,8 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> > > >  	rnp = rdp->mynode;
> > > >  	if (rcu_init_invoked())
> > > >  		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > > > -	if ((rnp->exp_seq_poll_rq & 0x1) || ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> > > > +	if ((rnp->exp_seq_poll_rq & RCU_EXP_SEQ_POLL_DONE) ||
> > > > +	    ULONG_CMP_LT(rnp->exp_seq_poll_rq, s)) {
> > > >  		WRITE_ONCE(rnp->exp_seq_poll_rq, s);
> > > >  		if (rcu_init_invoked())
> > > >  			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> > > > -- 
> > > > 2.25.1
> > > >