Re: [libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo
On 13.10.2016 00:46, John Ferlan wrote: [...] 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... > I think it's not libvirt's task to interpret the meaning of halted as reported by QEMU. That's why I added the new state in the first place instead of reusing e.g. offline, which would have introduced architecture depend code which I'd like to avoid where possible. >> 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? > In essence that would mean to implement a new API, which seems to be too heavy a hammer for this small nail. >> 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? > > That's what I'll do, expect a new series soon... [...] > 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. > There you go... >> John > >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list >> > -- 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo
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 > 4uqQR1Uxb
Re: [libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo
-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. > > 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. [...] - -- 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo
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. 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 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. > I noticed Peter has posted (we had a tendency for mid-air collisions lately) > https://www.redhat.com/archives/libvir-list/2016-October/msg00498.html They are virsh-only basically, so there should not be any collision. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
