[libvirt] [PATCHv2 6/6] qemu: refresh vcpu halted state only via query-cpus-fast

[John Ferlan]

On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:
> In order to not affect running VMs, refreshing the halted state
> is only performed if QEMU supports the query-cpus-fast QAPI.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_domain.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 

BTW: You'll need a patch to docs/news.xml as this is something
newsworthy I would think.  Although calling it "-fast" there may make
people wonder about the "slowness" aspect of the current algorithm.


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4079fb3..a5fcf22 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8801,8 +8801,13 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>          return 0;
>  
>      /* The halted state is interresting only on s390(x). On other platforms

"interesting"

> -     * the data would be stale at the time when it would be used. */
> -    if (!ARCH_IS_S390(vm->def->os.arch))
> +     * the data would be stale at the time when it would be used.
> +     * Calling qemuMonitorGetCpuHalted() can adversely affect the running
> +     * VM's performance unless QEMU supports query-cpus-fast.
> +     */
> +    if (!ARCH_IS_S390(vm->def->os.arch) ||
> +        !virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
> +                        QEMU_CAPS_QUERY_CPUS_FAST))

So IOW systems that reported this previously that don't get the updated
QEMU bits (e.g. > 2.12) would now stop reporting halted only because of
the adverse performance impact.

But that is a data change and "policy" we'd be inflicting upon them
which maybe they don't care about. Perhaps in that case we should have a
"flag" that handles whether the caller cares or not.

I understand the motivation and reasoning; however, based on how such
things have been historically acked or nacked, I'm leaning towards
saying this patch shouldn't go in.

John
>          return 0;
>  
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>