[libvirt] [PATCHv4 0/8] bulk stats: QEMU implementation
This patchset enhances the QEMU support for the new bulk stats API to include equivalents of these APIs: virDomainBlockInfo virDomainGetInfo - for balloon stats virDomainGetCPUStats virDomainBlockStatsFlags virDomainInterfaceStats virDomainGetVcpusFlags virDomainGetVcpus This subset of API is the one oVirt relies on. Scale/stress test on an oVirt test environment is in progress. The patchset is organized as follows: - the first patch enhances the internal stats gathering API to accomodate the needs of the groups which extract information using QEMU monitor jobs. - the next five patches implement the bulk stats groups, extracting helpers where do refactoring to extract internal helpers every time it is feasible and convenient. - the seventh patch enhances the virsh domstats command with options to use the new bulk stats. - the last patch enhances the block stats group adding the wr_highest_offset information, needed by oVirt for thin provisioned disks. ChangeLog v4: fixes and cleanups - addressed reviewers comments (Peter, Wang Rui). - pushed domain check into group stats functions. This follows the strategy to gather and report as much data as possible, silently skipping errors along the way. - moved the block allocation patch to the end of the series. v3: more polishing and fixes after first review - addressed Eric's comments. - squashed patches which extracts helpers with patches which use them. - changed gathering strategy: now code tries to reap as much information as possible instead to give up and bail out with error. Only critical errors cause the bulk stats to fail. - moved away from the transfer semantics. I find it error-prone and not flexible enough, I'd like to avoid as much as possible. - rearranged helpers to have one single QEMU query job with many monitor jobs nested inside. - fixed docs. - implemented missing virsh domstats bits. in v2: polishing and optimizations. - incorporated feedback from Li Wei (thanks). - added documentation. - optimized block group to gather all the information with just one call to QEMU monitor. - stripped to bare bones merged the 'block info' group into the 'block' group - oVirt actually needs just one stat from there. - reorganized the keys to be more consistent and shorter. 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 include/libvirt/libvirt.h.in | 5 + src/libvirt.c | 61 +++++ src/qemu/qemu_driver.c | 575 +++++++++++++++++++++++++++++++++++++------ 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 | 4 +- 9 files changed, 822 insertions(+), 136 deletions(-) -- 1.9.3
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, thus we must start a query job for each domain. The specific collectors will run nested monitor jobs inside that. * although requested, monitor could be not available. pass a flag to workers to signal the availability of monitor, in order to gather as much data as is possible anyway. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 917b286..279c8b3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17244,7 +17244,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) @@ -17267,8 +17268,17 @@ qemuDomainGetStatsState(virDomainObjPtr dom, } +typedef enum { + QEMU_DOMAIN_STATS_HAVE_MONITOR = (1 << 0), /* QEMU monitor available */ +} qemuDomainStatsFlags; + + +#define HAVE_MONITOR(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_MONITOR) + + typedef int -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, +(*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int flags); @@ -17276,11 +17286,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 } }; @@ -17312,6 +17323,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, @@ -17329,8 +17354,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; } } @@ -17369,6 +17394,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, int nstats = 0; size_t i; int ret = -1; + unsigned int privflags = 0; if (ndoms) virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); @@ -17403,6 +17429,9 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) goto cleanup; + if (qemuDomainGetStatsNeedMonitor(stats)) + privflags |= QEMU_DOMAIN_STATS_HAVE_MONITOR; + for (i = 0; i < ndoms; i++) { virDomainStatsRecordPtr tmp = NULL; @@ -17413,12 +17442,22 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) continue; - if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0) - goto cleanup; + if (HAVE_MONITOR(privflags) && + qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) + /* As it was never requested. Gather as much as possible anyway. */ + privflags &= ~QEMU_DOMAIN_STATS_HAVE_MONITOR; + + if (qemuDomainGetStats(conn, dom, stats, &tmp, privflags) < 0) + goto endjob; if (tmp) tmpstats[nstats++] = tmp; + if (HAVE_MONITOR(privflags) && !qemuDomainObjEndJob(driver, dom)) { + dom = NULL; + goto cleanup; + } + virObjectUnlock(dom); dom = NULL; } @@ -17428,6 +17467,11 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, ret = nstats; + endjob: + if (HAVE_MONITOR(privflags) && dom) + if (!qemuDomainObjEndJob(driver, dom)) + dom = NULL; + cleanup: if (dom) virObjectUnlock(dom); -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
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, thus we must start a query job for each domain. The specific collectors will run nested monitor jobs inside that. * although requested, monitor could be not available. pass a flag to workers to signal the availability of monitor, in order to gather as much data as is possible anyway.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 917b286..279c8b3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17244,7 +17244,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) @@ -17267,8 +17268,17 @@ qemuDomainGetStatsState(virDomainObjPtr dom, }
+typedef enum { + QEMU_DOMAIN_STATS_HAVE_MONITOR = (1 << 0), /* QEMU monitor available */ +} qemuDomainStatsFlags; + + +#define HAVE_MONITOR(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_MONITOR) + + typedef int -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, +(*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int flags); @@ -17276,11 +17286,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 } };
@@ -17312,6 +17323,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, @@ -17329,8 +17354,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; } } @@ -17369,6 +17394,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, int nstats = 0; size_t i; int ret = -1; + unsigned int privflags = 0;
if (ndoms) virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); @@ -17403,6 +17429,9 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) goto cleanup;
+ if (qemuDomainGetStatsNeedMonitor(stats)) + privflags |= QEMU_DOMAIN_STATS_HAVE_MONITOR; + for (i = 0; i < ndoms; i++) { virDomainStatsRecordPtr tmp = NULL;
@@ -17413,12 +17442,22 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) continue;
- if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0) - goto cleanup; + if (HAVE_MONITOR(privflags) && + qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) + /* As it was never requested. Gather as much as possible anyway. */ + privflags &= ~QEMU_DOMAIN_STATS_HAVE_MONITOR;
This masks out the monitor for every subsequent domain too. If a single domain is stuck in the job, other can work, so this should be done on a per-domain basis.
+ + if (qemuDomainGetStats(conn, dom, stats, &tmp, privflags) < 0) + goto endjob;
if (tmp) tmpstats[nstats++] = tmp;
+ if (HAVE_MONITOR(privflags) && !qemuDomainObjEndJob(driver, dom)) { + dom = NULL; + goto cleanup; + } + virObjectUnlock(dom); dom = NULL; } @@ -17428,6 +17467,11 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
ret = nstats;
+ endjob: + if (HAVE_MONITOR(privflags) && dom) + if (!qemuDomainObjEndJob(driver, dom)) + dom = NULL; + cleanup: if (dom) virObjectUnlock(dom);
Otherwise looks good. I'll see how the rest of the series goes and if this will be one of few problems I'll fix it myself. Peter
This patch implements the VIR_DOMAIN_STATS_CPU_TOTAL group of statistics. Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 7 +++++++ src/qemu/qemu_driver.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..8665c6c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2511,6 +2511,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 941c518..b22f9aa 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21563,6 +21563,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 279c8b3..9014976 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 @@ -17276,6 +17277,45 @@ typedef enum { #define HAVE_MONITOR(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_MONITOR) +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; + + 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, @@ -17291,6 +17331,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, + { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, { NULL, 0, false } }; -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
This patch implements the VIR_DOMAIN_STATS_CPU_TOTAL group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 7 +++++++ src/qemu/qemu_driver.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+)
ACK, Peter
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 | 73 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 8665c6c..c005442 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2512,6 +2512,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 b22f9aa..3fa86ab 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21569,6 +21569,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 9014976..f677884 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2525,6 +2525,48 @@ static int qemuDomainSendKey(virDomainPtr domain, return ret; } + +/* + * FIXME: this code is a stripped down version of what is done into + * qemuDomainGetInfo. Due to the different handling of jobs, it is not + * trivial to extract a common helper function. + */ +static int +qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, + unsigned long long *memory) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (vm->def->memballoon && + vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { + *memory = vm->def->mem.max_balloon; + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { + *memory = vm->def->mem.cur_balloon; + } else { + int rv; + unsigned long long balloon; + + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorGetBalloonInfo(priv->mon, &balloon); + qemuDomainObjExitMonitor(driver, vm); + + if (rv < 0) { + /* We couldn't get current memory allocation but that's not + * a show stopper; we wouldn't get it if there was a job + * active either + */ + *memory = vm->def->mem.cur_balloon; + } else if (rv > 0) { + *memory = balloon; + } else { + /* Balloon not supported, so maxmem is always the allocation */ + return -1; + } + } + return 0; +} + + static int qemuDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { @@ -17315,6 +17357,36 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return 0; } +static int +qemuDomainGetStatsBalloon(virQEMUDriverPtr driver, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags) +{ + if (HAVE_MONITOR(privflags) && virDomainObjIsActive(dom)) { + unsigned long long cur_balloon = 0; + int err = 0; + + err = qemuDomainGetBalloonMemory(driver, dom, &cur_balloon); + + 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, @@ -17332,6 +17404,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 } }; -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
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 | 73 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+)
Just one small nit:
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 8665c6c..c005442 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2512,6 +2512,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 b22f9aa..3fa86ab 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21569,6 +21569,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 9014976..f677884 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2525,6 +2525,48 @@ static int qemuDomainSendKey(virDomainPtr domain, return ret; }
+ +/* + * FIXME: this code is a stripped down version of what is done into + * qemuDomainGetInfo. Due to the different handling of jobs, it is not + * trivial to extract a common helper function. + */ +static int +qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, + unsigned long long *memory) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (vm->def->memballoon && + vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { + *memory = vm->def->mem.max_balloon; + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) {
If qemu supports the BALLOON_EVENT, you get the right data even if you can't acquire the job ...
+ *memory = vm->def->mem.cur_balloon; + } else { + int rv; + unsigned long long balloon; + + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorGetBalloonInfo(priv->mon, &balloon); + qemuDomainObjExitMonitor(driver, vm); + + if (rv < 0) { + /* We couldn't get current memory allocation but that's not + * a show stopper; we wouldn't get it if there was a job + * active either + */ + *memory = vm->def->mem.cur_balloon; + } else if (rv > 0) { + *memory = balloon; + } else { + /* Balloon not supported, so maxmem is always the allocation */ + return -1; + } + } + return 0; +} + + static int qemuDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { @@ -17315,6 +17357,36 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return 0; }
+static int +qemuDomainGetStatsBalloon(virQEMUDriverPtr driver, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags) +{ + if (HAVE_MONITOR(privflags) && virDomainObjIsActive(dom)) { + unsigned long long cur_balloon = 0; + int err = 0; + + err = qemuDomainGetBalloonMemory(driver, dom, &cur_balloon);
... but you'd skip it now. As the helper is separate anyways, you may inline the code here and have better control over what happens according to the presence of the monitor.
+ + 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, @@ -17332,6 +17404,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 } };
ACK, current version looks fine, but if you wan't to do it a bit more flexible you can follow up with another version. Peter
On 09/12/2014 07:05 AM, Peter Krempa wrote:
On 09/12/14 13:48, Francesco Romani wrote:
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 | 73 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+)
Just one small nit:
+ + if (vm->def->memballoon && + vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { + *memory = vm->def->mem.max_balloon; + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) {
If qemu supports the BALLOON_EVENT, you get the right data even if you can't acquire the job ...
Oh, that's right. The reason we strongly prefer the event instead of the old command is that the old command blocked until the guest responded, but an uncooperative guest can use that to cause denial of service. Most qemu QMP commands reply immediately without waiting for guest interaction; and the balloon event allowed the same behavior there. So I think we do NOT want to allow balloon stats to work UNLESS qemu is new enough to provide them without guest interaction. If we DO allow guest interaction, then we need to modify remote_protocol.x to add ACL checks, so that the ACL controls can deny an unprivileged user from attempting a query that will potentially starve a privileged user. So I'd rather go with the stance that we cannot query stats that would require guest interaction, or at a minimum, gate things by having an explicit flag the caller must pass to acknowledge the risk (and having a flag lets us have a conditional ACL check, where the common case of not using the flag doesn't need to be slowed down by a check). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 12 +++ src/qemu/qemu_driver.c | 200 +++++++++++++++++++++++++++++-------------- 3 files changed, 149 insertions(+), 64 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c005442..00eafc6 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,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 3fa86ab..ba7a780 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21576,6 +21576,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 f677884..12bf5e4 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) { @@ -5001,10 +5071,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; @@ -5019,67 +5086,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) @@ -17388,6 +17395,70 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver, 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) { + 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.%u.state", cpuinfo[i].number); + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].state) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "vcpu.%u.time", cpuinfo[i].number); + 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, @@ -17405,6 +17476,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 } }; -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
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> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 12 +++ src/qemu/qemu_driver.c | 200 +++++++++++++++++++++++++++++-------------- 3 files changed, 149 insertions(+), 64 deletions(-)
@@ -17388,6 +17395,70 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver, 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) { + ret = 0; /* it's ok to be silent and go ahead */
virResetLastError() as the function would report one.
+ goto cleanup; + } + + for (i = 0; i < dom->def->vcpus; i++) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "vcpu.%u.state", cpuinfo[i].number); + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].state) < 0) + goto cleanup; +
ACK otherwise. Peter
This patch implements the VIR_DOMAIN_STATS_INTERFACE group of statistics. Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 14 +++++++ src/qemu/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 00eafc6..b73d14b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2514,6 +2514,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 ba7a780..d9ffb6f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21588,6 +21588,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 12bf5e4..7e5d707 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17458,6 +17458,92 @@ 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; + + 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 (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) + continue; + + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", i, dom->def->nets[i]->ifname); + + 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, @@ -17477,6 +17563,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 } }; -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
This patch implements the VIR_DOMAIN_STATS_INTERFACE group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 14 +++++++ src/qemu/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 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; + + 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 (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0)
Again, reset the error as it's ignored.
+ continue; + + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", i, dom->def->nets[i]->ifname); + + 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,
ACK otherwise. Peter
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> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 20 +++++++ src/qemu/qemu_driver.c | 96 ++++++++++++++++++++++++++++++ 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, 260 insertions(+), 43 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b73d14b..da4b58e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2515,6 +2515,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 d9ffb6f..8611361 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21602,6 +21602,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 7e5d707..4644f4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9687,6 +9687,31 @@ qemuDomainBlockStats(virDomainPtr dom, return ret; } + +/* + * Returns at most the first `nstats' stats, then stops. + * Returns the number of stats filled. + */ +static int +qemuDomainHelperGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuBlockStatsPtr stats, + int nstats) +{ + int ret; + qemuDomainObjPrivatePtr priv = vm->privateData; + + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats); + + qemuDomainObjExitMonitor(driver, vm); + + return ret; +} + + static int qemuDomainBlockStatsFlags(virDomainPtr dom, const char *path, @@ -17541,6 +17566,76 @@ 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 = 0; + qemuBlockStatsPtr stats = NULL; + + if (!HAVE_MONITOR(privflags) || !virDomainObjIsActive(dom)) + return 0; /* it's ok, just go ahead silently */ + + if (VIR_ALLOC_N(stats, dom->def->ndisks) < 0) + return -1; + + nstats = qemuDomainHelperGetBlockStats(driver, dom, stats, + dom->def->ndisks); + if (nstats < 0) + 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 @@ -17564,6 +17659,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 702404a..9fe13bb 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 ced198e..8e3fb44 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 30f9ffb..847dcd4 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 a6d05b5..0744241 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, -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
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> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 20 +++++++ src/qemu/qemu_driver.c | 96 ++++++++++++++++++++++++++++++ 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, 260 insertions(+), 43 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7e5d707..4644f4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9687,6 +9687,31 @@ qemuDomainBlockStats(virDomainPtr dom, return ret; }
+ +/* + * Returns at most the first `nstats' stats, then stops. + * Returns the number of stats filled. + */ +static int +qemuDomainHelperGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuBlockStatsPtr stats, + int nstats) +{ + int ret; + qemuDomainObjPrivatePtr priv = vm->privateData; + + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats);
Humm, is it worth doing this helper? This pretty much can be inlined as it has only one caller.
+ + qemuDomainObjExitMonitor(driver, vm); + + return ret; +} + + static int qemuDomainBlockStatsFlags(virDomainPtr dom, const char *path, @@ -17541,6 +17566,76 @@ 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 = 0; + qemuBlockStatsPtr stats = NULL; + + if (!HAVE_MONITOR(privflags) || !virDomainObjIsActive(dom)) + return 0; /* it's ok, just go ahead silently */ + + if (VIR_ALLOC_N(stats, dom->def->ndisks) < 0) + return -1; + + nstats = qemuDomainHelperGetBlockStats(driver, dom, stats, + dom->def->ndisks); + if (nstats < 0)
Are we erroring out on block stats failure? Other statistics gatherers just skip it if it's not available.
+ 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
Otherwise looks good. Peter
----- Original Message -----
From: "Peter Krempa" <pkrempa@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Friday, September 12, 2014 3:56:06 PM Subject: Re: [libvirt] [PATCHv4 6/8] qemu: bulk stats: implement block group
On 09/12/14 13:48, Francesco Romani wrote:
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> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 20 +++++++ src/qemu/qemu_driver.c | 96 ++++++++++++++++++++++++++++++ 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, 260 insertions(+), 43 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7e5d707..4644f4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9687,6 +9687,31 @@ qemuDomainBlockStats(virDomainPtr dom, return ret; }
+ +/* + * Returns at most the first `nstats' stats, then stops. + * Returns the number of stats filled. + */ +static int +qemuDomainHelperGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuBlockStatsPtr stats, + int nstats) +{ + int ret; + qemuDomainObjPrivatePtr priv = vm->privateData; + + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats);
Humm, is it worth doing this helper? This pretty much can be inlined as it has only one caller.
Right, qemuDomainHelperGetBlockStats add little to none value, so I'll drop it. I believe qemuMonitorGetAllBlockStatsInfo should stay, however: I don't see JSON monitor being called directly anywhere. So I'll keep it. [...]
+static int +qemuDomainGetStatsBlock(virQEMUDriverPtr driver, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags) +{ + size_t i; + int ret = -1; + int nstats = 0; + qemuBlockStatsPtr stats = NULL; + + if (!HAVE_MONITOR(privflags) || !virDomainObjIsActive(dom)) + return 0; /* it's ok, just go ahead silently */ + + if (VIR_ALLOC_N(stats, dom->def->ndisks) < 0) + return -1; + + nstats = qemuDomainHelperGetBlockStats(driver, dom, stats, + dom->def->ndisks); + if (nstats < 0)
Are we erroring out on block stats failure? Other statistics gatherers just skip it if it's not available.
Right, will fix to make it silently skip as the other groups. Bests, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
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 60ee515..0a316dd 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> -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
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(-)
ACK stands, but please bump the version to be consistent with the rest next time. Git applied this one first as it ordered before the rest and of course it failed to build then :) Peter
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 8611361..294b948 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21621,6 +21621,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 4644f4a..4931e93 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17580,6 +17580,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, @@ -17625,6 +17638,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; @@ -17636,6 +17652,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 8e3fb44..97d7336 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 847dcd4..31b1676 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; } } -- 1.9.3
On 09/12/14 13:48, Francesco Romani wrote:
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(-)
The error reporting is a bit unusual, but should work fine. Thus: ACK Peter
participants (3)
-
Eric Blake -
Francesco Romani -
Peter Krempa