[libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo

Wed Oct 12 22:46:15 UTC 2016

On 10/12/2016 09:22 AM, Viktor Mihajlovski wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 12.10.2016 14:42, Peter Krempa wrote:
>> On Wed, Oct 12, 2016 at 14:06:14 +0200, Viktor Mihajlovski wrote:
>>> Currently, the virVcpuInfo returned by virDomainGetVcpus() will
>>> always report a state of VIR_VCPU_RUNNING for each defined domain
>>> vcpu even if the vcpu is currently in the halted state.
>>>
>>> As the monitor interface is in fact reporting the accurate state,
>>> it is rather easy to transport this information with the existing
>>> API.
>>>
>>> This is done by - adding a new state of VIR_VCPU_HALTED
>>
>> [1]
>>
>>> - extending the monitor to pass back the halted state for the
>>> vcpus - adding a new field to the private domain vcpu object
>>> reflecting the halted state for the vcpu - modifying the driver
>>> code to report the vcpu state based on the halted indicator -
>>> extending virsh vcpuinfo to also display the halted state
>>>
>>> The vcpu state is however not recorded in the internal XML
>>> format, since the state can change asynchronously (without
>>> notification).
>>>
>>> V2 is a rebase on top of Peter Krempa's CPU hotplug
>>> modernization.
>>>
>>> V3 tries to address the review comments by John and Peter.
>>
>> Despite fixing user strings in this version to show the word
>> 'running' the enum value [1] is still changed for the new one.
>> Since the actual value is used by programs and not humans that may
>> break and I'm not comfortable allowing such change in semantics.
> Hm, really? From an API perspective up to now it was perfectly OK to
> accept any of "offline", "running" and "blocked".
> I maintain that it is valid to extend the state enumeration and virsh
> shows how to handle previously unknown states.

I think the objection is for the vcpuinfo command which shouldn't need
to be CPU aware, but perhaps similar to how aware the hotplug code had
to be made for the differences between arch's, maybe the info code needs
that as well (as ugly as that could be). I'm just trying to throw some
suggestions out there to see if anything sticks.

It's painful that the "halted" bit has multiple meanings, but I think
it's handle-able - we just have to find the right way to do it...

Another "thought" that strikes me is whether there's a "valid" scenario
where someone can halt a CPU from the guest and how that gets propagated
back - even on x86. OK it's a slight divergence...

While I did suggest some sort of running (halted) output, does seeing
that on for a non x86 guest make sense for the state the CPU is in? In a
former job, I think we called it "running (idle)" mainly because
"halted" has a somewhat negative connotation.

So another option might be to allow a vcpuinfo user to pass a new flag
that would allow that return of this more detailed state. Only if the
flag was set would we check/return the new state. I think that's what
we've done in the past for similar things where changing the "default"
return/display value could cause "issues" for those programs that rely
on the string being "running" without " (halted)" or " (idle)".  Does
this make sense?

Then for the other (stats) output, returning the value of the "halted"
field could be an "additional" set of output/data to be added rather
than overloading the existing field. Does this make sense?

>>
>> As I've already pointed out, this would be the most common state on
>> x86 so basically everybody would see it and we were reporting
>> "RUNNING" for
> This is only true from a QEMU (x86) POV. Xen was always capable of
> reporting "blocked" (and, fwiw "offline") as well. If I interpret you
> correctly, you are not inclined to accept anything other than
> "running" for QEMU. Which makes it a pretty dull exercise to query the
> value at all, no?

>> ages. I still think this should be added as a separate indicator
>> (if at all). One possibility would be to add in into the bulk stats
>> API since the current structure can't be extended.
>>
> If that should be the way, patches 2/4 and 3/4 would still be the core
> of it and I would sincerely welcome a review of those.
> And, if you find value in the virsh.pod update in 1/4, you can either
> pick the parts you need or let me know and I can provide an abridged
> version.
> [...]

I do like the adjustments made to virsh.pod.

John
> 
> - -- 
> 
> Mit freundlichen Grüßen/Kind Regards
>    Viktor Mihajlovski
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzender des Aufsichtsrats: Martina Köderitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.22 (GNU/Linux)
> 
> iQIcBAEBAgAGBQJX/jkiAAoJEIZFfh8x4fgda5UP/AorvKJFOOYSSkjZJ134bwvS
> qGspiDcqCW84KNSIvFoaWBVFCZ0I6CUoUNgjC3/8HsAbyHIW43NP/cG5mkOfwJIH
> XslGG2b77EcSP+Xa7DXAeTdJg9qR47KfTV0hY6wxVljyx/v9ITbWKdVCduycve8d
> ia+oaiOI+GI4QNxR6K5na5sLRb0U2az0tR0QqpzRT0JJ6t89VJl1QC0nikCIdaRg
> 4uqQR1UxbhAcc/bAYNDhPEKDuKD9gkF80ETDd3UusalWhNWZGi8f0qwx3BtZEvrO
> 05XYRMHVKXoTaKyB6ki2XC6i9fE1EgnK+fCG2N+lnwUDNxdCr+cmW7tATFME3VAm
> 4JUj/TWDCwmsSJrfRrwiA4b5GR+mjeZ5JWYJBJ4nyOUWe7oqJNJY+0P3Ni7BplUP
> LeX24AGmDUkOvA/X2ORpv24B8oxO/ROXjtnUJ9ajiMRJvPj5VJNZlN3wASK9TFmB
> SFoSLmfydFh0sfl7RhIMFDWf0LSeVJheC/qHz901kGPRwAJ6CpFsDMWj3vOazGqg
> hH9oBAJ7EnH9OdL0KJCIPHFaPtpFoNGmH9Q6t4q4l+/yos5g6KF9mQMiJrz5g2sc
> 3dLt+yOerPTcfKPAkrDetBlwd4xzTtBA5WrgsWbxjpaRs+fOMgX/pO5jncuW70cy
> 5NeFkJoGPxooGH1DXvHz
> =AsM2
> -----END PGP SIGNATURE-----
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 