[PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Sebastian Andrzej Siewior]

Commit 3820b513a2e33 ("rcu/nocb: Detect unsafe checks for offloaded
rdp") added checks for safe rdp usage in the nocb case.

On PREEMPT_RT this checks triggers in the
 rcu_cpu_kthread() -> rcu_core() -> rcu_check_quiescent_state() ->
  rcu_rdp_is_offloaded()

call chain. On !PREEMPT_RT this warnings is suppressed because rcu_core()
is invoked with disabled BH which also disables preemption and
"preemptible()" is part of the checks which are considered safe.
On PREEMPT_RT disabling BH does not disable preemption and since there
is no other criteria the warning triggers.

According to the mentioned commit the goal is to check if the rcu data
pointer is "stable … and prevent from its value to be changed under us".
My understanding is that this may happen if the task is preemptible and
therefore free to be migrated to another CPU which would then lead to
another value of `rcu_data'.
One thing that has been overseen is that a task within a migrate-disable
region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
not be migrated to another CPU which should be enough to guarantee that
rdp remains stable.

Check also disabled migration of the task if the RCU data pointer is
from current CPU. Put the whole check within an SMP ifdef block since
without SMP there are not CPU migrations to worry about (also
task_struct::migration_disabled is missing).

Cc: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
I don't fully understand why the CPU-hotplug lock matters here but this
is beside the point ;)

 kernel/rcu/tree_plugin.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 0ff5e4fb933e7..d8a623ba7d243 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -57,16 +57,18 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
 	 * timers have their own means of synchronization against the
 	 * offloaded state updaters.
 	 */
+#ifdef CONFIG_SMP
 	RCU_LOCKDEP_WARN(
 		!(lockdep_is_held(&rcu_state.barrier_mutex) ||
 		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
 		  rcu_lockdep_is_held_nocb(rdp) ||
 		  (rdp == this_cpu_ptr(&rcu_data) &&
-		   !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
+		   (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) ||
+		    current->migration_disabled)) ||
 		  rcu_current_is_nocb_kthread(rdp)),
 		"Unsafe read of RCU_NOCB offloaded state"
 	);
-
+#endif
 	return rcu_segcblist_is_offloaded(&rdp->cblist);
 }
 
-- 
2.32.0

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Paul E. McKenney]

On Tue, Jul 27, 2021 at 06:38:15PM +0200, Sebastian Andrzej Siewior wrote:
> Commit 3820b513a2e33 ("rcu/nocb: Detect unsafe checks for offloaded
> rdp") added checks for safe rdp usage in the nocb case.
> 
> On PREEMPT_RT this checks triggers in the
>  rcu_cpu_kthread() -> rcu_core() -> rcu_check_quiescent_state() ->
>   rcu_rdp_is_offloaded()
> 
> call chain. On !PREEMPT_RT this warnings is suppressed because rcu_core()
> is invoked with disabled BH which also disables preemption and
> "preemptible()" is part of the checks which are considered safe.
> On PREEMPT_RT disabling BH does not disable preemption and since there
> is no other criteria the warning triggers.
> 
> According to the mentioned commit the goal is to check if the rcu data
> pointer is "stable … and prevent from its value to be changed under us".
> My understanding is that this may happen if the task is preemptible and
> therefore free to be migrated to another CPU which would then lead to
> another value of `rcu_data'.
> One thing that has been overseen is that a task within a migrate-disable
> region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
> not be migrated to another CPU which should be enough to guarantee that
> rdp remains stable.
> 
> Check also disabled migration of the task if the RCU data pointer is
> from current CPU. Put the whole check within an SMP ifdef block since
> without SMP there are not CPU migrations to worry about (also
> task_struct::migration_disabled is missing).
> 
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I don't fully understand why the CPU-hotplug lock matters here but this
> is beside the point ;)

