rcu.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report
@ 2021-09-16 12:10 Frederic Weisbecker
  2021-09-16 12:10 ` [PATCH 1/4] rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs() Frederic Weisbecker
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-16 12:10 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: LKML, Frederic Weisbecker, rcu, urezki, boqun.feng,
	Neeraj Upadhyay, joel

This eventually removes rcu_data.exp_deferred_qs to use
rcu_data.cpu no_qs.b.exp instead.

For those like me who need a headlamp to walk there: https://ibb.co/3d06r0V

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

HEAD: 7d9d8a0c6141f95cbac4367b12e755bfabb383ee

Thanks,
	Frederic
---

Frederic Weisbecker (4):
      rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs()
      rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
      rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp()
      rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp


 kernel/rcu/tree.h        |  1 -
 kernel/rcu/tree_exp.h    |  6 +++---
 kernel/rcu/tree_plugin.h | 24 +++++++++++++-----------
 3 files changed, 16 insertions(+), 15 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs()
  2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
@ 2021-09-16 12:10 ` Frederic Weisbecker
  2021-09-16 12:10 ` [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs Frederic Weisbecker
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-16 12:10 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: LKML, Frederic Weisbecker, rcu, urezki, boqun.feng,
	Neeraj Upadhyay, joel

The preemptible RCU flavour doesn't rely at all on
struct rcu_data::cpu_no_qs::b::exp and use its own
struct rcu_data::exp_deferred_qs field to record the need for an
expedited quiescent state.

In fact rdp.cpu_no_qs.b.exp should never be set in preemptible RCU.

Make that fact clear and disambiguate the expectations on the
preemptible RCU's rcu_qs() implementation. In this flavour, common
quiescent states aren't expected to deal with expedited grace periods
because those have their own way to report expedited quiescent states:
rcu_read_unlock_special, context switches, etc...

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree_plugin.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 1a6fdb03d0a5..4c9aeabec242 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -277,12 +277,16 @@ static void rcu_preempt_ctxt_queue(struct rcu_node *rnp, struct rcu_data *rdp)
  * current task, there might be any number of other tasks blocked while
  * in an RCU read-side critical section.
  *
+ * Unlike non-preemptible-RCU, quiescent state reports for expedited
+ * grace periods are handled seperately via deferred quiescent states
+ * and context switch events.
+ *
  * Callers to this function must disable preemption.
  */
 static void rcu_qs(void)
 {
 	RCU_LOCKDEP_WARN(preemptible(), "rcu_qs() invoked with preemption enabled!!!\n");
-	if (__this_cpu_read(rcu_data.cpu_no_qs.s)) {
+	if (__this_cpu_read(rcu_data.cpu_no_qs.b.norm)) {
 		trace_rcu_grace_period(TPS("rcu_preempt"),
 				       __this_cpu_read(rcu_data.gp_seq),
 				       TPS("cpuqs"));
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
  2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
  2021-09-16 12:10 ` [PATCH 1/4] rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs() Frederic Weisbecker
