Re: [PATCH 03/11] rcu/nocb: Invoke rcu_core() at the start of deoffloading

[Frederic Weisbecker <frederic@kernel.org>]

Hi Boqun,

On Thu, Oct 14, 2021 at 12:07:58AM +0800, Boqun Feng wrote:
> If offloading races with rcu_core(), can the following happen?
> 
> 	<offload work>				
> 	rcu_nocb_rdp_offload():
> 	    					rcu_core():
> 						  ...
> 						  rcu_nocb_lock_irqsave(); // no a lock
> 	  raw_spin_lock_irqsave(->nocb_lock);
> 	    rdp_offload_toggle():
> 	      <LOCKING | OFFLOADED set>
> 	      					  if (!rcu_segcblist_restempty(...))
> 						    rcu_accelerate_cbs_unlocked(...);
> 						  rcu_nocb_unlock_irqrestore();
> 						  // ^ a real unlock,
> 						  // and will preempt_enable()
> 	    // offload continue with ->nocb_lock not held
> 
> If this can happen, it means an unpaired preempt_enable() and an
> incorrect unlock. Thoughts? Maybe I'm missing something here?

Since we are unconditionally disabling IRQs on rcu_nocb_lock_irqsave(), we can't
be preempted by rcu_nocb_rdp_offload() until rcu_nocb_unlock_irqrestore() has
completed. And both have to run on the rdp target CPU. So this shouldn't happen.

Thanks.



> 
> Regards,
> Boqun
> 
> > +	 *
> > +	 * _ Deoffloading: In the worst case we miss callbacks acceleration or
> > +	 *                 processing. This is fine because the early stage
> > +	 *                 of deoffloading invokes rcu_core() after setting
> > +	 *                 SEGCBLIST_RCU_CORE. So we guarantee that we'll process
> > +	 *                 what could have been dismissed without the need to wait
> > +	 *                 for the next rcu_pending() check in the next jiffy.
> > +	 */
> >  	const bool do_batch = !rcu_segcblist_completely_offloaded(&rdp->cblist);
> >  
> >  	if (cpu_is_offline(smp_processor_id()))
> > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > index 71a28f50b40f..3b470113ae38 100644
> > --- a/kernel/rcu/tree_nocb.h
> > +++ b/kernel/rcu/tree_nocb.h
> > @@ -990,6 +990,15 @@ static long rcu_nocb_rdp_deoffload(void *arg)
> >  	 * will refuse to put anything into the bypass.
> >  	 */
> >  	WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));
> > +	/*
> > +	 * Start with invoking rcu_core() early. This way if the current thread
> > +	 * happens to preempt an ongoing call to rcu_core() in the middle,
> > +	 * leaving some work dismissed because rcu_core() still thinks the rdp is
> > +	 * completely offloaded, we are guaranteed a nearby future instance of
> > +	 * rcu_core() to catch up.
> > +	 */
> > +	rcu_segcblist_set_flags(cblist, SEGCBLIST_RCU_CORE);
> > +	invoke_rcu_core();
> >  	ret = rdp_offload_toggle(rdp, false, flags);
> >  	swait_event_exclusive(rdp->nocb_state_wq,
> >  			      !rcu_segcblist_test_flags(cblist, SEGCBLIST_KTHREAD_CB |
> > -- 
> > 2.25.1
> >