Thomas Gleixner
2023-Jan-19 21:12 UTC
[PATCH v2] x86/hotplug: Do not put offline vCPUs in mwait idle state
On Mon, Jan 16 2023 at 15:55, Igor Mammedov wrote:> "Srivatsa S. Bhat" <srivatsa at csail.mit.edu> wrote: >> Fix this by preventing the use of mwait idle state in the vCPU offline >> play_dead() path for any hypervisor, even if mwait support is >> available. > > if mwait is enabled, it's very likely guest to have cpuidle > enabled and using the same mwait as well. So exiting early from > mwait_play_dead(), might just punt workflow down: > native_play_dead() > ... > mwait_play_dead(); > if (cpuidle_play_dead()) <- possible mwait here > hlt_play_dead(); > > and it will end up in mwait again and only if that fails > it will go HLT route and maybe transition to VMM.Good point.> Instead of workaround on guest side, > shouldn't hypervisor force VMEXIT on being uplugged vCPU when it's > actually hot-unplugging vCPU? (ex: QEMU kicks vCPU out from guest > context when it is removing vCPU, among other things)For a pure guest side CPU unplug operation: guest$ echo 0 >/sys/devices/system/cpu/cpu$N/online the hypervisor is not involved at all. The vCPU is not removed in that case. So to ensure that this ends up in HLT something like the below is required. Note, the removal of the comment after mwait_play_dead() is intentional because the comment is completely bogus. Not having MWAIT is not a failure. But that wants to be a seperate patch. Thanks, tglx --- diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 55cad72715d9..3f1f20f71ec5 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1833,7 +1833,10 @@ void native_play_dead(void) play_dead_common(); tboot_shutdown(TB_SHUTDOWN_WFS); - mwait_play_dead(); /* Only returns on failure */ + if (this_cpu_has(X86_FEATURE_HYPERVISOR)) + hlt_play_dead(); + + mwait_play_dead(); if (cpuidle_play_dead()) hlt_play_dead(); }
Srivatsa S. Bhat
2023-Jan-20 13:55 UTC
[PATCH v2] x86/hotplug: Do not put offline vCPUs in mwait idle state
Hi Igor and Thomas, Thank you for your review! On 1/19/23 1:12 PM, Thomas Gleixner wrote:> On Mon, Jan 16 2023 at 15:55, Igor Mammedov wrote: >> "Srivatsa S. Bhat" <srivatsa at csail.mit.edu> wrote: >>> Fix this by preventing the use of mwait idle state in the vCPU offline >>> play_dead() path for any hypervisor, even if mwait support is >>> available. >> >> if mwait is enabled, it's very likely guest to have cpuidle >> enabled and using the same mwait as well. So exiting early from >> mwait_play_dead(), might just punt workflow down: >> native_play_dead() >> ... >> mwait_play_dead(); >> if (cpuidle_play_dead()) <- possible mwait here >> hlt_play_dead(); >> >> and it will end up in mwait again and only if that fails >> it will go HLT route and maybe transition to VMM. > > Good point. > >> Instead of workaround on guest side, >> shouldn't hypervisor force VMEXIT on being uplugged vCPU when it's >> actually hot-unplugging vCPU? (ex: QEMU kicks vCPU out from guest >> context when it is removing vCPU, among other things) > > For a pure guest side CPU unplug operation: > > guest$ echo 0 >/sys/devices/system/cpu/cpu$N/online > > the hypervisor is not involved at all. The vCPU is not removed in that > case. >Agreed, and this is indeed the scenario I was targeting with this patch, as opposed to vCPU removal from the host side. I'll add this clarification to the commit message.> So to ensure that this ends up in HLT something like the below is > required. > > Note, the removal of the comment after mwait_play_dead() is intentional > because the comment is completely bogus. Not having MWAIT is not a > failure. But that wants to be a seperate patch. >Sounds good, will do and post a new version. Thank you! Regards, Srivatsa VMware Photon OS> Thanks, > > tglx > --- > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 55cad72715d9..3f1f20f71ec5 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1833,7 +1833,10 @@ void native_play_dead(void) > play_dead_common(); > tboot_shutdown(TB_SHUTDOWN_WFS); > > - mwait_play_dead(); /* Only returns on failure */ > + if (this_cpu_has(X86_FEATURE_HYPERVISOR)) > + hlt_play_dead(); > + > + mwait_play_dead(); > if (cpuidle_play_dead()) > hlt_play_dead(); > } > > > >