Re: [Qemu-devel] [PATCHv2 2/3] qmp: add query-cpus-fast

[Eric Blake]

On 02/13/2018 11:18 AM, Viktor Mihajlovski wrote:

From: Luiz Capitulino 

The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.

This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:

  o Never interrupt vCPUs threads. query-cpus-fast only returns
vCPU information maintained by QEMU itself, which should be
sufficient for most management software needs

  o Make "halted" field optional: we only return it if the
halted state is maintained by QEMU. But this also gives
the option of dropping the field in the future (see below)

  o Drop irrelevant fields such as "current", "pc", "arch"
and "halted"



Others have pointed out the commit message inconsistencies; I'm focusing 
on the QMP.



+++ b/qapi-schema.json
@@ -552,6 +552,12 @@
  #
  # Returns a list of information about each virtual CPU.
  #
+# This command causes vCPU threads to exit to userspace, which causes
+# an small interruption guest CPU execution. This will have a negative


s/an small interruption/a small interruption to/


+# impact on realtime guests and other latency sensitive guest workloads.
+# It is recommended to use @query-cpus-fast instead of this command to
+# avoid the vCPU interruption.
+#
  # Returns: a list of @CpuInfo for each virtual CPU
  #
  # Since: 0.14.0
@@ -585,6 +591,70 @@
  { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
  
  ##

+# @CpuInfoFast:
+#
+# Information about a virtual CPU
+#
+# @cpu-index: index of the virtual CPU
+#
+# @qom-path: path to the CPU object in the QOM tree
+#
+# @thread-id: ID of the underlying host thread
+#
+# @props: properties describing to which node/socket/core/thread
+# virtual CPU belongs to, provided if supported by board
+#
+# Since: 2.12
+#
+##
+{ 'struct': 'CpuInfoFast',
+  'data': {'cpu-index': 'int', 'qom-path': 'str',
+   'thread-id': 'int', '*props': 'CpuInstanceProperties' } }


Comparing against the existing CpuInfo: if I'm not mistaken, you've renamed:

CPU -> cpu-index
qom_path -> qom-path
thread_id -> thread-id

and kept props unchanged.

If we REALLY cared about reducing duplication, we can do:

{ 'struct': 'CpuInfoFast',
  'data': { 'CPU': 'int', 'qom_path': 'str', 'thread_id': 'int',
'*props': 'CpuInstanceProperties' } }
{ 'struct': 'CpuInfoBase',
  'base': 'CpuInfoFast',
  'data': { 'current': 'bool', 'halted': 'bool',
'arch': 'CpuInfoArch' } }
{ 'union': 'CpuInfo', 'base': 'CpuInfoBase', 'discriminator': 'arch',
  'data': { ... } }

but I'm not yet convinced whether we need that.  If we're going to 
deprecate the old command, having the new command use modern spelling is 
a nice change, even though it costs some duplication in the qapi file; 
where sharing code may save qapi costs in the short run, but will burden 
us with poor naming down the road even when we delete the slow command.



+
+##
+# @query-cpus-fast:
+#
+# Returns information about all virtual CPUs. This command does not
+# incur a performance penalty and should be used in production
+# instead of query-cpus.
+#
+# Returns: list of @CpuInfoFast
+#
+# Notes: The CPU architecture name is not returned by query-cpus-fast.
+#Use query-target to retrieve that information.
+#
+# Since: 2.12



+{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] }
+
+##
  # @IOThreadInfo:
  #
  # Information about an iothread



I didn't inspect the code, so similar to 1/3, once you fix the grammar 
nit and commit message,


Acked-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCHv2 2/3] qmp: add query-cpus-fast

[Cornelia Huck]

On Tue, 13 Feb 2018 18:18:47 +0100
Viktor Mihajlovski  wrote:

> From: Luiz Capitulino 
> 
> The query-cpus command has an extremely serious side effect:
> it always interrupts all running vCPUs so that they can run
> ioctl calls. This can cause a huge performance degradation for
> some workloads. And most of the information retrieved by the
> ioctl calls are not even used by query-cpus.
> 
> This commit introduces a replacement for query-cpus called
> query-cpus-fast, which has the following features:
> 
>  o Never interrupt vCPUs threads. query-cpus-fast only returns
>vCPU information maintained by QEMU itself, which should be
>sufficient for most management software needs
> 
>  o Make "halted" field optional: we only return it if the
>halted state is maintained by QEMU. But this also gives
>the option of dropping the field in the future (see below)
> 
>  o Drop irrelevant fields such as "current", "pc", "arch"
>and "halted"

