[libvirt] [PATCH 0/4] qemu: Don't refresh 'halted' state for domain stats
The halted state information is useless on most platforms and calling 'query-cpus' is very expensive. Don't report the info on platforms where it does not make sense. Peter Krempa (3): qemu: driver: Extract vcpu halted state directly qemu: Remove unused 'cpuhalted' argument from qemuDomainHelperGetVcpus qemu: domain: Store vcpu halted state as a tristate Viktor Mihajlovski (1): qemu: Limit refresh of CPU halted state to s390 src/qemu/qemu_domain.c | 8 +++++++- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++--------------------- 3 files changed, 23 insertions(+), 23 deletions(-) -- 2.15.0
Don't extract the halted state into a separate array, but rater access the vcpu structures directly. We still need to call the vcpu helper to retrieve the performance statistics though. --- src/qemu/qemu_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bff49e7be6..172a8a3f73 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19660,12 +19660,14 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, int *maxparams, unsigned int privflags) { + virDomainVcpuDefPtr vcpu; + qemuDomainVcpuPrivatePtr vcpupriv; size_t i; int ret = -1; char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; virVcpuInfoPtr cpuinfo = NULL; unsigned long long *cpuwait = NULL; - bool *cpuhalted = NULL; + bool vcpuhalted = false; if (virTypedParamsAddUInt(&record->params, &record->nparams, @@ -19691,14 +19693,14 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, /* it's ok to be silent and go ahead, because halted vcpu info * wasn't here from the beginning */ virResetLastError(); - } else if (VIR_ALLOC_N(cpuhalted, virDomainDefGetVcpus(dom->def)) < 0) { - goto cleanup; + } else { + vcpuhalted = true; } } if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, virDomainDefGetVcpus(dom->def), - NULL, 0, cpuhalted) < 0) { + NULL, 0, NULL) < 0) { virResetLastError(); ret = 0; /* it's ok to be silent and go ahead */ goto cleanup; @@ -19735,14 +19737,20 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, cpuwait[i]) < 0) goto cleanup; - if (cpuhalted) { + /* state below is extracted from the individual vcpu structs */ + if (!(vcpu = virDomainDefGetVcpu(dom->def, cpuinfo[i].number))) + continue; + + vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); + + if (vcpuhalted) { snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "vcpu.%u.halted", cpuinfo[i].number); if (virTypedParamsAddBoolean(&record->params, &record->nparams, maxparams, param_name, - cpuhalted[i]) < 0) + vcpupriv->halted) < 0) goto cleanup; } } @@ -19752,7 +19760,6 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, cleanup: VIR_FREE(cpuinfo); VIR_FREE(cpuwait); - VIR_FREE(cpuhalted); return ret; } -- 2.15.0
The halted state is no longer extracted using this helper so the argument can be removed. --- src/qemu/qemu_driver.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 172a8a3f73..eaa6049fee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1490,8 +1490,7 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, unsigned long long *cpuwait, int maxinfo, unsigned char *cpumaps, - int maplen, - bool *cpuhalted) + int maplen) { size_t ncpuinfo = 0; size_t i; @@ -1511,9 +1510,6 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, if (cpumaps) memset(cpumaps, 0, sizeof(*cpumaps) * maxinfo); - if (cpuhalted) - memset(cpuhalted, 0, sizeof(*cpuhalted) * maxinfo); - for (i = 0; i < virDomainDefGetVcpusMax(vm->def) && ncpuinfo < maxinfo; i++) { virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); pid_t vcpupid = qemuDomainGetVcpuPid(vm, i); @@ -1551,9 +1547,6 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, return -1; } - if (cpuhalted) - cpuhalted[ncpuinfo] = qemuDomainGetVcpuHalted(vm, ncpuinfo); - ncpuinfo++; } @@ -5461,8 +5454,7 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; } - ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen, - NULL); + ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen); cleanup: virDomainObjEndAPI(&vm); @@ -19700,7 +19692,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, virDomainDefGetVcpus(dom->def), - NULL, 0, NULL) < 0) { + NULL, 0) < 0) { virResetLastError(); ret = 0; /* it's ok to be silent and go ahead */ goto cleanup; -- 2.15.0
Since it may be possible that the state is unknown in some cases we should store it as a tristate so that other code using it can determine whether the state was updated. --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 13 ++++--------- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 429660689b..5395cd093c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8737,7 +8737,8 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, for (i = 0; i < maxvcpus; i++) { vcpu = virDomainDefGetVcpu(vm->def, i); vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); - vcpupriv->halted = virBitmapIsBitSet(haltedmap, vcpupriv->qemu_id); + vcpupriv->halted = virTristateBoolFromBool(virBitmapIsBitSet(haltedmap, + vcpupriv->qemu_id)); } ret = 0; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 25328cd110..f3ec5d8042 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -399,7 +399,7 @@ struct _qemuDomainVcpuPrivate { 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; + virTristateBool halted; /* information for hotpluggable cpus */ char *type; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eaa6049fee..8d77d8913a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19659,7 +19659,6 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; virVcpuInfoPtr cpuinfo = NULL; unsigned long long *cpuwait = NULL; - bool vcpuhalted = false; if (virTypedParamsAddUInt(&record->params, &record->nparams, @@ -19679,15 +19678,11 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) goto cleanup; - if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { - if (qemuDomainRefreshVcpuHalted(driver, dom, - QEMU_ASYNC_JOB_NONE) < 0) { + if (HAVE_JOB(privflags) && virDomainObjIsActive(dom) && + qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE) < 0) { /* it's ok to be silent and go ahead, because halted vcpu info * wasn't here from the beginning */ virResetLastError(); - } else { - vcpuhalted = true; - } } if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, @@ -19735,14 +19730,14 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); - if (vcpuhalted) { + if (vcpupriv->halted != VIR_TRISTATE_BOOL_ABSENT) { snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "vcpu.%u.halted", cpuinfo[i].number); if (virTypedParamsAddBoolean(&record->params, &record->nparams, maxparams, param_name, - vcpupriv->halted) < 0) + vcpupriv->halted == VIR_TRISTATE_BOOL_YES) < 0) goto cleanup; } } -- 2.15.0
From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Refreshing the halted state can cause VM performance issues. Since s390 is currently the only architecture with a known interest in the halted state, we're avoiding to call QEMU on other platforms. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5395cd093c..aa652959ed 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8726,6 +8726,11 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver, if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) return 0; + /* The halted state is interresting only on s390(x). On other platforms + * the data would be stale at the time when it would be used. */ + if (!ARCH_IS_S390(vm->def->os.arch)) + return 0; + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; -- 2.15.0
On 06.02.2018 16:30, Peter Krempa wrote:
The halted state information is useless on most platforms and calling 'query-cpus' is very expensive. Don't report the info on platforms where it does not make sense.
Peter Krempa (3): qemu: driver: Extract vcpu halted state directly qemu: Remove unused 'cpuhalted' argument from qemuDomainHelperGetVcpus qemu: domain: Store vcpu halted state as a tristate
Viktor Mihajlovski (1): qemu: Limit refresh of CPU halted state to s390
src/qemu/qemu_domain.c | 8 +++++++- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++--------------------- 3 files changed, 23 insertions(+), 23 deletions(-)
Hi Peter, I was planning to pick up your tri-state proposal but you were quicker. Series looks good to me. Refactoring the halted state retrieval is probably the right thing do. Thanks! -- Regards, Viktor Mihajlovski
On Tue, Feb 06, 2018 at 16:30:51 +0100, Peter Krempa wrote:
The halted state information is useless on most platforms and calling 'query-cpus' is very expensive. Don't report the info on platforms where it does not make sense.
Peter Krempa (3): qemu: driver: Extract vcpu halted state directly qemu: Remove unused 'cpuhalted' argument from qemuDomainHelperGetVcpus qemu: domain: Store vcpu halted state as a tristate
Viktor Mihajlovski (1): qemu: Limit refresh of CPU halted state to s390
src/qemu/qemu_domain.c | 8 +++++++- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++--------------------- 3 files changed, 23 insertions(+), 23 deletions(-)
ACK series Jirka
participants (3)
-
Jiri Denemark -
Peter Krempa -
Viktor Mihajlovski