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@xxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxx/
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
>
