Re: sched: Unexpected reschedule of offline CPU#2!

[Phil Auld <pauld@redhat.com>]

Hi Thomas,

On Tue, Sep 03, 2024 at 05:27:58PM +0200 Thomas Gleixner wrote:
> On Tue, Jul 27 2021 at 10:46, Jan Kiszka wrote:
> 
> Picking up this dead thread again.

Necro-ing this again...

I keep getting occasional reports of this case. Unfortunately
though, I've never been able to reproduce it myself.

Did the below patch ever go anywhere?

It seems to be stable in my testing with the addition of
an "extern" in asm/cpu.h to get it to build.

> 
> > What is supposed to prevent the following in mainline:
> >
> > CPU 0                   CPU 1                      CPU 2
> >
> > native_stop_other_cpus                             <INTERRUPT>
> >   send_IPI_allbutself                              ...
> >                         <INTERRUPT>
> >                         sysvec_reboot
> >                           stop_this_cpu
> >                             set_cpu_online(false)
> >                                                    native_smp_send_reschedule(1)
> >                                                      if (cpu_is_offline(1)) ...
> 
> Nothing. And that's what probably happens if I read the stack trace
> correctly.
> 
> But we can be slightly smarter about this for the reboot IPI (the NMI
> case does not have that issue).
> 
> CPU 0                   CPU 1                      CPU 2
> 
> native_stop_other_cpus                             <INTERRUPT>
>    send_IPI_allbutself                              ...
>                          <IPI>
>                          sysvec_reboot
>                            wait_for_others();
>                                                     </INTERRUPT>
>                                                     <IPI>
>                                                     sysvec_reboot
>                                                     wait_for_others();
>                            stop_this_cpu();           stop_this_cpu();
>                              set_cpu_online(false);     set_cpu_online(false);
> 
> Something like the uncompiled below.
> 
> Thanks,
> 
>         tglx
> ---
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -68,5 +68,6 @@ bool intel_find_matching_signature(void
>  int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type);
>  
>  extern struct cpumask cpus_stop_mask;
> +atomic_t cpus_stop_in_ipi;

extern 

>  
>  #endif /* _ASM_X86_CPU_H */
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -721,7 +721,7 @@ bool xen_set_default_idle(void);
>  #define xen_set_default_idle 0
>  #endif
>  
> -void __noreturn stop_this_cpu(void *dummy);
> +void __noreturn stop_this_cpu(bool sync);
>  void microcode_check(struct cpuinfo_x86 *prev_info);
>  void store_cpu_caps(struct cpuinfo_x86 *info);
>  
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -791,9 +791,10 @@ bool xen_set_default_idle(void)
>  }
>  #endif
>  
> +atomic_t cpus_stop_in_ipi;
>  struct cpumask cpus_stop_mask;
>  
> -void __noreturn stop_this_cpu(void *dummy)
> +void __noreturn stop_this_cpu(bool sync)
>  {
>  	struct cpuinfo_x86 *c = this_cpu_ptr(&cpu_info);
>  	unsigned int cpu = smp_processor_id();
> @@ -801,6 +802,16 @@ void __noreturn stop_this_cpu(void *dumm
>  	local_irq_disable();
>  
>  	/*
> +	 * Account this CPU and loop until the other CPUs reached this
> +	 * point. If they don't react, the control CPU will raise an NMI.
> +	 */
> +	if (sync) {
> +		atomic_dec(&cpus_stop_in_ipi);
> +		while (atomic_read(&cpus_stop_in_ipi))
> +			cpu_relax();
> +	}
> +
> +	/*
>  	 * Remove this CPU from the online mask and disable it
>  	 * unconditionally. This might be redundant in case that the reboot
>  	 * vector was handled late and stop_other_cpus() sent an NMI.
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -788,7 +788,7 @@ static void native_machine_halt(void)
>  
>  	tboot_shutdown(TB_SHUTDOWN_HALT);
>  
> -	stop_this_cpu(NULL);
> +	stop_this_cpu(false);
>  }
>  
>  static void native_machine_power_off(void)
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -125,7 +125,7 @@ static int smp_stop_nmi_callback(unsigne
>  		return NMI_HANDLED;
>  
>  	cpu_emergency_disable_virtualization();
> -	stop_this_cpu(NULL);
> +	stop_this_cpu(false);
>  
>  	return NMI_HANDLED;
>  }
> @@ -137,7 +137,7 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_reboot)
>  {
>  	apic_eoi();
>  	cpu_emergency_disable_virtualization();
> -	stop_this_cpu(NULL);
> +	stop_this_cpu(true);
>  }
>  
>  static int register_stop_handler(void)
> @@ -189,6 +189,7 @@ static void native_stop_other_cpus(int w
>  	 */
>  	cpumask_copy(&cpus_stop_mask, cpu_online_mask);
>  	cpumask_clear_cpu(this_cpu, &cpus_stop_mask);
> +	atomic_set(&cpus_stop_in_ipi, num_online_cpus() - 1);
>  
>  	if (!cpumask_empty(&cpus_stop_mask)) {
>  		apic_send_IPI_allbutself(REBOOT_VECTOR);
> @@ -235,10 +236,12 @@ static void native_stop_other_cpus(int w
>  	local_irq_restore(flags);
>  
>  	/*
> -	 * Ensure that the cpus_stop_mask cache lines are invalidated on
> -	 * the other CPUs. See comment vs. SME in stop_this_cpu().
> +	 * Ensure that the cpus_stop_mask and cpus_stop_in_ipi cache lines
> +	 * are invalidated on the other CPUs. See comment vs. SME in
> +	 * stop_this_cpu().
>  	 */
>  	cpumask_clear(&cpus_stop_mask);
> +	atomic_set(&cpus_stop_in_ipi, 0);
>  }
>  
>  /*
> 


Thanks,
Phil
--