----- Original Message -----

From: "Peter Krempa" <pkrempa@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Tuesday, September 9, 2014 2:14:23 PM Subject: Re: [libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API

On 09/08/14 15:05, 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 connection 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.

Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d724eeb..2950a4b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c

...

@@ -17338,7 +17339,8 @@ qemuDomainGetStatsState(virDomainObjPtr dom,

typedef int -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, +(*qemuDomainGetStatsFunc)(virConnectPtr conn,

Looking through the rest of the series. Rather than the complete connection object you need just the virQEMUDriverPtr for entering the monitor, but I can live with this.

Since I need to resubmit to address your comments, I'll fix this to pass just virQEMUDriverPtr.

...

- if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0) + if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) goto cleanup;

- if (tmp) - tmpstats[nstats++] = tmp; + if ((needmon && virDomainObjIsActive(dom)) || !needmon) {

Hmm this skips offline domains entirely if one of the stats groups needs the monitor.

I think we should rather skip individual stats groups, or better stats fields that we can't provide.

Any ideas?

What about this (pseudo-C): unsigned int privflags = 0; if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) needmon = false; /* auto disable monitoring, the remainder of the function should be unchanged */ else privflags |= MONITOR_AVAILABLE; if ((needmon && virDomainObjIsActive(dom)) || !needmon) { if (qemuDomainGetStats(conn, dom, stats, &tmp, privflags) < 0) /* pass monitor availability down the chain. Individual workers will bail out immediately and silently if they need monitor but it is not available */ goto endjob; if (tmp) tmpstats[nstats++] = tmp; } No other change should be needed to this patch, and with trivial changes all the others can be fixed. -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani

On 09/08/14 15:05, 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 | 9 ++++++++ src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aced31c..e6ed803 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 4806535..4d504ff 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21554,6 +21554,15 @@ 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.count" - number as unsigned int of physical cpus available to + * this domain.

This is not really a VM property rather than a host property. I don't think we should report this as it will be the same for all VMs on the host.

+ * "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 2950a4b..cfc5941 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

@@ -17338,6 +17339,55 @@ qemuDomainGetStatsState(virConnectPtr conn ATTRIBUTE_UNUSED, }

+static int +qemuDomainGetStatsCpu(virConnectPtr conn 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 ncpus = 0; + int err; + + ncpus = nodeGetCPUCount();

As said, this is a host-wide value.

+ if (ncpus > 0 && + virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + "cpu.count", + (unsigned int)ncpus) < 0) + return -1;

Otherwise looks reasonable. Peter

On 2014/9/8 21:05, 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 | 9 ++++++++ src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) [...] diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2950a4b..cfc5941 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"

Hi, Francesco. I see the file including relationship. 'qemu_driver.c' includes 'qemu_cgroup.h' which includes 'vircgroup.h'. There are other virCgroupGet* functions called in qemu_driver.c now. So I think here "include vircgroup.h" is not necessary.

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -17338,6 +17339,55 @@ qemuDomainGetStatsState(virConnectPtr conn ATTRIBUTE_UNUSED, }

+static int +qemuDomainGetStatsCpu(virConnectPtr conn 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 ncpus = 0; + int err; + + ncpus = nodeGetCPUCount(); + if (ncpus > 0 && + virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + "cpu.count", + (unsigned int)ncpus) < 0) + return -1; + + 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;

1. If any of the 'err's is not zero, the function may returns 0 as success. Is this the expected return? Or at least we can give a warning that we miss some parameters. 2. I think it's better to report an error or warning log before return -1.

+ return 0; +} + + typedef int (*qemuDomainGetStatsFunc)(virConnectPtr conn, virDomainObjPtr dom, @@ -17353,6 +17403,7 @@ struct qemuDomainGetStatsWorker {

static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false }, + { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false }, { NULL, 0, false } };

On 09/10/14 15:46, Francesco Romani wrote:

----- Original Message -----

...

From: "Wang Rui" <moon.wangrui@huawei.com> To: "Francesco Romani" <fromani@redhat.com> Cc: libvir-list@redhat.com, pkrempa@redhat.com Sent: Wednesday, September 10, 2014 10:56:47 AM Subject: Re: [libvirt] [PATCHv3 2/8] qemu: bulk stats: implement CPU stats group

On 2014/9/8 21:05, 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 | 9 ++++++++ src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) [...] diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2950a4b..cfc5941 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"

