[PATCH 0/5 v2] rcu/exp updates

[PATCH 0/5 v2] rcu/exp updates

[Frederic Weisbecker]

Hi,

Changes in this version:

* [1/5] Explain why it's fine if a task unblocks between
  sync_exp_reset_tree() and __sync_rcu_exp_select_node_cpus(), per Paul's
  suggestion.

* [3/5] Add Paul's reviewed-by tag

* [4/5] and [5/5] are new patches after discussion.

Frederic Weisbecker (5):
  rcu/exp: Protect against early QS report
  rcu/exp: Remove confusing needless full barrier on task unblock
  rcu/exp: Remove needless CPU up quiescent state report
  rcu/exp: Warn on QS requested on dying CPU
  rcu/exp: Warn on CPU lagging for too long within hotplug IPI's
    blindspot

 kernel/rcu/tree.c        |  8 +++--
 kernel/rcu/tree_exp.h    | 69 +++++++++++-----------------------------
 kernel/rcu/tree_plugin.h |  1 -
 3 files changed, 25 insertions(+), 53 deletions(-)

-- 
2.48.1

[PATCH 1/5] rcu/exp: Protect against early QS report

[Frederic Weisbecker]

When a grace period is started, the ->expmask of each node is set up
from sync_exp_reset_tree(). Then later on each leaf node also initialize
its ->exp_tasks pointer.

This means that the initialization of the quiescent state of a node and
the initialization of its blocking tasks happen with an unlocked node
gap in-between.

It happens to be fine because nothing is expected to report an exp
quiescent state within this gap, since no IPI have been issued yet and
every rdp's ->cpu_no_qs.b.exp should be false.

However if it were to happen by accident, the quiescent state could be
reported and propagated while ignoring tasks that blocked _before_ the
start of the grace period.

Prevent such trouble to happen in the future and initialize both the
quiescent states mask to report and the blocked tasks head from the same
node locked block.

If a task blocks within an RCU read side critical section before
sync_exp_reset_tree() is called and is then unblocked between
sync_exp_reset_tree() and __sync_rcu_exp_select_node_cpus(), the QS
won't be reported because no RCU exp IPI had been issued to request it
through the setting of srdp->cpu_no_qs.b.exp.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree_exp.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index c36c7d5575ca..2fa7aa9155bd 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -141,6 +141,13 @@ static void __maybe_unused sync_exp_reset_tree(void)
 		raw_spin_lock_irqsave_rcu_node(rnp, flags);
 		WARN_ON_ONCE(rnp->expmask);
 		WRITE_ONCE(rnp->expmask, rnp->expmaskinit);
+		/*
+		 * Need to wait for any blocked tasks as well.	Note that
+		 * additional blocking tasks will also block the expedited GP
+		 * until such time as the ->expmask bits are cleared.
+		 */
+		if (rcu_is_leaf_node(rnp) && rcu_preempt_has_tasks(rnp))
+			WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
 		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 	}
 }
@@ -393,13 +400,6 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
 	}
 	mask_ofl_ipi = rnp->expmask & ~mask_ofl_test;
 
-	/*
-	 * Need to wait for any blocked tasks as well.	Note that
-	 * additional blocking tasks will also block the expedited GP
-	 * until such time as the ->expmask bits are cleared.
-	 */
-	if (rcu_preempt_has_tasks(rnp))
-		WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 
 	/* IPI the remaining CPUs for expedited quiescent state. */
-- 
2.48.1

Re: [PATCH 1/5] rcu/exp: Protect against early QS report

[Joel Fernandes]

Hi Frederic,

On Fri, Mar 14, 2025 at 03:36:38PM +0100, Frederic Weisbecker wrote:
> When a grace period is started, the ->expmask of each node is set up
> from sync_exp_reset_tree(). Then later on each leaf node also initialize
> its ->exp_tasks pointer.
> 
> This means that the initialization of the quiescent state of a node and
> the initialization of its blocking tasks happen with an unlocked node
> gap in-between.
> 
> It happens to be fine because nothing is expected to report an exp
> quiescent state within this gap, since no IPI have been issued yet and
> every rdp's ->cpu_no_qs.b.exp should be false.
> 
> However if it were to happen by accident, the quiescent state could be
> reported and propagated while ignoring tasks that blocked _before_ the
> start of the grace period.
> 
> Prevent such trouble to happen in the future and initialize both the
> quiescent states mask to report and the blocked tasks head from the same
> node locked block.
> 
> If a task blocks within an RCU read side critical section before
> sync_exp_reset_tree() is called and is then unblocked between
> sync_exp_reset_tree() and __sync_rcu_exp_select_node_cpus(), the QS
> won't be reported because no RCU exp IPI had been issued to request it
> through the setting of srdp->cpu_no_qs.b.exp.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>  kernel/rcu/tree_exp.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index c36c7d5575ca..2fa7aa9155bd 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -141,6 +141,13 @@ static void __maybe_unused sync_exp_reset_tree(void)
>  		raw_spin_lock_irqsave_rcu_node(rnp, flags);
>  		WARN_ON_ONCE(rnp->expmask);
>  		WRITE_ONCE(rnp->expmask, rnp->expmaskinit);
> +		/*
> +		 * Need to wait for any blocked tasks as well.	Note that
> +		 * additional blocking tasks will also block the expedited GP
> +		 * until such time as the ->expmask bits are cleared.
> +		 */
> +		if (rcu_is_leaf_node(rnp) && rcu_preempt_has_tasks(rnp))
> +			WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
>  		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  	}
>  }
> @@ -393,13 +400,6 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
>  	}
>  	mask_ofl_ipi = rnp->expmask & ~mask_ofl_test;
>  
> -	/*
> -	 * Need to wait for any blocked tasks as well.	Note that
> -	 * additional blocking tasks will also block the expedited GP
> -	 * until such time as the ->expmask bits are cleared.
> -	 */
> -	if (rcu_preempt_has_tasks(rnp))
> -		WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);

A small side effect of this patch could be:

In the existing code, if between the sync_exp_reset_tree() and the
__sync_rcu_exp_select_node_cpus(), if a pre-existing reader unblocked and
completed, then I think it wouldn't be responsible for blocking the GP
anymore.

Where as with this patch, it would not get a chance to be removed from the
blocked list because it would have to wait on the rnp lock, which after this
patch would now be held across the setting of exp_mask and exp_tasks?

