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@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));
 
     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));
 
     if (qemuDomainObjExitMonitor(driver, vm) < 0 || !haltedmap)
         goto cleanup;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index ad5c572..22b2091 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1852,15 +1852,16 @@ qemuMonitorGetCPUInfoLegacy(struct qemuMonitorQueryCpusEntry *cpuentries,
  *
  * This function stitches together data retrieved via query-hotpluggable-cpus
  * which returns entities on the hotpluggable level (which may describe more
- * than one guest logical vcpu) with the output of query-cpus, having an entry
- * per enabled guest logical vcpu.
+ * than one guest logical vcpu) with the output of query-cpus (or
+ * query-cpus-fast), having an entry per enabled guest logical vcpu.
  *
  * query-hotpluggable-cpus conveys following information:
  * - topology information and number of logical vcpus this entry creates
  * - device type name of the entry that needs to be used when hotplugging
- * - qom path in qemu which can be used to map the entry against query-cpus
+ * - qom path in qemu which can be used to map the entry against
+ *   query-cpus[-fast]
  *
- * query-cpus conveys following information:
+ * query-cpus[-fast] conveys following information:
  * - thread id of a given guest logical vcpu
  * - order in which the vcpus were inserted
  * - qom path to allow mapping the two together
@@ -1895,7 +1896,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl
     for (i = 0; i < nhotplugvcpus; i++)
         totalvcpus += hotplugvcpus[i].vcpus;
 
-    /* trim '/thread...' suffix from the data returned by query-cpus */
+    /* trim '/thread...' suffix from the data returned by query-cpus[-fast] */
     for (i = 0; i < ncpuentries; i++) {
         if (cpuentries[i].qom_path &&
             (tmp = strstr(cpuentries[i].qom_path, "/thread")))
@@ -1908,7 +1909,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl
     }
 
     /* Note the order in which the hotpluggable entities are inserted by
-     * matching them to the query-cpus entries */
+     * matching them to the query-cpus[-fast] entries */
     for (i = 0; i < ncpuentries; i++) {
         for (j = 0; j < nhotplugvcpus; j++) {
             if (!cpuentries[i].qom_path ||
@@ -1963,7 +1964,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl
         }
 
         if (anyvcpu == maxvcpus) {
-            VIR_DEBUG("too many query-cpus entries for a given "
+            VIR_DEBUG("too many query-cpus[-fast] entries for a given "
                       "query-hotpluggable-cpus entry");
             return -1;
         }
@@ -1991,6 +1992,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl
  * @vcpus: pointer filled by array of qemuMonitorCPUInfo structures
  * @maxvcpus: total possible number of vcpus
  * @hotplug: query data relevant for hotplug support
+ * @fast: use QMP query-cpus-fast if supported
  *
  * Detects VCPU information. If qemu doesn't support or fails reporting
  * information this function will return success as other parts of libvirt
@@ -2003,7 +2005,8 @@ int
 qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
                       qemuMonitorCPUInfoPtr *vcpus,
                       size_t maxvcpus,
-                      bool hotplug)
+                      bool hotplug,
+                      bool fast)
 {
     struct qemuMonitorQueryHotpluggableCpusEntry *hotplugcpus = NULL;
     size_t nhotplugcpus = 0;
@@ -2029,7 +2032,8 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
         goto cleanup;
 
     if (mon->json)
-        rc = qemuMonitorJSONQueryCPUs(mon, &cpuentries, &ncpuentries, hotplug);
+        rc = qemuMonitorJSONQueryCPUs(mon, &cpuentries, &ncpuentries, hotplug,
+                                      fast);
     else
         rc = qemuMonitorTextQueryCPUs(mon, &cpuentries, &ncpuentries);
 
@@ -2067,11 +2071,12 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
  * qemuMonitorGetCpuHalted:
  *
  * Returns a bitmap of vcpu id's that are halted. The id's correspond to the
- * 'CPU' field as reported by query-cpus'.
+ * 'CPU' field as reported by query-cpus[-fast]'.
  */
 virBitmapPtr
 qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
-                        size_t maxvcpus)
+                        size_t maxvcpus,
+                        bool fast ATTRIBUTE_UNUSED)
 {
     struct qemuMonitorQueryCpusEntry *cpuentries = NULL;
     size_t ncpuentries = 0;
@@ -2082,7 +2087,8 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
     QEMU_CHECK_MONITOR_NULL(mon);
 
     if (mon->json)
-        rc = qemuMonitorJSONQueryCPUs(mon, &cpuentries, &ncpuentries, false);
+        rc = qemuMonitorJSONQueryCPUs(mon, &cpuentries, &ncpuentries, false,
+                                      false);
     else
         rc = qemuMonitorTextQueryCPUs(mon, &cpuentries, &ncpuentries);
 
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 954ae88..7b92d41 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -530,8 +530,11 @@ void qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr list,
 int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
                           qemuMonitorCPUInfoPtr *vcpus,
                           size_t maxvcpus,
-                          bool hotplug);
-virBitmapPtr qemuMonitorGetCpuHalted(qemuMonitorPtr mon, size_t maxvcpus);
+                          bool hotplug,
+                          bool fast);
+virBitmapPtr qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
+                                     size_t maxvcpus,
+                                     bool fast);
 
 int qemuMonitorGetVirtType(qemuMonitorPtr mon,
                            virDomainVirtType *virtType);
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");
+        }
 
         cpus[i].qemu_id = cpuid;
         cpus[i].tid = thread;
