Re: [PATCH 2/3] rcu/nocb: Check for migratability rather than pure preemptability

From: Frederic Weisbecker <frederic@kernel.org>
To: Valentin Schneider <valentin.schneider@arm.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rt-users@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Steven Rostedt <rostedt@goodmis.org>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Steven Price <steven.price@arm.com>,
	Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH 2/3] rcu/nocb: Check for migratability rather than pure preemptability
Date: Wed, 28 Jul 2021 01:08:14 +0200	[thread overview]
Message-ID: <20210727230814.GC283787@lothringen> (raw)
In-Reply-To: <20210721115118.729943-3-valentin.schneider@arm.com>

On Wed, Jul 21, 2021 at 12:51:17PM +0100, Valentin Schneider wrote:
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  kernel/rcu/tree_plugin.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index ad0156b86937..6c3c4100da83 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -70,8 +70,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>  		!(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())) ||
> +		  (rdp == this_cpu_ptr(&rcu_data) && is_pcpu_safe()) ||

I fear that won't work. We really need any caller of rcu_rdp_is_offloaded()
on the local rdp to have preemption disabled and not just migration disabled,
because we must protect against concurrent offloaded state changes.

The offloaded state is changed by a workqueue that executes on the target rdp.

Here is a practical example where it matters:

           CPU 0
           -----
           // =======> task rcuc running
           rcu_core {
	       rcu_nocb_lock_irqsave(rdp, flags) {
                   if (!rcu_segcblist_is_offloaded(rdp->cblist)) {
		       // is not offloaded right now, so it's going
                       // to just disable IRQs. Oh no wait:
           // preemption
           // ========> workqueue running
           rcu_nocb_rdp_offload();
           // ========> task rcuc resume
	               local_irq_disable();
                   }
               }
	       ....
       	       rcu_nocb_unlock_irqrestore(rdp, flags) {
                   if (rcu_segcblist_is_offloaded(rdp->cblist)) {
                       // is offloaded right now so:
                       raw_spin_unlock_irqrestore(rdp, flags);

And that will explode because that's an impaired unlock on nocb_lock.