But I think it is not a big deal either way, and if you feel it is more
future proof to do it this way, that sounds good to me.

thanks,

 - Joel


>  
>  	/* IPI the remaining CPUs for expedited quiescent state. */
> -- 
> 2.48.1
> 

Re: [PATCH 1/5] rcu/exp: Protect against early QS report

[Frederic Weisbecker]

Le Sat, Mar 15, 2025 at 07:59:45PM -0400, Joel Fernandes a écrit :
> Hi Frederic,
> 
> On Fri, Mar 14, 2025 at 03:36:38PM +0100, Frederic Weisbecker wrote:
> > When a grace period is started, the ->expmask of each node is set up
> > from sync_exp_reset_tree(). Then later on each leaf node also initialize
> > its ->exp_tasks pointer.
> > 
> > This means that the initialization of the quiescent state of a node and
> > the initialization of its blocking tasks happen with an unlocked node
> > gap in-between.
> > 
> > It happens to be fine because nothing is expected to report an exp
> > quiescent state within this gap, since no IPI have been issued yet and
> > every rdp's ->cpu_no_qs.b.exp should be false.
> > 
> > However if it were to happen by accident, the quiescent state could be
> > reported and propagated while ignoring tasks that blocked _before_ the
> > start of the grace period.
> > 
> > Prevent such trouble to happen in the future and initialize both the
> > quiescent states mask to report and the blocked tasks head from the same
> > node locked block.
> > 
> > If a task blocks within an RCU read side critical section before
> > sync_exp_reset_tree() is called and is then unblocked between
> > sync_exp_reset_tree() and __sync_rcu_exp_select_node_cpus(), the QS
> > won't be reported because no RCU exp IPI had been issued to request it
> > through the setting of srdp->cpu_no_qs.b.exp.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > ---
> >  kernel/rcu/tree_exp.h | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index c36c7d5575ca..2fa7aa9155bd 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -141,6 +141,13 @@ static void __maybe_unused sync_exp_reset_tree(void)
> >  		raw_spin_lock_irqsave_rcu_node(rnp, flags);
> >  		WARN_ON_ONCE(rnp->expmask);
> >  		WRITE_ONCE(rnp->expmask, rnp->expmaskinit);
> > +		/*
> > +		 * Need to wait for any blocked tasks as well.	Note that
> > +		 * additional blocking tasks will also block the expedited GP
> > +		 * until such time as the ->expmask bits are cleared.
> > +		 */
> > +		if (rcu_is_leaf_node(rnp) && rcu_preempt_has_tasks(rnp))
> > +			WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
> >  		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> >  	}
> >  }
> > @@ -393,13 +400,6 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> >  	}
> >  	mask_ofl_ipi = rnp->expmask & ~mask_ofl_test;
> >  
> > -	/*
> > -	 * Need to wait for any blocked tasks as well.	Note that
> > -	 * additional blocking tasks will also block the expedited GP
> > -	 * until such time as the ->expmask bits are cleared.
> > -	 */
> > -	if (rcu_preempt_has_tasks(rnp))
> > -		WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
> >  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> 
> A small side effect of this patch could be:
> 
> In the existing code, if between the sync_exp_reset_tree() and the
> __sync_rcu_exp_select_node_cpus(), if a pre-existing reader unblocked and
> completed, then I think it wouldn't be responsible for blocking the GP
> anymore.

Hmm, I don't see how that changes after this patch.

> 
> Where as with this patch, it would not get a chance to be removed from the
> blocked list because it would have to wait on the rnp lock, which after this
> patch would now be held across the setting of exp_mask and exp_tasks?

So that's sync_exp_reset_tree(). I'm a bit confused. An unblocking task
contend on rnp lock in any case. But after this patch it is still going
to remove itself from the blocking task once the rnp lock is released by
sync_exp_reset_tree().

What am I missing?

Thanks.

> 
> But I think it is not a big deal either way, and if you feel it is more
> future proof to do it this way, that sounds good to me.
> 
> thanks,
> 
>  - Joel
> 
> 
> >  
> >  	/* IPI the remaining CPUs for expedited quiescent state. */
> > -- 
> > 2.48.1
> > 
> 

Re: [PATCH 1/5] rcu/exp: Protect against early QS report

[Joel Fernandes]

On 3/16/2025 7:07 AM, Frederic Weisbecker wrote:
>>> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
>>> index c36c7d5575ca..2fa7aa9155bd 100644
>>> --- a/kernel/rcu/tree_exp.h
>>> +++ b/kernel/rcu/tree_exp.h
>>> @@ -141,6 +141,13 @@ static void __maybe_unused sync_exp_reset_tree(void)
>>>  		raw_spin_lock_irqsave_rcu_node(rnp, flags);
>>>  		WARN_ON_ONCE(rnp->expmask);
>>>  		WRITE_ONCE(rnp->expmask, rnp->expmaskinit);
>>> +		/*
>>> +		 * Need to wait for any blocked tasks as well.	Note that
>>> +		 * additional blocking tasks will also block the expedited GP
>>> +		 * until such time as the ->expmask bits are cleared.
>>> +		 */
>>> +		if (rcu_is_leaf_node(rnp) && rcu_preempt_has_tasks(rnp))
>>> +			WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
>>>  		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>>>  	}
>>>  }
>>> @@ -393,13 +400,6 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
>>>  	}
>>>  	mask_ofl_ipi = rnp->expmask & ~mask_ofl_test;
>>>  
>>> -	/*
>>> -	 * Need to wait for any blocked tasks as well.	Note that
>>> -	 * additional blocking tasks will also block the expedited GP
>>> -	 * until such time as the ->expmask bits are cleared.
>>> -	 */
>>> -	if (rcu_preempt_has_tasks(rnp))
>>> -		WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
>>>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>> A small side effect of this patch could be:
>>
>> In the existing code, if between the sync_exp_reset_tree() and the
>> __sync_rcu_exp_select_node_cpus(), if a pre-existing reader unblocked and
>> completed, then I think it wouldn't be responsible for blocking the GP
>> anymore.
> Hmm, I don't see how that changes after this patch.
> 
>> Where as with this patch, it would not get a chance to be removed from the
>> blocked list because it would have to wait on the rnp lock, which after this
>> patch would now be held across the setting of exp_mask and exp_tasks?
> So that's sync_exp_reset_tree(). I'm a bit confused. An unblocking task
> contend on rnp lock in any case. But after this patch it is still going
> to remove itself from the blocking task once the rnp lock is released by
> sync_exp_reset_tree().
> 
> What am I missing?
You are probably not missing anything and I'm the one missing something.

