Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Eduardo Habkost]

(CCing qemu-devel)

On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
> On Fri, 2 Feb 2018 14:19:38 +0000
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:
[...]
> > > It would be also interesting to update QEMU QMP documentation to
> > > clarify the arch-specific semantics of "halted".  
> > 
> > Any also especially clarify the awful performance implications of running
> > this particular query command. In general I would not expect query-xxx
> > monitor commands to interrupt all vcpus, so we should clearly warn about
> > this !
> 
> Or deprecate it...

We could deprecate the expensive fields on query-cpus, and move
them to a more expensive query-cpu-state command.  I believe most
users of query-cpus are only interested in qom_path, thread_id,
and topology info.

Markus, Eric: from the QAPI point of view, is it OK to remove
fields between QEMU versions, as long as we follow our
deprecation policy?

-- 
Eduardo

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Luiz Capitulino]

On Fri, 2 Feb 2018 12:50:14 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> (CCing qemu-devel)
> 
> On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
> > On Fri, 2 Feb 2018 14:19:38 +0000
> > Daniel P. Berrangé <berrange@redhat.com> wrote:  
> > > On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:  
> [...]
> > > > It would be also interesting to update QEMU QMP documentation to
> > > > clarify the arch-specific semantics of "halted".    
> > > 
> > > Any also especially clarify the awful performance implications of running
> > > this particular query command. In general I would not expect query-xxx
> > > monitor commands to interrupt all vcpus, so we should clearly warn about
> > > this !  
> > 
> > Or deprecate it...  
> 
> We could deprecate the expensive fields on query-cpus, and move
> them to a more expensive query-cpu-state command.  I believe most
> users of query-cpus are only interested in qom_path, thread_id,
> and topology info.

Agree. The only thing I'm unsure about is that, is the performance
issue only present in x86? If yes, then do we deprecate it only
for x86 or for all archs? Maybe for all archs, otherwise this has
the potential to turn into a mess.

> Markus, Eric: from the QAPI point of view, is it OK to remove
> fields between QEMU versions, as long as we follow our
> deprecation policy?

I guess we can't remove fields, but maybe we could always return
"running" and skip interrupting the vCPU threads.

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Daniel P. Berrangé]

On Fri, Feb 02, 2018 at 12:50:14PM -0200, Eduardo Habkost wrote:
> (CCing qemu-devel)
> 
> On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
> > On Fri, 2 Feb 2018 14:19:38 +0000
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:
> [...]
> > > > It would be also interesting to update QEMU QMP documentation to
> > > > clarify the arch-specific semantics of "halted".  
> > > 
> > > Any also especially clarify the awful performance implications of running
> > > this particular query command. In general I would not expect query-xxx
> > > monitor commands to interrupt all vcpus, so we should clearly warn about
> > > this !
> > 
> > Or deprecate it...
> 
> We could deprecate the expensive fields on query-cpus, and move
> them to a more expensive query-cpu-state command.  I believe most
> users of query-cpus are only interested in qom_path, thread_id,
> and topology info.
> 
> Markus, Eric: from the QAPI point of view, is it OK to remove
> fields between QEMU versions, as long as we follow our
> deprecation policy?

I would expect that to not be OK.  A fully backwards compatible way to
deal with this would just be to add a flag to the query-cpus command
eg something like

    query-cpus arch-specific=false

to turn off all this arch specific state, and just report the cheap
generic info. If it defaults to arch-specific=true when omitted, then
there's no compat problems.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On 02/02/2018 08:50 AM, Eduardo Habkost wrote:
> (CCing qemu-devel)
> 
> On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
>> On Fri, 2 Feb 2018 14:19:38 +0000
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>>> On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:
> [...]
>>>> It would be also interesting to update QEMU QMP documentation to
>>>> clarify the arch-specific semantics of "halted".  
>>>
>>> Any also especially clarify the awful performance implications of running
>>> this particular query command. In general I would not expect query-xxx
>>> monitor commands to interrupt all vcpus, so we should clearly warn about
>>> this !
>>
>> Or deprecate it...
> 
> We could deprecate the expensive fields on query-cpus, and move
> them to a more expensive query-cpu-state command.  I believe most
> users of query-cpus are only interested in qom_path, thread_id,
> and topology info.
> 
> Markus, Eric: from the QAPI point of view, is it OK to remove
> fields between QEMU versions, as long as we follow our
> deprecation policy?

Removing an output field outright may break a client that depended on
the field; so a deprecation period is definitely required there.  But it
is okay, documentation-wise, to state that a field is output always as 0
for back-compatibility reasons and that modern clients should ignore it
(which would then let old clients still parse the field, but no longer
see a non-zero value), whether or not we also pursue the deprecation
course and eventually remove the field after more releases.

See CpuInfo::current, for an example.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Eduardo Habkost]