@ 2021-09-16 12:10 ` Frederic Weisbecker
  2021-09-16 16:43   ` Paul E. McKenney
  2021-09-16 12:10 ` [PATCH 3/4] rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp() Frederic Weisbecker
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-16 12:10 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: LKML, Frederic Weisbecker, rcu, urezki, boqun.feng,
	Neeraj Upadhyay, joel

This variable is never written nor read remotely. Remove this confusion.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree_exp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index f3947c49eee7..4266610b4587 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -255,7 +255,7 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
  */
 static void rcu_report_exp_rdp(struct rcu_data *rdp)
 {
-	WRITE_ONCE(rdp->exp_deferred_qs, false);
+	rdp->exp_deferred_qs = false;
 	rcu_report_exp_cpu_mult(rdp->mynode, rdp->grpmask, true);
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/4] rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp()
  2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
  2021-09-16 12:10 ` [PATCH 1/4] rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs() Frederic Weisbecker
  2021-09-16 12:10 ` [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs Frederic Weisbecker
@ 2021-09-16 12:10 ` Frederic Weisbecker
  2021-09-16 12:10 ` [PATCH 4/4] rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp Frederic Weisbecker
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-16 12:10 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: LKML, Frederic Weisbecker, rcu, urezki, boqun.feng,
	Neeraj Upadhyay, joel

On non-preemptible RCU, move clearing of rcu_data.cpu_no_qs.b.exp to
the actual expedited quiescent state report function, in a similar way
to preemptible RCU dealing with rdp->exp_deferred_qs.

This prepares to remove the exp_deferred_qs field and generalize the use
of cpu_no_qs.b.exp to both preemptible and non-preemptible RCU.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree_exp.h    | 1 +
 kernel/rcu/tree_plugin.h | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 4266610b4587..992f9475d243 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -256,6 +256,7 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
 static void rcu_report_exp_rdp(struct rcu_data *rdp)
 {
 	rdp->exp_deferred_qs = false;
+	rdp->cpu_no_qs.b.exp = false;
 	rcu_report_exp_cpu_mult(rdp->mynode, rdp->grpmask, true);
 }
 
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 4c9aeabec242..b20634fde43d 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -849,10 +849,8 @@ static void rcu_qs(void)
 	trace_rcu_grace_period(TPS("rcu_sched"),
 			       __this_cpu_read(rcu_data.gp_seq), TPS("cpuqs"));
 	__this_cpu_write(rcu_data.cpu_no_qs.b.norm, false);
-	if (!__this_cpu_read(rcu_data.cpu_no_qs.b.exp))
-		return;
-	__this_cpu_write(rcu_data.cpu_no_qs.b.exp, false);
-	rcu_report_exp_rdp(this_cpu_ptr(&rcu_data));
+	if (__this_cpu_read(rcu_data.cpu_no_qs.b.exp))
+		rcu_report_exp_rdp(this_cpu_ptr(&rcu_data));
 }
 
 /*
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/4] rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp
  2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
                   ` (2 preceding siblings ...)
  2021-09-16 12:10 ` [PATCH 3/4] rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp() Frederic Weisbecker
@ 2021-09-16 12:10 ` Frederic Weisbecker
  2021-09-16 16:40 ` [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Paul E. McKenney
  2021-09-28 18:13 ` Paul E. McKenney
  5 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-16 12:10 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: LKML, Frederic Weisbecker, rcu, urezki, boqun.feng,
	Neeraj Upadhyay, joel

Having two fields for the same purpose with subtle differences
on different RCU flavours is confusing, especially when both fields
always exist on both RCU flavours.

Fortunately it's now safe for preemptible RCU to rely on rcu_data.cpu
no_qs.b.exp instead of its own ad-hoc exp_deferred_qs field.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree.h        |  1 -
 kernel/rcu/tree_exp.h    |  5 ++---
 kernel/rcu/tree_plugin.h | 12 ++++++------
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 305cf6aeb408..ea46ed40f6bc 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -157,7 +157,6 @@ struct rcu_data {
 	bool		core_needs_qs;	/* Core waits for quiescent state. */
 	bool		beenonline;	/* CPU online at least once. */
 	bool		gpwrap;		/* Possible ->gp_seq wrap. */
-	bool		exp_deferred_qs; /* This CPU awaiting a deferred QS? */
 	bool		cpu_started;	/* RCU watching this onlining CPU. */
 	struct rcu_node *mynode;	/* This CPU's leaf of hierarchy */
 	unsigned long grpmask;		/* Mask to apply to leaf qsmask. */
diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 992f9475d243..64e454345d52 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -255,7 +255,6 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
  */
 static void rcu_report_exp_rdp(struct rcu_data *rdp)
 {
-	rdp->exp_deferred_qs = false;
 	rdp->cpu_no_qs.b.exp = false;
 	rcu_report_exp_cpu_mult(rdp->mynode, rdp->grpmask, true);
 }
@@ -656,7 +655,7 @@ static void rcu_exp_handler(void *unused)
 		    rcu_dynticks_curr_cpu_in_eqs()) {
 			rcu_report_exp_rdp(rdp);
 		} else {
-			rdp->exp_deferred_qs = true;
+			rdp->cpu_no_qs.b.exp = true;
 			set_tsk_need_resched(t);
 			set_preempt_need_resched();
 		}
