[libvirt] [PATCH 0/3] qemu: Don't call qemuMonitorGetCPUInfo to update vCPU halted state
The original implementation reused qemuMonitorGetCPUInfo to update the halted state. The function is very complex and should not be called all the time just to update a trivial parameter. Add infrastructure to properly update the state without the need to match in hotplug parameters. Peter Krempa (3): qemu: monitor: Extract qemu cpu id along with other data qemu: monitor: Extract halted state to a bitmap indexed by cpu id qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted src/qemu/qemu_domain.c | 20 ++++++++------------ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 11 +++++++++++ tests/qemumonitorjsontest.c | 8 ++++---- 7 files changed, 70 insertions(+), 16 deletions(-) -- 2.10.2
Storing of the ID will allow simpler extraction of data present only in query-cpus without the need to call qemuMonitorGetCPUInfo in statistics paths. --- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 11 +++++++++++ tests/qemumonitorjsontest.c | 8 ++++---- 6 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0f113a7..fbb291c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6318,6 +6318,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, VIR_FREE(vcpupriv->alias); VIR_STEAL_PTR(vcpupriv->alias, info[i].alias); vcpupriv->enable_id = info[i].id; + vcpupriv->qemu_id = info[i].qemu_id; if (hotplug && state) { vcpu->online = info[i].online; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f538d22..9df5266 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -322,6 +322,7 @@ struct _qemuDomainVcpuPrivate { pid_t tid; /* vcpu thread id */ int enable_id; /* order in which the vcpus were enabled in qemu */ + int qemu_id; /* ID reported by qemu as 'CPU' in query-cpus */ char *alias; bool halted; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index a0e5075..49d43bc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -392,6 +392,7 @@ int qemuMonitorSystemReset(qemuMonitorPtr mon); int qemuMonitorSystemPowerdown(qemuMonitorPtr mon); struct qemuMonitorQueryCpusEntry { + int qemu_id; /* id of the cpu as reported by qemu */ pid_t tid; char *qom_path; bool halted; @@ -422,6 +423,7 @@ void qemuMonitorQueryHotpluggableCpusFree(struct qemuMonitorQueryHotpluggableCpu struct _qemuMonitorCPUInfo { pid_t tid; int id; /* order of enabling of the given cpu */ + int qemu_id; /* identifier of the cpu as reported by query-cpus */ /* state data */ bool online; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9e06a4d..57b65ac 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1348,6 +1348,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data, for (i = 0; i < ncpus; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); + int cpuid = -1; int thread = 0; bool halted = false; const char *qom_path; @@ -1358,10 +1359,12 @@ 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, "CPU", &cpuid)); ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", &thread)); ignore_value(virJSONValueObjectGetBoolean(entry, "halted", &halted)); qom_path = virJSONValueObjectGetString(entry, "qom_path"); + cpus[i].qemu_id = cpuid; cpus[i].tid = thread; cpus[i].halted = halted; if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index f975347..4692d53 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -528,8 +528,18 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, do { char *offset = NULL; char *end = NULL; + int cpuid = -1; int tid = 0; + /* extract cpu number */ + if ((offset = strstr(line, "#")) == NULL) + goto cleanup; + + if (virStrToLong_i(offset + strlen("#"), &end, 10, &cpuid) < 0) + goto cleanup; + if (end == NULL || *end != ':') + goto cleanup; + /* Extract host Thread ID */ if ((offset = strstr(line, "thread_id=")) == NULL) goto cleanup; @@ -539,6 +549,7 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon, if (end == NULL || !c_isspace(*end)) goto cleanup; + cpu.qemu_id = cpuid; cpu.tid = tid; /* Extract halted indicator */ diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 9f889a9..ed4190b 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1338,10 +1338,10 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data) int ret = -1; struct qemuMonitorQueryCpusEntry *cpudata = NULL; struct qemuMonitorQueryCpusEntry expect[] = { - {17622, (char *) "/machine/unattached/device[0]", true}, - {17624, (char *) "/machine/unattached/device[1]", true}, - {17626, (char *) "/machine/unattached/device[2]", true}, - {17628, NULL, true}, + {0, 17622, (char *) "/machine/unattached/device[0]", true}, + {1, 17624, (char *) "/machine/unattached/device[1]", true}, + {2, 17626, (char *) "/machine/unattached/device[2]", true}, + {3, 17628, NULL, true}, }; size_t ncpudata = 0; size_t i; -- 2.10.2
We don't need to call qemuMonitorGetCPUInfo which is very inefficient to get data required to update the vcpu 'halted' state. Add a monitor helper that will retrieve the halted state and return it in a bitmap so that it can be indexed easily. --- src/qemu/qemu_monitor.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 1 + 2 files changed, 41 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0bfc1a8..3ff31e4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1952,6 +1952,46 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon, } +/** + * qemuMonitorGetCpuHalted: + * + * Returns a bitmap of vcpu id's that are halted. The id's correspond to the + * 'CPU' field as reported by query-cpus'. + */ +virBitmapPtr +qemuMonitorGetCpuHalted(qemuMonitorPtr mon, + size_t maxvcpus) +{ + struct qemuMonitorQueryCpusEntry *cpuentries = NULL; + size_t ncpuentries = 0; + size_t i; + int rc; + virBitmapPtr ret = NULL; + + QEMU_CHECK_MONITOR_NULL(mon); + + if (mon->json) + rc = qemuMonitorJSONQueryCPUs(mon, &cpuentries, &ncpuentries); + else + rc = qemuMonitorTextQueryCPUs(mon, &cpuentries, &ncpuentries); + + if (rc < 0) + goto cleanup; + + if (!(ret = virBitmapNew(maxvcpus))) + goto cleanup; + + for (i = 0; i < ncpuentries; i++) { + if (cpuentries[i].halted) + ignore_value(virBitmapSetBit(ret, cpuentries[i].qemu_id)); + } + + cleanup: + qemuMonitorQueryCpusFree(cpuentries, ncpuentries); + return ret; +} + + int qemuMonitorSetLink(qemuMonitorPtr mon, const char *name, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 49d43bc..100730b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -456,6 +456,7 @@ int qemuMonitorGetCPUInfo(qemuMonitorPtr mon, qemuMonitorCPUInfoPtr *vcpus, size_t maxvcpus, bool hotplug); +virBitmapPtr qemuMonitorGetCpuHalted(qemuMonitorPtr mon, size_t maxvcpus); int qemuMonitorGetVirtType(qemuMonitorPtr mon, virDomainVirtType *virtType); -- 2.10.2
Don't use qemuMonitorGetCPUInfo which does a lot of matching to get the full picture which is not necessary and would be mostly discarded. Refresh only the vcpu halted state using data from query-cpus. --- src/qemu/qemu_domain.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fbb291c..6de9ea5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6367,39 +6367,34 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, int asyncJob) { virDomainVcpuDefPtr vcpu; - qemuMonitorCPUInfoPtr info = NULL; + qemuDomainVcpuPrivatePtr vcpupriv; size_t maxvcpus = virDomainDefGetVcpusMax(vm->def); + virBitmapPtr haltedmap = NULL; 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); + haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), maxvcpus); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - - if (rc < 0) + if (qemuDomainObjExitMonitor(driver, vm) < 0 || !haltedmap) goto cleanup; for (i = 0; i < maxvcpus; i++) { vcpu = virDomainDefGetVcpu(vm->def, i); - QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = info[i].halted; + vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); + vcpupriv->halted = virBitmapIsBitSet(haltedmap, vcpupriv->qemu_id); } ret = 0; cleanup: - qemuMonitorCPUInfoFree(info, maxvcpus); + virBitmapFree(haltedmap); return ret; } -- 2.10.2
On Mon, Nov 21, 2016 at 04:30:07PM +0100, Peter Krempa wrote:
The original implementation reused qemuMonitorGetCPUInfo to update the halted state. The function is very complex and should not be called all the time just to update a trivial parameter.
Add infrastructure to properly update the state without the need to match in hotplug parameters.
Peter Krempa (3): qemu: monitor: Extract qemu cpu id along with other data qemu: monitor: Extract halted state to a bitmap indexed by cpu id qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted
src/qemu/qemu_domain.c | 20 ++++++++------------ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 11 +++++++++++ tests/qemumonitorjsontest.c | 8 ++++---- 7 files changed, 70 insertions(+), 16 deletions(-)
ACK series. Jan
On 21.11.2016 16:30, Peter Krempa wrote:
The original implementation reused qemuMonitorGetCPUInfo to update the halted state. The function is very complex and should not be called all the time just to update a trivial parameter.
Add infrastructure to properly update the state without the need to match in hotplug parameters.
Peter Krempa (3): qemu: monitor: Extract qemu cpu id along with other data qemu: monitor: Extract halted state to a bitmap indexed by cpu id qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted
src/qemu/qemu_domain.c | 20 ++++++++------------ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 11 +++++++++++ tests/qemumonitorjsontest.c | 8 ++++---- 7 files changed, 70 insertions(+), 16 deletions(-)
Could you please hold off pushing? I've just run a sniff test on s390x and see erratic values for halted. I'll try to investigate and get back to you. -- 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
On Mon, Nov 21, 2016 at 17:14:42 +0100, Viktor Mihajlovski wrote:
On 21.11.2016 16:30, Peter Krempa wrote:
The original implementation reused qemuMonitorGetCPUInfo to update the halted state. The function is very complex and should not be called all the time just to update a trivial parameter.
Add infrastructure to properly update the state without the need to match in hotplug parameters.
Peter Krempa (3): qemu: monitor: Extract qemu cpu id along with other data qemu: monitor: Extract halted state to a bitmap indexed by cpu id qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted
src/qemu/qemu_domain.c | 20 ++++++++------------ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 11 +++++++++++ tests/qemumonitorjsontest.c | 8 ++++---- 7 files changed, 70 insertions(+), 16 deletions(-)
Could you please hold off pushing? I've just run a sniff test on s390x and see erratic values for halted. I'll try to investigate and get back to you.
I've already pushed it. The hiccup might be in the fallback code that does not remember correctly the cpu numbers as reported by qemu. I'll post patches if it's so.
On 22.11.2016 09:22, Peter Krempa wrote:
On Mon, Nov 21, 2016 at 17:14:42 +0100, Viktor Mihajlovski wrote:
On 21.11.2016 16:30, Peter Krempa wrote:
The original implementation reused qemuMonitorGetCPUInfo to update the halted state. The function is very complex and should not be called all the time just to update a trivial parameter.
Add infrastructure to properly update the state without the need to match in hotplug parameters.
Peter Krempa (3): qemu: monitor: Extract qemu cpu id along with other data qemu: monitor: Extract halted state to a bitmap indexed by cpu id qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted
src/qemu/qemu_domain.c | 20 ++++++++------------ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_text.c | 11 +++++++++++ tests/qemumonitorjsontest.c | 8 ++++---- 7 files changed, 70 insertions(+), 16 deletions(-)
Could you please hold off pushing? I've just run a sniff test on s390x and see erratic values for halted. I'll try to investigate and get back to you.
I've already pushed it. The hiccup might be in the fallback code that does not remember correctly the cpu numbers as reported by qemu.
I'll post patches if it's so.
Yep ... with the following squashed in, it worked for me, (legacy) hotplug and all. diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3bace53..2d4ccbe 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1729,6 +1729,7 @@ qemuMonitorGetCPUInfoLegacy(struct qemuMonitorQueryCpusEntry *cpuentrie if (i < ncpuentries) { vcpus[i].tid = cpuentries[i].tid; vcpus[i].halted = cpuentries[i].halted; + vcpus[i].qemu_id = cpuentries[i].qemu_id; } /* for legacy hotplug to work we need to fake the vcpu count added by
On Tue, Nov 22, 2016 at 12:52:26 +0100, Viktor Mihajlovski wrote:
On 22.11.2016 09:22, Peter Krempa wrote:
On Mon, Nov 21, 2016 at 17:14:42 +0100, Viktor Mihajlovski wrote:
On 21.11.2016 16:30, Peter Krempa wrote:
The original implementation reused qemuMonitorGetCPUInfo to update the halted state. The function is very complex and should not be called all the time just to update a trivial parameter.
Add infrastructure to properly update the state without the need to match in hotplug parameters.
Peter Krempa (3): qemu: monitor: Extract qemu cpu id along with other data qemu: monitor: Extract halted state to a bitmap indexed by cpu id qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted
I've already pushed it. The hiccup might be in the fallback code that does not remember correctly the cpu numbers as reported by qemu.
I'll post patches if it's so.
Yep ... with the following squashed in, it worked for me, (legacy) hotplug and all.
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3bace53..2d4ccbe 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1729,6 +1729,7 @@ qemuMonitorGetCPUInfoLegacy(struct qemuMonitorQueryCpusEntry *cpuentrie if (i < ncpuentries) { vcpus[i].tid = cpuentries[i].tid; vcpus[i].halted = cpuentries[i].halted; + vcpus[i].qemu_id = cpuentries[i].qemu_id; }
/* for legacy hotplug to work we need to fake the vcpu count added by
I noticed that both the Hotplug and legacy code miss that line. It must have vanished when I was cutting up the changes into patches. See the series I've posted.
participants (3)
-
Ján Tomko -
Peter Krempa -
Viktor Mihajlovski