lkml.org 
[lkml]   [2022]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: objtool warning for next-20221118
From
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
[unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-11-24 06:29    [W:3.218 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site