@@ -678,7 +677,7 @@ static void rcu_exp_handler(void *unused)
 	if (depth > 0) {
 		raw_spin_lock_irqsave_rcu_node(rnp, flags);
 		if (rnp->expmask & rdp->grpmask) {
-			rdp->exp_deferred_qs = true;
+			rdp->cpu_no_qs.b.exp = true;
 			t->rcu_read_unlock_special.b.exp_hint = true;
 		}
 		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index b20634fde43d..796c4913bbb8 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -260,10 +260,10 @@ static void rcu_preempt_ctxt_queue(struct rcu_node *rnp, struct rcu_data *rdp)
 	 * no need to check for a subsequent expedited GP.  (Though we are
 	 * still in a quiescent state in any case.)
 	 */
-	if (blkd_state & RCU_EXP_BLKD && rdp->exp_deferred_qs)
+	if (blkd_state & RCU_EXP_BLKD && rdp->cpu_no_qs.b.exp)
 		rcu_report_exp_rdp(rdp);
 	else
-		WARN_ON_ONCE(rdp->exp_deferred_qs);
+		WARN_ON_ONCE(rdp->cpu_no_qs.b.exp);
 }
 
 /*
@@ -354,7 +354,7 @@ void rcu_note_context_switch(bool preempt)
 	 * means that we continue to block the current grace period.
 	 */
 	rcu_qs();
-	if (rdp->exp_deferred_qs)
+	if (rdp->cpu_no_qs.b.exp)
 		rcu_report_exp_rdp(rdp);
 	rcu_tasks_qs(current, preempt);
 	trace_rcu_utilization(TPS("End context switch"));
@@ -481,7 +481,7 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
 	 */
 	special = t->rcu_read_unlock_special;
 	rdp = this_cpu_ptr(&rcu_data);