Hi, Francesco. I see the file including relationship. 'qemu_driver.c' includes 'qemu_cgroup.h' which includes 'vircgroup.h'. There are other virCgroupGet* functions called in qemu_driver.c now. So I think here "include vircgroup.h" is not necessary.

Thanks for the research, I'll remove

I don't think that's necessary. We guard includes by #ifdef-ing it and also if something changes in the future we won't need to change the includes. If you use the functions from vircgroup.h, then just include it.

...

...

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -17338,6 +17339,55 @@ qemuDomainGetStatsState(virConnectPtr conn ATTRIBUTE_UNUSED, }

+static int +qemuDomainGetStatsCpu(virConnectPtr conn 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 ncpus = 0; + int err; + + ncpus = nodeGetCPUCount(); + if (ncpus > 0 && + virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + "cpu.count", + (unsigned int)ncpus) < 0) + return -1; + + 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;

1. If any of the 'err's is not zero, the function may returns 0 as success. Is this the expected return? Or at least we can give a warning that we miss some parameters. 2. I think it's better to report an error or warning log before return -1.

The idea here (well, at least my idea :) ) is to gather as much as data as possible, and to silently skip failures here. The lack of expected output is a good enough indicator that a domain is unresponsive. -1 is reported for really unrecoverable error, like memory (re)allocation failure.

That makes sense. I'd implement it the same way.

Otherwise, how could the client code be meaningfully informed that some group failed, maybe partially? It is possible that different groups fail for different domains: how could we convey this information?

I think that skipping information snippets that can't be gathered makes sense. (I hope that I've documented it that way) And if we'd like to make failure mandatory we still can add a flag. I think that this shouldn't be necessary though as long as the caller takes that into account.

I have no problems to go this route if there is consensus this is the preferred way to go, but then we'll need to discuss how to convey a meaningful error convention.

Bests,

Peter

On 09/08/14 15:05, 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 | 70 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e6ed803..1e4e428 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 4d504ff..f21eb39 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21562,6 +21562,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 cfc5941..4f8ccac 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2520,6 +2520,47 @@ 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 void +qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, + unsigned long *memory)

Use unsigned long long here. Unsigned long is 32 bit on 32bit systems.

