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

[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