[libvirt] [PATCH 0/6] qemu: Yet another set of fixes for the 'perf' event feature
More broken stuff ... sigh. Peter Krempa (6): qemu: perf: Don't ignore perf setup if allocation fails util: perf: Use 'error' label in virPerfCmtEnable util: perf: Adhere to coding style of error checks in virPerfEventEnable util: perf: Adhere to coding style of error checks in qemuDomainSetPerfEvents qemu: process: Fix failure semantics for perf events qemu: process: Don't needlesly clear the perf events in qemuDomainPerfRestart src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_process.c | 29 ++++++++++++----------------- src/util/virperf.c | 16 ++++++++-------- 3 files changed, 22 insertions(+), 27 deletions(-) -- 2.8.1
Reject the VM startup if the perf event structure can't be allocated. --- src/qemu/qemu_process.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0ccc3ac..f8d13aa 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5410,12 +5410,12 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0) goto cleanup; - priv->perf = virPerfNew(); - if (priv->perf) { - for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { - if (vm->def->perf->events[i] == VIR_TRISTATE_BOOL_YES) - virPerfEventEnable(priv->perf, i, vm->pid); - } + if (!(priv->perf = virPerfNew())) + goto cleanup; + + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + if (vm->def->perf->events[i] == VIR_TRISTATE_BOOL_YES) + virPerfEventEnable(priv->perf, i, vm->pid); } /* This must be done after cgroup placement to avoid resetting CPU -- 2.8.1
The label is used only for the error path, thus rename cleanup. --- src/util/virperf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/util/virperf.c b/src/util/virperf.c index 359a9c3..0ae4744 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -120,7 +120,7 @@ virPerfCmtEnable(virPerfEventPtr event, if (virFileReadAll("/sys/devices/intel_cqm/type", 10, &buf) < 0) - goto cleanup; + goto error; if ((tmp = strchr(buf, '\n'))) *tmp = '\0'; @@ -128,18 +128,18 @@ virPerfCmtEnable(virPerfEventPtr event, if (virStrToLong_ui(buf, NULL, 10, &event_type) < 0) { virReportSystemError(errno, "%s", _("failed to get cmt event type")); - goto cleanup; + goto error; } VIR_FREE(buf); if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale", 10, &buf) < 0) - goto cleanup; + goto error; if (virStrToLong_ui(buf, NULL, 10, &scale) < 0) { virReportSystemError(errno, "%s", _("failed to get cmt scaling factor")); - goto cleanup; + goto error; } event->efields.cmt.scale = scale; @@ -157,19 +157,19 @@ virPerfCmtEnable(virPerfEventPtr event, virReportSystemError(errno, _("Unable to open perf type=%d for pid=%d"), event_type, pid); - goto cleanup; + goto error; } if (ioctl(event->fd, PERF_EVENT_IOC_ENABLE) < 0) { virReportSystemError(errno, "%s", _("Unable to enable perf event for CMT")); - goto cleanup; + goto error; } event->enabled = true; return 0; - cleanup: + error: VIR_FORCE_CLOSE(event->fd); VIR_FREE(buf); return -1; -- 2.8.1
--- src/util/virperf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virperf.c b/src/util/virperf.c index 0ae4744..bd65587 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -186,7 +186,7 @@ virPerfEventEnable(virPerfPtr perf, switch (type) { case VIR_PERF_EVENT_CMT: - if (virPerfCmtEnable(event, pid)) + if (virPerfCmtEnable(event, pid) < 0) return -1; break; case VIR_PERF_EVENT_LAST: -- 2.8.1
--- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 542d13c..0860822 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10070,9 +10070,9 @@ qemuDomainSetPerfEvents(virDomainPtr dom, enabled = params->value.b; type = virPerfEventTypeFromString(param->field); - if (!enabled && virPerfEventDisable(priv->perf, type)) + if (!enabled && virPerfEventDisable(priv->perf, type) < 0) goto endjob; - if (enabled && virPerfEventEnable(priv->perf, type, vm->pid)) + if (enabled && virPerfEventEnable(priv->perf, type, vm->pid) < 0) goto endjob; def->perf->events[type] = enabled ? -- 2.8.1
For strange reasons if a perf event type was not supported or failed to be enabled at VM start libvirt would ignore the failure. On the other hand on restart if the event could not be re-enabled libvirt would fail to reconnect to the VM and kill it. Both don't make really sense. Fix it by failing to start the VM if the event is not supported and change the event to disabled if it can't be reconnected (unlikely). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1329045 --- src/qemu/qemu_process.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f8d13aa..af06aa6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3501,17 +3501,14 @@ qemuDomainPerfRestart(virDomainObjPtr vm) for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { if (def->perf->events[i] && def->perf->events[i] == VIR_TRISTATE_BOOL_YES) { - if (virPerfEventEnable(priv->perf, i, vm->pid)) - goto cleanup; + + /* Failure to re-enable the perf event should not be fatal */ + if (virPerfEventEnable(priv->perf, i, vm->pid) < 0) + def->perf->events[i] = VIR_TRISTATE_BOOL_NO; } } return 0; - - cleanup: - virPerfFree(priv->perf); - priv->perf = NULL; - return -1; } struct qemuProcessReconnectData { @@ -5414,8 +5411,9 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { - if (vm->def->perf->events[i] == VIR_TRISTATE_BOOL_YES) - virPerfEventEnable(priv->perf, i, vm->pid); + if (vm->def->perf->events[i] == VIR_TRISTATE_BOOL_YES && + virPerfEventEnable(priv->perf, i, vm->pid) < 0) + goto cleanup; } /* This must be done after cgroup placement to avoid resetting CPU -- 2.8.1
At that point the perf events struct should not be allocated so there's no use in clearing it. --- src/qemu/qemu_process.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index af06aa6..41a2b35 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3492,10 +3492,7 @@ qemuDomainPerfRestart(virDomainObjPtr vm) virDomainDefPtr def = vm->def; qemuDomainObjPrivatePtr priv = vm->privateData; - virPerfFree(priv->perf); - - priv->perf = virPerfNew(); - if (!priv->perf) + if (!(priv->perf = virPerfNew())) return -1; for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { -- 2.8.1
participants (2)
-
Pavel Hrdina -
Peter Krempa