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

[Eduardo Habkost <ehabkost@redhat.com>]

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 <lcapitulino@redhat.com>
[...]
> 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