Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Luiz Capitulino]

On Fri, 9 Feb 2018 15:50:00 +0100
Viktor Mihajlovski  wrote:

> On 09.02.2018 15:27, Eduardo Habkost wrote:
> [...]
> >> I'm keeping it mainly for s390. Viktor, libvirt is still using
> >> this field in s390, no?
> >>
> >> Dropping halted and having management software still using query-cpus
> >> because of halted would be a total failure of query-cpus-fast.  
> > 
> > If I understood correctly, the CpuInfoS390::cpu_state field added
> > by Viktor in another patch[1] would replace "halted" for the s390
> > case.  
> Right, CPUState.halted is derived from CPUS390XState.cpu_state on s390:
> A cpu_state of CPU_STATE_STOPPED or CPU_STATE_CHECK_STOPPED results in
> halted = true. This derivation can be done by libvirt for s390.
> > 
> > I'm assuming QEMU will be able to return that field without
> > interrupting the VCPUs.  Viktor, is that correct?
> > 
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html
> >   
> That's correct.
> >>  
>  Also, the code that sets/clears cpu->halted is target-specific,
>  so I wouldn't be so sure that simply checking for
>  !kvm_irqchip_in_kernel() is enough on all targets.  
> >>
> >> I checked the code and had the impression it was enough, but
> >> I don't have experience with other archs. So, would be nice
> >> if other archs maintainers could review this. I'll try to ping them.  
> > 
> > I think we need to take a step back and rethink:
> > 
> > 1) What the field is supposed to mean?  The semantics of "halted"
> >are completely unclear.  What exactly we want to communicate
> >to libvirt/management?
> > 2) On which cases the information (whatever it means) is really
> >useful/important?  If you are excluding cases with in-kernel
> >irqchip, you are already excluding most users.
> > 
> >   
> Given that nobody (including myself) sees a need for halted we can
> remove it for the fast version of query-cpus without surprising anyone.
> [...]

My conclusion too, I'll drop it.

Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Viktor Mihajlovski]

On 09.02.2018 15:27, Eduardo Habkost wrote:
[...]
>> I'm keeping it mainly for s390. Viktor, libvirt is still using
>> this field in s390, no?
>>
>> Dropping halted and having management software still using query-cpus
>> because of halted would be a total failure of query-cpus-fast.
> 
> If I understood correctly, the CpuInfoS390::cpu_state field added
> by Viktor in another patch[1] would replace "halted" for the s390
> case.
Right, CPUState.halted is derived from CPUS390XState.cpu_state on s390:
A cpu_state of CPU_STATE_STOPPED or CPU_STATE_CHECK_STOPPED results in
halted = true. This derivation can be done by libvirt for s390.
> 
> I'm assuming QEMU will be able to return that field without
> interrupting the VCPUs.  Viktor, is that correct?
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html
> 
That's correct.
>>
 Also, the code that sets/clears cpu->halted is target-specific,
 so I wouldn't be so sure that simply checking for
 !kvm_irqchip_in_kernel() is enough on all targets.
>>
>> I checked the code and had the impression it was enough, but
>> I don't have experience with other archs. So, would be nice
>> if other archs maintainers could review this. I'll try to ping them.
> 
> I think we need to take a step back and rethink:
> 
> 1) What the field is supposed to mean?  The semantics of "halted"
>are completely unclear.  What exactly we want to communicate
>to libvirt/management?
> 2) On which cases the information (whatever it means) is really
>useful/important?  If you are excluding cases with in-kernel
>irqchip, you are already excluding most users.
> 
> 
Given that nobody (including myself) sees a need for halted we can
remove it for the fast version of query-cpus without surprising anyone.
[...]

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Eduardo Habkost]

On Fri, Feb 09, 2018 at 08:49:49AM -0500, Luiz Capitulino wrote:
> On Fri, 9 Feb 2018 08:56:19 +0100
> Viktor Mihajlovski  wrote:
> 
> > On 08.02.2018 21:33, Eduardo Habkost wrote:
> > > On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote:
> > > [...]  
> > >> 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.  
> > > 
> > > I'd just drop it, unless we find a use case where it's really
> > > useful.
> 
> I don't think there's any, unless for debugging purposes.
> 
> I'm keeping it mainly for s390. Viktor, libvirt is still using
> this field in s390, no?
> 
> Dropping halted and having management software still using query-cpus
> because of halted would be a total failure of query-cpus-fast.

If I understood correctly, the CpuInfoS390::cpu_state field added
by Viktor in another patch[1] would replace "halted" for the s390
case.

I'm assuming QEMU will be able to return that field without
interrupting the VCPUs.  Viktor, is that correct?

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

> 
> > > Also, the code that sets/clears cpu->halted is target-specific,
> > > so I wouldn't be so sure that simply checking for
> > > !kvm_irqchip_in_kernel() is enough on all targets.
> 
> I checked the code and had the impression it was enough, but
> I don't have experience with other archs. So, would be nice
> if other archs maintainers could review this. I'll try to ping them.

I think we need to take a step back and rethink:

1) What the field is supposed to mean?  The semantics of "halted"
   are completely unclear.  What exactly we want to communicate
   to libvirt/management?
2) On which cases the information (whatever it means) is really
   useful/important?  If you are excluding cases with in-kernel
   irqchip, you are already excluding most users.


> 
> > Right, the present patch effectively disables halted anyway (including
> > s390). 
> 
> No, it doesn't. It only disables halted for archs that require going
> to the kernel to get it.

It disables it for all architectures that implement in-kernel
irqchip: x86, arm, s390.