On Fri, Feb 02, 2018 at 03:07:18PM +0000, Daniel P. Berrangé wrote:
> On Fri, Feb 02, 2018 at 12:50:14PM -0200, Eduardo Habkost wrote:
> > (CCing qemu-devel)
> > 
> > On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
> > > On Fri, 2 Feb 2018 14:19:38 +0000
> > > Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:
> > [...]
> > > > > It would be also interesting to update QEMU QMP documentation to
> > > > > clarify the arch-specific semantics of "halted".  
> > > > 
> > > > Any also especially clarify the awful performance implications of running
> > > > this particular query command. In general I would not expect query-xxx
> > > > monitor commands to interrupt all vcpus, so we should clearly warn about
> > > > this !
> > > 
> > > Or deprecate it...
> > 
> > We could deprecate the expensive fields on query-cpus, and move
> > them to a more expensive query-cpu-state command.  I believe most
> > users of query-cpus are only interested in qom_path, thread_id,
> > and topology info.
> > 
> > Markus, Eric: from the QAPI point of view, is it OK to remove
> > fields between QEMU versions, as long as we follow our
> > deprecation policy?
> 
> I would expect that to not be OK.  A fully backwards compatible way to
> deal with this would just be to add a flag to the query-cpus command
> eg something like
> 
>     query-cpus arch-specific=false
> 
> to turn off all this arch specific state, and just report the cheap
> generic info. If it defaults to arch-specific=true when omitted, then
> there's no compat problems.

This would work, too.  I would name it "full-state",
"extended-state" or something similar, though.  Not all
arch-specific data is expensive to fetch, and not all
non-arch-specific data is unexpensive.

But I'd like to confirm if it's OK to make existing non-optional
struct fields optional in the QAPI schema.  Markus, Eric?

-- 
Eduardo

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On 02/02/2018 09:25 AM, Eduardo Habkost wrote:

>>> Markus, Eric: from the QAPI point of view, is it OK to remove
>>> fields between QEMU versions, as long as we follow our
>>> deprecation policy?
>>
>> I would expect that to not be OK.  A fully backwards compatible way to
>> deal with this would just be to add a flag to the query-cpus command
>> eg something like
>>
>>     query-cpus arch-specific=false
>>
>> to turn off all this arch specific state, and just report the cheap
>> generic info. If it defaults to arch-specific=true when omitted, then
>> there's no compat problems.
> 
> This would work, too.  I would name it "full-state",
> "extended-state" or something similar, though.  Not all
> arch-specific data is expensive to fetch, and not all
> non-arch-specific data is unexpensive.
> 
> But I'd like to confirm if it's OK to make existing non-optional
> struct fields optional in the QAPI schema.  Markus, Eric?

If you add an optional bool/enum to opt-in to the command in order to
provide different levels of reporting on output, and the default remains
that when the new input field is absent, all information that was output
in earlier versions of qemu is still output, then you are backwards
compatible.  This is true even if you switched some fields from
mandatory output to optional output, because of the use of the new input
flag for opt-in semantics.  Newer callers that use the opt-in field get
what they want, older callers get what they always expected.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> (CCing qemu-devel)
> 
> On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
> > On Fri, 2 Feb 2018 14:19:38 +0000
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:
> [...]
> > > > It would be also interesting to update QEMU QMP documentation to
> > > > clarify the arch-specific semantics of "halted".  
> > > 
> > > Any also especially clarify the awful performance implications of running
> > > this particular query command. In general I would not expect query-xxx
> > > monitor commands to interrupt all vcpus, so we should clearly warn about
> > > this !
> > 
> > Or deprecate it...
> 
> We could deprecate the expensive fields on query-cpus, and move
> them to a more expensive query-cpu-state command.  I believe most
> users of query-cpus are only interested in qom_path, thread_id,
> and topology info.

Would that data be available without the bql?  I ask because if it is
then a small advantage to having a separate command is that the command
could be marked OOB with Peter's new series and never take the lock.

Dave

> Markus, Eric: from the QAPI point of view, is it OK to remove
> fields between QEMU versions, as long as we follow our
> deprecation policy?
> 
> -- 
> Eduardo
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC] kvm: x86: export vCPU halted state to sysfs

[Eduardo Habkost]

On Fri, Feb 02, 2018 at 05:23:59PM +0000, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > (CCing qemu-devel)
> > 
> > On Fri, Feb 02, 2018 at 09:21:59AM -0500, Luiz Capitulino wrote:
> > > On Fri, 2 Feb 2018 14:19:38 +0000
> > > Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > On Fri, Feb 02, 2018 at 12:15:54PM -0200, Eduardo Habkost wrote:
> > [...]
> > > > > It would be also interesting to update QEMU QMP documentation to
> > > > > clarify the arch-specific semantics of "halted".  
> > > > 
> > > > Any also especially clarify the awful performance implications of running
> > > > this particular query command. In general I would not expect query-xxx
> > > > monitor commands to interrupt all vcpus, so we should clearly warn about
> > > > this !
> > > 
> > > Or deprecate it...
> > 
> > We could deprecate the expensive fields on query-cpus, and move
> > them to a more expensive query-cpu-state command.  I believe most
> > users of query-cpus are only interested in qom_path, thread_id,
> > and topology info.
> 
> Would that data be available without the bql?  I ask because if it is
> then a small advantage to having a separate command is that the command
> could be marked OOB with Peter's new series and never take the lock.

We would need a mechanism to safely walk the CPU list without the
BQL, so I wouldn't bother trying to make a OOB-capable version of
query-cpus unless really necessary.

-- 
Eduardo