If I remember correctly, any attempt to change the offloaded state
must hold off CPU-hotplug operations.  So if the current thread is
holding off CPU-hotplug operations, no other thread can be doing
an offload or de-offload operation.

>  kernel/rcu/tree_plugin.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 0ff5e4fb933e7..d8a623ba7d243 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -57,16 +57,18 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>  	 * timers have their own means of synchronization against the
>  	 * offloaded state updaters.
>  	 */
> +#ifdef CONFIG_SMP
>  	RCU_LOCKDEP_WARN(
>  		!(lockdep_is_held(&rcu_state.barrier_mutex) ||
>  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
>  		  rcu_lockdep_is_held_nocb(rdp) ||
>  		  (rdp == this_cpu_ptr(&rcu_data) &&
> -		   !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
> +		   (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) ||


> +		    current->migration_disabled)) ||

How does this change interact with the one proposed by Valentin?

https://lore.kernel.org/lkml/20210721115118.729943-3-valentin.schneider@arm.com/

							Thanx, Paul

>  		  rcu_current_is_nocb_kthread(rdp)),
>  		"Unsafe read of RCU_NOCB offloaded state"
>  	);
> -
> +#endif
>  	return rcu_segcblist_is_offloaded(&rdp->cblist);
>  }
>  
> -- 
> 2.32.0
> 

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Sebastian Andrzej Siewior]

On 2021-07-27 10:23:51 [-0700], Paul E. McKenney wrote:
> > I don't fully understand why the CPU-hotplug lock matters here but this
> > is beside the point ;)
> 
> If I remember correctly, any attempt to change the offloaded state
> must hold off CPU-hotplug operations.  So if the current thread is
> holding off CPU-hotplug operations, no other thread can be doing
> an offload or de-offload operation.

I'm not sure what you mean by "change the offloaded state". If the
CPU-hotplug read-lock is acquired you are still preemptible. So you
could migrate to another CPU at which point this_cpu_ptr(&rcu_data)
would change.

> >  kernel/rcu/tree_plugin.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 0ff5e4fb933e7..d8a623ba7d243 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -57,16 +57,18 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
> >  	 * timers have their own means of synchronization against the
> >  	 * offloaded state updaters.
> >  	 */
> > +#ifdef CONFIG_SMP
> >  	RCU_LOCKDEP_WARN(
> >  		!(lockdep_is_held(&rcu_state.barrier_mutex) ||
> >  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
> >  		  rcu_lockdep_is_held_nocb(rdp) ||
> >  		  (rdp == this_cpu_ptr(&rcu_data) &&
> > -		   !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
> > +		   (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) ||
> 
> 
> > +		    current->migration_disabled)) ||
> 
> How does this change interact with the one proposed by Valentin?
> 
> https://lore.kernel.org/lkml/20210721115118.729943-3-valentin.schneider@arm.com/

So by looking at the series, it does the same thing. I would prefer
is_pcpu_stable() rather then is_pcpu_safe() but that is a different
topic.
If we settle for this series instead someone should respond to that
thread. Let me see if I find someone.
I'm fine either way :)

Thanks Paul.

> 							Thanx, Paul
> 
> >  		  rcu_current_is_nocb_kthread(rdp)),
> >  		"Unsafe read of RCU_NOCB offloaded state"
> >  	);
> > -
> > +#endif
> >  	return rcu_segcblist_is_offloaded(&rdp->cblist);
> >  }
> >  

Sebastian

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Thomas Gleixner]

