* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-27 17:23 ` Paul E. McKenney
@ 2021-07-27 19:26 ` Sebastian Andrzej Siewior
2021-07-27 23:26 ` Frederic Weisbecker
2021-07-27 19:33 ` Thomas Gleixner
2021-07-27 23:20 ` Frederic Weisbecker
2 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-07-27 19:26 UTC (permalink / raw)
To: Paul E. McKenney
Cc: rcu, Josh Triplett, Steven Rostedt, Mathieu Desnoyers,
Lai Jiangshan, Joel Fernandes, Thomas Gleixner,
Frederic Weisbecker
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-27 19:26 ` Sebastian Andrzej Siewior
@ 2021-07-27 23:26 ` Frederic Weisbecker
0 siblings, 0 replies; 9+ messages in thread
From: Frederic Weisbecker @ 2021-07-27 23:26 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Paul E. McKenney, rcu, Josh Triplett, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes,
Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-27 17:23 ` Paul E. McKenney
2021-07-27 19:26 ` Sebastian Andrzej Siewior
@ 2021-07-27 19:33 ` Thomas Gleixner
2021-07-27 23:32 ` Frederic Weisbecker
2021-07-27 23:20 ` Frederic Weisbecker
2 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2021-07-27 19:33 UTC (permalink / raw)
To: paulmck, Sebastian Andrzej Siewior
Cc: rcu, Josh Triplett, Steven Rostedt, Mathieu Desnoyers,
Lai Jiangshan, Joel Fernandes, Frederic Weisbecker
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-27 19:33 ` Thomas Gleixner
@ 2021-07-27 23:32 ` Frederic Weisbecker
2021-07-28 8:34 ` Thomas Gleixner
0 siblings, 1 reply; 9+ messages in thread
From: Frederic Weisbecker @ 2021-07-27 23:32 UTC (permalink / raw)
To: Thomas Gleixner
Cc: paulmck, Sebastian Andrzej Siewior, rcu, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-27 23:32 ` Frederic Weisbecker
@ 2021-07-28 8:34 ` Thomas Gleixner
2021-07-28 11:50 ` Frederic Weisbecker
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2021-07-28 8:34 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: paulmck, Sebastian Andrzej Siewior, rcu, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-28 8:34 ` Thomas Gleixner
@ 2021-07-28 11:50 ` Frederic Weisbecker
0 siblings, 0 replies; 9+ messages in thread
From: Frederic Weisbecker @ 2021-07-28 11:50 UTC (permalink / raw)
To: Thomas Gleixner
Cc: paulmck, Sebastian Andrzej Siewior, rcu, Josh Triplett,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] rcu/nocb: Extend checks for offloaded rdp by migrate_disable
2021-07-27 17:23 ` Paul E. McKenney
2021-07-27 19:26 ` Sebastian Andrzej Siewior
2021-07-27 19:33 ` Thomas Gleixner
@ 2021-07-27 23:20 ` Frederic Weisbecker
2 siblings, 0 replies; 9+ messages in thread
From: Frederic Weisbecker @ 2021-07-27 23:20 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Sebastian Andrzej Siewior, rcu, Josh Triplett, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes,
Thomas Gleixner
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! :)
^ permalink raw reply [flat|nested] 9+ messages in thread