[libvirt] [PATCHv2 2/6] qemu: use query-cpus-fast in JSON monitor

[John Ferlan]

On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:
> Use query-cpus-fast instead of query-cpus if supported by QEMU.
> Based on the QEMU_CAPS_QUERY_CPUS_FAST capability.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_domain.c       | 12 ++++++++++--
>  src/qemu/qemu_monitor.c      | 30 ++++++++++++++++++------------
>  src/qemu/qemu_monitor.h      |  7 +++++--
>  src/qemu/qemu_monitor_json.c | 37 +++++++++++++++++++++++++++----------
>  src/qemu/qemu_monitor_json.h |  3 ++-
>  tests/qemumonitorjsontest.c  |  4 ++--
>  6 files changed, 64 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8b4efc8..4079fb3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8683,7 +8683,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>          return -1;
>  
> -    rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, hotplug);
> +    rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm),
> +                               &info,
> +                               maxvcpus,
> +                               hotplug,
> +                               virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
> +                                              QEMU_CAPS_QUERY_CPUS_FAST));

Perhaps we should create a qemuMonitorGetCPUInfoFast API since we should
also be able to return the @props {core-id, thread-id, socket-id} and
report them. I'm not steadfast on this suggestion - maybe Peter has a
stronger opinion one way or the other. Still it allows future
adjustments and additions to -fast to be more easily "handled".

It would seem those values could be useful although I do see there could
be some "impact" with how hotplug works and the default setting to -1 of
the values by qemuMonitorCPUInfoClear.

FWIW: Below this point as the returned @info structure is parsed,
there's a comment about query-cpus that would then of course not
necessarily be true or at the very least "adjusted properly" for this
new -fast model.

>  
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
> @@ -8803,7 +8808,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>          return -1;
>  
> -    haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), maxvcpus);
> +    haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm),
> +                                        maxvcpus,
> +                                        virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
> +                                                       QEMU_CAPS_QUERY_CPUS_FAST));

Maybe this one should pass 'false' for now until patch 4 comes along to
add the code that fetches the cpu-state and alters halted for the -fast
model. Depending on how one feels about larger patches vs having a 2
patch gap to not have the support for -fast... I dunno, I could see
reason to merge patch 4 here too in order to be in a patch feature for
feature compatible.

>  
>      if (qemuDomainObjExitMonitor(driver, vm) < 0 || !haltedmap)
>          goto cleanup;

[...]

> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index a09e93e..6a5fb12 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1466,7 +1466,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
>  static int
>  qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>                                struct qemuMonitorQueryCpusEntry **entries,
> -                              size_t *nentries)
> +                              size_t *nentries,
> +                              bool fast)
>  {
>      struct qemuMonitorQueryCpusEntry *cpus = NULL;
>      int ret = -1;
> @@ -1491,11 +1492,19 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>          }
>  
>          /* Some older qemu versions don't report the thread_id so treat this as
> -         * non-fatal, simply returning no data */
> -        ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", &cpuid));
> -        ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread));
> -        ignore_value(virJSONValueObjectGetBoolean(entry, "halted", &halted));
> -        qom_path = virJSONValueObjectGetString(entry, "qom_path");
> +         * non-fatal, simply returning no data.
> +         * The return data of query-cpus-fast has different field names
> +         */
> +        if (fast) {
> +            ignore_value(virJSONValueObjectGetNumberInt(entry, "cpu-index", &cpuid));
> +            ignore_value(virJSONValueObjectGetNumberInt(entry, "thread-id", &thread));
> +            qom_path = virJSONValueObjectGetString(entry, "qom-path");
> +        } else {
> +            ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", &cpuid));
> +            ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread));
> +            ignore_value(virJSONValueObjectGetBoolean(entry, "halted", &halted));
> +            qom_path = virJSONValueObjectGetString(entry, "qom_path");
> +        }

So query-fast doesn't report "halted" which means we default to false
for every arch other than s390 which gets fixed in 2 patches. IIRC other
arches weren't ever reporting halted = true for the not fast and only
our test environment had the halted set. Although I could be wrong -
Peter was much more involved in that code, so I assume he'd have a more
definitive answer.

Anyway, from the 0/6 cover, I see:

"query-cpus-fast doesn't return the halted state for a virtual CPU,
meaning that the vcpu.<n>.halted value won't be reported with
'virsh domstats' anymore. This is OK, as stats values are not
guaranteed to be reported under all circumstances and on all
architectures."

That's not really the case here since halted defaults to false, but
figured I'd ask just to be sure. If not reporting it was the desire,
then "bool halted" would need to to turn into a virTristateBool in
qemuMonitorQueryCpusEntry and _qemuMonitorCPUInfo. That way it would be
either properly reported as halted or not and if not provided, it would
not be printed. But that may cause issues for other code that uses
halted... Of course I'm not sure

>  
>          cpus[i].qemu_id = cpuid;
>          cpus[i].tid = thread;

John