Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-09 Thread Viktor Mihajlovski
On 08.02.2018 22:41, Eduardo Habkost wrote:
> On Thu, Feb 08, 2018 at 02:59:17PM -0600, Eric Blake wrote:
>> On 02/08/2018 01:59 PM, Eduardo Habkost wrote:
>>> On Wed, Feb 07, 2018 at 12:50:13PM -0500, Luiz Capitulino wrote:
 The query-cpus command has an extremely serious side effect:
 it always interrupt 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:

>>
 +# Notes: @halted is a transient state that changes frequently.  By the 
 time the
 +#data is sent to the client, the guest may no longer be halted.
 +##
 +{ 'struct': 'CpuInfo2',
 +  'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
 +   'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>>>
>>> This will require duplicating struct fields every time we add a
>>> new field to query-cpus-fast (e.g. how would VIktor's
>>> CpuInfoS390State patch[1] look like if rebased on top of yours?).
>>>
>>> One way to avoid that is to use CpuInfo for both, and make all
>>> "slow"  fields optional.  Another option is to use QAPI
>>> inheritance, but it could be a little complicated if unions are
>>> involved?
>>
>> Inheritance is better than optional fields for the sake of introspection
>> learning which fields to expect.
>>
>> Put the common fields to both interfaces in the base class, then have the
>> slower (older) CpuInfo class extend the base class to add the additional
>> fields.
>>
>> Unions should be able to inherit just fine from structs (after all, a flat
>> union requires a struct base); but if we need two layers of unions, we'll
>> need to enhance QAPI code generation first.
> 
> If we can't do union-union inheritance yet, maybe we can work around it this
> way:
> 
> # fields that are always returned by both query-cpus and query-cpus-fast
> { 'struct': 'BothCpuInfoBase',
>   'data': {'cpu': 'int', 'qom_path': 'str', 'thread_id': 'int',
>'*props': 'CpuInstanceProperties' } }
> 
> # fields that are always returned by query-cpus
> { 'struct': 'CpuInfoBase',
>   'base': 'BothCpuInfoBase',
>   'data': {'current': 'bool', 'halted': 'bool', 'arch': 'CpuInfoArch' } }
> 
> # query-cpus return value
> { 'union': 'CpuInfo',
>   'base': 'CpuInfoBase',
>   'discriminator': 'arch',
>   'data': { 'x86': 'CpuInfoX86',
> 's390': 'CpuInfoS390',
> 'sparc': 'CpuInfoSPARC',
> 'ppc': 'CpuInfoPPC',
> 'mips': 'CpuInfoMIPS',
> 'tricore': 'CpuInfoTricore',
> 'other': 'CpuInfoOther' } }
> 
> # fields that are always returned by query-cpus-fast
> { 'struct': 'FastCpuInfoBase',
>   'base': 'BothCpuInfoBase',
>   'data': { 'arch': 'CpuInfoArch' } }
> 
> # return value of query-cpus-fast
> { 'union': 'FastCpuInfo',
>   'base': 'FastCpuInfoBase',
>   'discriminator': 'arch',
>   'data': { 'x86': 'CpuInfoOther',
> 's390': 'FastCpuInfoS390',
> 'sparc': 'CpuInfoOther',
> 'ppc': 'CpuInfoOther',
> 'mips': 'CpuInfoOther',
> 'tricore': 'CpuInfoOther',
> 'other': 'CpuInfoOther' } }
> 
> # fields returned by both query-cpus and query-cpus-fast on s390
> { 'struct': 'FastCpuInfoS390',
>   'data': { 'fast_field': 'int' } }
> 
> # fields returned by query-cpus on s390
> { 'struct': 'CpuInfoS390',
>   'base': 'FastCpuInfoS390',
>   'data': { 'slow_field': 'int' } }
> 
This is not exactly pretty, but would provide the functionality (and
flexibility) I'd expect.

-- 
Regards,
 Viktor Mihajlovski




Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-08 Thread Eduardo Habkost
On Thu, Feb 08, 2018 at 02:59:17PM -0600, Eric Blake wrote:
> On 02/08/2018 01:59 PM, Eduardo Habkost wrote:
> > On Wed, Feb 07, 2018 at 12:50:13PM -0500, Luiz Capitulino wrote:
> > > The query-cpus command has an extremely serious side effect:
> > > it always interrupt 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:
> > > 
> 
> > > +# Notes: @halted is a transient state that changes frequently.  By the 
> > > time the
> > > +#data is sent to the client, the guest may no longer be halted.
> > > +##
> > > +{ 'struct': 'CpuInfo2',
> > > +  'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
> > > +   'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> > 
> > This will require duplicating struct fields every time we add a
> > new field to query-cpus-fast (e.g. how would VIktor's
> > CpuInfoS390State patch[1] look like if rebased on top of yours?).
> > 
> > One way to avoid that is to use CpuInfo for both, and make all
> > "slow"  fields optional.  Another option is to use QAPI
> > inheritance, but it could be a little complicated if unions are
> > involved?
> 
> Inheritance is better than optional fields for the sake of introspection
> learning which fields to expect.
> 
> Put the common fields to both interfaces in the base class, then have the
> slower (older) CpuInfo class extend the base class to add the additional
> fields.
> 
> Unions should be able to inherit just fine from structs (after all, a flat
> union requires a struct base); but if we need two layers of unions, we'll
> need to enhance QAPI code generation first.

If we can't do union-union inheritance yet, maybe we can work around it this
way:

# fields that are always returned by both query-cpus and query-cpus-fast
{ 'struct': 'BothCpuInfoBase',
  'data': {'cpu': 'int', 'qom_path': 'str', 'thread_id': 'int',
   '*props': 'CpuInstanceProperties' } }

# fields that are always returned by query-cpus
{ 'struct': 'CpuInfoBase',
  'base': 'BothCpuInfoBase',
  'data': {'current': 'bool', 'halted': 'bool', 'arch': 'CpuInfoArch' } }

# query-cpus return value
{ 'union': 'CpuInfo',
  'base': 'CpuInfoBase',
  'discriminator': 'arch',
  'data': { 'x86': 'CpuInfoX86',
's390': 'CpuInfoS390',
'sparc': 'CpuInfoSPARC',
'ppc': 'CpuInfoPPC',
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
'other': 'CpuInfoOther' } }

# fields that are always returned by query-cpus-fast
{ 'struct': 'FastCpuInfoBase',
  'base': 'BothCpuInfoBase',
  'data': { 'arch': 'CpuInfoArch' } }

# return value of query-cpus-fast
{ 'union': 'FastCpuInfo',
  'base': 'FastCpuInfoBase',
  'discriminator': 'arch',
  'data': { 'x86': 'CpuInfoOther',
's390': 'FastCpuInfoS390',
'sparc': 'CpuInfoOther',
'ppc': 'CpuInfoOther',
'mips': 'CpuInfoOther',
'tricore': 'CpuInfoOther',
'other': 'CpuInfoOther' } }

# fields returned by both query-cpus and query-cpus-fast on s390
{ 'struct': 'FastCpuInfoS390',
  'data': { 'fast_field': 'int' } }

# fields returned by query-cpus on s390
{ 'struct': 'CpuInfoS390',
  'base': 'FastCpuInfoS390',
  'data': { 'slow_field': 'int' } }

-- 
Eduardo



Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-08 Thread Eric Blake

On 02/08/2018 01:59 PM, Eduardo Habkost wrote:

On Wed, Feb 07, 2018 at 12:50:13PM -0500, Luiz Capitulino wrote:

The query-cpus command has an extremely serious side effect:
it always interrupt 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:




+# Notes: @halted is a transient state that changes frequently.  By the time the
+#data is sent to the client, the guest may no longer be halted.
+##
+{ 'struct': 'CpuInfo2',
+  'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
+   'thread-id': 'int', '*props': 'CpuInstanceProperties' } }


This will require duplicating struct fields every time we add a
new field to query-cpus-fast (e.g. how would VIktor's
CpuInfoS390State patch[1] look like if rebased on top of yours?).

One way to avoid that is to use CpuInfo for both, and make all
"slow"  fields optional.  Another option is to use QAPI
inheritance, but it could be a little complicated if unions are
involved?


Inheritance is better than optional fields for the sake of introspection 
learning which fields to expect.


Put the common fields to both interfaces in the base class, then have 
the slower (older) CpuInfo class extend the base class to add the 
additional fields.


Unions should be able to inherit just fine from structs (after all, a 
flat union requires a struct base); but if we need two layers of unions, 
we'll need to enhance QAPI code generation first.


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



Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-08 Thread Eduardo Habkost
On Wed, Feb 07, 2018 at 12:50:13PM -0500, Luiz Capitulino wrote:
> The query-cpus command has an extremely serious side effect:
> it always interrupt 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" and "arch"
> 
>  o Rename some fields for better clarification & proper naming
>standard
> 
> The "halted" field is somewhat controversial. On the one hand,
> it offers a convenient way to know if a guest CPU is idle or
> running. On the other hand, it's a field that can change many
> times a second. In fact, the halted state can change even
> before query-cpus-fast has returned. This makes one wonder if
> this field should be dropped all together. Having the "halted"
> field as optional gives a better option for dropping it in
> the future, since we can just stop returning it.
> 
> Signed-off-by: Luiz Capitulino 
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745c79..82d6f12b53 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -558,6 +558,77 @@
>  ##
>  { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
>  
> +##
> +# @CpuInfo2:
> +#
> +# Information about a virtual CPU
> +#
> +# @cpu-index: index of the virtual CPU
> +#
> +# @halted: true if the virtual CPU is in the halt state.  Halt usually refers
> +#  to a processor specific low power mode. This field is optional,
> +#  it is only present if the halted state can be retrieved without
> +#  a performance penalty
> +#
> +# @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
> +#
> +# Notes: @halted is a transient state that changes frequently.  By the time 
> the
> +#data is sent to the client, the guest may no longer be halted.
> +##
> +{ 'struct': 'CpuInfo2',
> +  'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
> +   'thread-id': 'int', '*props': 'CpuInstanceProperties' } }

This will require duplicating struct fields every time we add a
new field to query-cpus-fast (e.g. how would VIktor's
CpuInfoS390State patch[1] look like if rebased on top of yours?).

One way to avoid that is to use CpuInfo for both, and make all
"slow"  fields optional.  Another option is to use QAPI
inheritance, but it could be a little complicated if unions are
involved?

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html

-- 
Eduardo



Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-08 Thread Luiz Capitulino
On Thu, 8 Feb 2018 08:41:31 +0100
Viktor Mihajlovski  wrote:

> On 07.02.2018 18:50, Luiz Capitulino wrote:
> > The query-cpus command has an extremely serious side effect:
> > it always interrupt 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" and "arch"  
> I disagree that arch is irrelevant and would strongly suggest to keep
> arch and arch-specific fields. At least in the case of s390 there's a
> cpu_state field that can be obtained cheaply.

The arch name can be queried with query-target. The only other
arch field I'm dropping is pc, which should be considered debug
only if anything.

Also, if this need to query CPU registers increase, then we
probably should port 'info registers' to QMP. Otherwise, we'll
eventually run into the performance problem once again.

> [...]
> > diff --git a/cpus.c b/cpus.c
> > index 2cb0af9b22..3b68a8146c 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -2083,6 +2083,50 @@ CpuInfoList *qmp_query_cpus(Error **errp)
> >  return head;
> >  }
> > 
> > +/*
> > + * fast means: we NEVER interrupt vCPU threads to retrieve
> > + * information from KVM.
> > + */
> > +CpuInfo2List *qmp_query_cpus_fast(Error **errp)
> > +{
> > +MachineState *ms = MACHINE(qdev_get_machine());
> > +MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +CpuInfo2List *head = NULL, *cur_item = NULL;
> > +CPUState *cpu;
> > +
> > +CPU_FOREACH(cpu) {
> > +CpuInfo2List *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 in kernel irqchip is used, we don't have 'halted' */
> > +info->value->has_halted = !kvm_irqchip_in_kernel();  
> This is definitely not true for s390. Externally observable CPU state
> changes are handled by QEMU there. We may still drop halted if we add a
> more appropriate arch-specific field.
> > +if (info->value->has_halted) {
> > +info->value->halted = cpu->halted;
> > +}  
> [...]
> 




Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-08 Thread Viktor Mihajlovski
On 08.02.2018 08:41, Viktor Mihajlovski wrote:
> On 07.02.2018 18:50, Luiz Capitulino wrote:
>> The query-cpus command has an extremely serious side effect:
>> it always interrupt 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" and "arch"
> I disagree that arch is irrelevant and would strongly suggest to keep
> arch and arch-specific fields. At least in the case of s390 there's a
> cpu_state field that can be obtained cheaply.
I've posted a patch [1] to add s390-specific state info to the
query-cpus output. This state *can* be obtained without kicking the CPU
out of VM execution. With this info in the query-cpus-fast return data
we can eventually get rid of halted and its ramifications.
[...]

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html
-- 
Regards,
 Viktor Mihajlovski




Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-07 Thread Viktor Mihajlovski
On 07.02.2018 18:50, Luiz Capitulino wrote:
> The query-cpus command has an extremely serious side effect:
> it always interrupt 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" and "arch"
I disagree that arch is irrelevant and would strongly suggest to keep
arch and arch-specific fields. At least in the case of s390 there's a
cpu_state field that can be obtained cheaply.
[...]
> diff --git a/cpus.c b/cpus.c
> index 2cb0af9b22..3b68a8146c 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2083,6 +2083,50 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  return head;
>  }
> 
> +/*
> + * fast means: we NEVER interrupt vCPU threads to retrieve
> + * information from KVM.
> + */
> +CpuInfo2List *qmp_query_cpus_fast(Error **errp)
> +{
> +MachineState *ms = MACHINE(qdev_get_machine());
> +MachineClass *mc = MACHINE_GET_CLASS(ms);
> +CpuInfo2List *head = NULL, *cur_item = NULL;
> +CPUState *cpu;
> +
> +CPU_FOREACH(cpu) {
> +CpuInfo2List *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 in kernel irqchip is used, we don't have 'halted' */
> +info->value->has_halted = !kvm_irqchip_in_kernel();
This is definitely not true for s390. Externally observable CPU state
changes are handled by QEMU there. We may still drop halted if we add a
more appropriate arch-specific field.
> +if (info->value->has_halted) {
> +info->value->halted = cpu->halted;
> +}
[...]

-- 
Regards,
 Viktor Mihajlovski




Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-07 Thread Eric Blake

On 02/07/2018 11:50 AM, Luiz Capitulino wrote:

The query-cpus command has an extremely serious side effect:
it always interrupt all running vCPUs so that they can run


s/interrupt/interrupts/


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" and "arch"

  o Rename some fields for better clarification & proper naming
standard

The "halted" field is somewhat controversial. On the one hand,
it offers a convenient way to know if a guest CPU is idle or
running. On the other hand, it's a field that can change many
times a second. In fact, the halted state can change even
before query-cpus-fast has returned. This makes one wonder if
this field should be dropped all together. Having the "halted"
field as optional gives a better option for dropping it in
the future, since we can just stop returning it.


Makes sense to me.



Signed-off-by: Luiz Capitulino 
---
  cpus.c   | 44 
  hmp-commands-info.hx | 14 +++
  hmp.c| 24 ++
  hmp.h|  1 +
  qapi-schema.json | 71 
  5 files changed, 154 insertions(+)




+++ b/hmp-commands-info.hx
@@ -157,6 +157,20 @@ STEXI
  @item info cpus
  @findex info cpus
  Show infos for each CPU.


Pre-existing, but...


+ETEXI
+
+{
+.name   = "cpus_fast",
+.args_type  = "",
+.params = "",
+.help   = "show infos for each CPU without performance penalty",


...copied here.  s/infos/information/ in public-facing documentation, 
while we're touching it.




+++ b/qapi-schema.json
@@ -558,6 +558,77 @@
  ##
  { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
  
+##

+# @CpuInfo2:


Good thing that type names aren't part of our introspection ABI.  Would 
CpuInfoLite or CpuInfoFast make the code any more legible?



+#
+# Information about a virtual CPU
+#
+# @cpu-index: index of the virtual CPU
+#
+# @halted: true if the virtual CPU is in the halt state.  Halt usually refers
+#  to a processor specific low power mode. This field is optional,
+#  it is only present if the halted state can be retrieved without
+#  a performance penalty


Disclaimer is good.


+#
+# @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
+#
+# Notes: @halted is a transient state that changes frequently.  By the time the
+#data is sent to the client, the guest may no longer be halted.
+##
+{ 'struct': 'CpuInfo2',
+  'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
+   'thread-id': 'int', '*props': 'CpuInstanceProperties' } }


Looks reasonable.


+
+##
+# @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 @CpuInfo2
+#
+# Notes: The CPU architecture name is not returned by query-cpus-fast.
+#Use query-target to retreive that information.


s/retreive/retrieve/



+##
+{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfo2' ] }
+


Also, please add some docs to 'query-cpus' pointing to 'query-cpus-fast'.

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



[Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

2018-02-07 Thread Luiz Capitulino
The query-cpus command has an extremely serious side effect:
it always interrupt 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" and "arch"

 o Rename some fields for better clarification & proper naming
   standard

The "halted" field is somewhat controversial. On the one hand,
it offers a convenient way to know if a guest CPU is idle or
running. On the other hand, it's a field that can change many
times a second. In fact, the halted state can change even
before query-cpus-fast has returned. This makes one wonder if
this field should be dropped all together. Having the "halted"
field as optional gives a better option for dropping it in
the future, since we can just stop returning it.

Signed-off-by: Luiz Capitulino 
---
 cpus.c   | 44 
 hmp-commands-info.hx | 14 +++
 hmp.c| 24 ++
 hmp.h|  1 +
 qapi-schema.json | 71 
 5 files changed, 154 insertions(+)

diff --git a/cpus.c b/cpus.c
index 2cb0af9b22..3b68a8146c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2083,6 +2083,50 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 return head;
 }
 
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfo2List *qmp_query_cpus_fast(Error **errp)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+CpuInfo2List *head = NULL, *cur_item = NULL;
+CPUState *cpu;
+
+CPU_FOREACH(cpu) {
+CpuInfo2List *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 in kernel irqchip is used, we don't have 'halted' */
+info->value->has_halted = !kvm_irqchip_in_kernel();
+if (info->value->has_halted) {
+info->value->halted = cpu->halted;
+}
+
+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 ad590a4ffb..8657ceceb0 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -157,6 +157,20 @@ STEXI
 @item info cpus
 @findex info cpus
 Show infos for each CPU.
+ETEXI
+
+{
+.name   = "cpus_fast",
+.args_type  = "",
+.params = "",
+.help   = "show infos for each CPU without performance penalty",
+.cmd= hmp_info_cpus_fast,
+},
+
+STEXI
+@item info cpus_fast
+@findex info cpus_fast
+Show infos for each CPU without performance penalty.
 ETEXI
 
 {
diff --git a/hmp.c b/hmp.c
index b3de32d219..3d32333fd2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -404,6 +404,30 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
 qapi_free_CpuInfoList(cpu_list);
 }
 
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
+{
+CpuInfo2List *head, *cpu;
+TargetInfo *target;
+
+target = qmp_query_target(NULL);
+monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch);
+qapi_free_TargetInfo(target);
+
+head = qmp_query_cpus_fast(NULL);
+
+for (cpu = head; cpu; cpu = cpu->next) {
+monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
+monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+if (cpu->value->has_halted) {
+monitor_printf(mon, " halted=%d\n", cpu->value->halted);
+}
+monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
+monitor_printf(mon, "\n");
+}
+
+qapi_free_CpuInfo2List(head);
+}
+
 static void print_bl