Messages in this thread | | | Date | Tue, 4 Jun 2019 01:14:35 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH HACK RFC] cpu: Prevent late-arriving interrupts from disrupting offline |
| |
On Mon, Jun 03, 2019 at 10:38:48AM +0200, Peter Zijlstra wrote: > On Sat, Jun 01, 2019 at 06:12:53PM -0700, Paul E. McKenney wrote: > > Scheduling-clock interrupts can arrive late in the CPU-offline process, > > after idle entry and the subsequent call to cpuhp_report_idle_dead(). > > Once execution passes the call to rcu_report_dead(), RCU is ignoring > > the CPU, which results in lockdep complaints when the interrupt handler > > uses RCU: > > > diff --git a/kernel/cpu.c b/kernel/cpu.c > > index 448efc06bb2d..3b33d83b793d 100644 > > --- a/kernel/cpu.c > > +++ b/kernel/cpu.c > > @@ -930,6 +930,7 @@ void cpuhp_report_idle_dead(void) > > struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state); > > > > BUG_ON(st->state != CPUHP_AP_OFFLINE); > > + local_irq_disable(); > > rcu_report_dead(smp_processor_id()); > > st->state = CPUHP_AP_IDLE_DEAD; > > udelay(1000); > > Urgh... I'd almost suggest we do something like the below. > > > But then I started looking at the various arch_cpu_idle_dead() > implementations and ran into arm's implementation, which is calling > complete() where generic code already established this isn't possible > (see for example cpuhp_report_idle_dead()).
Yeah, my patch that would have changed that never was acked or taken by the maintainer, as discussed later in this thread.
> And then there's powerpc which for some obscure reason thinks it needs > to enable preemption when dying ?! pseries_cpu_die() actually calls > msleep() ?!?!
Isn't pseries_cpu_die() invoked via the smp_ops->cpu_die() function pointer, whch is invoked from __cpu_die() in arch/powerpc/kernel/smp.c? Then, if I am reading the code correctly, __cpu_die() is invoked from takedown_cpu(), which is invoked not from the dying CPU but rather from a surviving CPU. Or am I misreading the code?
> Sparc64 agains things it should enable preemption when playing dead. > > So clearly this isn't going to work well :/
Well, it looks like it will work at least as well as my patch. I will test it out this evening, ten timezones east of my usual location. ;-)
Thanx, Paul
> --- > include/linux/tick.h | 10 ---------- > kernel/sched/idle.c | 5 +++-- > 2 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/include/linux/tick.h b/include/linux/tick.h > index f92a10b5e112..196a0a7bfc4f 100644 > --- a/include/linux/tick.h > +++ b/include/linux/tick.h > @@ -134,14 +134,6 @@ extern unsigned long tick_nohz_get_idle_calls(void); > extern unsigned long tick_nohz_get_idle_calls_cpu(int cpu); > extern u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time); > extern u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time); > - > -static inline void tick_nohz_idle_stop_tick_protected(void) > -{ > - local_irq_disable(); > - tick_nohz_idle_stop_tick(); > - local_irq_enable(); > -} > - > #else /* !CONFIG_NO_HZ_COMMON */ > #define tick_nohz_enabled (0) > static inline int tick_nohz_tick_stopped(void) { return 0; } > @@ -164,8 +156,6 @@ static inline ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next) > } > static inline u64 get_cpu_idle_time_us(int cpu, u64 *unused) { return -1; } > static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } > - > -static inline void tick_nohz_idle_stop_tick_protected(void) { } > #endif /* !CONFIG_NO_HZ_COMMON */ > > #ifdef CONFIG_NO_HZ_FULL > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c > index 80940939b733..e4bc4aa739b8 100644 > --- a/kernel/sched/idle.c > +++ b/kernel/sched/idle.c > @@ -241,13 +241,14 @@ static void do_idle(void) > check_pgt_cache(); > rmb(); > > + local_irq_disable(); > + > if (cpu_is_offline(cpu)) { > - tick_nohz_idle_stop_tick_protected(); > + tick_nohz_idle_stop_tick(); > cpuhp_report_idle_dead(); > arch_cpu_idle_dead(); > } > > - local_irq_disable(); > arch_cpu_idle_enter(); > > /* >
| |