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

From: Luiz Capitulino <lcapitulino@redhat.com>

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

 o Drop field "halted" since it can't be provided fast reliably
   and is too volatile on most architectures to be really useful

 o Rename some fields for better clarification & proper naming
   standard

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 cpus.c               | 38 ++++++++++++++++++++++++++++
 hmp-commands-info.hx | 14 +++++++++++
 hmp.c                | 21 ++++++++++++++++
 hmp.h                |  1 +
 qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+)

On Mon, 12 Feb 2018 13:14:31 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> 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" and "arch"
> 
>  o Drop field "halted" since it can't be provided fast reliably
>    and is too volatile on most architectures to be really useful
> 
>  o Rename some fields for better clarification & proper naming
>    standard
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  cpus.c               | 38 ++++++++++++++++++++++++++++
>  hmp-commands-info.hx | 14 +++++++++++
>  hmp.c                | 21 ++++++++++++++++
>  hmp.h                |  1 +
>  qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 144 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

* Viktor Mihajlovski (mihajlov@linux.vnet.ibm.com) wrote:
> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> 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" and "arch"
> 
>  o Drop field "halted" since it can't be provided fast reliably
>    and is too volatile on most architectures to be really useful
> 
>  o Rename some fields for better clarification & proper naming
>    standard
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  cpus.c               | 38 ++++++++++++++++++++++++++++
>  hmp-commands-info.hx | 14 +++++++++++
>  hmp.c                | 21 ++++++++++++++++
>  hmp.h                |  1 +
>  qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 144 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..598bfe5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -410,6 +410,27 @@ 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;
> +    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);
> +        monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
> +        monitor_printf(mon, "\n");
> +    }
> +
> +    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);

For HMP:

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> diff --git a/qapi-schema.json b/qapi-schema.json
> index 66e0927..2a614af 100644
> --- a/qapi-schema.json
> +++ 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
> +# 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' } }
> +
> +##
> +# @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
> +#
> +# Example:
> +#
> +# -> { "execute": "query-cpus-fast" }
> +# <- { "return": [
> +#         {
> +#             "thread-id": 25627,
> +#             "props": {
> +#                 "core-id": 0,
> +#                 "thread-id": 0,
> +#                 "socket-id": 0
> +#             },
> +#             "qom-path": "/machine/unattached/device[0]",
> +#             "cpu-index": 0
> +#         },
> +#         {
> +#             "thread-id": 25628,
> +#             "props": {
> +#                 "core-id": 0,
> +#                 "thread-id": 0,
> +#                 "socket-id": 1
> +#             },
> +#             "qom-path": "/machine/unattached/device[2]",
> +#             "cpu-index": 1
> +#         }
> +#     ]
> +# }
> +##
> +{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] }
> +
> +##
>  # @IOThreadInfo:
>  #
>  # Information about an iothread
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 12.02.2018 13:14, Viktor Mihajlovski wrote:
> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> 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)
> 

If I'm not wrong, this comment is superseded by ...

>  o Drop irrelevant fields such as "current", "pc" and "arch"
> 
>  o Drop field "halted" since it can't be provided fast reliably
>    and is too volatile on most architectures to be really useful
> 

this comment :)

>  o Rename some fields for better clarification & proper naming
>    standard>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

Wondering if we could tweak the old interface with a simple flag "fast =
true".

On 12.02.2018 17:50, Dr. David Alan Gilbert wrote:
[...]
>> diff --git a/hmp.c b/hmp.c
>> index a6b94b7..598bfe5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -410,6 +410,27 @@ 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;
>> +    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);
>> +        monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
>> +        monitor_printf(mon, "\n");
>> +    }
I started some prototyping in libvirt and stumbled over the changed
layout of "info cpus_fast" vs. "info cpus". IMHO it would be better to
stick with the original format (one line per CPU). I'll post a v2.
>> +
>> +    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);
> 
> For HMP:
> 
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
[...]