On Wed, Oct 9, 2024 at 4:56 PM Frederic Weisbecker <frede...@kernel.org> 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) <j...@joelfernandes.org>
thanks,
- Joel
