[libvirt] [PATCH 0/5] 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 - adding a monitor function 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). Viktor Mihajlovski (5): domain: Add new VCPU state "halted" qemu: Add monitor APIs for the detection of halted VCPUs tests: Add testcase for qemuMonitorJSONGetCPUState 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 | 82 ++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++ src/qemu/qemu_driver.c | 23 ++++++++- src/qemu/qemu_monitor.c | 22 ++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++-- src/qemu/qemu_monitor_json.h | 2 + src/qemu/qemu_monitor_text.c | 54 ++++++++++++++++---- src/qemu/qemu_monitor_text.h | 2 + tests/qemumonitorjsontest.c | 106 ++++++++++++++++++++++++--------------- tools/virsh-domain.c | 3 +- 12 files changed, 295 insertions(+), 58 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> --- include/libvirt/libvirt-domain.h | 1 + tools/virsh-domain.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 7ea93aa..98b9420 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1656,6 +1656,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 dbdee5b..4650eb3 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_("halted")) static const char * virshDomainVcpuStateToString(int state) -- 1.9.1
New monitor function qemuMonitorGetCPUState added to retrieve the halted state of VCPUs via the query-cpus command. 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> --- src/qemu/qemu_monitor.c | 22 ++++++++++++++++++ src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 52 ++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_monitor_json.h | 2 ++ src/qemu/qemu_monitor_text.c | 54 +++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_monitor_text.h | 2 ++ 6 files changed, 119 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 098e654..0fdee29 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1667,6 +1667,28 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon, } +/** + * qemuMonitorGetCPUState: + * @mon: monitor + * @halted: returned array of boolean containing the vCPUs halted state + * + * Detects whether vCPUs are halted. Returns count of detected vCPUs on success, + * 0 if qemu didn't report vcpus (does not report libvirt error), + * -1 on error (reports libvirt error). + */ +int +qemuMonitorGetCPUState(qemuMonitorPtr mon, + bool **halted) +{ + QEMU_CHECK_MONITOR(mon); + + if (mon->json) + return qemuMonitorJSONGetCPUState(mon, halted); + else + return qemuMonitorTextGetCPUState(mon, halted); +} + + int qemuMonitorSetLink(qemuMonitorPtr mon, const char *name, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cb4cca8..6bbb8a5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -392,6 +392,8 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon); int qemuMonitorGetCPUInfo(qemuMonitorPtr mon, int **pids); +int qemuMonitorGetCPUState(qemuMonitorPtr mon, + bool **halted); int qemuMonitorGetVirtType(qemuMonitorPtr mon, virDomainVirtType *virtType); int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index bb426dc..878efaa 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1324,13 +1324,15 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon) */ static int qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, - int **pids) + int **pids, + bool **halted) { virJSONValuePtr data; int ret = -1; size_t i; int *threads = NULL; ssize_t ncpus; + bool *haltedcpus = NULL; if (!(data = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1347,9 +1349,13 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, if (VIR_ALLOC_N(threads, ncpus) < 0) goto cleanup; + if (VIR_ALLOC_N(haltedcpus, ncpus) < 0) + goto cleanup; + for (i = 0; i < ncpus; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); int thread; + bool ishalted; if (!entry) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpu information was missing an array element")); @@ -1363,15 +1369,31 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, goto cleanup; } + if (virJSONValueObjectGetBoolean(entry, "halted", &ishalted) < 0) { + /* Some older qemu versions may not report the halted state, + * for backward compatibility we assume it's not halted */ + ishalted = false; + } + threads[i] = thread; + haltedcpus[i] = ishalted; } - *pids = threads; - threads = NULL; + if (pids) { + VIR_FREE(*pids); + *pids = threads; + threads = NULL; + } + if (halted) { + VIR_FREE(*halted); + *halted = haltedcpus; + haltedcpus = NULL; + } ret = ncpus; cleanup: VIR_FREE(threads); + VIR_FREE(haltedcpus); return ret; } @@ -1394,8 +1416,30 @@ int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; + ret = qemuMonitorJSONExtractCPUInfo(reply, pids, NULL); + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + +int qemuMonitorJSONGetCPUState(qemuMonitorPtr mon, + bool **halted) +{ + int ret = -1; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", + NULL); + virJSONValuePtr reply = NULL; + + if (!cmd) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; - ret = qemuMonitorJSONExtractCPUInfo(reply, pids); + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + ret = qemuMonitorJSONExtractCPUInfo(reply, NULL, halted); cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 37a739e..5469079 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -60,6 +60,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon); int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, int **pids); +int qemuMonitorJSONGetCPUState(qemuMonitorPtr mon, + bool **halted); int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon, virDomainVirtType *virtType); int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 9295219..bdbf1a3 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -500,13 +500,17 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon) } -int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, - int **pids) +static int qemuMonitorTextGetCPUInfoInternal(qemuMonitorPtr mon, + int **pids, + bool **halted) { char *qemucpus = NULL; char *line; pid_t *cpupids = NULL; size_t ncpupids = 0; + bool *cpuhalted = NULL; + size_t ncpuhalted = 0; + int ret = 0; if (qemuMonitorHMPCommand(mon, "info cpus", &qemucpus) < 0) return -1; @@ -517,7 +521,7 @@ int qemuMonitorTextGetCPUInfo(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; @@ -525,18 +529,26 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, char *offset = NULL; char *end = NULL; int tid = 0; + bool ishalted = false; /* Extract host Thread ID */ if ((offset = strstr(line, "thread_id=")) == NULL) - goto error; + goto cleanup; if (virStrToLong_i(offset + strlen("thread_id="), &end, 10, &tid) < 0) - goto error; + goto cleanup; if (end == NULL || !c_isspace(*end)) - goto error; + goto cleanup; if (VIR_APPEND_ELEMENT_COPY(cpupids, ncpupids, tid) < 0) - goto error; + goto cleanup; + + /* Extract halted indicator */ + if ((offset = strstr(line, "(halted)")) != NULL) + ishalted = true; + + if (VIR_APPEND_ELEMENT_COPY(cpuhalted, ncpuhalted, ishalted) < 0) + goto cleanup; VIR_DEBUG("tid=%d", tid); @@ -548,18 +560,38 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, /* Validate we got data for all VCPUs we expected */ VIR_FREE(qemucpus); - *pids = cpupids; - return ncpupids; + if (pids) { + VIR_FREE(*pids); + *pids = cpupids; + } + if (halted) { + VIR_FREE(*halted); + *halted = cpuhalted; + } + ret = ncpupids; - error: + cleanup: VIR_FREE(qemucpus); VIR_FREE(cpupids); + VIR_FREE(cpuhalted); /* Returning 0 to indicate non-fatal failure, since * older QEMU does not have VCPU<->PID mapping and * we don't want to fail on that */ - return 0; + return ret; +} + +int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, + int **pids) +{ + return qemuMonitorTextGetCPUInfoInternal(mon, pids, NULL); +} + +int qemuMonitorTextGetCPUState(qemuMonitorPtr mon, + bool **halted) +{ + return qemuMonitorTextGetCPUInfoInternal(mon, NULL, halted); } diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index eeaca52..7c185ff 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -51,6 +51,8 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon); int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, int **pids); +int qemuMonitorTextGetCPUState(qemuMonitorPtr mon, + bool **halted); int qemuMonitorTextGetVirtType(qemuMonitorPtr mon, virDomainVirtType *virtType); int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, -- 1.9.1
On Thu, Jul 14, 2016 at 16:35:39 +0200, Viktor Mihajlovski wrote:
New monitor function qemuMonitorGetCPUState added to retrieve the halted state of VCPUs via the query-cpus command.
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> --- src/qemu/qemu_monitor.c | 22 ++++++++++++++++++ src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 52 ++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_monitor_json.h | 2 ++ src/qemu/qemu_monitor_text.c | 54 +++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_monitor_text.h | 2 ++ 6 files changed, 119 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 098e654..0fdee29 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1667,6 +1667,28 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon, }
+/** + * qemuMonitorGetCPUState: + * @mon: monitor + * @halted: returned array of boolean containing the vCPUs halted state + * + * Detects whether vCPUs are halted. Returns count of detected vCPUs on success, + * 0 if qemu didn't report vcpus (does not report libvirt error), + * -1 on error (reports libvirt error). + */ +int +qemuMonitorGetCPUState(qemuMonitorPtr mon, + bool **halted)
Extraction of the data is cheap compared to calling the monitor. Also this adds a lot of duplicated code. I've merged this to the code returnign the thread ids and dropped the duplicate monitor helpers. I've kept the extraction code though. Peter
Mix of halted and non-halted VCPUs used for verification. 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> --- tests/qemumonitorjsontest.c | 106 +++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 40 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f698c14..555af7f 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1210,57 +1210,64 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUInfo(const void *data) int ret = -1; pid_t *cpupids = NULL; pid_t expected_cpupids[] = {17622, 17624, 17626, 17628}; - int ncpupids; + bool *cpuhalted = NULL; + bool expected_cpuhalted[] = {true, true, false, false}; + int ncpusfound; size_t i; + const char *replystring; if (!test) return -1; - if (qemuMonitorTestAddItem(test, "query-cpus", - "{" - " \"return\": [" - " {" - " \"current\": true," - " \"CPU\": 0," - " \"pc\": -2130530478," - " \"halted\": true," - " \"thread_id\": 17622" - " }," - " {" - " \"current\": false," - " \"CPU\": 1," - " \"pc\": -2130530478," - " \"halted\": true," - " \"thread_id\": 17624" - " }," - " {" - " \"current\": false," - " \"CPU\": 2," - " \"pc\": -2130530478," - " \"halted\": true," - " \"thread_id\": 17626" - " }," - " {" - " \"current\": false," - " \"CPU\": 3," - " \"pc\": -2130530478," - " \"halted\": true," - " \"thread_id\": 17628" - " }" - " ]," - " \"id\": \"libvirt-7\"" - "}") < 0) - goto cleanup; + replystring = + "{" + " \"return\": [" + " {" + " \"current\": true," + " \"CPU\": 0," + " \"pc\": -2130530478," + " \"halted\": true," + " \"thread_id\": 17622" + " }," + " {" + " \"current\": false," + " \"CPU\": 1," + " \"pc\": -2130530478," + " \"halted\": true," + " \"thread_id\": 17624" + " }," + " {" + " \"current\": false," + " \"CPU\": 2," + " \"pc\": -2130530478," + " \"halted\": false," + " \"thread_id\": 17626" + " }," + " {" + " \"current\": false," + " \"CPU\": 3," + " \"pc\": -2130530478," + " \"halted\": false," + " \"thread_id\": 17628" + " }" + " ]," + " \"id\": \"libvirt-7\"" + "}"; - ncpupids = qemuMonitorJSONGetCPUInfo(qemuMonitorTestGetMonitor(test), &cpupids); + if (qemuMonitorTestAddItem(test, "query-cpus", replystring) < 0) + goto cleanup; + if (qemuMonitorTestAddItem(test, "query-cpus", replystring) < 0) + goto cleanup; - if (ncpupids != 4) { + ncpusfound = qemuMonitorJSONGetCPUInfo(qemuMonitorTestGetMonitor(test), + &cpupids); + if (ncpusfound != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, - "Expecting ncpupids = 4 but got %d", ncpupids); + "Expecting ncpusfound = 4 but got %d", ncpusfound); goto cleanup; } - for (i = 0; i < ncpupids; i++) { + for (i = 0; i < ncpusfound; i++) { if (cpupids[i] != expected_cpupids[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, "Expecting cpupids[%zu] = %d but got %d", @@ -1269,10 +1276,29 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUInfo(const void *data) } } + ncpusfound = qemuMonitorJSONGetCPUState(qemuMonitorTestGetMonitor(test), + &cpuhalted); + + if (ncpusfound != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expecting ncpusfound = 4 but got %d", ncpusfound); + goto cleanup; + } + + for (i = 0; i < ncpusfound; i++) { + if (cpuhalted[i] != expected_cpuhalted[i]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expecting cpuhalted[%zu] = %d but got %d", + i, expected_cpuhalted[i], cpuhalted[i]); + goto cleanup; + } + } + ret = 0; cleanup: VIR_FREE(cpupids); + VIR_FREE(cpuhalted); qemuMonitorTestFree(test); return ret; } -- 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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_domain.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ 2 files changed, 86 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 286f096..945a75d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5650,6 +5650,88 @@ qemuDomainDetectVcpuPids(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 number of detected vCPUs on success, -1 on error and reports + * an appropriate error, -2 if the domain doesn't exist any more. + */ +int +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + virDomainVcpuDefPtr vcpu; + size_t maxvcpus = virDomainDefGetVcpusMax(vm->def); + bool *cpuhalted = NULL; + int ncpuhalted; + size_t i; + int ret = -1; + + /* Not supported currently for TCG, see above + */ + if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + ncpuhalted = qemuMonitorGetCPUState(qemuDomainGetMonitor(vm), &cpuhalted); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -2; + goto cleanup; + } + + /* Don't fail for older QEMUs + */ + if (ncpuhalted <= 0) { + virResetLastError(); + ret = 0; + goto cleanup; + } + + for (i = 0; i < maxvcpus; i++) { + vcpu = virDomainDefGetVcpu(vm->def, i); + + if (i < ncpuhalted) + QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = cpuhalted[i]; + else + QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = false; + } + + if (ncpuhalted != virDomainDefGetVcpus(vm->def)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of vCPUs from QEMU monitor. " + "got %d, wanted %d"), + ncpuhalted, virDomainDefGetVcpus(vm->def)); + goto cleanup; + } + + ret = ncpuhalted; + + cleanup: + VIR_FREE(cpuhalted); + return ret; +} bool qemuDomainSupportsNicdev(virDomainDefPtr def, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 114db98..44add02 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -316,6 +316,7 @@ struct _qemuDomainVcpuPrivate { virObject parent; pid_t tid; /* vcpu thread id */ + bool halted; /* vcpu halted state */ }; # define QEMU_DOMAIN_VCPU_PRIVATE(vcpu) \ @@ -649,6 +650,9 @@ bool qemuDomainHasVcpuPids(virDomainObjPtr vm); pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid); int qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob); +bool qemuDomainGetVcpuHalted(virDomainObjPtr vm, unsigned int vcpu); +int qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, virDomainObjPtr vm, + int asyncJob); bool qemuDomainSupportsNicdev(virDomainDefPtr def, virDomainNetDefPtr net); -- 1.9.1
On Thu, Jul 14, 2016 at 16:35:41 +0200, Viktor Mihajlovski wrote:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_domain.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ 2 files changed, 86 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 286f096..945a75d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5650,6 +5650,88 @@ qemuDomainDetectVcpuPids(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 number of detected vCPUs on success, -1 on error and reports + * an appropriate error, -2 if the domain doesn't exist any more. + */ +int +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int asyncJob) +{ + virDomainVcpuDefPtr vcpu; + size_t maxvcpus = virDomainDefGetVcpusMax(vm->def); + bool *cpuhalted = NULL; + int ncpuhalted; + size_t i; + int ret = -1; + + /* Not supported currently for TCG, see above + */ + if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + ncpuhalted = qemuMonitorGetCPUState(qemuDomainGetMonitor(vm), &cpuhalted); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -2; + goto cleanup; + } + + /* Don't fail for older QEMUs + */ + if (ncpuhalted <= 0) { + virResetLastError(); + ret = 0; + goto cleanup; + } + + for (i = 0; i < maxvcpus; i++) { + vcpu = virDomainDefGetVcpu(vm->def, i); + + if (i < ncpuhalted) + QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = cpuhalted[i]; + else + QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = false;
I've merged the 4 lines above to the function refreshing other VCPU (since I'll be adding a patch that will make it not refresh the pids all the time) and thus dropped the rest of this function. I've tweaked the commit message an I'll resend it with the rest of the patches soon. Peter
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cda85f6..de9be99 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1477,13 +1477,17 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, for (i = 0; i < virDomainDefGetVcpusMax(vm->def) && ncpuinfo < maxinfo; i++) { virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); pid_t vcpupid = qemuDomainGetVcpuPid(vm, i); + bool vcpuhalted = qemuDomainGetVcpuHalted(vm, i); if (!vcpu->online) continue; if (info) { info[i].number = i; - info[i].state = VIR_VCPU_RUNNING; + if (vcpuhalted) + info[i].state = VIR_VCPU_HALTED; + else + info[i].state = VIR_VCPU_RUNNING; if (qemuGetProcessInfo(&(info[i].cpuTime), &(info[i].cpu), NULL, vm->pid, vcpupid) < 0) { @@ -5267,6 +5271,7 @@ qemuDomainGetVcpus(virDomainPtr dom, unsigned char *cpumaps, int maplen) { + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; @@ -5283,6 +5288,13 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; } + if (qemuDomainRefreshVcpuHalted(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("could not refresh CPU states")); + return -1; + } + ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen); cleanup: @@ -18535,7 +18547,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, static int -qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, @@ -18565,6 +18577,13 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) goto cleanup; + if (qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("could not refresh CPU states")); + return -1; + } + if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, virDomainDefGetVcpus(dom->def), NULL, 0) < 0) { -- 1.9.1
On Thu, Jul 14, 2016 at 16:35:42 +0200, Viktor Mihajlovski wrote:
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> Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
[..]
@@ -5283,6 +5288,13 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; }
+ if (qemuDomainRefreshVcpuHalted(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) {
APIs calling the monitor need to enter a job since monitor calls unlock the domain. There wouldn't be anything guarding the API.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("could not refresh CPU states")); + return -1;
This skips cleanup section where the VM object needs to be unlocked.
+ } + ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
cleanup:
@@ -18565,6 +18577,13 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) goto cleanup;
[1]
+ if (qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("could not refresh CPU states")); + return -1;
This would leak memory allocated just above this hunk.
+ } +
I've fixed the problems mentioned above and tweaked the code to comply with changes done to previous patches. I've also dropped the reviewed-by tag since I'll be sending this for review again due to the changes needed. Peter
On Thu, Jul 14, 2016 at 16:35:37 +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 - adding a monitor function 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).
I'm going to pick this up in my work-in-progress series of adding vCPU hot(un)plug using the new APIs in qemu and I'll let you know about the issues once I see how well it integrates since it is touching the same places. Peter
On Mon, Jul 18, 2016 at 07:56:36 +0200, Peter Krempa wrote:
On Thu, Jul 14, 2016 at 16:35:37 +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 - adding a monitor function 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).
I'm going to pick this up in my work-in-progress series of adding vCPU hot(un)plug using the new APIs in qemu and I'll let you know about the issues once I see how well it integrates since it is touching the same places.
So I was testing the data reported from the monitor while doing some work on othe vCPU hotplug series and found that for x86 the halted state is reported if the CPU is idle and thus halted. This state it therefore almost always reported when the VM is idle. Quoting conversation on previous version [1]:
Could you please elaborat how you expect to use this info? Dpending on the architecture, the halted state is an indication that the virtual CPU is not in use. On s390x for example the halted state is only reported if the CPU is stopped or in disabled wait, both indicating that the virtual CPUs are not enabled for the operating system.
While for the s390 platform the state may reasonably express that the vcpu is unused, other platforms have apparently different meaning. Addditionally on x86_64 if the cpu is hotplugged but not grabbed by the guest it is reporting non-halted state.
This information can be used by a libvirt client application for hotplugging decisions, and this is in fact the intention.
Wouldn't it be possible for you to use the virDomainGetGuestVcpus [2] API to determine this information more reliably using the guest agent? I'm also a bit worried that the new state might cause problems with older apps actually using the value for any reason. Said that. I still keep the series in a branch and I'll post a updated version including rebase on top of the oncomming vCPU hotplug series once I finish it since it completely rewrites the exact part of the monitor code requesting the information due to changes necessary to detect hotpluggable cpus in recent qemu. Peter [1] https://www.redhat.com/archives/libvir-list/2016-July/msg00234.html [2] http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetGuestVcpus
On 29.07.2016 08:30, Peter Krempa wrote:
So I was testing the data reported from the monitor while doing some work on othe vCPU hotplug series and found that for x86 the halted state is reported if the CPU is idle and thus halted. This state it therefore almost always reported when the VM is idle.
Quoting conversation on previous version [1]:
Could you please elaborat how you expect to use this info? Dpending on the architecture, the halted state is an indication that the virtual CPU is not in use. On s390x for example the halted state is only reported if the CPU is stopped or in disabled wait, both indicating that the virtual CPUs are not enabled for the operating system.
While for the s390 platform the state may reasonably express that the vcpu is unused, other platforms have apparently different meaning.
I agree, however I haven't claimed that s390x and x86 use the same mechanism or have the same semantics, even if in both cases the CPUs are doing "nothing" (in the x86 case in a much more transient way).
Addditionally on x86_64 if the cpu is hotplugged but not grabbed by the guest it is reporting non-halted state.
This information can be used by a libvirt client application for hotplugging decisions, and this is in fact the intention.
Wouldn't it be possible for you to use the virDomainGetGuestVcpus [2] API to determine this information more reliably using the guest agent? As I said, on s390x the hypervisor-reported state IS reliable. Requiring
I don't have a current x86 QEMU setup to check with, but I am wondering what the CPU thread is actually doing (actively looping?) if it's not executing the HLT instruction. Anyway, it may be a correct representation of the CPU doing "something". the guest agent may not be acceptable for all use cases.
I'm also a bit worried that the new state might cause problems with older apps actually using the value for any reason.
That could of course be, but otoh apps should really not assume that enumerations like the cpu state will never be extended. virsh is a nice example showing how to deal with it.
Said that. I still keep the series in a branch and I'll post a updated version including rebase on top of the oncomming vCPU hotplug series once I finish it since it completely rewrites the exact part of the monitor code requesting the information due to changes necessary to detect hotpluggable cpus in recent qemu.
Great, looking forward to it.
Peter
[1] https://www.redhat.com/archives/libvir-list/2016-July/msg00234.html [2] http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetGuestVcpus
-- 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 (2)
-
Peter Krempa -
Viktor Mihajlovski