I'd suggest updating this description, as "halted" is now gone
completely... what about:

o Drop the "halted" field, even if the halted state is maintained by
QEMU. It had unclear semantics anyway.

o Drop irrelevant fields such as "current", "pc", or "arch"

> 
>  o Rename some fields for better clarification & proper naming
>standard
> 
> Signed-off-by: Luiz Capitulino 
> Signed-off-by: Viktor Mihajlovski 
> ---
>  cpus.c   | 38 
>  hmp-commands-info.hx | 14 +++
>  hmp.c| 14 +++
>  hmp.h|  1 +
>  qapi-schema.json | 70 
> 
>  5 files changed, 137 insertions(+)

Otherwise, looks good to me.

Reviewed-by: Cornelia Huck 

[Qemu-devel] [PATCHv2 2/3] qmp: add query-cpus-fast

[Viktor Mihajlovski]

From: Luiz Capitulino 

The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.

This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:

 o Never interrupt vCPUs threads. query-cpus-fast only returns
   vCPU information maintained by QEMU itself, which should be
   sufficient for most management software needs

 o Make "halted" field optional: we only return it if the
   halted state is maintained by QEMU. But this also gives
   the option of dropping the field in the future (see below)

 o Drop irrelevant fields such as "current", "pc", "arch"
   and "halted"

 o Rename some fields for better clarification & proper naming
   standard

Signed-off-by: Luiz Capitulino 
Signed-off-by: Viktor Mihajlovski 
---
 cpus.c   | 38 
 hmp-commands-info.hx | 14 +++
 hmp.c| 14 +++
 hmp.h|  1 +
 qapi-schema.json | 70 
 5 files changed, 137 insertions(+)

diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 return head;
 }
 
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfoFastList *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfoFastList *info = g_malloc0(sizeof(*info));
+info->value = g_malloc0(sizeof(*info->value));
+
+info->value->cpu_index = cpu->cpu_index;
+info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+info->value->thread_id = cpu->thread_id;
+
+info->value->has_props = !!mc->cpu_index_to_instance_props;
+if (info->value->has_props) {
+CpuInstanceProperties *props;
+props = g_malloc0(sizeof(*props));
+*props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+info->value->props = props;
+}
+
+if (!cur_item) {
+head = cur_item = info;
+} else {
+cur_item->next = info;
+cur_item = info;
+}
+}
+
+return head;
+}
+
 void qmp_memsave(int64_t addr, int64_t size, const char *filename,
  bool has_cpu, int64_t cpu_index, Error **errp)
 {
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad590a4..16ac602 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -160,6 +160,20 @@ Show infos for each CPU.
 ETEXI
 
 {
+.name   = "cpus_fast",
+.args_type  = "",
+.params = "",
+.help   = "show information for each CPU without interrupting 
them",
+.cmd= hmp_info_cpus_fast,
+},
+
+STEXI
+@item info cpus_fast
+@findex info cpus_fast
+Show infos for each CPU without performance penalty.
+ETEXI
+
+{
 .name   = "history",
 .args_type  = "",
 .params = "",
diff --git a/hmp.c b/hmp.c
index a6b94b7..0bd3b3a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -410,6 +410,20 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
 qapi_free_CpuInfoList(cpu_list);
 }
 
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
+{
+CpuInfoFastList *head, *cpu;
+
+head = qmp_query_cpus_fast(NULL);
+
+for (cpu = head; cpu; cpu = cpu->next) {
+monitor_printf(mon, "  CPU #%" PRId64 ":", cpu->value->cpu_index);
+monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+}
+
+qapi_free_CpuInfoFastList(head);
+}
+
 static void print_block_info(Monitor *mon, BlockInfo *info,
  BlockDeviceInfo *inserted, bool verbose)
 {
diff --git a/hmp.h b/hmp.h
index 1143db4..93fb4e4 100644
--- a/hmp.h
+++ b/hmp.h
@@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict 
*qdict);
 void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
 void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
 void hmp_info_cpus(Monitor *mon, const QDict *qdict);
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
 void hmp_info_block(Monitor *mon, const QDict *qdict);
 void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
 void hmp_info_vnc(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index 94d560e..dc07729 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -552,6 +552,12 @@
 #
 # Returns a list of information about each virtual CPU.
 #