@@ -1520,10 +1529,12 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
  * @mon: monitor object
  * @entries: filled with detected entries on success
  * @nentries: number of entries returned
+ * @force: force exit on error
+ * @fast: use query-cpus-fast
  *
  * Queries qemu for cpu-related information. Failure to execute the command or
  * extract results does not produce an error as libvirt can continue without
- * this information.
+ * this information, unless the caller has specified @force == true.
  *
  * Returns 0 on success, -1 on a fatal error (oom ...) and -2 if the
  * query failed gracefully.
@@ -1532,13 +1543,19 @@ int
 qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
                          struct qemuMonitorQueryCpusEntry **entries,
                          size_t *nentries,
-                         bool force)
+                         bool force,
+                         bool fast)
 {
     int ret = -1;
-    virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL);
+    virJSONValuePtr cmd;
     virJSONValuePtr reply = NULL;
     virJSONValuePtr data;
 
+    if (fast)
+        cmd = qemuMonitorJSONMakeCommand("query-cpus-fast", NULL);
+    else
+        cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL);
+
     if (!cmd)
         return -1;
 
@@ -1553,7 +1570,7 @@ qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
         goto cleanup;
     }
 
-    ret = qemuMonitorJSONExtractCPUInfo(data, entries, nentries);
+    ret = qemuMonitorJSONExtractCPUInfo(data, entries, nentries, fast);
 
  cleanup:
     virJSONValueFree(cmd);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index ec243be..e03299a 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -60,7 +60,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);
 int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
                              struct qemuMonitorQueryCpusEntry **entries,
                              size_t *nentries,
-                             bool force);
+                             bool force,
+                             bool fast);
 int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
                                virDomainVirtType *virtType);
 int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 908ec3a..2e685ce 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1420,7 +1420,7 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
         goto cleanup;
 
     if (qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test),
-                                 &cpudata, &ncpudata, true) < 0)
+                                 &cpudata, &ncpudata, true, false) < 0)
         goto cleanup;
 
     if (ncpudata != 4) {
@@ -2716,7 +2716,7 @@ testQemuMonitorCPUInfo(const void *opaque)
         goto cleanup;
 
     rc = qemuMonitorGetCPUInfo(qemuMonitorTestGetMonitor(test),
-                               &vcpus, data->maxvcpus, true);
+                               &vcpus, data->maxvcpus, true, false);
 
     if (rc < 0)
         goto cleanup;
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 26.03.2018 10:12, Viktor Mihajlovski wrote:
> 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.
Well, I have to take that back as I ignored that QEMU is providing the
props since 2.10 with query-cpus (and now with query-cpus-fast). If we
decide to parse the props, we could do it with the pre-existing
qemuMonitorGetCPUInfo function, but I would prefer this to be done in
separate patch.
[...]

-- 
Regards,
 Viktor Mihajlovski

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/26/2018 07:28 AM, Viktor Mihajlovski wrote:
> On 26.03.2018 10:12, Viktor Mihajlovski wrote:
>> 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.
> Well, I have to take that back as I ignored that QEMU is providing the
> props since 2.10 with query-cpus (and now with query-cpus-fast). If we
> decide to parse the props, we could do it with the pre-existing
> qemuMonitorGetCPUInfo function, but I would prefer this to be done in
> separate patch.
> [...]
> 

I didn't even look at the query-cpus and @props - I was just considering
the -fast code.  And yes, looking at @props for query-cpus would have to
be a separate patch with it's own capability - if of course it was even
deemed worthwhile to do...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list