But I was thinking:

In in the original code, in __sync_rcu_exp_select_node_cpus() if
rcu_preempt_has_tasks() returns FALSE because of the finer grained locking, then
there is a chance for the GP to conclude sooner,

On the other hand, after the patch because the unblocking task had to wait (on
the lock) to remove itself from the blocked task list, the GP may conclude later
than usual. This is just an intuitive guess.

Because this is an expedited GP, my intuition is to unblock + reader unlock and
get out of the way ASAP than hoping that it will get access to the lock before
any IPIs go out or quiescent state reports/checks happen which are required to
conclude the GP

Its just a theory and you're right, if it acquires the lock soon enough and gets
out of the way, then it doesn't matter either way.

Thanks!

 - Joel

Re: [PATCH 1/5] rcu/exp: Protect against early QS report

[Frederic Weisbecker]

Le Sun, Mar 16, 2025 at 10:23:45AM -0400, Joel Fernandes a écrit :
> >> A small side effect of this patch could be:
> >>
> >> In the existing code, if between the sync_exp_reset_tree() and the
> >> __sync_rcu_exp_select_node_cpus(), if a pre-existing reader unblocked and
> >> completed, then I think it wouldn't be responsible for blocking the GP
> >> anymore.
> > Hmm, I don't see how that changes after this patch.
> > 
> >> Where as with this patch, it would not get a chance to be removed from the
> >> blocked list because it would have to wait on the rnp lock, which after this
> >> patch would now be held across the setting of exp_mask and exp_tasks?
> > So that's sync_exp_reset_tree(). I'm a bit confused. An unblocking task
> > contend on rnp lock in any case. But after this patch it is still going
> > to remove itself from the blocking task once the rnp lock is released by
> > sync_exp_reset_tree().
> > 
> > What am I missing?
> You are probably not missing anything and I'm the one missing something.
> 
> But I was thinking:
> 
> In in the original code, in __sync_rcu_exp_select_node_cpus() if
> rcu_preempt_has_tasks() returns FALSE because of the finer grained locking, then
> there is a chance for the GP to conclude sooner,

Why do you think it's finer grained locking?
 
> On the other hand, after the patch because the unblocking task had to wait (on
> the lock) to remove itself from the blocked task list, the GP may conclude later
> than usual. This is just an intuitive guess.
> 
> Because this is an expedited GP, my intuition is to unblock + reader unlock and
> get out of the way ASAP than hoping that it will get access to the lock before
> any IPIs go out or quiescent state reports/checks happen which are required to
> conclude the GP
> 
> Its just a theory and you're right, if it acquires the lock soon enough and gets
> out of the way, then it doesn't matter either way.

I think I understand where the confusion is. A task that is preempted within an
RCU read side section _always_ adds itself to the rnp's list of blocked tasks
(&rnp->blkd_tasks). The only thing that changes with expedited GPs is that
rnp->exp_tasks may or may not be updated on the way. But rnp->exp_tasks is only
a pointer to an arbitrary element within the rnp->blkd_tasks list.

This means that an unblocking task must always delete itself from
rnp->blkd_tasks, and possibly update rnp->exp_tasks along the way.

Both the add and the delete happen with rnp locked.

Therefore a task unblocking before __sync_rcu_exp_select_node_cpus()
can make __sync_rcu_exp_select_node_cpus() contend on rnp locking.

But this patch doesn't change the behaviour in this regard.

Thanks.


> 
> Thanks!
> 
>  - Joel
> 
> 
> 

Re: [PATCH 1/5] rcu/exp: Protect against early QS report

[Paul E. McKenney]

On Fri, Mar 14, 2025 at 03:36:38PM +0100, Frederic Weisbecker wrote:
> When a grace period is started, the ->expmask of each node is set up
> from sync_exp_reset_tree(). Then later on each leaf node also initialize
> its ->exp_tasks pointer.
> 
> This means that the initialization of the quiescent state of a node and
> the initialization of its blocking tasks happen with an unlocked node
> gap in-between.
> 
> It happens to be fine because nothing is expected to report an exp
> quiescent state within this gap, since no IPI have been issued yet and
> every rdp's ->cpu_no_qs.b.exp should be false.
> 
> However if it were to happen by accident, the quiescent state could be
> reported and propagated while ignoring tasks that blocked _before_ the
> start of the grace period.
> 
> Prevent such trouble to happen in the future and initialize both the
> quiescent states mask to report and the blocked tasks head from the same
> node locked block.
> 
> If a task blocks within an RCU read side critical section before
> sync_exp_reset_tree() is called and is then unblocked between
> sync_exp_reset_tree() and __sync_rcu_exp_select_node_cpus(), the QS
> won't be reported because no RCU exp IPI had been issued to request it
> through the setting of srdp->cpu_no_qs.b.exp.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>

OK, and because ->expmaskinit has all bits set for all CPUs that have
ever been online, the ends of any corresponding readers will give up at
the beginning of the first pass of the loop in __rcu_report_exp_rnp().
This is because the ->expmask is guaranteed to be non-zero.  (Which is
kind of what you are saying in the last paragraph of your commit log,
just digging down another layer.)

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/rcu/tree_exp.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index c36c7d5575ca..2fa7aa9155bd 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -141,6 +141,13 @@ static void __maybe_unused sync_exp_reset_tree(void)
>  		raw_spin_lock_irqsave_rcu_node(rnp, flags);
>  		WARN_ON_ONCE(rnp->expmask);
>  		WRITE_ONCE(rnp->expmask, rnp->expmaskinit);
> +		/*
> +		 * Need to wait for any blocked tasks as well.	Note that
> +		 * additional blocking tasks will also block the expedited GP
> +		 * until such time as the ->expmask bits are cleared.
> +		 */
> +		if (rcu_is_leaf_node(rnp) && rcu_preempt_has_tasks(rnp))
> +			WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
>  		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  	}
>  }
> @@ -393,13 +400,6 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
>  	}
>  	mask_ofl_ipi = rnp->expmask & ~mask_ofl_test;
>  
> -	/*
> -	 * Need to wait for any blocked tasks as well.	Note that
> -	 * additional blocking tasks will also block the expedited GP
> -	 * until such time as the ->expmask bits are cleared.
> -	 */
> -	if (rcu_preempt_has_tasks(rnp))
> -		WRITE_ONCE(rnp->exp_tasks, rnp->blkd_tasks.next);
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  
>  	/* IPI the remaining CPUs for expedited quiescent state. */
> -- 
> 2.48.1
> 

