[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: objtool warning for next-20221118



On 24.11.22 03:39, Josh Poimboeuf wrote:
On Wed, Nov 23, 2022 at 09:03:40AM -0800, Josh Poimboeuf wrote:
On Wed, Nov 23, 2022 at 10:52:09AM +0000, Andrew Cooper wrote:
Well, if you return from arch_cpu_idle_dead() you're back in the idle
loop -- exactly where you would be if you were to bootstrap the whole
CPU -- provided you have it remember the whole state (easier with a
vCPU).

play_dead() really needs sane semantics.  Not only does it introduce a
surprise to the offlining code in do_idle(), it also skips the entire
hotplug state machine.  Not sure if that introduces any bugs, but at the
very least it's subtle and surprising.

But maybe I'm missing something, lets add Xen folks on.

Calling VCPUOP_down on oneself always succeeds, but all it does is
deschedule the vCPU.

It can be undone at a later point by a different vcpu issuing VCPUOP_up
against the previously-downed CPU, at which point the vCPU gets rescheduled.

This is why the VCPUOP_down hypercall returns normally.  All state
really is intact.

As for what Linux does, this is how xen_pv_cpu_up() currently behaves.
If you want to make Xen behave more everything else, then bug a BUG()
after VCPUOP_down, and adjust xen_pv_cpu_up() to skip its initialised
check and always use VCPUOP_initialise to bring the vCPU back online.

Or we could do what sev_es_play_dead() does and just call start_cpu0()
after the hypercall returns?

Something like so (untested).  This is only the x86 bits.

I think I convinced myself that start_cpu0() isn't buggy.  I'm looking
at other cleanups, e.g. converging cpu_bringup_and_idle() with
start_secondary().

I can pick it up again next week, post-turkey.

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index b4dbb20dab1a..e6d1d2810e38 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -93,9 +93,10 @@ static inline void __cpu_die(unsigned int cpu)
        smp_ops.cpu_die(cpu);
  }
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
  {
        smp_ops.play_dead();
+       BUG();
  }
static inline void smp_send_reschedule(int cpu)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 26e8f57c75ad..8e2841deb1eb 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -700,7 +700,7 @@ EXPORT_SYMBOL(boot_option_idle_override);
  static void (*x86_idle)(void);
#ifndef CONFIG_SMP
-static inline void play_dead(void)
+static inline void __noreturn play_dead(void)
  {
        BUG();
  }
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 55cad72715d9..d8b12ac1a7c5 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1833,9 +1833,12 @@ void native_play_dead(void)
        play_dead_common();
        tboot_shutdown(TB_SHUTDOWN_WFS);
- mwait_play_dead(); /* Only returns on failure */
+       mwait_play_dead();      /* Only returns if mwait is not supported */
+
        if (cpuidle_play_dead())
                hlt_play_dead();
+
+       BUG();
  }
#else /* ... !CONFIG_HOTPLUG_CPU */
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 480be82e9b7b..30dc904ca990 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -385,17 +385,9 @@ static void xen_pv_play_dead(void) /* used only with 
HOTPLUG_CPU */
  {
        play_dead_common();
        HYPERVISOR_vcpu_op(VCPUOP_down, xen_vcpu_nr(smp_processor_id()), NULL);
-       cpu_bringup();
-       /*
-        * commit 4b0c0f294 (tick: Cleanup NOHZ per cpu data on cpu down)
-        * clears certain data that the cpu_idle loop (which called us
-        * and that we return from) expects. The only way to get that
-        * data back is to call:
-        */
-       tick_nohz_idle_enter();
-       tick_nohz_idle_stop_tick_protected();
- cpuhp_online_idle(CPUHP_AP_ONLINE_IDLE);
+       /* FIXME: converge cpu_bringup_and_idle() and start_secondary() */
+       cpu_bringup_and_idle();

I think this will leak stack memory. Multiple cpu offline/online cycles of
the same cpu will finally exhaust the idle stack.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.