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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 9 Feb 2018 08:56:19 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> 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.