The only existing user of "halted" is s390-specific code in
libvirt, and your patch won't return it on s390, so nobody seems
to benefit from it.


> 
> > So it may be cleaner to just drop it right now.
> > Assuming the presence of architecure-specific data, libvirt can derive a
> > halted state (or an equivalent thereof) from query-cpus-fast returned
> > information.
> 
> This is a different proposal. You're proposing moving the halted state
> to a CPU-specific field. This is doable if that's what we want.

I think it's a valid approach to return a target-specific field
first, and later try to come up with a generic (and clearly
defined) abstraction to represent the same information (either
inside QEMU or inside libvirt).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Viktor Mihajlovski]

On 09.02.2018 14:49, Luiz Capitulino wrote:
> On Fri, 9 Feb 2018 08:56:19 +0100
> Viktor Mihajlovski  wrote:
> 
>> On 08.02.2018 21:33, Eduardo Habkost wrote:
>>> On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote:
>>> [...]  
 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.  
>>>
>>> I'd just drop it, unless we find a use case where it's really
>>> useful.
> 
> I don't think there's any, unless for debugging purposes.
> 
> I'm keeping it mainly for s390. Viktor, libvirt is still using
> this field in s390, no?
see below
> 
> Dropping halted and having management software still using query-cpus
> because of halted would be a total failure of query-cpus-fast.
> 
>>> Also, the code that sets/clears cpu->halted is target-specific,
>>> so I wouldn't be so sure that simply checking for
>>> !kvm_irqchip_in_kernel() is enough on all targets.
> 
> I checked the code and had the impression it was enough, but
> I don't have experience with other archs. So, would be nice
> if other archs maintainers could review this. I'll try to ping them.
> 
>> Right, the present patch effectively disables halted anyway (including
>> s390). 
> 
> No, it doesn't. It only disables halted for archs that require going
> to the kernel to get it.>
>> So it may be cleaner to just drop it right now.
>> Assuming the presence of architecure-specific data, libvirt can derive a
>> halted state (or an equivalent thereof) from query-cpus-fast returned
>> information.
> 
> This is a different proposal. You're proposing moving the halted state
> to a CPU-specific field. This is doable if that's what we want.
> 
In order to use query-cpus-fast, libvirt has to change code anyway.
Since libvirt is only providing cpu.n.halted for s390, and this value
can be derived from the cpu-state, there's no need for QEMU to return
halted in query-cpus-fast any more.


-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Luiz Capitulino]

On Fri, 9 Feb 2018 08:56:19 +0100
Viktor Mihajlovski  wrote:

> On 08.02.2018 21:33, Eduardo Habkost wrote:
> > On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote:
> > [...]  
> >> 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.  
> > 
> > I'd just drop it, unless we find a use case where it's really
> > useful.

I don't think there's any, unless for debugging purposes.

I'm keeping it mainly for s390. Viktor, libvirt is still using
this field in s390, no?

Dropping halted and having management software still using query-cpus
because of halted would be a total failure of query-cpus-fast.

> > Also, the code that sets/clears cpu->halted is target-specific,
> > so I wouldn't be so sure that simply checking for
> > !kvm_irqchip_in_kernel() is enough on all targets.

I checked the code and had the impression it was enough, but
I don't have experience with other archs. So, would be nice
if other archs maintainers could review this. I'll try to ping them.

> Right, the present patch effectively disables halted anyway (including
> s390). 

No, it doesn't. It only disables halted for archs that require going
to the kernel to get it.

> So it may be cleaner to just drop it right now.
> Assuming the presence of architecure-specific data, libvirt can derive a
> halted state (or an equivalent thereof) from query-cpus-fast returned
> information.

This is a different proposal. You're proposing moving the halted state
to a CPU-specific field. This is doable if that's what we want.

Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Viktor Mihajlovski]

On 08.02.2018 21:33, Eduardo Habkost wrote:
> On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote:
> [...]
>> 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.
> 
> I'd just drop it, unless we find a use case where it's really
> useful.
> 
> Also, the code that sets/clears cpu->halted is target-specific,
> so I wouldn't be so sure that simply checking for
> !kvm_irqchip_in_kernel() is enough on all targets.
> 
Right, the present patch effectively disables halted anyway (including
s390). So it may be cleaner to just drop it right now.
Assuming the presence of architecure-specific data, libvirt can derive a
halted state (or an equivalent thereof) from query-cpus-fast returned
information.

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[Eduardo Habkost]

On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote:
[...]
> 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.

I'd just drop it, unless we find a use case where it's really
useful.

Also, the code that sets/clears cpu->halted is target-specific,
so I wouldn't be so sure that simply checking for
!kvm_irqchip_in_kernel() is enough on all targets.

-- 
Eduardo

[Qemu-devel] [PATCH v2] qmp: add query-cpus-fast

[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" 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 
---

o v2

 - Many English and naming corrections by Eric
 - Adopt query-cpus text from Daniel
 - squash query-cpus text change into this patch

 cpus.c   | 44 ++
 hmp-commands-info.hx | 14 ++
 hmp.c| 24 
 hmp.h|  1 +
 qapi-schema.json | 77 
 5 files changed, 160 insertions(+)

diff --git a/cpus.c b/cpus.c
index 182caf764e..905b1f4d79 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2150,6 +2150,50 @@ 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 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..16ac60285f 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 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
 
 {
diff --git a/hmp.c b/hmp.c
index b3de32d219..90717c13b2 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)
+{
+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);
+if (cpu->value->has_halted) {
+monitor_printf(mon, " halted=%d\n", cpu->value->halted);
+