Re: [PATCH 2/3] rcu/nocb: Fix rcuog wake-up from offline softirq

[Joel Fernandes]

On Wed, Oct 9, 2024 at 4:56 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>
[...]
> > >         ? tick_handle_periodic
> > >         sysvec_apic_timer_interrupt
> > >         </IRQ>
> > >
> > > The periodic tick must be shutdown when the CPU is offline, just like is
> > > done for oneshot tick. This must be fixed but this is not enough:
> > > softirqs can happen on any hardirq tail and reproduce the above scenario.
> > >
> > > Fix this with introducing a special deferred rcuog wake up mode when the
> > > CPU is offline. This deferred wake up doesn't arm any timer and simply
> > > wait for rcu_report_cpu_dead() to be called in order to flush any
> > > pending rcuog wake up.
> > [...]
> > > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> > > index a9a811d9d7a3..7ed060edd12b 100644
> > > --- a/kernel/rcu/tree.h
> > > +++ b/kernel/rcu/tree.h
> > > @@ -290,6 +290,7 @@ struct rcu_data {
> > >  #define RCU_NOCB_WAKE_LAZY     2
> > >  #define RCU_NOCB_WAKE          3
> > >  #define RCU_NOCB_WAKE_FORCE    4
> > > +#define RCU_NOCB_WAKE_OFFLINE   5
> > >
> > >  #define RCU_JIFFIES_TILL_FORCE_QS (1 + (HZ > 250) + (HZ > 500))
> > >                                         /* For jiffies_till_first_fqs and */
> > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > > index 2fb803f863da..8648233e1717 100644
> > > --- a/kernel/rcu/tree_nocb.h
> > > +++ b/kernel/rcu/tree_nocb.h
> > > @@ -295,6 +295,8 @@ static void wake_nocb_gp_defer(struct rcu_data *rdp, int waketype,
> > >         case RCU_NOCB_WAKE_FORCE:
> > >                 if (rdp_gp->nocb_defer_wakeup < RCU_NOCB_WAKE)
> > >                         mod_timer(&rdp_gp->nocb_timer, jiffies + 1);
> > > +               fallthrough;
> > > +       case RCU_NOCB_WAKE_OFFLINE:
> > >                 if (rdp_gp->nocb_defer_wakeup < waketype)
> > >                         WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> > >                 break;
> > > @@ -562,8 +564,16 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
> > >         lazy_len = READ_ONCE(rdp->lazy_len);
> > >         if (was_alldone) {
> > >                 rdp->qlen_last_fqs_check = len;
> > > -               // Only lazy CBs in bypass list
> > > -               if (lazy_len && bypass_len == lazy_len) {
> > > +               if (cpu_is_offline(rdp->cpu)) {
> > > +                       /*
> > > +                        * Offline CPUs can't call swake_up_one_online() from IRQs. Rely
> > > +                        * on the final deferred wake-up rcutree_report_cpu_dead()
> > > +                        */
> > > +                       rcu_nocb_unlock(rdp);
> > > +                       wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_OFFLINE,
> > > +                                          TPS("WakeEmptyIsDeferredOffline"));
> > > +               } else if (lazy_len && bypass_len == lazy_len) {
> >
> > Since the call stack is when softirqs are disabled,  would an
> > alternative fix be (pseudocode):
> >
> > Change the following in the "if (was_alldone)" block:
> >
> >                if (!irqs_disabled_flags(flags)) {
> >
> > to:
> >                if (!irqs_disabled_flags(flags) && !in_softirq())
> >
> > ?
> >
> > That way perhaps an additional RCU_NOCB flag is not needed.
> >
> > Or does that not work for some reason?
>
> It works but this forces the wake-up through the timer when a callback is
> enqueued from softirqs. And waking up from the timer is a bit more overhead
> and also added GP delay. It could be this though:
>
>     if (!irqs_disabled_flags(flags) && cpu_online(smp_processor_id()))
>

This makes sense to me and also will future-proof this code path from
potential users who end up here. I think it will work.

Feel free to add to this and the next patch:
Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>

thanks,

 - Joel