[libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo
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 - 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. 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 I've applied the patches on top of mine and have seen no issues so far. Viktor Mihajlovski (4): domain: Add new VCPU state "halted" qemu: Add monitor support for CPU halted state qemu: Add domain support for VCPU halted state qemu: Ensure reported VCPU state is current in driver API include/libvirt/libvirt-domain.h | 1 + src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_driver.c | 15 +++++++-- src/qemu/qemu_monitor.c | 6 +++- src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 3 ++ src/qemu/qemu_monitor_text.c | 8 ++++- tests/qemumonitorjsontest.c | 8 ++--- tools/virsh-domain.c | 3 +- tools/virsh.pod | 57 ++++++++++++++++++++++++++++++++++ 11 files changed, 166 insertions(+), 9 deletions(-) -- 1.9.1
QEMU virtual CPUs can assume a halted state, which - depending on the architecture - can indicate whether a CPU is in use by the guest operating system. A new VCPU state halted is introduced in preparation of the support in the QEMU driver (and optionally others where deemed appropriate). Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- V3 Changes: - textual representation of halted state changed to 'running (halted)' - virsh help/man page updated with description of VCPU states and scope include/libvirt/libvirt-domain.h | 1 + tools/virsh-domain.c | 3 ++- tools/virsh.pod | 57 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 2342820..1172e16 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1682,6 +1682,7 @@ typedef enum { VIR_VCPU_OFFLINE = 0, /* the virtual CPU is offline */ VIR_VCPU_RUNNING = 1, /* the virtual CPU is running */ VIR_VCPU_BLOCKED = 2, /* the virtual CPU is blocked on resource */ + VIR_VCPU_HALTED = 3, /* the virtual CPU is halted */ # ifdef VIR_ENUM_SENTINELS VIR_VCPU_LAST diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b19f499..6aaf0bb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -181,7 +181,8 @@ VIR_ENUM_IMPL(virshDomainVcpuState, VIR_VCPU_LAST, N_("offline"), N_("running"), - N_("blocked")) + N_("blocked"), + N_("running (halted)")) static const char * virshDomainVcpuStateToString(int state) diff --git a/tools/virsh.pod b/tools/virsh.pod index f38aacf..6284e47 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2529,6 +2529,63 @@ vCPUs, the running time, the affinity to physical processors. With I<--pretty>, cpu affinities are shown as ranges. +An example output is + + $ virsh vcpuinfo fedora + VCPU: 0 + CPU: 0 + State: running + CPU time: 7,0s + CPU Affinity: yyyy + + VCPU: 1 + CPU: 1 + State: running (halted) + CPU time: 0,7s + CPU Affinity: yyyy + +B<STATES> + +The State field displays the current operating state of a virtual CPU + +=over 4 + +=item B<offline> + +The virtual CPU is offline and not usable by the domain. +This state is not supported by all hypervisors. + +=item B<running> + +The virtual CPU is available to the domain and is operating. + +=item B<blocked> + +The virtual CPU is available to the domain but is waiting for a resource. +This state is not supported by all hypervisors, in which case I<running> +may be reported instead. + +=item B<running (halted)> + +The virtual CPU is available to the domain but is currently halted. +The meaning of halted depends on the CPU architecture. E.g., on i386 +this would mean that the CPU is idle, whereas on s390 this designates +a disabled wait state. This state is not supported by all hypervisors, +in which case I<running> may be reported instead. + +=item B<no state> + +The virtual CPU state could not be determined. This could happen if +the hypervisor is newer than virsh. + +=item B<N/A> + +There's no information about the virtual CPU state available. This can +be the case if the domain is not running or the hypervisor does +not report the virtual CPU state. + +=back + =item B<vcpupin> I<domain> [I<vcpu>] [I<cpulist>] [[I<--live>] [I<--config>] | [I<--current>]] -- 1.9.1
Extended the qemuMonitorCPUInfo with a halted flag. Extract the halted flag for both text and JSON monitor. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- V3 Changes: - redundant comments removed - halted default set to false, so original behavior is retained src/qemu/qemu_monitor.c | 6 +++++- src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 8 +++++++- tests/qemumonitorjsontest.c | 8 ++++---- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5175f4e..d9f66a2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1677,6 +1677,7 @@ qemuMonitorCPUInfoClear(qemuMonitorCPUInfoPtr cpus, cpus[i].thread_id = -1; cpus[i].vcpus = 0; cpus[i].tid = 0; + cpus[i].halted = false; VIR_FREE(cpus[i].qom_path); VIR_FREE(cpus[i].alias); @@ -1725,8 +1726,10 @@ qemuMonitorGetCPUInfoLegacy(struct qemuMonitorQueryCpusEntry *cpuentries, size_t i; for (i = 0; i < maxvcpus; i++) { - if (i < ncpuentries) + if (i < ncpuentries) { vcpus[i].tid = cpuentries[i].tid; + vcpus[i].halted = cpuentries[i].halted; + } /* for legacy hotplug to work we need to fake the vcpu count added by * enabling a given vcpu */ @@ -1864,6 +1867,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl } vcpus[anyvcpu].tid = cpuentries[j].tid; + vcpus[anyvcpu].halted = cpuentries[j].halted; } return 0; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7d78e5b..cb27412 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -394,6 +394,7 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon); struct qemuMonitorQueryCpusEntry { pid_t tid; char *qom_path; + bool halted; }; void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries, size_t nentries); @@ -441,6 +442,8 @@ struct _qemuMonitorCPUInfo { /* internal for use in the matching code */ char *qom_path; + + bool halted; }; typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo; typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index b93220b..dff6d42 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1349,6 +1349,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, for (i = 0; i < ncpus; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); int thread = 0; + bool halted = false; const char *qom_path; if (!entry) { ret = -2; @@ -1358,9 +1359,11 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, /* Some older qemu versions don't report the thread_id so treat this as * non-fatal, simply returning no data */ ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread)); + ignore_value(virJSONValueObjectGetBoolean(entry, "halted", &halted)); qom_path = virJSONValueObjectGetString(entry, "qom_path"); cpus[i].tid = thread; + cpus[i].halted = halted; if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0) goto cleanup; } diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index ff7cc79..f975347 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -521,7 +521,7 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, * (qemu) info cpus * * CPU #0: pc=0x00000000000f0c4a thread_id=30019 * CPU #1: pc=0x00000000fffffff0 thread_id=30020 - * CPU #2: pc=0x00000000fffffff0 thread_id=30021 + * CPU #2: pc=0x00000000fffffff0 (halted) thread_id=30021 * */ line = qemucpus; @@ -541,6 +541,12 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, cpu.tid = tid; + /* Extract halted indicator */ + if ((offset = strstr(line, "(halted)")) != NULL) + cpu.halted = true; + else + cpu.halted = false; + if (VIR_APPEND_ELEMENT_COPY(cpus, ncpus, cpu) < 0) { ret = -1; goto cleanup; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 993a373..5e72a0f 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1332,10 +1332,10 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) int ret = -1; struct qemuMonitorQueryCpusEntry *cpudata = NULL; struct qemuMonitorQueryCpusEntry expect[] = { - {17622, (char *) "/machine/unattached/device[0]"}, - {17624, (char *) "/machine/unattached/device[1]"}, - {17626, (char *) "/machine/unattached/device[2]"}, - {17628, NULL}, + {17622, (char *) "/machine/unattached/device[0]", true}, + {17624, (char *) "/machine/unattached/device[1]", true}, + {17626, (char *) "/machine/unattached/device[2]", true}, + {17628, NULL, true}, }; size_t ncpudata = 0; size_t i; -- 1.9.1
Adding a field to the domain's private vcpu object to hold the halted state information. Adding two functions in support of the halted state: - qemuDomainGetVcpuHalted: retrieve the halted state from a private vcpu object - qemuDomainRefreshVcpuHalted: obtain the per-vcpu halted states via qemu monitor and store the results in the private vcpu objects Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Hao QingFeng <haoqf@linux.vnet.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- V3 Changes: - redundant comments removed - dropped useless return value -2 for qemuDomainRefreshVcpuHalted() src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 ++++ 2 files changed, 71 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 83ac389..6d95a97 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5970,6 +5970,72 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, return ret; } +/** + * qemuDomainGetVcpuHalted: + * @vm: domain object + * @vcpu: cpu id + * + * Returns the vCPU halted state. + */ +bool +qemuDomainGetVcpuHalted(virDomainObjPtr vm, + unsigned int vcpuid) +{ + virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid); + return QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted; +} + +/** + * qemuDomainRefreshVcpuHalted: + * @driver: qemu driver data + * @vm: domain object + * @asyncJob: current asynchronous job type + * + * Updates vCPU halted state in the private data of @vm. + * + * Returns 0 on success and -1 on error + */ +int +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + virDomainVcpuDefPtr vcpu; + qemuMonitorCPUInfoPtr info = NULL; + size_t maxvcpus = virDomainDefGetVcpusMax(vm->def); + size_t i; + bool hotplug; + int rc; + int ret = -1; + + /* Not supported currently for TCG, see qemuDomainRefreshVcpuInfo */ + if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) + return 0; + + hotplug = qemuDomainSupportsNewVcpuHotplug(vm); + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, hotplug); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (rc < 0) + goto cleanup; + + for (i = 0; i < maxvcpus; i++) { + vcpu = virDomainDefGetVcpu(vm->def, i); + QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = info[i].halted; + } + + ret = 0; + + cleanup: + qemuMonitorCPUInfoFree(info, maxvcpus); + return ret; +} bool qemuDomainSupportsNicdev(virDomainDefPtr def, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 521531b..048ccb9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -320,6 +320,7 @@ struct _qemuDomainVcpuPrivate { pid_t tid; /* vcpu thread id */ int enable_id; /* order in which the vcpus were enabled in qemu */ char *alias; + bool halted; /* information for hotpluggable cpus */ char *type; @@ -665,6 +666,10 @@ int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, bool state); +bool qemuDomainGetVcpuHalted(virDomainObjPtr vm, unsigned int vcpu); +int qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob); bool qemuDomainSupportsNicdev(virDomainDefPtr def, virDomainNetDefPtr net); -- 1.9.1
Refresh the VCPU halted states in API functions returning domain VCPU state information to make sure it's current. This affects qemuDomainGetVcpus and qemuDomainGetStatsVcpu Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Hao QingFeng <haoqf@linux.vnet.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- V3 Changes: - don't emit errors for refresh calls and let the qemuDomainHelperGetVcpus() call decide whether the API call succeeded or not src/qemu/qemu_driver.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6f845d..4d3fc21 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1478,13 +1478,17 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); pid_t vcpupid = qemuDomainGetVcpuPid(vm, i); virVcpuInfoPtr vcpuinfo = info + ncpuinfo; + bool vcpuhalted = qemuDomainGetVcpuHalted(vm, i); if (!vcpu->online) continue; if (info) { vcpuinfo->number = i; - vcpuinfo->state = VIR_VCPU_RUNNING; + if (vcpuhalted) + vcpuinfo->state = VIR_VCPU_HALTED; + else + vcpuinfo->state = VIR_VCPU_RUNNING; if (qemuGetProcessInfo(&vcpuinfo->cpuTime, &vcpuinfo->cpu, NULL, @@ -5440,6 +5444,7 @@ qemuDomainGetVcpus(virDomainPtr dom, unsigned char *cpumaps, int maplen) { + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; @@ -5455,6 +5460,9 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; } + /* ignoring potential failure retains legacy behavior */ + qemuDomainRefreshVcpuHalted(driver, vm, QEMU_ASYNC_JOB_NONE); + ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen); cleanup: @@ -18922,7 +18930,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver, static int -qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, @@ -18952,6 +18960,9 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) goto cleanup; + /* ignoring potential failure retains legacy behavior */ + qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE); + if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, virDomainDefGetVcpus(dom->def), NULL, 0) < 0) { -- 1.9.1
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
-----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-----
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
participants (3)
-
John Ferlan -
Peter Krempa -
Viktor Mihajlovski