[libvirt] [PATCHv6 00/11] bulk stats: QEMU implementation

After my review on Francesco's series I've tested the patches a bit more and found a few problems. I'm re-sending the series and also I've added a few patches that tweak the documentation for this. Francesco Romani (8): qemu: bulk stats: extend internal collection API qemu: bulk stats: implement CPU stats group qemu: bulk stats: implement balloon group qemu: bulk stats: implement VCPU group qemu: bulk stats: implement interface group qemu: bulk stats: implement block group virsh: add options to query bulk stats group qemu: bulk stats: add block allocation information Peter Krempa (3): lib: De-duplicate stats group documentation for all stats functions lib: Document that virConnectGetAllDomainStats may omit some stats fields man: virsh: Add docs for supported stats groups include/libvirt/libvirt.h.in | 5 + src/libvirt.c | 76 +++++- src/qemu/qemu_driver.c | 543 +++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_monitor.c | 26 +++ src/qemu/qemu_monitor.h | 21 ++ src/qemu/qemu_monitor_json.c | 227 +++++++++++++----- src/qemu/qemu_monitor_json.h | 4 + tools/virsh-domain-monitor.c | 35 +++ tools/virsh.pod | 51 +++- 9 files changed, 844 insertions(+), 144 deletions(-) -- 2.1.0