Re: [PATCH 1/5] rcu/exp: Protect against early QS report

[Frederic Weisbecker]

Le Tue, Mar 18, 2025 at 10:17:16AM -0700, Paul E. McKenney a écrit :
> On Fri, Mar 14, 2025 at 03:36:38PM +0100, Frederic Weisbecker wrote:
> > When a grace period is started, the ->expmask of each node is set up
> > from sync_exp_reset_tree(). Then later on each leaf node also initialize
> > its ->exp_tasks pointer.
> > 
> > This means that the initialization of the quiescent state of a node and
> > the initialization of its blocking tasks happen with an unlocked node
> > gap in-between.
> > 
> > It happens to be fine because nothing is expected to report an exp
> > quiescent state within this gap, since no IPI have been issued yet and
> > every rdp's ->cpu_no_qs.b.exp should be false.
> > 
> > However if it were to happen by accident, the quiescent state could be
> > reported and propagated while ignoring tasks that blocked _before_ the
> > start of the grace period.
> > 
> > Prevent such trouble to happen in the future and initialize both the
> > quiescent states mask to report and the blocked tasks head from the same
> > node locked block.
> > 
> > If a task blocks within an RCU read side critical section before
> > sync_exp_reset_tree() is called and is then unblocked between
> > sync_exp_reset_tree() and __sync_rcu_exp_select_node_cpus(), the QS
> > won't be reported because no RCU exp IPI had been issued to request it
> > through the setting of srdp->cpu_no_qs.b.exp.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> 
> OK, and because ->expmaskinit has all bits set for all CPUs that have
> ever been online, the ends of any corresponding readers will give up at
> the beginning of the first pass of the loop in __rcu_report_exp_rnp().
> This is because the ->expmask is guaranteed to be non-zero.  (Which is
> kind of what you are saying in the last paragraph of your commit log,
> just digging down another layer.)

Exactly!

Thanks.

[PATCH 2/5] rcu/exp: Remove confusing needless full barrier on task unblock

[Frederic Weisbecker]

A full memory barrier in the RCU-PREEMPT task unblock path advertizes
to order the context switch (or rather the accesses prior to
rcu_read_unlock()) with the expedited grace period fastpath.

However the grace period can not complete without the rnp calling into
rcu_report_exp_rnp() with the node locked. This reports the quiescent
state in a fully ordered fashion against updater's accesses thanks to:

1) The READ-SIDE smp_mb__after_unlock_lock() barrier accross nodes
   locking while propagating QS up to the root.

2) The UPDATE-SIDE smp_mb__after_unlock_lock() barrier while holding the
   the root rnp to wait/check for the GP completion.

3) The (perhaps redundant given step 1) and 2)) smp_mb() in rcu_seq_end()
   before the grace period completes.

This makes the explicit barrier in this place superflous. Therefore
remove it as it is confusing.

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

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 3c0bbbbb686f..d51cc7a5dfc7 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -534,7 +534,6 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
 		WARN_ON_ONCE(rnp->completedqs == rnp->gp_seq &&
 			     (!empty_norm || rnp->qsmask));
 		empty_exp = sync_rcu_exp_done(rnp);
-		smp_mb(); /* ensure expedited fastpath sees end of RCU c-s. */
 		np = rcu_next_node_entry(t, rnp);
 		list_del_init(&t->rcu_node_entry);
 		t->rcu_blocked_node = NULL;
-- 
2.48.1

Re: [PATCH 2/5] rcu/exp: Remove confusing needless full barrier on task unblock

[Paul E. McKenney]

On Fri, Mar 14, 2025 at 03:36:39PM +0100, Frederic Weisbecker wrote:
> A full memory barrier in the RCU-PREEMPT task unblock path advertizes
> to order the context switch (or rather the accesses prior to
> rcu_read_unlock()) with the expedited grace period fastpath.
> 
> However the grace period can not complete without the rnp calling into
> rcu_report_exp_rnp() with the node locked. This reports the quiescent
> state in a fully ordered fashion against updater's accesses thanks to:
> 
> 1) The READ-SIDE smp_mb__after_unlock_lock() barrier accross nodes
>    locking while propagating QS up to the root.
> 
> 2) The UPDATE-SIDE smp_mb__after_unlock_lock() barrier while holding the
>    the root rnp to wait/check for the GP completion.
> 
> 3) The (perhaps redundant given step 1) and 2)) smp_mb() in rcu_seq_end()
>    before the grace period completes.
> 
> This makes the explicit barrier in this place superflous. Therefore
> remove it as it is confusing.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>

Still cannot see a problem with this, but still a bit nervous.

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/rcu/tree_plugin.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 3c0bbbbb686f..d51cc7a5dfc7 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -534,7 +534,6 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>  		WARN_ON_ONCE(rnp->completedqs == rnp->gp_seq &&
>  			     (!empty_norm || rnp->qsmask));
>  		empty_exp = sync_rcu_exp_done(rnp);
> -		smp_mb(); /* ensure expedited fastpath sees end of RCU c-s. */
>  		np = rcu_next_node_entry(t, rnp);
>  		list_del_init(&t->rcu_node_entry);
>  		t->rcu_blocked_node = NULL;
> -- 
> 2.48.1
> 

Re: [PATCH 2/5] rcu/exp: Remove confusing needless full barrier on task unblock

[Frederic Weisbecker]

