7:06 a.m.
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
7:06 a.m.
New subject: [libvirt] [PATCH V3 1/4] domain: Add new VCPU state "halted"
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(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)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
7:06 a.m.
New subject: [libvirt] [PATCH V3 2/4] qemu: Add monitor support for CPU halted state
Extended the qemuMonitorCPUInfo with a halted flag. Extract the halted
flag for both text and JSON monitor.
Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)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
7:06 a.m.
New subject: [libvirt] [PATCH V3 3/4] qemu: Add domain support for VCPU halted state
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(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Hao QingFeng <haoqf(a)linux.vnet.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)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
7:06 a.m.
New subject: [libvirt] [PATCH V3 4/4] qemu: Ensure reported VCPU state is current in driver API
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(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Hao QingFeng <haoqf(a)linux.vnet.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)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
7:42 a.m.
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
8:22 a.m.
-----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-----
5:46 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:37 a.m.
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(a)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
2962
days inactive
2963
days old
8 comments
3 participants
participants (3)
-
John Ferlan -
Peter Krempa -
Viktor Mihajlovski