-	if (!special.s && !rdp->exp_deferred_qs) {
+	if (!special.s && !rdp->cpu_no_qs.b.exp) {
 		local_irq_restore(flags);
 		return;
 	}
@@ -501,7 +501,7 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
 	 * tasks are handled when removing the task from the
 	 * blocked-tasks list below.
 	 */
-	if (rdp->exp_deferred_qs)
+	if (rdp->cpu_no_qs.b.exp)
 		rcu_report_exp_rdp(rdp);
 
 	/* Clean up if blocked during RCU read-side critical section. */
@@ -584,7 +584,7 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
  */
 static bool rcu_preempt_need_deferred_qs(struct task_struct *t)
 {
-	return (__this_cpu_read(rcu_data.exp_deferred_qs) ||
+	return (__this_cpu_read(rcu_data.cpu_no_qs.b.exp) ||
 		READ_ONCE(t->rcu_read_unlock_special.s)) &&
 	       rcu_preempt_depth() == 0;
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report
  2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
                   ` (3 preceding siblings ...)
  2021-09-16 12:10 ` [PATCH 4/4] rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp Frederic Weisbecker
@ 2021-09-16 16:40 ` Paul E. McKenney
  2021-09-28 18:13 ` Paul E. McKenney
  5 siblings, 0 replies; 11+ messages in thread
From: Paul E. McKenney @ 2021-09-16 16:40 UTC (permalink / raw)
  To: Frederic Weisbecker; +Cc: LKML, rcu, urezki, boqun.feng, Neeraj Upadhyay, joel

On Thu, Sep 16, 2021 at 02:10:44PM +0200, Frederic Weisbecker wrote:
> This eventually removes rcu_data.exp_deferred_qs to use
> rcu_data.cpu no_qs.b.exp instead.
> 
> For those like me who need a headlamp to walk there: https://ibb.co/3d06r0V

Can't have too many headlamps with this code!  ;-)

							Thanx, Paul

> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> 	rcu/dev
> 
> HEAD: 7d9d8a0c6141f95cbac4367b12e755bfabb383ee
> 
> Thanks,
> 	Frederic
> ---
> 
> Frederic Weisbecker (4):
>       rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs()
>       rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
>       rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp()
>       rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp
> 
> 
>  kernel/rcu/tree.h        |  1 -
>  kernel/rcu/tree_exp.h    |  6 +++---
>  kernel/rcu/tree_plugin.h | 24 +++++++++++++-----------
>  3 files changed, 16 insertions(+), 15 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
  2021-09-16 12:10 ` [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs Frederic Weisbecker
@ 2021-09-16 16:43   ` Paul E. McKenney
  2021-09-16 21:05     ` Frederic Weisbecker
  0 siblings, 1 reply; 11+ messages in thread
From: Paul E. McKenney @ 2021-09-16 16:43 UTC (permalink / raw)
  To: Frederic Weisbecker; +Cc: LKML, rcu, urezki, boqun.feng, Neeraj Upadhyay, joel

On Thu, Sep 16, 2021 at 02:10:46PM +0200, Frederic Weisbecker wrote:
> This variable is never written nor read remotely. Remove this confusion.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>  kernel/rcu/tree_exp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index f3947c49eee7..4266610b4587 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -255,7 +255,7 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
>   */
>  static void rcu_report_exp_rdp(struct rcu_data *rdp)
>  {
> -	WRITE_ONCE(rdp->exp_deferred_qs, false);
> +	rdp->exp_deferred_qs = false;

Are you sure that this can never be invoked from an interrupt handler?
And that rdp->exp_deferred_qs is never read from an interrupt handler?
If either can happen, then the WRITE_ONCE() does play a role, right?

							Thanx, Paul

>  	rcu_report_exp_cpu_mult(rdp->mynode, rdp->grpmask, true);
>  }
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
  2021-09-16 16:43   ` Paul E. McKenney
@ 2021-09-16 21:05     ` Frederic Weisbecker
  2021-09-17 18:10       ` Paul E. McKenney
  0 siblings, 1 reply; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-16 21:05 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: LKML, rcu, urezki, boqun.feng, Neeraj Upadhyay, joel

On Thu, Sep 16, 2021 at 09:43:40AM -0700, Paul E. McKenney wrote:
> On Thu, Sep 16, 2021 at 02:10:46PM +0200, Frederic Weisbecker wrote:
> > This variable is never written nor read remotely. Remove this confusion.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > ---
> >  kernel/rcu/tree_exp.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index f3947c49eee7..4266610b4587 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -255,7 +255,7 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
> >   */
> >  static void rcu_report_exp_rdp(struct rcu_data *rdp)
> >  {
> > -	WRITE_ONCE(rdp->exp_deferred_qs, false);
> > +	rdp->exp_deferred_qs = false;
> 
> Are you sure that this can never be invoked from an interrupt handler?
> And that rdp->exp_deferred_qs is never read from an interrupt handler?
> If either can happen, then the WRITE_ONCE() does play a role, right?

Well, the only effect I can imagine is that it can partly prevent from an
interrupt to report concurrently the quiescent state during the few
instructions before we mask interrupts and lock the node.

That's a micro performance benefit that avoid a second call to
rcu_report_exp_cpu_mult() with the extra locking and early exit.

But then that racy interrupt can still happen before we clear exp_deferred_qs.
In this case __this_cpu_cmpxchg() would have been more efficient.

Thanks.

> 							Thanx, Paul
> 
> >  	rcu_report_exp_cpu_mult(rdp->mynode, rdp->grpmask, true);
> >  }
> >  
> > -- 
> > 2.25.1
> > 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
  2021-09-16 21:05     ` Frederic Weisbecker
@ 2021-09-17 18:10       ` Paul E. McKenney
  2021-09-17 22:00         ` Frederic Weisbecker
  0 siblings, 1 reply; 11+ messages in thread
From: Paul E. McKenney @ 2021-09-17 18:10 UTC (permalink / raw)
  To: Frederic Weisbecker; +Cc: LKML, rcu, urezki, boqun.feng, Neeraj Upadhyay, joel

On Thu, Sep 16, 2021 at 11:05:14PM +0200, Frederic Weisbecker wrote:
> On Thu, Sep 16, 2021 at 09:43:40AM -0700, Paul E. McKenney wrote:
> > On Thu, Sep 16, 2021 at 02:10:46PM +0200, Frederic Weisbecker wrote:
> > > This variable is never written nor read remotely. Remove this confusion.
> > > 
> > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > ---
> > >  kernel/rcu/tree_exp.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > index f3947c49eee7..4266610b4587 100644
> > > --- a/kernel/rcu/tree_exp.h
> > > +++ b/kernel/rcu/tree_exp.h
> > > @@ -255,7 +255,7 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
> > >   */
> > >  static void rcu_report_exp_rdp(struct rcu_data *rdp)
> > >  {
> > > -	WRITE_ONCE(rdp->exp_deferred_qs, false);
> > > +	rdp->exp_deferred_qs = false;
> > 
> > Are you sure that this can never be invoked from an interrupt handler?
> > And that rdp->exp_deferred_qs is never read from an interrupt handler?
> > If either can happen, then the WRITE_ONCE() does play a role, right?
> 
> Well, the only effect I can imagine is that it can partly prevent from an
> interrupt to report concurrently the quiescent state during the few
> instructions before we mask interrupts and lock the node.
> 
> That's a micro performance benefit that avoid a second call to
> rcu_report_exp_cpu_mult() with the extra locking and early exit.

I am not claiming that current compilers would mess this up, though I
have learned to have great respect for what future compilers might do...

> But then that racy interrupt can still happen before we clear exp_deferred_qs.
> In this case __this_cpu_cmpxchg() would have been more efficient.

Except that __this_cpu_cmpxchg() would have a possibility of failure,
and thus an extra branch not needed by WRITE_ONCE().  Or am I missing
your point here?

I should hasten to add that getting rid of ->exp_deferred_qs is quite
attractive!

							Thanx, Paul

> > >  	rcu_report_exp_cpu_mult(rdp->mynode, rdp->grpmask, true);
> > >  }
> > >  
> > > -- 
> > > 2.25.1
> > > 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
  2021-09-17 18:10       ` Paul E. McKenney
@ 2021-09-17 22:00         ` Frederic Weisbecker
  0 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2021-09-17 22:00 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: LKML, rcu, urezki, boqun.feng, Neeraj Upadhyay, joel

On Fri, Sep 17, 2021 at 11:10:24AM -0700, Paul E. McKenney wrote:
> On Thu, Sep 16, 2021 at 11:05:14PM +0200, Frederic Weisbecker wrote:
> > On Thu, Sep 16, 2021 at 09:43:40AM -0700, Paul E. McKenney wrote:
> > > On Thu, Sep 16, 2021 at 02:10:46PM +0200, Frederic Weisbecker wrote:
> > > > This variable is never written nor read remotely. Remove this confusion.
> > > > 
> > > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > > > ---
> > > >  kernel/rcu/tree_exp.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > > index f3947c49eee7..4266610b4587 100644
> > > > --- a/kernel/rcu/tree_exp.h
> > > > +++ b/kernel/rcu/tree_exp.h
> > > > @@ -255,7 +255,7 @@ static void rcu_report_exp_cpu_mult(struct rcu_node *rnp,
> > > >   */
> > > >  static void rcu_report_exp_rdp(struct rcu_data *rdp)
> > > >  {
> > > > -	WRITE_ONCE(rdp->exp_deferred_qs, false);
> > > > +	rdp->exp_deferred_qs = false;
> > > 
> > > Are you sure that this can never be invoked from an interrupt handler?
> > > And that rdp->exp_deferred_qs is never read from an interrupt handler?
> > > If either can happen, then the WRITE_ONCE() does play a role, right?
> > 
> > Well, the only effect I can imagine is that it can partly prevent from an
> > interrupt to report concurrently the quiescent state during the few
> > instructions before we mask interrupts and lock the node.
> > 
> > That's a micro performance benefit that avoid a second call to
> > rcu_report_exp_cpu_mult() with the extra locking and early exit.
> 
> I am not claiming that current compilers would mess this up, though I
> have learned to have great respect for what future compilers might do...

:)

> 
> > But then that racy interrupt can still happen before we clear exp_deferred_qs.
> > In this case __this_cpu_cmpxchg() would have been more efficient.
> 
> Except that __this_cpu_cmpxchg() would have a possibility of failure,
> and thus an extra branch not needed by WRITE_ONCE().  Or am I missing
> your point here?

Right, but an extra branch that could spare a call to rcu_report_exp_cpu_mult().

Anyway I don't mind the WRITE_ONCE(), but you know how ordering (whether
compiler or CPU) makes me anxious when undocumented or not self-explanatory,
(although arguably the latter can vary depending on the reviewer :)

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report
  2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
                   ` (4 preceding siblings ...)
  2021-09-16 16:40 ` [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Paul E. McKenney
@ 2021-09-28 18:13 ` Paul E. McKenney
  5 siblings, 0 replies; 11+ messages in thread
From: Paul E. McKenney @ 2021-09-28 18:13 UTC (permalink / raw)
  To: Frederic Weisbecker; +Cc: LKML, rcu, urezki, boqun.feng, Neeraj Upadhyay, joel

On Thu, Sep 16, 2021 at 02:10:44PM +0200, Frederic Weisbecker wrote:
> This eventually removes rcu_data.exp_deferred_qs to use
> rcu_data.cpu no_qs.b.exp instead.
> 
> For those like me who need a headlamp to walk there: https://ibb.co/3d06r0V
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> 	rcu/dev
> 
> HEAD: 7d9d8a0c6141f95cbac4367b12e755bfabb383ee

Applied and pushed with some additional marking and commit-log editing.
The marking is needed because rcu_qs() can be invoked with interrupts
enabled, which can result in a low-probability data race with the
expedited IPI handler rcu_exp_handler().  There was another access in
sync_sched_exp_online_cleanup() than needed marking, so I am queuing a
separate commit for that one.

I suspect that this problem predated your series.  This is a low
probability data race, so KCSAN might be hard-pressed to spot it.

Anyway, the result in much much nicer with the converged use of the
->cpu_no_qs.b.exp field, so thank you!!!

Might simplify a few of your future headlamp diagrams as well?  ;-)

						Thanx, Paul

> Thanks,
> 	Frederic
> ---
> 
> Frederic Weisbecker (4):
>       rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs()
>       rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs
>       rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp()
>       rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp
> 
> 
>  kernel/rcu/tree.h        |  1 -
>  kernel/rcu/tree_exp.h    |  6 +++---
>  kernel/rcu/tree_plugin.h | 24 +++++++++++++-----------
>  3 files changed, 16 insertions(+), 15 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-09-28 18:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 12:10 [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Frederic Weisbecker
2021-09-16 12:10 ` [PATCH 1/4] rcu: Ignore rdp.cpu_no_qs.b.exp on premptible RCU's rcu_qs() Frederic Weisbecker
2021-09-16 12:10 ` [PATCH 2/4] rcu: Remove useless WRITE_ONCE() on rcu_data.exp_deferred_qs Frederic Weisbecker
2021-09-16 16:43   ` Paul E. McKenney
2021-09-16 21:05     ` Frederic Weisbecker
2021-09-17 18:10       ` Paul E. McKenney
2021-09-17 22:00         ` Frederic Weisbecker
2021-09-16 12:10 ` [PATCH 3/4] rcu: Move rcu_data.cpu_no_qs.b.exp reset to rcu_export_exp_rdp() Frederic Weisbecker
2021-09-16 12:10 ` [PATCH 4/4] rcu: Remove rcu_data.exp_deferred_qs and convert to rcu_data.cpu no_qs.b.exp Frederic Weisbecker
2021-09-16 16:40 ` [PATCH 0/4] rcu: Unify a bit [non-]PREEMPT expedited quiescent state report Paul E. McKenney
2021-09-28 18:13 ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).