On Tue, Jul 27 2021 at 10:23, Paul E. McKenney wrote:
> On Tue, Jul 27, 2021 at 06:38:15PM +0200, Sebastian Andrzej Siewior wrote:
>> One thing that has been overseen is that a task within a migrate-disable
>> region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
>> not be migrated to another CPU which should be enough to guarantee that
>> rdp remains stable.
>> 
>> Check also disabled migration of the task if the RCU data pointer is
>> from current CPU. Put the whole check within an SMP ifdef block since
>> without SMP there are not CPU migrations to worry about (also
>> task_struct::migration_disabled is missing).
>> 
>> Cc: Frederic Weisbecker <frederic@kernel.org>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>> I don't fully understand why the CPU-hotplug lock matters here but this
>> is beside the point ;)
>
> If I remember correctly, any attempt to change the offloaded state
> must hold off CPU-hotplug operations.  So if the current thread is
> holding off CPU-hotplug operations, no other thread can be doing
> an offload or de-offload operation.

It only prevents unplugging of a CPU, but not plugging a CPU.

>>  kernel/rcu/tree_plugin.h | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>> index 0ff5e4fb933e7..d8a623ba7d243 100644
>> --- a/kernel/rcu/tree_plugin.h
>> +++ b/kernel/rcu/tree_plugin.h
>> @@ -57,16 +57,18 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>>  	 * timers have their own means of synchronization against the
>>  	 * offloaded state updaters.
>>  	 */
>> +#ifdef CONFIG_SMP
>>  	RCU_LOCKDEP_WARN(
>>  		!(lockdep_is_held(&rcu_state.barrier_mutex) ||
>>  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
>>  		  rcu_lockdep_is_held_nocb(rdp) ||
>>  		  (rdp == this_cpu_ptr(&rcu_data) &&
>> -		   !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
>> +		   (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) ||
>
>
>> +		    current->migration_disabled)) ||
>
> How does this change interact with the one proposed by Valentin?
>
> https://lore.kernel.org/lkml/20210721115118.729943-3-valentin.schneider@arm.com/

It does not interact, it conflicts and Valentin's is definitely the
better solution.

Thanks,

        tglx

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Frederic Weisbecker]

