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

> 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