On 23.03.2018 17:05, John Ferlan wrote:
[...]
>> 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,
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.
>
Agreed, this one escaped me. Will change.
>>
>> 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
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