On Tue, Jul 27, 2021 at 10:23:51AM -0700, Paul E. McKenney wrote:
> On Tue, Jul 27, 2021 at 06:38:15PM +0200, Sebastian Andrzej Siewior wrote:
> > Commit 3820b513a2e33 ("rcu/nocb: Detect unsafe checks for offloaded
> > rdp") added checks for safe rdp usage in the nocb case.
> > 
> > On PREEMPT_RT this checks triggers in the
> >  rcu_cpu_kthread() -> rcu_core() -> rcu_check_quiescent_state() ->
> >   rcu_rdp_is_offloaded()
> > 
> > call chain. On !PREEMPT_RT this warnings is suppressed because rcu_core()
> > is invoked with disabled BH which also disables preemption and
> > "preemptible()" is part of the checks which are considered safe.
> > On PREEMPT_RT disabling BH does not disable preemption and since there
> > is no other criteria the warning triggers.
> > 
> > According to the mentioned commit the goal is to check if the rcu data
> > pointer is "stable … and prevent from its value to be changed under us".
> > My understanding is that this may happen if the task is preemptible and
> > therefore free to be migrated to another CPU which would then lead to
> > another value of `rcu_data'.
> > One thing that has been overseen is that a task within a migrate-disable
> > region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
> > not be migrated to another CPU which should be enough to guarantee that
> > rdp remains stable.
> > 
> > Check also disabled migration of the task if the RCU data pointer is
> > from current CPU. Put the whole check within an SMP ifdef block since
> > without SMP there are not CPU migrations to worry about (also
> > task_struct::migration_disabled is missing).
> > 
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > I don't fully understand why the CPU-hotplug lock matters here but this
> > is beside the point ;)
> 
> If I remember correctly, any attempt to change the offloaded state
> must hold off CPU-hotplug operations.  So if the current thread is
> holding off CPU-hotplug operations, no other thread can be doing
> an offload or de-offload operation.

Exactly! :)

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Frederic Weisbecker]

On Tue, Jul 27, 2021 at 09:26:05PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-07-27 10:23:51 [-0700], Paul E. McKenney wrote:
> > > I don't fully understand why the CPU-hotplug lock matters here but this
> > > is beside the point ;)
> > 
> > If I remember correctly, any attempt to change the offloaded state
> > must hold off CPU-hotplug operations.  So if the current thread is
> > holding off CPU-hotplug operations, no other thread can be doing
> > an offload or de-offload operation.
> 
> I'm not sure what you mean by "change the offloaded state". If the
> CPU-hotplug read-lock is acquired you are still preemptible. So you
> could migrate to another CPU at which point this_cpu_ptr(&rcu_data)
> would change.

Sure but it's fine to read the offloaded state of a remote CPU if
CPU hotplug lock is held.

> > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > > index 0ff5e4fb933e7..d8a623ba7d243 100644
> > > --- a/kernel/rcu/tree_plugin.h
> > > +++ b/kernel/rcu/tree_plugin.h
> > > @@ -57,16 +57,18 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
> > >  	 * timers have their own means of synchronization against the
> > >  	 * offloaded state updaters.
> > >  	 */
> > > +#ifdef CONFIG_SMP
> > >  	RCU_LOCKDEP_WARN(
> > >  		!(lockdep_is_held(&rcu_state.barrier_mutex) ||
> > >  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
> > >  		  rcu_lockdep_is_held_nocb(rdp) ||
> > >  		  (rdp == this_cpu_ptr(&rcu_data) &&
> > > -		   !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
> > > +		   (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) ||
> > 
> > 
> > > +		    current->migration_disabled)) ||
> > 
> > How does this change interact with the one proposed by Valentin?
> > 
> > https://lore.kernel.org/lkml/20210721115118.729943-3-valentin.schneider@arm.com/
> 
> So by looking at the series, it does the same thing. I would prefer
> is_pcpu_stable() rather then is_pcpu_safe() but that is a different
> topic.
> If we settle for this series instead someone should respond to that
> thread. Let me see if I find someone.
> I'm fine either way :)

Either way I believe it won't work, please check:

https://lore.kernel.org/lkml/20210727230814.GC283787@lothringen/

I wonder also, how many similar assumption out there do we have that
rely on softirqs not being preemptible. Most of them probably silent.

Thanks.

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Frederic Weisbecker]

On Tue, Jul 27, 2021 at 09:33:42PM +0200, Thomas Gleixner wrote:
> On Tue, Jul 27 2021 at 10:23, Paul E. McKenney wrote:
> > On Tue, Jul 27, 2021 at 06:38:15PM +0200, Sebastian Andrzej Siewior wrote:
> >> One thing that has been overseen is that a task within a migrate-disable
> >> region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
> >> not be migrated to another CPU which should be enough to guarantee that
> >> rdp remains stable.
> >> 
> >> Check also disabled migration of the task if the RCU data pointer is
> >> from current CPU. Put the whole check within an SMP ifdef block since
> >> without SMP there are not CPU migrations to worry about (also
> >> task_struct::migration_disabled is missing).
> >> 
> >> Cc: Frederic Weisbecker <frederic@kernel.org>
> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >> ---
> >> I don't fully understand why the CPU-hotplug lock matters here but this
> >> is beside the point ;)
> >
> > If I remember correctly, any attempt to change the offloaded state
> > must hold off CPU-hotplug operations.  So if the current thread is
> > holding off CPU-hotplug operations, no other thread can be doing
> > an offload or de-offload operation.
> 
> It only prevents unplugging of a CPU, but not plugging a CPU.

Hmm, but both _cpu_down() and _cpu_up() do cpus_write_lock().
What did I overlook?

PS: just had a quick look and no RCU cpu up operation seem to even check
RCU nocb offload state. So we should be fine ( -ENOSPC for further
famous last words to engrave).

Thanks.

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Thomas Gleixner]

On Wed, Jul 28 2021 at 01:32, Frederic Weisbecker wrote:
> On Tue, Jul 27, 2021 at 09:33:42PM +0200, Thomas Gleixner wrote:
>> On Tue, Jul 27 2021 at 10:23, Paul E. McKenney wrote:
>> > On Tue, Jul 27, 2021 at 06:38:15PM +0200, Sebastian Andrzej Siewior wrote:
>> >> One thing that has been overseen is that a task within a migrate-disable
>> >> region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
>> >> not be migrated to another CPU which should be enough to guarantee that
>> >> rdp remains stable.
>> >> 
>> >> Check also disabled migration of the task if the RCU data pointer is
>> >> from current CPU. Put the whole check within an SMP ifdef block since
>> >> without SMP there are not CPU migrations to worry about (also
>> >> task_struct::migration_disabled is missing).
>> >> 
>> >> Cc: Frederic Weisbecker <frederic@kernel.org>
>> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> >> ---
>> >> I don't fully understand why the CPU-hotplug lock matters here but this
>> >> is beside the point ;)
>> >
>> > If I remember correctly, any attempt to change the offloaded state
>> > must hold off CPU-hotplug operations.  So if the current thread is
>> > holding off CPU-hotplug operations, no other thread can be doing
>> > an offload or de-offload operation.
>> 
>> It only prevents unplugging of a CPU, but not plugging a CPU.
>
> Hmm, but both _cpu_down() and _cpu_up() do cpus_write_lock().
> What did I overlook?

I meant, that preemption disable does not prevent plugging a CPU, but
the final unplug step is prevented because the stomp machine thread
cannot run. Similar for migrate_disable(). The final unplug step can
only happen when all non-pinned tasks have left the migrate disabled
section.

Thanks,

        tglx

Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable

[Frederic Weisbecker]

On Wed, Jul 28, 2021 at 10:34:01AM +0200, Thomas Gleixner wrote:
> On Wed, Jul 28 2021 at 01:32, Frederic Weisbecker wrote:
> > On Tue, Jul 27, 2021 at 09:33:42PM +0200, Thomas Gleixner wrote:
> >> On Tue, Jul 27 2021 at 10:23, Paul E. McKenney wrote:
> >> > On Tue, Jul 27, 2021 at 06:38:15PM +0200, Sebastian Andrzej Siewior wrote:
> >> >> One thing that has been overseen is that a task within a migrate-disable
> >> >> region (as on PREEMPT_RT with disabled BH) is fully preemptible but may
> >> >> not be migrated to another CPU which should be enough to guarantee that
> >> >> rdp remains stable.
> >> >> 
> >> >> Check also disabled migration of the task if the RCU data pointer is
> >> >> from current CPU. Put the whole check within an SMP ifdef block since
> >> >> without SMP there are not CPU migrations to worry about (also
> >> >> task_struct::migration_disabled is missing).
> >> >> 
> >> >> Cc: Frederic Weisbecker <frederic@kernel.org>
> >> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >> >> ---
> >> >> I don't fully understand why the CPU-hotplug lock matters here but this
> >> >> is beside the point ;)
> >> >
> >> > If I remember correctly, any attempt to change the offloaded state
> >> > must hold off CPU-hotplug operations.  So if the current thread is
> >> > holding off CPU-hotplug operations, no other thread can be doing
> >> > an offload or de-offload operation.
> >> 
> >> It only prevents unplugging of a CPU, but not plugging a CPU.
> >
> > Hmm, but both _cpu_down() and _cpu_up() do cpus_write_lock().
> > What did I overlook?
> 
> I meant, that preemption disable does not prevent plugging a CPU, but
> the final unplug step is prevented because the stomp machine thread
> cannot run. Similar for migrate_disable(). The final unplug step can
> only happen when all non-pinned tasks have left the migrate disabled
> section.

Ah ok so that should be fine since we rely on cpu_hotplug_lock to protect
against CPU hotplug here, preemption disabled is there for other purposes.

Thanks.