Le Tue, Mar 18, 2025 at 10:18:12AM -0700, Paul E. McKenney a écrit :
> On Fri, Mar 14, 2025 at 03:36:39PM +0100, Frederic Weisbecker wrote:
> > A full memory barrier in the RCU-PREEMPT task unblock path advertizes
> > to order the context switch (or rather the accesses prior to
> > rcu_read_unlock()) with the expedited grace period fastpath.
> > 
> > However the grace period can not complete without the rnp calling into
> > rcu_report_exp_rnp() with the node locked. This reports the quiescent
> > state in a fully ordered fashion against updater's accesses thanks to:
> > 
> > 1) The READ-SIDE smp_mb__after_unlock_lock() barrier accross nodes
> >    locking while propagating QS up to the root.
> > 
> > 2) The UPDATE-SIDE smp_mb__after_unlock_lock() barrier while holding the
> >    the root rnp to wait/check for the GP completion.
> > 
> > 3) The (perhaps redundant given step 1) and 2)) smp_mb() in rcu_seq_end()
> >    before the grace period completes.
> > 
> > This makes the explicit barrier in this place superflous. Therefore
> > remove it as it is confusing.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> 
> Still cannot see a problem with this, but still a bit nervous.

Where is the challenge in life if we manage to fall alseep within a minute
at bedtime?

> 
> Acked-by: Paul E. McKenney <paulmck@kernel.org>

Thanks!

Re: [PATCH 2/5] rcu/exp: Remove confusing needless full barrier on task unblock

[Paul E. McKenney]

On Wed, Mar 19, 2025 at 10:01:36AM +0100, Frederic Weisbecker wrote:
> Le Tue, Mar 18, 2025 at 10:18:12AM -0700, Paul E. McKenney a écrit :
> > On Fri, Mar 14, 2025 at 03:36:39PM +0100, Frederic Weisbecker wrote:
> > > A full memory barrier in the RCU-PREEMPT task unblock path advertizes
> > > to order the context switch (or rather the accesses prior to
> > > rcu_read_unlock()) with the expedited grace period fastpath.
> > > 
> > > However the grace period can not complete without the rnp calling into
> > > rcu_report_exp_rnp() with the node locked. This reports the quiescent
> > > state in a fully ordered fashion against updater's accesses thanks to:
> > > 
> > > 1) The READ-SIDE smp_mb__after_unlock_lock() barrier accross nodes
> > >    locking while propagating QS up to the root.
> > > 
> > > 2) The UPDATE-SIDE smp_mb__after_unlock_lock() barrier while holding the
> > >    the root rnp to wait/check for the GP completion.
> > > 
> > > 3) The (perhaps redundant given step 1) and 2)) smp_mb() in rcu_seq_end()
> > >    before the grace period completes.
> > > 
> > > This makes the explicit barrier in this place superflous. Therefore
> > > remove it as it is confusing.
> > > 
> > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > 
> > Still cannot see a problem with this, but still a bit nervous.
> 
> Where is the challenge in life if we manage to fall alseep within a minute
> at bedtime?

;-) ;-) ;-)

Suppose that there was an issue with this that we are somehow not spotting.
How would you go about debugging it?

							Thanx, Paul

> > Acked-by: Paul E. McKenney <paulmck@kernel.org>
> 
> Thanks!

[PATCH 3/5] rcu/exp: Remove needless CPU up quiescent state report

[Frederic Weisbecker]

