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

[Viktor Mihajlovski]

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