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

Frederic Weisbecker
2021-10-14T11:07:57Z

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
> >