Le Wed, Oct 09, 2024 at 02:23:15PM -0400, Joel Fernandes a écrit :
> Hi Frederic,
>
> On Wed, Oct 2, 2024 at 10:57 AM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> >
> > After a CPU has set itself offline and before it eventually calls
> > rcutree_report_cpu_dead(), there are still opportunities for callbacks
> > to be enqueued, for example from an IRQ. When that happens on NOCB, the
> > rcuog wake-up is deferred through an IPI to an online CPU in order not
> > to call into the scheduler and risk arming the RT-bandwidth after
> > hrtimers have been migrated out and disabled.
> >
> > But performing a synchronized IPI from an IRQ is buggy as reported in
> > the following scenario:
> >
> > WARNING: CPU: 1 PID: 26 at kernel/smp.c:633 smp_call_function_single
> > Modules linked in: rcutorture torture
> > CPU: 1 UID: 0 PID: 26 Comm: migration/1 Not tainted 6.11.0-rc1-00012-g9139f93209d1 #1
> > Stopper: multi_cpu_stop+0x0/0x320 <- __stop_cpus+0xd0/0x120
> > RIP: 0010:smp_call_function_single
> > <IRQ>
> > swake_up_one_online
> > __call_rcu_nocb_wake
> > __call_rcu_common
> > ? rcu_torture_one_read
> > call_timer_fn
> > __run_timers
> > run_timer_softirq
> > handle_softirqs
> > irq_exit_rcu
>
> This call stack seems a bit confusing with the context of the first
> paragraph. In the beginning of changelog, you had mentioned the issue
> happens from IRQ, but in fact the callstack here is from softirq
> right? The IRQ issue should already be resolved in current code
> AFAICS.
Indeed, I need to s/IRQ/softirq for clarity.
>
> > ? 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()))
Hmm?
Thanks.
>
> thanks,
>
> - Joel
