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. Actually, query-cpus[-fast] is not reporting the topology (socket, core,
On 23.03.2018 17:05, John Ferlan wrote: [...] thread). The reported thread id is referring to the QEMU CPU thread.
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.> FWIW, I intended to make the patches deprecation-safe, i.e. prevent calling query-cpus on a QEMU not supporting it anymore. I see your point
Agreed, this one escaped me. Will change. though and would have no principal objections to merging patches 2 and 4 (plus the testcase patches 3 and 5). Would be great to hear more opinions...
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. Halted was reported at least for x86, and a value of true was rather common ony multi-core systems. However, defaulting to false doesn't hurt (beyond the temporary impact on s390), because the user visible halted value is a tristate for a while now.
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
-- Regards, Viktor Mihajlovski