+{ + 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 err; + unsigned long long balloon;

Note this ...

+ + qemuDomainObjEnterMonitor(driver, vm); + err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); + qemuDomainObjExitMonitor(driver, vm); + + if (err < 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 (err == 0) { + /* Balloon not supported, so maxmem is always the allocation */

Should we in such case drop the balloon stat from the output?

+ *memory = vm->def->mem.max_balloon; + } else { + *memory = balloon;

Here it'd break.

+ } + } +} + + static int qemuDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { @@ -17387,6 +17428,34 @@ qemuDomainGetStatsCpu(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; }

+static int +qemuDomainGetStatsBalloon(virConnectPtr conn, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + virQEMUDriverPtr driver = conn->privateData; + unsigned long cur_balloon = 0; + + qemuDomainGetBalloonMemory(driver, dom, &cur_balloon); + + if (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)(virConnectPtr conn, @@ -17404,6 +17473,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 } };

Peter

----- Original Message -----

From: "Peter Krempa" <pkrempa@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Tuesday, September 9, 2014 1:42:15 PM Subject: Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface group

...

+ * 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 long long. + * "net.<num>.rx.pkts" - packets received as long long. + * "net.<num>.rx.errs" - receive errors as long long. + * "net.<num>.rx.drop" - receive packets dropped as long long. + * "net.<num>.tx.bytes" - bytes transmitted as long long. + * "net.<num>.tx.pkts" - packets transmitted as long long. + * "net.<num>.tx.errs" - transmission errors as long long. + * "net.<num>.tx.drop" - transmit packets dropped as long long.

Why are all of those represented as long long instead of unsigned long long? I don't see how these could be negative. If we need to express that the value is unsupported we can just drop it from here and not waste half of the range here.

Any other opinions on this?

I used long long because of this: struct _virDomainInterfaceStats { long long rx_bytes; long long rx_packets; long long rx_errs; long long rx_drop; long long tx_bytes; long long tx_packets; long long tx_errs; long long tx_drop; }; But I don't have any problem to cast them as unsigned, with something like: #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.%u.%s", num, name); \ if (virTypedParamsAddULLong(&(record)->params, \ &(record)->nparams, \ maxparams, \ param_name, \ (unsigned long long)value) < 0) \ return -1; \ } while (0)

...

+ * * 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 6bcbfb5..989eb3e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17537,6 +17537,92 @@ qemuDomainGetStatsVcpu(virConnectPtr conn 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.%lu.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.%lu.%s", num, name); \

%lu? the count is unsigned int so you should be fine with %d

Yep but the cycle counter is size_t and then... $ git diff diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9d53883..e90a8c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17487,7 +17487,7 @@ do { \ do { \ char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \ snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \ - "net.%lu.%s", num, name); \ + "net.%u.%s", num, name); \ if (virTypedParamsAddLLong(&(record)->params, \ &(record)->nparams, \ maxparams, \ $ make [...] make[1]: Entering directory `/home/fromani/Projects/libvirt/src' CC qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo qemu/qemu_driver.c: In function 'qemuDomainGetStatsInterface': qemu/qemu_driver.c:17521:9: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' [-Werror=format=] QEMU_ADD_NET_PARAM(record, maxparams, i, ^ qemu/qemu_driver.c:17521:9: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' [-Werror=format=] $ gcc --version gcc (GCC) 4.8.3 20140624 (Red Hat 4.8.3-1) same story with '%d'. Keeping '%lu' for this reason. -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani

On 09/08/14 15:05, 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 | 17 ++++++ src/qemu/qemu_driver.c | 93 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 22 +++++++ src/qemu/qemu_monitor.h | 20 +++++++ src/qemu/qemu_monitor_json.c | 135 ++++++++++++++++++++++++++++++------------- src/qemu/qemu_monitor_json.h | 4 ++ src/qemu/qemu_monitor_text.c | 13 +++++ src/qemu/qemu_monitor_text.h | 4 ++ 9 files changed, 269 insertions(+), 40 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 93aa1fb..c8e0089 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 8aa6cb1..e041fa2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21596,6 +21596,23 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "net.<num>.tx.errs" - transmission errors as long long. * "net.<num>.tx.drop" - transmit packets dropped as 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 long long. + * "block.<num>.rd.bytes" - number of read bytes as long long. + * "block.<num>.rd.times" - total time (ns) spent on reads as long long. + * "block.<num>.wr.reqs" - number of write requests as long long. + * "block.<num>.wr.bytes" - number of written bytes as long long. + * "block.<num>.wr.times" - total time (ns) spent on writes as long long. + * "block.<num>.fl.reqs" - total flush requests as long long. + * "block.<num>.fl.times" - total time (ns) spent on cache flushing + * as long long. + * "block.<num>.errors" - Xen only: the 'oo_req' value as long long.

Same as for the interface stats. How about using unsigned long long and dropping stats that are not available?

+ * * 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 989eb3e..93afb7e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9678,6 +9678,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, @@ -17620,6 +17645,73 @@ qemuDomainGetStatsInterface(virConnectPtr conn ATTRIBUTE_UNUSED,

#undef QEMU_ADD_NET_PARAM

+#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.%lu.%s", num, name); \ + if (virTypedParamsAddLLong(&(record)->params, \ + &(record)->nparams, \ + maxparams, \ + param_name, \ + value) < 0) \ + goto cleanup; \ +} while (0) + +static int +qemuDomainGetStatsBlock(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + int ret = -1; + int nstats = 0; + qemuBlockStatsPtr stats = NULL; + virQEMUDriverPtr driver = conn->privateData; + + 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 + #undef QEMU_ADD_NAME_PARAM

#undef QEMU_ADD_COUNT_PARAM @@ -17643,6 +17735,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..d8ce875 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -29,6 +29,8 @@ #include <unistd.h> #include <fcntl.h>

+#include "internal.h" + #include "qemu_monitor.h" #include "qemu_monitor_text.h" #include "qemu_monitor_json.h" @@ -1760,6 +1762,26 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, return ret; }

+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 { + ret = qemuMonitorTextGetAllBlockStatsInfo(mon, dev_name, + stats, nstats); + } + + 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 4373ba2..aa95e71 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1734,13 +1734,9 @@ 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 nstats = 1; + int ret = -1;

*rd_req = *rd_bytes = -1; *wr_req = *wr_bytes = *errs = -1; @@ -1754,9 +1750,51 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, if (flush_total_times) *flush_total_times = -1;

+ if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, + &stats, nstats) != nstats)

Writing '1' instead of 'nstats' would make it a bit more readable as you don't expect to get more or less stats.

+ 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; + size_t i; + bool found = false; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats", + NULL); + virJSONValuePtr reply = NULL; + virJSONValuePtr devices; + if (!cmd) return -1;

+ if (!bstats || nstats <= 0) + return -1;

These are okay, they don't report an error, but also are for develoeper errors only.

+ ret = qemuMonitorJSONCommand(mon, cmd, &reply);

if (ret == 0) @@ -1772,108 +1810,125 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; }

+ ret = 0;

Here you set ret to 0.

for (i = 0; i < virJSONValueArraySize(devices); i++) {

After a few iterations it may be > 0. (Or 0 on the first)

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")); + _("blockstats device entry was not " + "in expected format")); goto cleanup;

If you hit this error, you return ret, which is now in success mode so no one will see it.

}

- if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats device entry was not in expected format")); - goto cleanup; + if (ret > nstats) { + break; }

- /* 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; + }

- 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; } + + ret++; + bstats++; }

- if (!found) { + if (dev_name && !found) {

'found' here will be true if and only if ret == 1, so you don't really need a separate variable

virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find statistics for device '%s'"), dev_name); + ret = 0;

I'd return -1 here as you've reported an error.

goto cleanup; } - ret = 0;

cleanup: virJSONValueFree(cmd);

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 2bc8261..5cdbef3 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1048,6 +1048,19 @@ int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon ATTRIBUTE_UNUSED, return -1; }

+ +int +qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + const char *dev_name ATTRIBUTE_UNUSED, + qemuBlockStatsPtr blockstats ATTRIBUTE_UNUSED, + int nstats ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to query all block stats with this QEMU"));

You can save adding this function and fold it right into the monitor wrapper function as we do now with commadns that don't have HMP counterparts.

+ return -1; +} + + /* Return 0 on success, -1 on failure, or -2 if not supported. Size * is in bytes. */ int qemuMonitorTextBlockResize(qemuMonitorPtr mon,

Peter

On 09/08/14 15:05, Francesco Romani wrote:

Management software, want to be able to allocate disk space on demand.

s/, want/ wants/

To support this, they need keep track of the space occupation

s/,//

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, an helper is extracted from

s/,// s/an/a/

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 | 15 +++++++++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 76 ++++++++++++++++++++++++++++++++------------ 4 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index aa95e71..bc5616d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1776,6 +1776,55 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, }

+/* This helper function could be called in the + * qemuMonitorJSONGetAllBlockStatsInfo + * path - which is used also by the + * qemuMonitorJSONGetBlockStatsInfo + * path. In this case, we don't know in advance if the wr_highest_offset + * field is there, so it is OK to fail silently. + * However, we can get here by the + * qemuMonitorJSONGetBlockExtent + * path, and in that case we _must_ fail loudly. + */ +static int +qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev, + bool report_error, + unsigned long long *extent) +{ + virJSONValuePtr stats; + virJSONValuePtr parent; + + if ((parent = virJSONValueObjectGet(dev, "parent")) == NULL || + parent->type != VIR_JSON_TYPE_OBJECT) { + if (report_error) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats parent entry was not in " + "expected format")); + return -1; + } + + if ((stats = virJSONValueObjectGet(parent, "stats")) == NULL || + stats->type != VIR_JSON_TYPE_OBJECT) { + if (report_error) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats stats entry was not in " + "expected format")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", + extent) < 0) { + if (report_error) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot read %s statistic"), + "wr_highest_offset"); + return -1; + } + + return 0; +} + + int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, const char *dev_name, qemuBlockStatsPtr bstats, @@ -1919,6 +1968,10 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; }

+ /* it's ok to not have this information here. Just skip silently. */ + qemuMonitorJSONDevGetBlockExtent(dev, false, + &bstats->wr_highest_offset);

As you want to ignore errors, it would probably be better just to copy the extraction code here without error reporting rather than extracting it to a helper ... this isn't something that would be reused any more.

+ ret++; bstats++; } @@ -2010,6 +2063,7 @@ int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon, return ret; }

+ int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, const char *dev_name, unsigned long long *extent) @@ -2044,8 +2098,6 @@ int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,

for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - virJSONValuePtr stats; - virJSONValuePtr parent; const char *thisdev; if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2070,26 +2122,8 @@ 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")); + if (qemuMonitorJSONDevGetBlockExtent(dev, true, extent) < 0) 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"); - goto cleanup; - } }

if (!found) {

Peter