From: Francesco Romani <fromani@redhat.com> Future patches which will implement more bulk stats groups for QEMU will need to access the connection object. To accomodate that, a few changes are needed: * enrich internal prototype to pass qemu driver object * add per-group flag to mark if one collector needs monitor access or not * If at least one collector of the requested stats needs monitor access we must start a query job for each domain. The specific collectors will run nested monitor jobs inside that. * If the job can't be acquired we pass flags to the collector so specific collectors that need monitor access can be skipped in order to gather as much data as is possible. Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 6: - renamed HAVE_MONITOR to HAVE_JOB src/qemu/qemu_driver.c | 63 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 73edda3..8bf893e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17356,7 +17356,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, static int -qemuDomainGetStatsState(virDomainObjPtr dom, +qemuDomainGetStatsState(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int privflags ATTRIBUTE_UNUSED) @@ -17379,8 +17380,18 @@ qemuDomainGetStatsState(virDomainObjPtr dom, } +typedef enum { + QEMU_DOMAIN_STATS_HAVE_JOB = (1 << 0), /* job is entered, monitor can be + accessed */ +} qemuDomainStatsFlags; + + +#define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB) + + typedef int -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, +(*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int flags); @@ -17388,11 +17399,12 @@ typedef int struct qemuDomainGetStatsWorker { qemuDomainGetStatsFunc func; unsigned int stats; + bool monitor; }; static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { - { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, - { NULL, 0 } + { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, + { NULL, 0, false } }; @@ -17424,6 +17436,20 @@ qemuDomainGetStatsCheckSupport(unsigned int *stats, } +static bool +qemuDomainGetStatsNeedMonitor(unsigned int stats) +{ + size_t i; + + for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) + if (stats & qemuDomainGetStatsWorkers[i].stats) + if (qemuDomainGetStatsWorkers[i].monitor) + return true; + + return false; +} + + static int qemuDomainGetStats(virConnectPtr conn, virDomainObjPtr dom, @@ -17441,8 +17467,8 @@ qemuDomainGetStats(virConnectPtr conn, for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) { if (stats & qemuDomainGetStatsWorkers[i].stats) { - if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams, - flags) < 0) + if (qemuDomainGetStatsWorkers[i].func(conn->privateData, dom, tmp, + &maxparams, flags) < 0) goto cleanup; } } @@ -17481,6 +17507,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, int nstats = 0; size_t i; int ret = -1; + unsigned int privflags = 0; + unsigned int domflags = 0; if (ndoms) virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); @@ -17515,7 +17543,11 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) goto cleanup; + if (qemuDomainGetStatsNeedMonitor(stats)) + privflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + for (i = 0; i < ndoms; i++) { + domflags = privflags; virDomainStatsRecordPtr tmp = NULL; if (!(dom = qemuDomObjFromDomain(doms[i]))) @@ -17525,12 +17557,22 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) continue; - if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0) - goto cleanup; + if (HAVE_JOB(domflags) && + qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) + /* As it was never requested. Gather as much as possible anyway. */ + domflags &= ~QEMU_DOMAIN_STATS_HAVE_JOB; + + if (qemuDomainGetStats(conn, dom, stats, &tmp, domflags) < 0) + goto endjob; if (tmp) tmpstats[nstats++] = tmp; + if (HAVE_JOB(domflags) && !qemuDomainObjEndJob(driver, dom)) { + dom = NULL; + continue; + } + virObjectUnlock(dom); dom = NULL; } @@ -17540,6 +17582,11 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, ret = nstats; + endjob: + if (HAVE_JOB(domflags) && dom) + if (!qemuDomainObjEndJob(driver, dom)) + dom = NULL; + cleanup: if (dom) virObjectUnlock(dom); -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
Future patches which will implement more bulk stats groups for QEMU will need to access the connection object.
To accomodate that, a few changes are needed:
s/accomodate/accommodate/
* enrich internal prototype to pass qemu driver object
* add per-group flag to mark if one collector needs monitor access or not
* If at least one collector of the requested stats needs monitor access we must start a query job for each domain. The specific collectors will run nested monitor jobs inside that.
* If the job can't be acquired we pass flags to the collector so specific collectors that need monitor access can be skipped in order to gather as much data as is possible.
Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Notes: Version 6: - renamed HAVE_MONITOR to HAVE_JOB
src/qemu/qemu_driver.c | 63 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 8 deletions(-)
+static bool +qemuDomainGetStatsNeedMonitor(unsigned int stats) +{ + size_t i; + + for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) + if (stats & qemuDomainGetStatsWorkers[i].stats) + if (qemuDomainGetStatsWorkers[i].monitor)
Could combine two 'if' into one with &&, but as-is works as well.
@@ -17525,12 +17557,22 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) continue;
- if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0) - goto cleanup; + if (HAVE_JOB(domflags) && + qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0)
Indentation is off. ACK with nit fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> This patch implements the VIR_DOMAIN_STATS_CPU_TOTAL group of statistics. Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 6: - added a check that skips the stats group in case cgroups aren't available for the given domain include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 7 +++++++ src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c2f9d26..eb62f96 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,7 @@ struct _virDomainStatsRecord { typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ + VIR_DOMAIN_STATS_CPU_TOTAL = (1 << 1), /* return domain CPU info */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt.c b/src/libvirt.c index f7e5a37..dfbd5c7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21596,6 +21596,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "state.reason" - reason for entering given state, returned as int from * virDomain*Reason enum corresponding to given state. * + * VIR_DOMAIN_STATS_CPU_TOTAL: Return CPU statistics and usage information. + * The typed parameter keys are in this format: + * "cpu.time" - total cpu time spent for this domain as unsigned long long. + * "cpu.user" - user cpu time spent as unsigned long long. + * "cpu.system" - system cpu time spent as unsigned long long. + * + * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8bf893e..9c4de94 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -96,6 +96,7 @@ #include "storage/storage_driver.h" #include "virhostdev.h" #include "domain_capabilities.h" +#include "vircgroup.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -17389,6 +17390,48 @@ typedef enum { #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB) +static int +qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv = dom->privateData; + unsigned long long cpu_time = 0; + unsigned long long user_time = 0; + unsigned long long sys_time = 0; + int err = 0; + + if (!priv->cgroup) + return 0; + + err = virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time); + if (!err && virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "cpu.time", + cpu_time) < 0) + return -1; + + err = virCgroupGetCpuacctStat(priv->cgroup, &user_time, &sys_time); + if (!err && virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "cpu.user", + user_time) < 0) + return -1; + if (!err && virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "cpu.system", + sys_time) < 0) + return -1; + + return 0; +} + + typedef int (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, virDomainObjPtr dom, @@ -17404,6 +17447,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, + { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, { NULL, 0, false } }; -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
This patch implements the VIR_DOMAIN_STATS_CPU_TOTAL group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Notes: Version 6: - added a check that skips the stats group in case cgroups aren't available for the given domain
include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 7 +++++++ src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+)
+++ b/src/libvirt.c @@ -21596,6 +21596,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "state.reason" - reason for entering given state, returned as int from * virDomain*Reason enum corresponding to given state. * + * VIR_DOMAIN_STATS_CPU_TOTAL: Return CPU statistics and usage information. + * The typed parameter keys are in this format: + * "cpu.time" - total cpu time spent for this domain as unsigned long long. + * "cpu.user" - user cpu time spent as unsigned long long. + * "cpu.system" - system cpu time spent as unsigned long long.
Missing the word "nanoseconds" on each of these three (at least, I'm assuming that's the unit, based on VIR_DOMAIN_CPU_STATS_CPUTIME). ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> This patch implements the VIR_DOMAIN_STATS_BALLOON group of statistics. Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 6 ++++++ src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index eb62f96..a5033ed 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2514,6 +2514,7 @@ struct _virDomainStatsRecord { typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ VIR_DOMAIN_STATS_CPU_TOTAL = (1 << 1), /* return domain CPU info */ + VIR_DOMAIN_STATS_BALLOON = (1 << 2), /* return domain balloon info */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt.c b/src/libvirt.c index dfbd5c7..b3b71a0 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21602,6 +21602,12 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.user" - user cpu time spent as unsigned long long. * "cpu.system" - system cpu time spent as unsigned long long. * + * VIR_DOMAIN_STATS_BALLOON: Return memory balloon device information. + * The typed parameter keys are in this format: + * "balloon.current" - the memory in kiB currently used + * as unsigned long long. + * "balloon.maximum" - the maximum memory in kiB allowed + * as unsigned long long. * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c4de94..825727d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17431,6 +17431,42 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return 0; } +static int +qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv = dom->privateData; + unsigned long long cur_balloon = 0; + int err = 0; + + if (dom->def->memballoon && + dom->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { + cur_balloon = dom->def->mem.max_balloon; + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { + cur_balloon = dom->def->mem.cur_balloon; + } else { + err = -1; + } + + if (!err && virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "balloon.current", + cur_balloon) < 0) + return -1; + + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "balloon.maximum", + dom->def->mem.max_balloon) < 0) + return -1; + + return 0; +} typedef int (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, @@ -17448,6 +17484,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, + { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true }, { NULL, 0, false } }; -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
This patch implements the VIR_DOMAIN_STATS_BALLOON group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 6 ++++++ src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> This patch implements the VIR_DOMAIN_STATS_VCPU group of statistics. To do so, this patch also extracts a helper to gather the vCPU information. Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 6: - changed indexing of the returned parameters from the retuned array that is not filled if the guest is offline to the local index and stopped returning the CPU time once the guest is offline. include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 12 +++ src/qemu/qemu_driver.c | 205 +++++++++++++++++++++++++++++-------------- 3 files changed, 154 insertions(+), 64 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a5033ed..4b851a5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2515,6 +2515,7 @@ typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ VIR_DOMAIN_STATS_CPU_TOTAL = (1 << 1), /* return domain CPU info */ VIR_DOMAIN_STATS_BALLOON = (1 << 2), /* return domain balloon info */ + VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt.c b/src/libvirt.c index b3b71a0..1d91c99 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21609,6 +21609,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "balloon.maximum" - the maximum memory in kiB allowed * as unsigned long long. * + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics. + * Due to VCPU hotplug, the vcpu.<num>.* array could be sparse. + * The actual size of the array correspond to "vcpu.current". + * The array size will never exceed "vcpu.maximum". + * The typed parameter keys are in this format: + * "vcpu.current" - current number of online virtual CPUs as unsigned int. + * "vcpu.maximum" - maximum number of online virtual CPUs as unsigned int. + * "vcpu.<num>.state" - state of the virtual CPU <num>, as int + * from virVcpuState enum. + * "vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> + * as unsigned long long. + * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 825727d..fd4bcae 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1380,6 +1380,76 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, } +static int +qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info, int maxinfo, + unsigned char *cpumaps, int maplen) +{ + int maxcpu, hostcpus; + size_t i, v; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((hostcpus = nodeGetCPUCount()) < 0) + return -1; + + maxcpu = maplen * 8; + if (maxcpu > hostcpus) + maxcpu = hostcpus; + + /* Clamp to actual number of vcpus */ + if (maxinfo > priv->nvcpupids) + maxinfo = priv->nvcpupids; + + if (maxinfo >= 1) { + if (info != NULL) { + memset(info, 0, sizeof(*info) * maxinfo); + for (i = 0; i < maxinfo; i++) { + info[i].number = i; + info[i].state = VIR_VCPU_RUNNING; + + if (priv->vcpupids != NULL && + qemuGetProcessInfo(&(info[i].cpuTime), + &(info[i].cpu), + NULL, + vm->pid, + priv->vcpupids[i]) < 0) { + virReportSystemError(errno, "%s", + _("cannot get vCPU placement & pCPU time")); + return -1; + } + } + } + + if (cpumaps != NULL) { + memset(cpumaps, 0, maplen * maxinfo); + if (priv->vcpupids != NULL) { + for (v = 0; v < maxinfo; v++) { + unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, v); + virBitmapPtr map = NULL; + unsigned char *tmpmap = NULL; + int tmpmapLen = 0; + + if (virProcessGetAffinity(priv->vcpupids[v], + &map, maxcpu) < 0) + return -1; + virBitmapToData(map, &tmpmap, &tmpmapLen); + if (tmpmapLen > maplen) + tmpmapLen = maplen; + memcpy(cpumap, tmpmap, tmpmapLen); + + VIR_FREE(tmpmap); + virBitmapFree(map); + } + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cpu affinity is not available")); + return -1; + } + } + } + return maxinfo; +} + + static virDomainPtr qemuDomainLookupByID(virConnectPtr conn, int id) { @@ -4959,10 +5029,7 @@ qemuDomainGetVcpus(virDomainPtr dom, int maplen) { virDomainObjPtr vm; - size_t i; - int v, maxcpu, hostcpus; int ret = -1; - qemuDomainObjPrivatePtr priv; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -4977,67 +5044,7 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; } - priv = vm->privateData; - - if ((hostcpus = nodeGetCPUCount()) < 0) - goto cleanup; - - maxcpu = maplen * 8; - if (maxcpu > hostcpus) - maxcpu = hostcpus; - - /* Clamp to actual number of vcpus */ - if (maxinfo > priv->nvcpupids) - maxinfo = priv->nvcpupids; - - if (maxinfo >= 1) { - if (info != NULL) { - memset(info, 0, sizeof(*info) * maxinfo); - for (i = 0; i < maxinfo; i++) { - info[i].number = i; - info[i].state = VIR_VCPU_RUNNING; - - if (priv->vcpupids != NULL && - qemuGetProcessInfo(&(info[i].cpuTime), - &(info[i].cpu), - NULL, - vm->pid, - priv->vcpupids[i]) < 0) { - virReportSystemError(errno, "%s", - _("cannot get vCPU placement & pCPU time")); - goto cleanup; - } - } - } - - if (cpumaps != NULL) { - memset(cpumaps, 0, maplen * maxinfo); - if (priv->vcpupids != NULL) { - for (v = 0; v < maxinfo; v++) { - unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, v); - virBitmapPtr map = NULL; - unsigned char *tmpmap = NULL; - int tmpmapLen = 0; - - if (virProcessGetAffinity(priv->vcpupids[v], - &map, maxcpu) < 0) - goto cleanup; - virBitmapToData(map, &tmpmap, &tmpmapLen); - if (tmpmapLen > maplen) - tmpmapLen = maplen; - memcpy(cpumap, tmpmap, tmpmapLen); - - VIR_FREE(tmpmap); - virBitmapFree(map); - } - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cpu affinity is not available")); - goto cleanup; - } - } - } - ret = maxinfo; + ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen); cleanup: if (vm) @@ -17468,6 +17475,75 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return 0; } + +static int +qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + int ret = -1; + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + virVcpuInfoPtr cpuinfo = NULL; + + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + "vcpu.current", + (unsigned) dom->def->vcpus) < 0) + return -1; + + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + "vcpu.maximum", + (unsigned) dom->def->maxvcpus) < 0) + return -1; + + if (VIR_ALLOC_N(cpuinfo, dom->def->vcpus) < 0) + return -1; + + if (qemuDomainHelperGetVcpus(dom, cpuinfo, dom->def->vcpus, + NULL, 0) < 0) { + virResetLastError(); + ret = 0; /* it's ok to be silent and go ahead */ + goto cleanup; + } + + for (i = 0; i < dom->def->vcpus; i++) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "vcpu.%zu.state", i); + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].state) < 0) + goto cleanup; + + /* stats below are available only if the VM is alive */ + if (!virDomainObjIsActive(dom)) + continue; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "vcpu.%zu.time", i); + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].cpuTime) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(cpuinfo); + return ret; +} + + typedef int (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, virDomainObjPtr dom, @@ -17485,6 +17561,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true }, + { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false }, { NULL, 0, false } }; -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
This patch implements the VIR_DOMAIN_STATS_VCPU group of statistics. To do so, this patch also extracts a helper to gather the vCPU information.
Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Notes: Version 6: - changed indexing of the returned parameters from the retuned array that is not filled if the guest is offline to the local index and stopped returning the CPU time once the guest is offline.
include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 12 +++ src/qemu/qemu_driver.c | 205 +++++++++++++++++++++++++++++-------------- 3 files changed, 154 insertions(+), 64 deletions(-)
I might have split this into two patches - one for the function extraction (mostly code motion, no real new code - with the first use being the existing API) and the other for the new code (libvirt.h.in addition, new code also calling into the function extracted in part 1). But it's not worth making you respin it now, so I can live with it as one patch.
+++ b/src/libvirt.c @@ -21609,6 +21609,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "balloon.maximum" - the maximum memory in kiB allowed * as unsigned long long. * + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics. + * Due to VCPU hotplug, the vcpu.<num>.* array could be sparse. + * The actual size of the array correspond to "vcpu.current".
s/correspond/corresponds/
+ /* Clamp to actual number of vcpus */ + if (maxinfo > priv->nvcpupids) + maxinfo = priv->nvcpupids; + + if (maxinfo >= 1) { + if (info != NULL) {
Weak style preference for 'if (info)' in this file, but not strong enough to require that it be changed.
+ memset(info, 0, sizeof(*info) * maxinfo); + for (i = 0; i < maxinfo; i++) { + info[i].number = i; + info[i].state = VIR_VCPU_RUNNING; + + if (priv->vcpupids != NULL &&
Another verbose comparison to NULL.
+ + if (cpumaps != NULL) { + memset(cpumaps, 0, maplen * maxinfo); + if (priv->vcpupids != NULL) {
and a couple more
- - if (maxinfo >= 1) { - if (info != NULL) {
Then again, you're just doing code motion, so keep this patch as-is, and save any comparison to NULL cleanups to something separate.
+ + if (VIR_ALLOC_N(cpuinfo, dom->def->vcpus) < 0) + return -1;
Hmm. cpuinfo is guaranteed to be zero-initialized here; can we avoid the memset() to zero in the helper function? But that could be a followup patch.
+ + if (qemuDomainHelperGetVcpus(dom, cpuinfo, dom->def->vcpus, + NULL, 0) < 0) { + virResetLastError(); + ret = 0; /* it's ok to be silent and go ahead */
Technically, the error still gets listed in the logs; we'll probably get people complaining about "why is my log showing an error even though nothing went wrong". So in the future, we may need to teach qemuDomainHelperGetVcpus to be silent to begin with, rather than logging failures that we later ignore; but that can be a followup patch.
+ goto cleanup; + } + + for (i = 0; i < dom->def->vcpus; i++) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "vcpu.%zu.state", i);
Coverity might complain that the result is unchecked, if so, adding an ignore_value() will be sufficient (I'm satisfied that sizeof("vcpu..state") plus the longest possible 64-bit unsigned stringized number fits comfortably within the field length, so I agree with your choice of not bothering to check for failure). So, although I was rather chatty, I don't see anything preventing me from saying: ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> This patch implements the VIR_DOMAIN_STATS_INTERFACE group of statistics. Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 6: - this stats group is now skipped if the guest is offline as the interface names are not available - also an additional check checks if the interface has a name to avoid crashing include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 14 +++++++ src/qemu/qemu_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 4b851a5..17b1b43 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2516,6 +2516,7 @@ typedef enum { VIR_DOMAIN_STATS_CPU_TOTAL = (1 << 1), /* return domain CPU info */ VIR_DOMAIN_STATS_BALLOON = (1 << 2), /* return domain balloon info */ VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */ + VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt.c b/src/libvirt.c index 1d91c99..5534b2f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21621,6 +21621,20 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> * as unsigned long long. * + * VIR_DOMAIN_STATS_INTERFACE: Return network interface statistics. + * The typed parameter keys are in this format: + * "net.count" - number of network interfaces on this domain + * as unsigned int. + * "net.<num>.name" - name of the interface <num> as string. + * "net.<num>.rx.bytes" - bytes received as unsigned long long. + * "net.<num>.rx.pkts" - packets received as unsigned long long. + * "net.<num>.rx.errs" - receive errors as unsigned long long. + * "net.<num>.rx.drop" - receive packets dropped as unsigned long long. + * "net.<num>.tx.bytes" - bytes transmitted as unsigned long long. + * "net.<num>.tx.pkts" - packets transmitted as unsigned long long. + * "net.<num>.tx.errs" - transmission errors as unsigned long long. + * "net.<num>.tx.drop" - transmit packets dropped as unsigned long long. + * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fd4bcae..4744c8e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17543,6 +17543,100 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return ret; } +#define QEMU_ADD_COUNT_PARAM(record, maxparams, type, count) \ +do { \ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "%s.count", type); \ + if (virTypedParamsAddUInt(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + count) < 0) \ + return -1; \ +} while (0) + +#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \ +do { \ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \ + "%s.%zu.name", type, num); \ + if (virTypedParamsAddString(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + name) < 0) \ + return -1; \ +} while (0) + +#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \ +do { \ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \ + "net.%zu.%s", num, name); \ + if (value >= 0 && virTypedParamsAddULLong(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + value) < 0) \ + return -1; \ +} while (0) + +static int +qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + struct _virDomainInterfaceStats tmp; + + if (!virDomainObjIsActive(dom)) + return 0; + + QEMU_ADD_COUNT_PARAM(record, maxparams, "net", dom->def->nnets); + + /* Check the path is one of the domain's network interfaces. */ + for (i = 0; i < dom->def->nnets; i++) { + memset(&tmp, 0, sizeof(tmp)); + + if (!dom->def->nets[i]->ifname) + continue; + + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", i, dom->def->nets[i]->ifname); + + if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { + virResetLastError(); + continue; + } + + QEMU_ADD_NET_PARAM(record, maxparams, i, + "rx.bytes", tmp.rx_bytes); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "rx.pkts", tmp.rx_packets); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "rx.errs", tmp.rx_errs); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "rx.drop", tmp.rx_drop); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "tx.bytes", tmp.tx_bytes); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "tx.pkts", tmp.tx_packets); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "tx.errs", tmp.tx_errs); + QEMU_ADD_NET_PARAM(record, maxparams, i, + "tx.drop", tmp.tx_drop); + } + + return 0; +} + +#undef QEMU_ADD_NET_PARAM + +#undef QEMU_ADD_NAME_PARAM + +#undef QEMU_ADD_COUNT_PARAM typedef int (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, @@ -17562,6 +17656,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false }, + { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false }, { NULL, 0, false } }; -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
This patch implements the VIR_DOMAIN_STATS_INTERFACE group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Notes: Version 6: - this stats group is now skipped if the guest is offline as the interface names are not available - also an additional check checks if the interface has a name to avoid crashing
include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 14 +++++++ src/qemu/qemu_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+)
+static int +qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + struct _virDomainInterfaceStats tmp;
Depending on whether this gets in first, or James' patches, the other will want to rebase: https://www.redhat.com/archives/libvir-list/2014-September/msg01025.html
+ + if (!virDomainObjIsActive(dom)) + return 0; + + QEMU_ADD_COUNT_PARAM(record, maxparams, "net", dom->def->nnets); + + /* Check the path is one of the domain's network interfaces. */ + for (i = 0; i < dom->def->nnets; i++) { + memset(&tmp, 0, sizeof(tmp)); + + if (!dom->def->nets[i]->ifname) + continue;
Micro-optimization: swap these two statements to avoid the memset() if you are going to skip the loop iteration.
+ + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", i, dom->def->nets[i]->ifname); + + if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { + virResetLastError(); + continue;
Again, this will pollute the log, so we may need to make virNetInterfaceStats gain a quiet mode. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> This patch implements the VIR_DOMAIN_STATS_BLOCK group of statistics. To do so, an helper function to get the block stats of all the disks of a domain is added. Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 6: - fixed init of "nstats" to actually contain the number of allocated elements. The block stats didn't work without that change. include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 20 +++++++ src/qemu/qemu_driver.c | 81 ++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 26 +++++++++ src/qemu/qemu_monitor.h | 20 +++++++ src/qemu/qemu_monitor_json.c | 136 +++++++++++++++++++++++++++++-------------- src/qemu/qemu_monitor_json.h | 4 ++ 7 files changed, 245 insertions(+), 43 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 17b1b43..724314e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2517,6 +2517,7 @@ typedef enum { VIR_DOMAIN_STATS_BALLOON = (1 << 2), /* return domain balloon info */ VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */ VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */ + VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt.c b/src/libvirt.c index 5534b2f..ab10a3a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21635,6 +21635,26 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "net.<num>.tx.errs" - transmission errors as unsigned long long. * "net.<num>.tx.drop" - transmit packets dropped as unsigned long long. * + * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics. + * The typed parameter keys are in this format: + * "block.count" - number of block devices on this domain + * as unsigned int. + * "block.<num>.name" - name of the block device <num> as string. + * matches the name of the block device. + * "block.<num>.rd.reqs" - number of read requests as unsigned long long. + * "block.<num>.rd.bytes" - number of read bytes as unsigned long long. + * "block.<num>.rd.times" - total time (ns) spent on reads as + * unsigned long long. + * "block.<num>.wr.reqs" - number of write requests as unsigned long long. + * "block.<num>.wr.bytes" - number of written bytes as unsigned long long. + * "block.<num>.wr.times" - total time (ns) spent on writes as + * unsigned long long. + * "block.<num>.fl.reqs" - total flush requests as unsigned long long. + * "block.<num>.fl.times" - total time (ns) spent on cache flushing as + * unsigned long long. + * "block.<num>.errors" - Xen only: the 'oo_req' value as + * unsigned long long. + * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4744c8e..37d2a9a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9663,6 +9663,7 @@ qemuDomainBlockStats(virDomainPtr dom, return ret; } + static int qemuDomainBlockStatsFlags(virDomainPtr dom, const char *path, @@ -17634,6 +17635,85 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, #undef QEMU_ADD_NET_PARAM +/* expects a LL, but typed parameter must be ULL */ +#define QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, num, name, value) \ +do { \ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \ + "block.%zu.%s", num, name); \ + if (value >= 0 && virTypedParamsAddULLong(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + value) < 0) \ + goto cleanup; \ +} while (0) + +static int +qemuDomainGetStatsBlock(virQEMUDriverPtr driver, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags) +{ + size_t i; + int ret = -1; + int nstats = dom->def->ndisks; + qemuBlockStatsPtr stats = NULL; + qemuDomainObjPrivatePtr priv = dom->privateData; + + if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) + return 0; /* it's ok, just go ahead silently */ + + if (VIR_ALLOC_N(stats, nstats) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, dom); + + nstats = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats); + + qemuDomainObjExitMonitor(driver, dom); + + if (nstats < 0) { + virResetLastError(); + ret = 0; /* still ok, again go ahead silently */ + goto cleanup; + } + + QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks); + + for (i = 0; i < nstats; i++) { + QEMU_ADD_NAME_PARAM(record, maxparams, + "block", i, dom->def->disks[i]->dst); + + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "rd.reqs", stats[i].rd_req); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "rd.bytes", stats[i].rd_bytes); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "rd.times", stats[i].rd_total_times); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "wr.reqs", stats[i].wr_req); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "wr.bytes", stats[i].wr_bytes); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "wr.times", stats[i].wr_total_times); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "fl.reqs", stats[i].flush_req); + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, + "fl.times", stats[i].flush_total_times); + } + + ret = 0; + + cleanup: + VIR_FREE(stats); + return ret; +} + +#undef QEMU_ADD_BLOCK_PARAM_LL + #undef QEMU_ADD_NAME_PARAM #undef QEMU_ADD_COUNT_PARAM @@ -17657,6 +17737,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false }, { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false }, + { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true }, { NULL, 0, false } }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..1ca8d55 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1760,6 +1760,32 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, return ret; } +/* Fills the first 'nstats' block stats. 'stats' must be an array. + * Returns <0 on error, otherwise the number of block stats retrieved. + * if 'dev_name' is != NULL, look for this device only and skip + * any other. In that case return value cannot be greater than 1. + */ +int +qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + qemuBlockStatsPtr stats, + int nstats) +{ + int ret; + VIR_DEBUG("mon=%p dev=%s", mon, dev_name); + + if (mon->json) { + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, + stats, nstats); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to query all block stats with this QEMU")); + return -1; + } + + return ret; +} + /* Return 0 and update @nparams with the number of block stats * QEMU supports if success. Return -1 if failure. */ diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c32001d..6eb0036 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -346,6 +346,26 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, long long *flush_req, long long *flush_total_times, long long *errs); + +typedef struct _qemuBlockStats qemuBlockStats; +typedef qemuBlockStats *qemuBlockStatsPtr; +struct _qemuBlockStats { + long long rd_req; + long long rd_bytes; + long long wr_req; + long long wr_bytes; + long long rd_total_times; + long long wr_total_times; + long long flush_req; + long long flush_total_times; +}; + +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + qemuBlockStatsPtr stats, + int nstats) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); + int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon, int *nparams); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 0c4832a..d45c41f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1734,13 +1734,8 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, long long *flush_total_times, long long *errs) { - int ret; - size_t i; - bool found = false; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats", - NULL); - virJSONValuePtr reply = NULL; - virJSONValuePtr devices; + qemuBlockStats stats; + int ret = -1; *rd_req = *rd_bytes = -1; *wr_req = *wr_bytes = *errs = -1; @@ -1754,9 +1749,49 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, if (flush_total_times) *flush_total_times = -1; + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, &stats, 1) != 1) + goto cleanup; + + *rd_req = stats.rd_req; + *rd_bytes = stats.rd_bytes; + *wr_req = stats.wr_req; + *wr_bytes = stats.wr_bytes; + *errs = -1; /* QEMU does not have this */ + + if (rd_total_times) + *rd_total_times = stats.rd_total_times; + if (wr_total_times) + *wr_total_times = stats.wr_total_times; + if (flush_req) + *flush_req = stats.flush_req; + if (flush_total_times) + *flush_total_times = stats.flush_total_times; + + ret = 0; + + cleanup: + return ret; +} + + +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + qemuBlockStatsPtr bstats, + int nstats) +{ + int ret, count; + size_t i; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats", + NULL); + virJSONValuePtr reply = NULL; + virJSONValuePtr devices; + if (!cmd) return -1; + if (!bstats || nstats <= 0) + return -1; + ret = qemuMonitorJSONCommand(mon, cmd, &reply); if (ret == 0) @@ -1772,108 +1807,123 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; } - for (i = 0; i < virJSONValueArraySize(devices); i++) { + count = 0; + for (i = 0; i < virJSONValueArraySize(devices) && count < nstats; i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); virJSONValuePtr stats; - const char *thisdev; if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats device entry was not in expected format")); - goto cleanup; - } - - if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats device entry was not in expected format")); + _("blockstats device entry was not " + "in expected format")); goto cleanup; } - /* New QEMU has separate names for host & guest side of the disk - * and libvirt gives the host side a 'drive-' prefix. The passed - * in dev_name is the guest side though + /* If dev_name is specified, we are looking for a specific device, + * so we must be stricter. */ - if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX)) - thisdev += strlen(QEMU_DRIVE_HOST_PREFIX); + if (dev_name) { + const char *thisdev = virJSONValueObjectGetString(dev, "device"); + if (!thisdev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats device entry was not " + "in expected format")); + goto cleanup; + } - if (STRNEQ(thisdev, dev_name)) - continue; + /* New QEMU has separate names for host & guest side of the disk + * and libvirt gives the host side a 'drive-' prefix. The passed + * in dev_name is the guest side though + */ + if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX)) + thisdev += strlen(QEMU_DRIVE_HOST_PREFIX); + + if (STRNEQ(thisdev, dev_name)) + continue; + } - found = true; if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL || stats->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats stats entry was not in expected format")); + _("blockstats stats entry was not " + "in expected format")); goto cleanup; } - if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", rd_bytes) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", + &bstats->rd_bytes) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "rd_bytes"); goto cleanup; } - if (virJSONValueObjectGetNumberLong(stats, "rd_operations", rd_req) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "rd_operations", + &bstats->rd_req) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "rd_operations"); goto cleanup; } - if (rd_total_times && - virJSONValueObjectHasKey(stats, "rd_total_time_ns") && + if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") && (virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns", - rd_total_times) < 0)) { + &bstats->rd_total_times) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "rd_total_time_ns"); goto cleanup; } - if (virJSONValueObjectGetNumberLong(stats, "wr_bytes", wr_bytes) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "wr_bytes", + &bstats->wr_bytes) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "wr_bytes"); goto cleanup; } - if (virJSONValueObjectGetNumberLong(stats, "wr_operations", wr_req) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "wr_operations", + &bstats->wr_req) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "wr_operations"); goto cleanup; } - if (wr_total_times && - virJSONValueObjectHasKey(stats, "wr_total_time_ns") && + if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") && (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns", - wr_total_times) < 0)) { + &bstats->wr_total_times) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "wr_total_time_ns"); goto cleanup; } - if (flush_req && - virJSONValueObjectHasKey(stats, "flush_operations") && + if (virJSONValueObjectHasKey(stats, "flush_operations") && (virJSONValueObjectGetNumberLong(stats, "flush_operations", - flush_req) < 0)) { + &bstats->flush_req) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "flush_operations"); goto cleanup; } - if (flush_total_times && - virJSONValueObjectHasKey(stats, "flush_total_time_ns") && + if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") && (virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns", - flush_total_times) < 0)) { + &bstats->flush_total_times) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "flush_total_time_ns"); goto cleanup; } + + count++; + bstats++; + + if (dev_name && count) + break; } - if (!found) { + if (dev_name && !count) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find statistics for device '%s'"), dev_name); goto cleanup; } - ret = 0; + + ret = count; cleanup: virJSONValueFree(cmd); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index b5c61ca..b6d46d4 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -79,6 +79,10 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, long long *flush_req, long long *flush_total_times, long long *errs); +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + qemuBlockStatsPtr stats, + int nstats); int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon, int *nparams); int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
This patch implements the VIR_DOMAIN_STATS_BLOCK group of statistics.
To do so, an helper function to get the block stats of all the disks of
s/an helper/a helper/ (the "h" in helper is pronounced, and that's the key for which article to use; remember "a half of an hour")
a domain is added.
Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
@@ -21635,6 +21635,26 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "net.<num>.tx.errs" - transmission errors as unsigned long long. * "net.<num>.tx.drop" - transmit packets dropped as unsigned long long. * + * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics.
Hmm. My work on allocation watermark makes me wonder if we'll eventually want to report stats on backing files (possibly as a new stats group or only with a flag). I also wonder if this series is missing stats reporting for block jobs. But the nice thing about the API is that it is extensible so we can add more later as we find a need for things in clients.
+ * The typed parameter keys are in this format: + * "block.count" - number of block devices on this domain + * as unsigned int. + * "block.<num>.name" - name of the block device <num> as string. + * matches the name of the block device.
Is this the "<target dev='vda'/>" name? Management can then map that name back to a file path or network disk source, but it still might be worth being just a bit more verbose on what a block's name really entails.
+/* expects a LL, but typed parameter must be ULL */ +#define QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, num, name, value) \ +do { \ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \ + "block.%zu.%s", num, name); \ + if (value >= 0 && virTypedParamsAddULLong(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + value) < 0) \ + goto cleanup; \ +} while (0)
Just noticing that we aren't very consistent on whether multi-line macros line up the \ in a final column vs. this style of one space per line. Doesn't really matter too much.
+ nstats = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats); + + qemuDomainObjExitMonitor(driver, dom); + + if (nstats < 0) { + virResetLastError(); + ret = 0; /* still ok, again go ahead silently */
Another case where we may be polluting the log. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> Exports to the domstats commands the new bulk stats groups. Signed-off-by: Francesco Romani <fromani@redhat.com> --- tools/virsh-domain-monitor.c | 35 +++++++++++++++++++++++++++++++++++ tools/virsh.pod | 4 +++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 055d8d2..d013ca8 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1972,6 +1972,26 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("report domain state"), }, + {.name = "cpu-total", + .type = VSH_OT_BOOL, + .help = N_("report domain physical cpu usage"), + }, + {.name = "balloon", + .type = VSH_OT_BOOL, + .help = N_("report domain balloon statistics"), + }, + {.name = "vcpu", + .type = VSH_OT_BOOL, + .help = N_("report domain virtual cpu information"), + }, + {.name = "interface", + .type = VSH_OT_BOOL, + .help = N_("report domain network interface information"), + }, + {.name = "block", + .type = VSH_OT_BOOL, + .help = N_("report domain block device statistics"), + }, {.name = "list-active", .type = VSH_OT_BOOL, .help = N_("list only active domains"), @@ -2063,6 +2083,21 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "state")) stats |= VIR_DOMAIN_STATS_STATE; + if (vshCommandOptBool(cmd, "cpu-total")) + stats |= VIR_DOMAIN_STATS_CPU_TOTAL; + + if (vshCommandOptBool(cmd, "balloon")) + stats |= VIR_DOMAIN_STATS_BALLOON; + + if (vshCommandOptBool(cmd, "vcpu")) + stats |= VIR_DOMAIN_STATS_VCPU; + + if (vshCommandOptBool(cmd, "interface")) + stats |= VIR_DOMAIN_STATS_INTERFACE; + + if (vshCommandOptBool(cmd, "block")) + stats |= VIR_DOMAIN_STATS_BLOCK; + if (vshCommandOptBool(cmd, "list-active")) flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE; diff --git a/tools/virsh.pod b/tools/virsh.pod index 5d4b12b..b929480 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -814,6 +814,7 @@ I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command. =item B<domstats> [I<--raw>] [I<--enforce>] [I<--state>] +[I<--cpu-total>][I<--balloon>][I<--vcpu>][I<--interface>][I<--block>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] [I<--list-transient>] [I<--list-running>] [I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] @@ -831,7 +832,8 @@ behavior use the I<--raw> flag. The individual statistics groups are selectable via specific flags. By default all supported statistics groups are returned. Supported -statistics groups flags are: I<--state>. +statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>, +I<--vcpu>, I<--interface>, I<--block>. Selecting a specific statistics groups doesn't guarantee that the daemon supports the selected group of stats. Flag I<--enforce> -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
Exports to the domstats commands the new bulk stats groups.
A lot of trailing 's'. I might say: Add new bulk stats groups to the domstats command.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- tools/virsh-domain-monitor.c | 35 +++++++++++++++++++++++++++++++++++ tools/virsh.pod | 4 +++- 2 files changed, 38 insertions(+), 1 deletion(-)
+++ b/tools/virsh.pod @@ -814,6 +814,7 @@ I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command.
=item B<domstats> [I<--raw>] [I<--enforce>] [I<--state>] +[I<--cpu-total>][I<--balloon>][I<--vcpu>][I<--interface>][I<--block>]
Missing spaces between each option. ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Francesco Romani <fromani@redhat.com> Management software wants to be able to allocate disk space on demand. To support this they need keep track of the space occupation of the block device. This information is reported by qemu as part of block stats. This patch extend the block information in the bulk stats with the allocation information. To keep the same behaviour a helper is extracted from qemuMonitorJSONGetBlockExtent in order to get per-device allocation information. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/libvirt.c | 2 + src/qemu/qemu_driver.c | 18 +++++++++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 91 ++++++++++++++++++++++++++++++++++---------- 4 files changed, 92 insertions(+), 20 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ab10a3a..a8892a2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21654,6 +21654,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * unsigned long long. * "block.<num>.errors" - Xen only: the 'oo_req' value as * unsigned long long. + * "block.<num>.allocation" - offset of the highest written sector + * as unsigned long long. * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37d2a9a..3cba6a5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17649,6 +17649,19 @@ do { \ goto cleanup; \ } while (0) +#define QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, num, name, value) \ +do { \ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \ + "block.%zu.%s", num, name); \ + if (virTypedParamsAddULLong(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + value) < 0) \ + goto cleanup; \ +} while (0) + static int qemuDomainGetStatsBlock(virQEMUDriverPtr driver, virDomainObjPtr dom, @@ -17703,6 +17716,9 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, "fl.reqs", stats[i].flush_req); QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i, "fl.times", stats[i].flush_total_times); + + QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i, + "allocation", stats[i].wr_highest_offset); } ret = 0; @@ -17714,6 +17730,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, #undef QEMU_ADD_BLOCK_PARAM_LL +#undef QEMU_ADD_BLOCK_PARAM_ULL + #undef QEMU_ADD_NAME_PARAM #undef QEMU_ADD_COUNT_PARAM diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6eb0036..d0f0bf5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -358,6 +358,7 @@ struct _qemuBlockStats { long long wr_total_times; long long flush_req; long long flush_total_times; + unsigned long long wr_highest_offset; }; int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d45c41f..1948b85 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1774,6 +1774,40 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, } +typedef enum { + QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET +} qemuMonitorBlockExtentError; + + +static int +qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev, + unsigned long long *extent) +{ + virJSONValuePtr stats; + virJSONValuePtr parent; + + if ((parent = virJSONValueObjectGet(dev, "parent")) == NULL || + parent->type != VIR_JSON_TYPE_OBJECT) { + return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT; + } + + if ((stats = virJSONValueObjectGet(parent, "stats")) == NULL || + stats->type != VIR_JSON_TYPE_OBJECT) { + return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS; + } + + if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", + extent) < 0) { + return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET; + } + + return QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK; +} + + int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, const char *dev_name, qemuBlockStatsPtr bstats, @@ -1910,6 +1944,9 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; } + /* it's ok to not have this information here. Just skip silently. */ + qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset); + count++; bstats++; @@ -2005,6 +2042,36 @@ int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon, return ret; } + +static int +qemuMonitorJSONReportBlockExtentError(qemuMonitorBlockExtentError error) +{ + switch (error) { + case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats parent entry was not in " + "expected format")); + break; + + case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats stats entry was not in " + "expected format")); + + case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot read %s statistic"), + "wr_highest_offset"); + break; + + case QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK: + return 0; + } + + return -1; +} + + int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, const char *dev_name, unsigned long long *extent) @@ -2039,9 +2106,8 @@ int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - virJSONValuePtr stats; - virJSONValuePtr parent; const char *thisdev; + int err; if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("blockstats device entry was not in expected format")); @@ -2065,24 +2131,9 @@ int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, continue; found = true; - if ((parent = virJSONValueObjectGet(dev, "parent")) == NULL || - parent->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats parent entry was not in expected format")); - goto cleanup; - } - - if ((stats = virJSONValueObjectGet(parent, "stats")) == NULL || - stats->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats stats entry was not in expected format")); - goto cleanup; - } - - if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", extent) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "wr_highest_offset"); + if ((err = qemuMonitorJSONDevGetBlockExtent(dev, extent)) != + QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK) { + qemuMonitorJSONReportBlockExtentError(err); goto cleanup; } } -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
Management software wants to be able to allocate disk space on demand. To support this they need keep track of the space occupation of the block device. This information is reported by qemu as part of block stats.
This patch extend the block information in the bulk stats with the allocation information.
To keep the same behaviour a helper is extracted from qemuMonitorJSONGetBlockExtent in order to get per-device allocation information.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/libvirt.c | 2 + src/qemu/qemu_driver.c | 18 +++++++++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 91 ++++++++++++++++++++++++++++++++++---------- 4 files changed, 92 insertions(+), 20 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index ab10a3a..a8892a2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21654,6 +21654,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * unsigned long long. * "block.<num>.errors" - Xen only: the 'oo_req' value as * unsigned long long. + * "block.<num>.allocation" - offset of the highest written sector + * as unsigned long long.
If we are repeating virDomainGetBlockInfo() here, I'd rather see us report all 3 pieces of information (allocation, capacity, physical) instead of just one.
+typedef enum { + QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET +} qemuMonitorBlockExtentError;
We require C99, so please have a trailing comma in the enum (it makes adding new elements easier, because they don't have to modify an existing line). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 09/17/14 00:37, Eric Blake wrote:
On 09/15/2014 09:42 AM, Peter Krempa wrote:
From: Francesco Romani <fromani@redhat.com>
Management software wants to be able to allocate disk space on demand. To support this they need keep track of the space occupation of the block device. This information is reported by qemu as part of block stats.
This patch extend the block information in the bulk stats with the allocation information.
To keep the same behaviour a helper is extracted from qemuMonitorJSONGetBlockExtent in order to get per-device allocation information.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/libvirt.c | 2 + src/qemu/qemu_driver.c | 18 +++++++++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 91 ++++++++++++++++++++++++++++++++++---------- 4 files changed, 92 insertions(+), 20 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index ab10a3a..a8892a2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21654,6 +21654,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * unsigned long long. * "block.<num>.errors" - Xen only: the 'oo_req' value as * unsigned long long. + * "block.<num>.allocation" - offset of the highest written sector + * as unsigned long long.
If we are repeating virDomainGetBlockInfo() here, I'd rather see us report all 3 pieces of information (allocation, capacity, physical) instead of just one.
+typedef enum { + QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS, + QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET +} qemuMonitorBlockExtentError;
We require C99, so please have a trailing comma in the enum (it makes adding new elements easier, because they don't have to modify an existing line).
Thanks for the reviews, I've pushed this series with the changes you've suggested except this patch as either I or Francesco will add the requested fields. Peter

State that full stats for the stats groups are available in the virConnectGetAllDomainStats documentation section rather than duplicating the docs. --- src/libvirt.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index a8892a2..03f7e5b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21734,14 +21734,8 @@ virConnectGetAllDomainStats(virConnectPtr conn, * followed by a group specific description of the statistic value. * * The statistic groups are enabled using the @stats parameter which is a - * binary-OR of enum virDomainStatsTypes. The following groups are available - * (although not necessarily implemented for each hypervisor): - * - * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that - * state. The typed parameter keys are in this format: - * "state.state" - state of the VM, returned as int from virDomainState enum - * "state.reason" - reason for entering given state, returned as int from - * virDomain*Reason enum corresponding to given state. + * binary-OR of enum virDomainStatsTypes. The stats groups are documented + * in along with virConnectGetAllDomainStats. * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
State that full stats for the stats groups are available in the virConnectGetAllDomainStats documentation section rather than duplicating the docs. --- src/libvirt.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index a8892a2..03f7e5b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21734,14 +21734,8 @@ virConnectGetAllDomainStats(virConnectPtr conn, * followed by a group specific description of the statistic value. * * The statistic groups are enabled using the @stats parameter which is a - * binary-OR of enum virDomainStatsTypes. The following groups are available - * (although not necessarily implemented for each hypervisor): - * - * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that - * state. The typed parameter keys are in this format: - * "state.state" - state of the VM, returned as int from virDomainState enum - * "state.reason" - reason for entering given state, returned as int from - * virDomain*Reason enum corresponding to given state. + * binary-OR of enum virDomainStatsTypes. The stats groups are documented + * in along with virConnectGetAllDomainStats.
s/along with // ACK with that simplification. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Add a note to make the users aware that some stats groups or fields may be missing in certain cases. --- src/libvirt.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index 03f7e5b..b3ddf95 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21657,6 +21657,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "block.<num>.allocation" - offset of the highest written sector * as unsigned long long. * + * Note that entire stats groups or individual stat fields may be missing from + * the output in case they are not supported by the given hypervisor, are not + * applicable for the current state of the guest domain or their retrieval + * was not successful. + * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
Add a note to make the users aware that some stats groups or fields may be missing in certain cases. --- src/libvirt.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK.
diff --git a/src/libvirt.c b/src/libvirt.c index 03f7e5b..b3ddf95 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21657,6 +21657,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "block.<num>.allocation" - offset of the highest written sector * as unsigned long long. * + * Note that entire stats groups or individual stat fields may be missing from + * the output in case they are not supported by the given hypervisor, are not + * applicable for the current state of the guest domain or their retrieval + * was not successful.
Depending on which grammar expert you ask, some people prefer "a, b, or c" while others prefer "a, b or c" when listing three or more things (the use of a comma before the conjunction that lists the last element of a list). I'm in the former group, but not strongly enough to request that others change their habits. So up to you if you want to add a comma before that last "or". -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 09/15/14 19:00, Eric Blake wrote:
On 09/15/2014 09:42 AM, Peter Krempa wrote:
Add a note to make the users aware that some stats groups or fields may be missing in certain cases. --- src/libvirt.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK.
diff --git a/src/libvirt.c b/src/libvirt.c index 03f7e5b..b3ddf95 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21657,6 +21657,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "block.<num>.allocation" - offset of the highest written sector * as unsigned long long. * + * Note that entire stats groups or individual stat fields may be missing from + * the output in case they are not supported by the given hypervisor, are not + * applicable for the current state of the guest domain or their retrieval + * was not successful.
Depending on which grammar expert you ask, some people prefer "a, b, or c" while others prefer "a, b or c" when listing three or more things (the use of a comma before the conjunction that lists the last element of a list). I'm in the former group, but not strongly enough to request that others change their habits. So up to you if you want to add a comma before that last "or".
Well, english is not my first neither second language so I'll gladly learn something new. I'll go with the first option as it makes sense to me in retrospect. (I should also get into the habit of re-reading things I write) Peter (No I didn't re-read this message :) )

Document the fields returned. --- tools/virsh.pod | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tools/virsh.pod b/tools/virsh.pod index b929480..789641c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -835,6 +835,53 @@ default all supported statistics groups are returned. Supported statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>, I<--vcpu>, I<--interface>, I<--block>. +When selecting the I<--state> group the following fields are returned: +"state.state" - state of the VM, returned as number from virDomainState enum, +"state.reason" - reason for entering given state, returned as int from, +virDomain*Reason enum corresponding to given state. + +I<--cpu-total> returns: +"cpu.time" - total cpu time spent for this domain, +"cpu.user" - user cpu time spent, +"cpu.system" - system cpu time spent + +I<--balloon> returns: +"balloon.current" - the memory in kiB currently used, +"balloon.maximum" - the maximum memory in kiB allowed + +I<--vcpu> returns: +"vcpu.current" - current number of online virtual CPUs as unsigned int, +"vcpu.maximum" - maximum number of online virtual CPUs as unsigned int, +"vcpu.<num>.state" - state of the virtual CPU <num>, as number +from virVcpuState enum, +"vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> + +I<--interface> returns: +"net.count" - number of network interfaces on this domain, +"net.<num>.name" - name of the interface <num>, +"net.<num>.rx.bytes" - number of bytes received, +"net.<num>.rx.pkts" - number of packets received, +"net.<num>.rx.errs" - number of receive errors, +"net.<num>.rx.drop" - number of receive packets dropped, +"net.<num>.tx.bytes" - number of bytes transmitted, +"net.<num>.tx.pkts" - number of packets transmitted, +"net.<num>.tx.errs" - number of transmission errors, +"net.<num>.tx.drop" - number of transmit packets dropped, + +I<block> returns: +"block.count" - number of block devices on this domain, +"block.<num>.name" - name of the block device <num>, +"block.<num>.rd.reqs" - number of read requests, +"block.<num>.rd.bytes" - number of read bytes, +"block.<num>.rd.times" - total time (ns) spent on reads, +"block.<num>.wr.reqs" - number of write requests, +"block.<num>.wr.bytes" - number of written bytes, +"block.<num>.wr.times" - total time (ns) spent on writes, +"block.<num>.fl.reqs" - total flush requests, +"block.<num>.fl.times" - total time (ns) spent on cache flushing, +"block.<num>.errors" - Xen only: the 'oo_req' value, +"block.<num>.allocation" - offset of the highest written sector + Selecting a specific statistics groups doesn't guarantee that the daemon supports the selected group of stats. Flag I<--enforce> forces the command to fail if the daemon doesn't support the -- 2.1.0

On 09/15/2014 09:42 AM, Peter Krempa wrote:
Document the fields returned. --- tools/virsh.pod | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/tools/virsh.pod b/tools/virsh.pod index b929480..789641c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -835,6 +835,53 @@ default all supported statistics groups are returned. Supported statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>, I<--vcpu>, I<--interface>, I<--block>.
+When selecting the I<--state> group the following fields are returned: +"state.state" - state of the VM, returned as number from virDomainState enum, +"state.reason" - reason for entering given state, returned as int from, +virDomain*Reason enum corresponding to given state.
Hmm, we still ought to get our pretty printer working, but that's a separate patch.
+ +I<--cpu-total> returns: +"cpu.time" - total cpu time spent for this domain, +"cpu.user" - user cpu time spent, +"cpu.system" - system cpu time spent
Probably should add "in nanoseconds" ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Peter Krempa