9:30 a.m.
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
9:30 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: monitor: Extract qemu cpu id along with other data
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
9:30 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: monitor: Extract halted state to a bitmap indexed by cpu id
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
9:30 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: domain: Refresh vcpu halted state using qemuMonitorGetCpuHalted
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
9:56 a.m.
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
10:14 a.m.
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
2:22 a.m.
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.
5:52 a.m.
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
6:15 a.m.
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.
2922
days inactive
2923
days old
8 comments
3 participants
participants (3)
-
Ján Tomko -
Peter Krempa -
Viktor Mihajlovski