A CPU coming online checks for an ongoing grace period and reports
a quiescent state accordingly if needed. This special treatment that
shortcuts the expedited IPI finds its origin as an optimization purpose
on the following commit:

	338b0f760e84 (rcu: Better hotplug handling for synchronize_sched_expedited()

The point is to avoid an IPI while waiting for a CPU to become online
or failing to become offline.

However this is pointless and even error prone for several reasons:

* If the CPU has been seen offline in the first round scanning offline
  and idle CPUs, no IPI is even tried and the quiescent state is
  reported on behalf of the CPU.

* This means that if the IPI fails, the CPU just became offline. So
  it's unlikely to become online right away, unless the cpu hotplug
  operation failed and rolled back, which is a rare event that can
  wait a jiffy for a new IPI to be issued.

* But then the "optimization" applying on failing CPU hotplug down only
  applies to !PREEMPT_RCU.

* This force reports a quiescent state even if ->cpu_no_qs.b.exp is not
  set. As a result it can race with remote QS reports on the same rdp.
  Fortunately it happens to be OK but an accident is waiting to happen.

For all those reasons, remove this optimization that doesn't look worthy
to keep around.

Reported-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree.c     |  2 --
 kernel/rcu/tree_exp.h | 45 ++-----------------------------------------
 2 files changed, 2 insertions(+), 45 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d7f3800c7c8e..3fe68057d8b4 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -151,7 +151,6 @@ static void rcu_report_qs_rnp(unsigned long mask, struct rcu_node *rnp,
 			      unsigned long gps, unsigned long flags);
 static void invoke_rcu_core(void);
 static void rcu_report_exp_rdp(struct rcu_data *rdp);
-static void sync_sched_exp_online_cleanup(int cpu);
 static void check_cb_ovld_locked(struct rcu_data *rdp, struct rcu_node *rnp);
 static bool rcu_rdp_is_offloaded(struct rcu_data *rdp);
 static bool rcu_rdp_cpu_online(struct rcu_data *rdp);
@@ -4233,7 +4232,6 @@ int rcutree_online_cpu(unsigned int cpu)
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 	if (rcu_scheduler_active == RCU_SCHEDULER_INACTIVE)
 		return 0; /* Too early in boot for scheduler work. */
-	sync_sched_exp_online_cleanup(cpu);
 
 	// Stop-machine done, so allow nohz_full to disable tick.
 	tick_dep_clear(TICK_DEP_BIT_RCU);
diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 2fa7aa9155bd..6058a734090c 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -751,12 +751,8 @@ static void rcu_exp_handler(void *unused)
 	struct task_struct *t = current;
 
 	/*
-	 * First, is there no need for a quiescent state from this CPU,
-	 * or is this CPU already looking for a quiescent state for the
-	 * current grace period?  If either is the case, just leave.
-	 * However, this should not happen due to the preemptible
-	 * sync_sched_exp_online_cleanup() implementation being a no-op,
-	 * so warn if this does happen.
+	 * WARN if the CPU is unexpectedly already looking for a
+	 * QS or has already reported one.
 	 */
 	ASSERT_EXCLUSIVE_WRITER_SCOPED(rdp->cpu_no_qs.b.exp);
 	if (WARN_ON_ONCE(!(READ_ONCE(rnp->expmask) & rdp->grpmask) ||
@@ -803,11 +799,6 @@ static void rcu_exp_handler(void *unused)
 	WARN_ON_ONCE(1);
 }
 
-/* PREEMPTION=y, so no PREEMPTION=n expedited grace period to clean up after. */
-static void sync_sched_exp_online_cleanup(int cpu)
-{
-}
-
 /*
  * Scan the current list of tasks blocked within RCU read-side critical
  * sections, printing out the tid of each that is blocking the current
@@ -885,38 +876,6 @@ static void rcu_exp_handler(void *unused)
 	rcu_exp_need_qs();
 }
 
-/* Send IPI for expedited cleanup if needed at end of CPU-hotplug operation. */
-static void sync_sched_exp_online_cleanup(int cpu)
-{
-	unsigned long flags;
-	int my_cpu;
-	struct rcu_data *rdp;
-	int ret;
-	struct rcu_node *rnp;
-
-	rdp = per_cpu_ptr(&rcu_data, cpu);
-	rnp = rdp->mynode;
-	my_cpu = get_cpu();
-	/* Quiescent state either not needed or already requested, leave. */
-	if (!(READ_ONCE(rnp->expmask) & rdp->grpmask) ||
-	    READ_ONCE(rdp->cpu_no_qs.b.exp)) {
-		put_cpu();
-		return;
-	}
-	/* Quiescent state needed on current CPU, so set it up locally. */
-	if (my_cpu == cpu) {
-		local_irq_save(flags);
-		rcu_exp_need_qs();
-		local_irq_restore(flags);
-		put_cpu();
-		return;
-	}
-	/* Quiescent state needed on some other CPU, send IPI. */
-	ret = smp_call_function_single(cpu, rcu_exp_handler, NULL, 0);
-	put_cpu();
-	WARN_ON_ONCE(ret);
-}
-
 /*
  * Because preemptible RCU does not exist, we never have to check for
  * tasks blocked within RCU read-side critical sections that are
-- 
2.48.1

[PATCH 4/5] rcu/exp: Warn on QS requested on dying CPU

[Frederic Weisbecker]

It is not possible to send an IPI to a dying CPU that has passed the
CPUHP_TEARDOWN_CPU stage. Remaining unhandled IPIs are handled later at
CPUHP_AP_SMPCFD_DYING stage by stop machine. This is the last
opportunity for RCU exp handler to request an expedited quiescent state.
And the upcoming final context switch between stop machine and idle must
have reported the requested context switch.

Therefore, it should not be possible to observe a pending requested
expedited quiescent state when RCU finally stops watching the outgoing
CPU. Once IPIs aren't possible anymore, the QS for the target CPU will
be reported on its behalf by the RCU exp kworker.

Provide an assertion to verify those expectations.

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 3fe68057d8b4..79dced5fb72e 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4321,6 +4321,12 @@ void rcutree_report_cpu_dead(void)
 	 * may introduce a new READ-side while it is actually off the QS masks.
 	 */
 	lockdep_assert_irqs_disabled();
+	/*
+	 * CPUHP_AP_SMPCFD_DYING was the last call for rcu_exp_handler() execution.
+	 * The requested QS must have been reported on the last context switch
+	 * from stop machine to idle.
+	 */
+	WARN_ON_ONCE(rdp->cpu_no_qs.b.exp);
 	// Do any dangling deferred wakeups.
 	do_nocb_deferred_wakeup(rdp);
 
-- 
2.48.1

Re: [PATCH 4/5] rcu/exp: Warn on QS requested on dying CPU

[Paul E. McKenney]

On Fri, Mar 14, 2025 at 03:36:41PM +0100, Frederic Weisbecker wrote:
> It is not possible to send an IPI to a dying CPU that has passed the
> CPUHP_TEARDOWN_CPU stage. Remaining unhandled IPIs are handled later at
> CPUHP_AP_SMPCFD_DYING stage by stop machine. This is the last
> opportunity for RCU exp handler to request an expedited quiescent state.
> And the upcoming final context switch between stop machine and idle must
> have reported the requested context switch.
> 
> Therefore, it should not be possible to observe a pending requested
> expedited quiescent state when RCU finally stops watching the outgoing
> CPU. Once IPIs aren't possible anymore, the QS for the target CPU will
> be reported on its behalf by the RCU exp kworker.
> 
> Provide an assertion to verify those expectations.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>

But what do we do if this assertion triggers?  And do we want it to take
effect only in kernels built with CONFIG_PROVE_RCU?  Or is such a broken
assumption bad enough to justify a splat in production kernels?

If the answer to the last question is "yes" (and you, not me, work for
a distro, so it is your question to answer):

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 3fe68057d8b4..79dced5fb72e 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4321,6 +4321,12 @@ void rcutree_report_cpu_dead(void)
>  	 * may introduce a new READ-side while it is actually off the QS masks.
>  	 */
>  	lockdep_assert_irqs_disabled();
> +	/*
> +	 * CPUHP_AP_SMPCFD_DYING was the last call for rcu_exp_handler() execution.
> +	 * The requested QS must have been reported on the last context switch
> +	 * from stop machine to idle.
> +	 */
> +	WARN_ON_ONCE(rdp->cpu_no_qs.b.exp);
>  	// Do any dangling deferred wakeups.
>  	do_nocb_deferred_wakeup(rdp);
>  
> -- 
> 2.48.1
> 

Re: [PATCH 4/5] rcu/exp: Warn on QS requested on dying CPU

[Frederic Weisbecker]

Le Tue, Mar 18, 2025 at 10:21:48AM -0700, Paul E. McKenney a écrit :
> On Fri, Mar 14, 2025 at 03:36:41PM +0100, Frederic Weisbecker wrote:
> > It is not possible to send an IPI to a dying CPU that has passed the
> > CPUHP_TEARDOWN_CPU stage. Remaining unhandled IPIs are handled later at
> > CPUHP_AP_SMPCFD_DYING stage by stop machine. This is the last
> > opportunity for RCU exp handler to request an expedited quiescent state.
> > And the upcoming final context switch between stop machine and idle must
> > have reported the requested context switch.
> > 
> > Therefore, it should not be possible to observe a pending requested
> > expedited quiescent state when RCU finally stops watching the outgoing
> > CPU. Once IPIs aren't possible anymore, the QS for the target CPU will
> > be reported on its behalf by the RCU exp kworker.
> > 
> > Provide an assertion to verify those expectations.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> 
> But what do we do if this assertion triggers?

It means there is likely something to fix because an IPI has been sent
and somehow the CPU missed it.

> And do we want it to take
> effect only in kernels built with CONFIG_PROVE_RCU?  Or is such a broken
> assumption bad enough to justify a splat in production kernels?
> 
> If the answer to the last question is "yes" (and you, not me, work for
> a distro, so it is your question to answer):

I think it's bad enough to deserve a real warning. Also this is a slow path.

> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

Thanks!

> 
> 							Thanx, Paul
> 
> > ---
> >  kernel/rcu/tree.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 3fe68057d8b4..79dced5fb72e 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -4321,6 +4321,12 @@ void rcutree_report_cpu_dead(void)
> >  	 * may introduce a new READ-side while it is actually off the QS masks.
> >  	 */
> >  	lockdep_assert_irqs_disabled();
> > +	/*
> > +	 * CPUHP_AP_SMPCFD_DYING was the last call for rcu_exp_handler() execution.
> > +	 * The requested QS must have been reported on the last context switch
> > +	 * from stop machine to idle.
> > +	 */
> > +	WARN_ON_ONCE(rdp->cpu_no_qs.b.exp);
> >  	// Do any dangling deferred wakeups.
> >  	do_nocb_deferred_wakeup(rdp);
> >  
> > -- 
> > 2.48.1
> > 

[PATCH 5/5] rcu/exp: Warn on CPU lagging for too long within hotplug IPI's blindspot

[Frederic Weisbecker]

A CPU within hotplug operations can make the RCU exp kworker lagging if:

* The dying CPU is running after CPUHP_TEARDOWN_CPU but before
  rcutree_report_cpu_dead(). It is too late to send an IPI but RCU is
  still watching the CPU. Therefore the exp kworker can only wait for
  the target to reach rcutree_report_cpu_dead().

* The booting CPU is running after rcutree_report_cpu_starting() but
  before set_cpu_online(). RCU is watching the CPU but it is too early
  to be able to send an IPI. Therefore the exp kworker can only wait
  until it observes the CPU as officially online.

Such a lag is expected to be very short. However #VMEXIT and other
hazards can stay on the way. Report long delays, 10 jiffies is
considered a high threshold already.

Reported-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree_exp.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 6058a734090c..87a44423927d 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -406,8 +406,18 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
 	for_each_leaf_node_cpu_mask(rnp, cpu, mask_ofl_ipi) {
 		struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
 		unsigned long mask = rdp->grpmask;
+		int nr_retries = 0;
 
 retry_ipi:
+		/*
+		 * In case of retrying, CPU either is lagging:
+		 *
+		 * - between CPUHP_TEARDOWN_CPU and rcutree_report_cpu_dead()
+		 * or:
+		 * - between rcutree_report_cpu_starting() and set_cpu_online()
+		 */
+		WARN_ON_ONCE(nr_retries++ > 10);
+
 		if (rcu_watching_snap_stopped_since(rdp, rdp->exp_watching_snap)) {
 			mask_ofl_test |= mask;
 			continue;
-- 
2.48.1

Re: [PATCH 5/5] rcu/exp: Warn on CPU lagging for too long within hotplug IPI's blindspot

[Paul E. McKenney]

On Fri, Mar 14, 2025 at 03:36:42PM +0100, Frederic Weisbecker wrote:
> A CPU within hotplug operations can make the RCU exp kworker lagging if:
> 
> * The dying CPU is running after CPUHP_TEARDOWN_CPU but before
>   rcutree_report_cpu_dead(). It is too late to send an IPI but RCU is
>   still watching the CPU. Therefore the exp kworker can only wait for
>   the target to reach rcutree_report_cpu_dead().
> 
> * The booting CPU is running after rcutree_report_cpu_starting() but
>   before set_cpu_online(). RCU is watching the CPU but it is too early
>   to be able to send an IPI. Therefore the exp kworker can only wait
>   until it observes the CPU as officially online.
> 
> Such a lag is expected to be very short. However #VMEXIT and other
> hazards can stay on the way. Report long delays, 10 jiffies is
> considered a high threshold already.
> 
> Reported-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>

Same CONFIG_PROVE_RCU question, same conditional:

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/rcu/tree_exp.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 6058a734090c..87a44423927d 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -406,8 +406,18 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
>  	for_each_leaf_node_cpu_mask(rnp, cpu, mask_ofl_ipi) {
>  		struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
>  		unsigned long mask = rdp->grpmask;
> +		int nr_retries = 0;
>  
>  retry_ipi:
> +		/*
> +		 * In case of retrying, CPU either is lagging:
> +		 *
> +		 * - between CPUHP_TEARDOWN_CPU and rcutree_report_cpu_dead()
> +		 * or:
> +		 * - between rcutree_report_cpu_starting() and set_cpu_online()
> +		 */
> +		WARN_ON_ONCE(nr_retries++ > 10);
> +
>  		if (rcu_watching_snap_stopped_since(rdp, rdp->exp_watching_snap)) {
>  			mask_ofl_test |= mask;
>  			continue;
> -- 
> 2.48.1
> 

Re: [PATCH 5/5] rcu/exp: Warn on CPU lagging for too long within hotplug IPI's blindspot

[Frederic Weisbecker]

Le Tue, Mar 18, 2025 at 10:22:33AM -0700, Paul E. McKenney a écrit :
> On Fri, Mar 14, 2025 at 03:36:42PM +0100, Frederic Weisbecker wrote:
> > A CPU within hotplug operations can make the RCU exp kworker lagging if:
> > 
> > * The dying CPU is running after CPUHP_TEARDOWN_CPU but before
> >   rcutree_report_cpu_dead(). It is too late to send an IPI but RCU is
> >   still watching the CPU. Therefore the exp kworker can only wait for
> >   the target to reach rcutree_report_cpu_dead().
> > 
> > * The booting CPU is running after rcutree_report_cpu_starting() but
> >   before set_cpu_online(). RCU is watching the CPU but it is too early
> >   to be able to send an IPI. Therefore the exp kworker can only wait
> >   until it observes the CPU as officially online.
> > 
> > Such a lag is expected to be very short. However #VMEXIT and other
> > hazards can stay on the way. Report long delays, 10 jiffies is
> > considered a high threshold already.
> > 
> > Reported-by: Paul E. McKenney <paulmck@kernel.org>
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> 
> Same CONFIG_PROVE_RCU question, same conditional:
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

I don't have a strong opinion whether to keep this warning unconditional.
Perhaps this can depend on CONFIG_PROVE_RCU.

Thanks.

> 
> > ---
> >  kernel/rcu/tree_exp.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index 6058a734090c..87a44423927d 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -406,8 +406,18 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> >  	for_each_leaf_node_cpu_mask(rnp, cpu, mask_ofl_ipi) {
> >  		struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> >  		unsigned long mask = rdp->grpmask;
> > +		int nr_retries = 0;
> >  
> >  retry_ipi:
> > +		/*
> > +		 * In case of retrying, CPU either is lagging:
> > +		 *
> > +		 * - between CPUHP_TEARDOWN_CPU and rcutree_report_cpu_dead()
> > +		 * or:
> > +		 * - between rcutree_report_cpu_starting() and set_cpu_online()
> > +		 */
> > +		WARN_ON_ONCE(nr_retries++ > 10);
> > +
> >  		if (rcu_watching_snap_stopped_since(rdp, rdp->exp_watching_snap)) {
> >  			mask_ofl_test |= mask;
> >  			continue;
> > -- 
> > 2.48.1
> > 

Re: [PATCH 5/5] rcu/exp: Warn on CPU lagging for too long within hotplug IPI's blindspot

[Paul E. McKenney]

On Wed, Mar 19, 2025 at 10:42:38AM +0100, Frederic Weisbecker wrote:
> Le Tue, Mar 18, 2025 at 10:22:33AM -0700, Paul E. McKenney a écrit :
> > On Fri, Mar 14, 2025 at 03:36:42PM +0100, Frederic Weisbecker wrote:
> > > A CPU within hotplug operations can make the RCU exp kworker lagging if:
> > > 
> > > * The dying CPU is running after CPUHP_TEARDOWN_CPU but before
> > >   rcutree_report_cpu_dead(). It is too late to send an IPI but RCU is
> > >   still watching the CPU. Therefore the exp kworker can only wait for
> > >   the target to reach rcutree_report_cpu_dead().
> > > 
> > > * The booting CPU is running after rcutree_report_cpu_starting() but
> > >   before set_cpu_online(). RCU is watching the CPU but it is too early
> > >   to be able to send an IPI. Therefore the exp kworker can only wait
> > >   until it observes the CPU as officially online.
> > > 
> > > Such a lag is expected to be very short. However #VMEXIT and other
> > > hazards can stay on the way. Report long delays, 10 jiffies is
> > > considered a high threshold already.
> > > 
> > > Reported-by: Paul E. McKenney <paulmck@kernel.org>
> > > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > 
> > Same CONFIG_PROVE_RCU question, same conditional:
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> 
> I don't have a strong opinion whether to keep this warning unconditional.
> Perhaps this can depend on CONFIG_PROVE_RCU.

You are the expert on that question, so your choice.  On this one,
I am but asking the questions.  ;-)

							Thanx, Paul

> Thanks.
> 
> > 
> > > ---
> > >  kernel/rcu/tree_exp.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > index 6058a734090c..87a44423927d 100644
> > > --- a/kernel/rcu/tree_exp.h
> > > +++ b/kernel/rcu/tree_exp.h
> > > @@ -406,8 +406,18 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp)
> > >  	for_each_leaf_node_cpu_mask(rnp, cpu, mask_ofl_ipi) {
> > >  		struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > >  		unsigned long mask = rdp->grpmask;
> > > +		int nr_retries = 0;
> > >  
> > >  retry_ipi:
> > > +		/*
> > > +		 * In case of retrying, CPU either is lagging:
> > > +		 *
> > > +		 * - between CPUHP_TEARDOWN_CPU and rcutree_report_cpu_dead()
> > > +		 * or:
> > > +		 * - between rcutree_report_cpu_starting() and set_cpu_online()
> > > +		 */
> > > +		WARN_ON_ONCE(nr_retries++ > 10);
> > > +
> > >  		if (rcu_watching_snap_stopped_since(rdp, rdp->exp_watching_snap)) {
> > >  			mask_ofl_test |= mask;
> > >  			continue;
> > > -- 
> > > 2.48.1
> > > 

[PATCH 0/5 v3] rcu/exp updates

[Frederic Weisbecker]

Hi,

No real updates in this set. Just collected a few tags.

Thanks.

Frederic Weisbecker (5):
  rcu/exp: Protect against early QS report
  rcu/exp: Remove confusing needless full barrier on task unblock
  rcu/exp: Remove needless CPU up quiescent state report
  rcu/exp: Warn on QS requested on dying CPU
  rcu/exp: Warn on CPU lagging for too long within hotplug IPI's
    blindspot

 kernel/rcu/tree.c        |  8 +++--
 kernel/rcu/tree_exp.h    | 69 +++++++++++-----------------------------
 kernel/rcu/tree_plugin.h |  1 -
 3 files changed, 25 insertions(+), 53 deletions(-)

-- 
2.48.1

[PATCH 2/5] rcu/exp: Remove confusing needless full barrier on task unblock

[Frederic Weisbecker]

A full memory barrier in the RCU-PREEMPT task unblock path advertizes
to order the context switch (or rather the accesses prior to
rcu_read_unlock()) with the expedited grace period fastpath.

However the grace period can not complete without the rnp calling into
rcu_report_exp_rnp() with the node locked. This reports the quiescent
state in a fully ordered fashion against updater's accesses thanks to:

1) The READ-SIDE smp_mb__after_unlock_lock() barrier accross nodes
   locking while propagating QS up to the root.

2) The UPDATE-SIDE smp_mb__after_unlock_lock() barrier while holding the
   the root rnp to wait/check for the GP completion.

3) The (perhaps redundant given step 1) and 2)) smp_mb() in rcu_seq_end()
   before the grace period completes.

This makes the explicit barrier in this place superflous. Therefore
remove it as it is confusing.

Acked-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/tree_plugin.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 3c0bbbbb686f..d51cc7a5dfc7 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -534,7 +534,6 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
 		WARN_ON_ONCE(rnp->completedqs == rnp->gp_seq &&
 			     (!empty_norm || rnp->qsmask));
 		empty_exp = sync_rcu_exp_done(rnp);
-		smp_mb(); /* ensure expedited fastpath sees end of RCU c-s. */
 		np = rcu_next_node_entry(t, rnp);
 		list_del_init(&t->rcu_node_entry);
 		t->rcu_blocked_node = NULL;
-- 
2.48.1