[libvirt] [PATCHv2 00/11] 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. changes 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. The patchset is organized as follows: - the first 4 patches do refactoring to extract internal helper functions to be used by the old API and by the new bulk one. For block stats on helper is actually added instead of extracted. - since some groups require access to the QEMU monitor, one patch extend the internal interface to easily accomodate that - finally, the last six patches implement the support for the bulk API. Francesco Romani (11): qemu: extract helper to get the current balloon qemu: extract helper to gather vcpu data qemu: add helper to get the block stats qemu: report highest offset into block stats qemu: bulk stats: pass connection to workers 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 qemu: bulk stats: add block allocation information include/libvirt/libvirt.h.in | 5 + src/libvirt.c | 47 ++++ src/qemu/qemu_driver.c | 500 +++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_monitor.c | 23 ++ src/qemu/qemu_monitor.h | 19 ++ src/qemu/qemu_monitor_json.c | 125 +++++++---- src/qemu/qemu_monitor_json.h | 4 + 7 files changed, 639 insertions(+), 84 deletions(-) -- 1.9.3
Refactor the code to extract an helper method to get the current balloon settings. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 239a300..bbd16ed 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -168,6 +168,9 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver); +static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned long *memory); virQEMUDriverPtr qemu_driver = NULL; @@ -2519,6 +2522,60 @@ static int qemuDomainSendKey(virDomainPtr domain, return ret; } +static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned long *memory) +{ + int ret = -1; + int err = 0; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((vm->def->memballoon != NULL) && + (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 if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { + unsigned long long balloon; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) + err = 0; + else { + qemuDomainObjEnterMonitor(driver, vm); + err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); + qemuDomainObjExitMonitor(driver, vm); + } + if (!qemuDomainObjEndJob(driver, vm)) { + vm = NULL; + goto cleanup; + } + + 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 */ + *memory = vm->def->mem.max_balloon; + } else { + *memory = balloon; + } + } else { + *memory = vm->def->mem.cur_balloon; + } + + ret = 0; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + static int qemuDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { @@ -2526,7 +2583,6 @@ static int qemuDomainGetInfo(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; int err; - unsigned long long balloon; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -2549,43 +2605,9 @@ static int qemuDomainGetInfo(virDomainPtr dom, info->maxMem = vm->def->mem.max_balloon; if (virDomainObjIsActive(vm)) { - qemuDomainObjPrivatePtr priv = vm->privateData; - - if ((vm->def->memballoon != NULL) && - (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) { - info->memory = vm->def->mem.max_balloon; - } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { - info->memory = vm->def->mem.cur_balloon; - } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - if (!virDomainObjIsActive(vm)) - err = 0; - else { - qemuDomainObjEnterMonitor(driver, vm); - err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - qemuDomainObjExitMonitor(driver, vm); - } - if (!qemuDomainObjEndJob(driver, vm)) { - vm = NULL; - goto cleanup; - } - - 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 - */ - info->memory = vm->def->mem.cur_balloon; - } else if (err == 0) { - /* Balloon not supported, so maxmem is always the allocation */ - info->memory = vm->def->mem.max_balloon; - } else { - info->memory = balloon; - } - } else { - info->memory = vm->def->mem.cur_balloon; - } + err = qemuDomainGetBalloonMemory(driver, vm, &info->memory); + if (err) + return err; } else { info->memory = 0; } -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Refactor the code to extract an helper method to get the current balloon settings.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 239a300..bbd16ed 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -168,6 +168,9 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver);
+static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned long *memory);
Forward declarations of non-recursive static functions is usually a sign that you didn't topologically sort your code correctly. Just implement the function here, instead of later on.
virQEMUDriverPtr qemu_driver = NULL;
@@ -2519,6 +2522,60 @@ static int qemuDomainSendKey(virDomainPtr domain, return ret; }
+static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned long *memory)
Libvirt style is tending towards two blank lines between functions, and return type on separate line (although we don't enforce either of these yet, due to the large existing code base that used other styles), as in: static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, ...
+{ + int ret = -1; + int err = 0; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((vm->def->memballoon != NULL) && + (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) {
[1] Over-parenthesized. Sufficient to write: 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 if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { + unsigned long long balloon; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) + err = 0; + else {
[2] If one leg of if-else has {}, both legs must have it. This is documented in HACKING (and I really ought to add a syntax check that forbids obvious cases of unbalanced braces).
+ + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +}
[3] Ouch. This function is unlocking vm, even though it did not obtain the lock. Which it kind of has to do because of the way that qemuDomainObjEndJob may end up invalidating vm. While transfer semantics are workable, they require good comments at the start of the function, and making sure that the caller doesn't duplicate the efforts, nor forget anything else.
@@ -2526,7 +2583,6 @@ static int qemuDomainGetInfo(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; int err; - unsigned long long balloon;
if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -2549,43 +2605,9 @@ static int qemuDomainGetInfo(virDomainPtr dom, info->maxMem = vm->def->mem.max_balloon;
if (virDomainObjIsActive(vm)) { - qemuDomainObjPrivatePtr priv = vm->privateData; - - if ((vm->def->memballoon != NULL) && - (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) {
[1] Then again, your parenthesis...
- info->memory = vm->def->mem.max_balloon; - } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { - info->memory = vm->def->mem.cur_balloon; - } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - if (!virDomainObjIsActive(vm)) - err = 0; - else {
[2] ...and broken if-else bracing is just code motion. So I could overlook those (although cleaning them up at some point in the series, even if in a separate patch, would still be nice). But
- if (!qemuDomainObjEndJob(driver, vm)) { - vm = NULL; - goto cleanup; - } - - 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 - */ - info->memory = vm->def->mem.cur_balloon; - } else if (err == 0) { - /* Balloon not supported, so maxmem is always the allocation */ - info->memory = vm->def->mem.max_balloon; - } else { - info->memory = balloon; - } - } else { - info->memory = vm->def->mem.cur_balloon; - } + err = qemuDomainGetBalloonMemory(driver, vm, &info->memory); + if (err) + return err;
[3] the fact that you don't use 'goto cleanup' here, but rely on the awkward transfer semantics, is just confusing the situation. I'd rather avoid transfer semantics, but understand why you used them. But I'd still rewrite this as: err = qemuDomainGetBalloonMemory(...); vm = NULL; and fall through to the cleanup label, rather than doing an early return, to make it obvious that the call did a transfer of vm. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
From: "Eric Blake" <eblake@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Tuesday, September 2, 2014 11:01:25 PM Subject: Re: [libvirt] [PATCH 01/11] qemu: extract helper to get the current balloon
Hi Eric, thanks for the review(s).
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Refactor the code to extract an helper method to get the current balloon settings.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 239a300..bbd16ed 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -168,6 +168,9 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver);
+static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned long *memory);
Forward declarations of non-recursive static functions is usually a sign that you didn't topologically sort your code correctly. Just implement the function here, instead of later on.
Will do.
virQEMUDriverPtr qemu_driver = NULL;
@@ -2519,6 +2522,60 @@ static int qemuDomainSendKey(virDomainPtr domain, return ret; }
+static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned long *memory)
Libvirt style is tending towards two blank lines between functions,
My mistake. I did run 'make syntax-check', I wonder if that was supposed to catch this.
and return type on separate line (although we don't enforce either of these yet, due to the large existing code base that used other styles), as in: static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, ...
I was a bit confused by the mixed styles found in the code. Will stick with the one you pointed out in this and in future patches.
+{ + int ret = -1; + int err = 0; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((vm->def->memballoon != NULL) && + (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) {
[1] Over-parenthesized. Sufficient to write:
if (vm->def->memballoon && vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
Will change.
+ *memory = vm->def->mem.max_balloon; + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { + *memory = vm->def->mem.cur_balloon; + } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { + unsigned long long balloon; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) + err = 0; + else {
[2] If one leg of if-else has {}, both legs must have it. This is documented in HACKING (and I really ought to add a syntax check that forbids obvious cases of unbalanced braces).
I must have missed. Will fix.
+ + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +}
[3] Ouch. This function is unlocking vm, even though it did not obtain the lock. Which it kind of has to do because of the way that qemuDomainObjEndJob may end up invalidating vm. While transfer semantics are workable, they require good comments at the start of the function, and making sure that the caller doesn't duplicate the efforts, nor forget anything else.
Will add comment documenting this. Is this sufficient or there is something better I could do?
@@ -2526,7 +2583,6 @@ static int qemuDomainGetInfo(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; int err; - unsigned long long balloon;
if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -2549,43 +2605,9 @@ static int qemuDomainGetInfo(virDomainPtr dom, info->maxMem = vm->def->mem.max_balloon;
if (virDomainObjIsActive(vm)) { - qemuDomainObjPrivatePtr priv = vm->privateData; - - if ((vm->def->memballoon != NULL) && - (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) {
[1] Then again, your parenthesis...
- info->memory = vm->def->mem.max_balloon; - } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { - info->memory = vm->def->mem.cur_balloon; - } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - if (!virDomainObjIsActive(vm)) - err = 0; - else {
[2] ...and broken if-else bracing is just code motion. So I could overlook those (although cleaning them up at some point in the series, even if in a separate patch, would still be nice). But
No need to overlook. Will fix.
- if (!qemuDomainObjEndJob(driver, vm)) { - vm = NULL; - goto cleanup; - } - - 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 - */ - info->memory = vm->def->mem.cur_balloon; - } else if (err == 0) { - /* Balloon not supported, so maxmem is always the allocation */ - info->memory = vm->def->mem.max_balloon; - } else { - info->memory = balloon; - } - } else { - info->memory = vm->def->mem.cur_balloon; - } + err = qemuDomainGetBalloonMemory(driver, vm, &info->memory); + if (err) + return err;
[3] the fact that you don't use 'goto cleanup' here, but rely on the awkward transfer semantics, is just confusing the situation. I'd rather avoid transfer semantics, but understand why you used them. But I'd still rewrite this as:
err = qemuDomainGetBalloonMemory(...); vm = NULL;
and fall through to the cleanup label, rather than doing an early return, to make it obvious that the call did a transfer of vm.
Looks better to me. Will change in this direction. Thanks, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
On 09/03/2014 12:41 AM, Francesco Romani wrote:
[3] Ouch. This function is unlocking vm, even though it did not obtain the lock. Which it kind of has to do because of the way that qemuDomainObjEndJob may end up invalidating vm. While transfer semantics are workable, they require good comments at the start of the function, and making sure that the caller doesn't duplicate the efforts, nor forget anything else.
Will add comment documenting this. Is this sufficient or there is something better I could do?
A comment is sufficient. Here's a similar comment I'm adding in my series for virDomainBlockCopy: /* bandwidth in bytes/s. Caller must lock vm beforehand, and not * access it afterwards. */ static int qemuDomainBlockCopyCommon(virDomainObjPtr vm, virConnectPtr conn, const char *path, virStorageSourcePtr dest, unsigned long long bandwidth, unsigned int flags) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
From: "Francesco Romani" <fromani@redhat.com> To: libvir-list@redhat.com Sent: Wednesday, September 3, 2014 8:41:13 AM Subject: Re: [libvirt] [PATCH 01/11] qemu: extract helper to get the current balloon
[...]
+ + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +}
[3] Ouch. This function is unlocking vm, even though it did not obtain the lock. Which it kind of has to do because of the way that qemuDomainObjEndJob may end up invalidating vm. While transfer semantics are workable, they require good comments at the start of the function, and making sure that the caller doesn't duplicate the efforts, nor forget anything else.
Will add comment documenting this. Is this sufficient or there is something better I could do?
After some thought and an enlightening chat, turns out I can do better, by changing a bit more qemuDomainGetStats: * let qemuDomainGetStats handle the job * let the helper run the monitor jobs inside the job provided by the caller (this requirement will be of course documented) This way, we will have one will * have one job with multiple monitor jobs inside and * avoid alltogether the awkward transfer semantings And this should ultimately lead to both clearer and faster code. So, I'll explore this direction first. Moreover, after of course addressing all other comments, I'm going to squash the patch which extracts the helper with the one which add the bulk stats group which will make use of it, in order to make obvious how the helpers are going to be used. V3 with all the above will be posted ASAP. Cheers, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
On 09/02/2014 03:01 PM, Eric Blake wrote:
+ if (!virDomainObjIsActive(vm)) + err = 0; + else {
[2] If one leg of if-else has {}, both legs must have it. This is documented in HACKING (and I really ought to add a syntax check that forbids obvious cases of unbalanced braces).
As penance for pointing it out, I've written the syntax check: https://www.redhat.com/archives/libvir-list/2014-September/msg00164.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Extracts an helper to gether the VCpu information. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bbd16ed..1842e60 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -172,6 +172,12 @@ static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned long *memory); +static int qemuDomainHelperGetVcpus(virDomainObjPtr vm, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen); + virQEMUDriverPtr qemu_driver = NULL; @@ -4974,10 +4980,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; @@ -4992,7 +4995,25 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; } - priv = vm->privateData; + ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen); + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int +qemuDomainHelperGetVcpus(virDomainObjPtr vm, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen) +{ + int ret = -1; + int v, maxcpu, hostcpus; + size_t i; + qemuDomainObjPrivatePtr priv = vm->privateData; if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Extracts an helper to gether the VCpu
s/an/a/ s/gether/gather/
information.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bbd16ed..1842e60 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -172,6 +172,12 @@ static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned long *memory);
+static int qemuDomainHelperGetVcpus(virDomainObjPtr vm, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen); +
As I commented in 1, a non-recursive static function generally should not need forward declarations. Just hoist the implementation here.
virQEMUDriverPtr qemu_driver = NULL;
@@ -4974,10 +4980,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; @@ -4992,7 +4995,25 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; }
- priv = vm->privateData; + ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen); + + cleanup: + if (vm) + virObjectUnlock(vm);
Ouch. You have a double free. This frees vm, even though it was calling...
+ return ret; +} + +static int +qemuDomainHelperGetVcpus(virDomainObjPtr vm, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen) +{ + int ret = -1; + int v, maxcpu, hostcpus; + size_t i; + qemuDomainObjPrivatePtr priv = vm->privateData;
...a function that now has transfer semantics. But unlike patch 1, where transfer semantics were necessary because of the way you drop lock in order to do a monitor call, this patch appears to not need them; and the solution is to just sanitize the cleanup label (at which point it becomes a mere 'return ret', so you could then replace all 'goto cleanup' with a direct return). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
From: "Eric Blake" <eblake@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Tuesday, September 2, 2014 11:42:09 PM Subject: Re: [libvirt] [PATCH 02/11] qemu: extract helper to gather vcpu data
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Extracts an helper to gether the VCpu
s/an/a/ s/gether/gather/
Will fix,
virQEMUDriverPtr qemu_driver = NULL;
@@ -4974,10 +4980,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; @@ -4992,7 +4995,25 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; }
- priv = vm->privateData; + ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen); + + cleanup: + if (vm) + virObjectUnlock(vm);
Ouch. You have a double free. This frees vm, even though it was calling...
+ return ret; +} + +static int +qemuDomainHelperGetVcpus(virDomainObjPtr vm, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen) +{ + int ret = -1; + int v, maxcpu, hostcpus; + size_t i; + qemuDomainObjPrivatePtr priv = vm->privateData;
...a function that now has transfer semantics. But unlike patch 1, where transfer semantics were necessary because of the way you drop lock in order to do a monitor call, this patch appears to not need them; and the solution is to just sanitize the cleanup label (at which point it becomes a mere 'return ret', so you could then replace all 'goto cleanup' with a direct return).
Thanks, will fix. -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
Add an helper function to get the block stats of a disk. This helper is meant to be used by the bulk stats API. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 41 +++++++++++++++ src/qemu/qemu_monitor.c | 23 +++++++++ src/qemu/qemu_monitor.h | 18 +++++++ src/qemu/qemu_monitor_json.c | 118 +++++++++++++++++++++++++++++-------------- src/qemu/qemu_monitor_json.h | 4 ++ 5 files changed, 165 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1842e60..39e2c1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -178,6 +178,12 @@ static int qemuDomainHelperGetVcpus(virDomainObjPtr vm, unsigned char *cpumaps, int maplen); +static int qemuDomainGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + struct qemuBlockStats *stats, + int nstats); + + virQEMUDriverPtr qemu_driver = NULL; @@ -9672,6 +9678,41 @@ qemuDomainBlockStats(virDomainPtr dom, return ret; } + +/* + * returns at most the first `nstats' stats, then stops. + * Returns the number of stats filled. + */ +static int +qemuDomainGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + struct qemuBlockStats *stats, + int nstats) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + priv = vm->privateData; + + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats); + + qemuDomainObjExitMonitor(driver, vm); + + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + static int qemuDomainBlockStatsFlags(virDomainPtr dom, const char *path, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5b2952a..8aadba5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1754,6 +1754,29 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, return ret; } +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + struct qemuBlockStats *stats, + int nstats) +{ + int ret; + VIR_DEBUG("mon=%p dev=%s", mon, dev_name); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (mon->json) + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, + stats, nstats); + else + ret = -1; /* not supported */ + + 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 4fd6f01..1b7d00b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -346,6 +346,24 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, long long *flush_req, long long *flush_total_times, long long *errs); + +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; + long long errs; /* meaningless for QEMU */ +}; + +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + struct qemuBlockStats *stats, + int nstats); + int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon, int *nparams); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..68c5cf8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1712,13 +1712,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; + struct qemuBlockStats stats; + int nstats = 1; + int ret = -1; *rd_req = *rd_bytes = -1; *wr_req = *wr_bytes = *errs = -1; @@ -1732,9 +1728,45 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, if (flush_total_times) *flush_total_times = -1; + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, &stats, nstats) != nstats) + goto cleanup; + + *rd_req = stats.rd_req; + *rd_bytes = stats.rd_bytes; + *rd_total_times = stats.rd_total_times; + *wr_req = stats.wr_req; + *wr_bytes = stats.wr_bytes; + *wr_total_times = stats.wr_total_times; + *flush_req = stats.flush_req; + *flush_total_times = stats.flush_total_times; + *errs = stats.errs; + + ret = 0; + + cleanup: + return ret; +} + + +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + struct qemuBlockStats *blockstats, + 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 (!blockstats || nstats <= 0) + return -1; + ret = qemuMonitorJSONCommand(mon, cmd, &reply); if (ret == 0) @@ -1750,33 +1782,41 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; } + ret = 0; for (i = 0; i < virJSONValueArraySize(devices); 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")); - 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 (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX)) - thisdev += strlen(QEMU_DRIVE_HOST_PREFIX); + if (dev_name != NULL) { + const char *thisdev; + if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { + 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", @@ -1784,74 +1824,74 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; } - if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", rd_bytes) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", &blockstats->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", &blockstats->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)) { + &blockstats->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", &blockstats->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", &blockstats->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)) { + &blockstats->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)) { + &blockstats->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)) { + &blockstats->flush_total_times) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot read %s statistic"), "flush_total_time_ns"); goto cleanup; } + blockstats->errs = -1; /* meaningless for QEMU */ + + ret++; + blockstats++; } - if (!found) { + if (dev_name != NULL && !found) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find statistics for device '%s'"), dev_name); + ret = 0; goto cleanup; } - ret = 0; cleanup: virJSONValueFree(cmd); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d8c9308..79058c1 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, + struct qemuBlockStats *stats, + int nstats); int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon, int *nparams); int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Add an helper function to get the block stats of a disk. This helper is meant to be used by the bulk stats API.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 41 +++++++++++++++ src/qemu/qemu_monitor.c | 23 +++++++++ src/qemu/qemu_monitor.h | 18 +++++++ src/qemu/qemu_monitor_json.c | 118 +++++++++++++++++++++++++++++-------------- src/qemu/qemu_monitor_json.h | 4 ++ 5 files changed, 165 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1842e60..39e2c1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -178,6 +178,12 @@ static int qemuDomainHelperGetVcpus(virDomainObjPtr vm, unsigned char *cpumaps, int maplen);
+static int qemuDomainGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + struct qemuBlockStats *stats, + int nstats); + +
Another forward declaration to be avoided. Why do you need 'struct qemuBlockStats' in the declaration? Did you forget a typedef somewhere?
virQEMUDriverPtr qemu_driver = NULL;
@@ -9672,6 +9678,41 @@ qemuDomainBlockStats(virDomainPtr dom, return ret; }
+ +/* + * returns at most the first `nstats' stats, then stops. + * Returns the number of stats filled. + */ +static int +qemuDomainGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + struct qemuBlockStats *stats, + int nstats) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + priv = vm->privateData; + + qemuDomainObjEnterMonitor(driver, vm);
Missing a call to check if the domain is still running. The mere act of calling qemuDomainObjBeginJob causes us to temporarily drop locks, and while the locks are down, the VM can shutdown independently and that makes the monitor go away; but qemuDomainObjEnterMonitor is only safe to call if we know the monitor still exists. There should be plenty of examples to copy from.
+ + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats); + + qemuDomainObjExitMonitor(driver, vm); + + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm);
Another case of required transfer semantics. Is this patch complete? I don't see any caller of the new qemuDomainGetBlockStats, and gcc generally gives a compiler warning (which then turns into a failed build due to -Werror) if you have an unused static function.
+ return ret; +} + static int qemuDomainBlockStatsFlags(virDomainPtr dom, const char *path, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5b2952a..8aadba5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1754,6 +1754,29 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, return ret; }
+int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
Style: two blank lines between functions, return type on its own line.
+ const char *dev_name, + struct qemuBlockStats *stats, + int nstats) +{ + int ret; + VIR_DEBUG("mon=%p dev=%s", mon, dev_name); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + }
This if can be nuked if you'd just mark the mon parameter as ATTRIBUTE_NONNULL (all our callers use it correctly, after all).
+ + if (mon->json) + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, + stats, nstats); + else + ret = -1; /* not supported */
Returning -1 without printing an error message is bad.
+ + 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 4fd6f01..1b7d00b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -346,6 +346,24 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, long long *flush_req, long long *flush_total_times, long long *errs); + +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; + long long errs; /* meaningless for QEMU */
Umm, why do we need an 'errs' parameter, if it is meaningless? I can see that this is sort of a superset of the public virDomainBlockStats struct, but that struct is generic to multiple hypervisors; and it also looks like the additional fields correspond to typed parameters available from virDomainBlockStatsFlags. But I see no point in reporting an 'errs' typed param if it makes no sense for qemu.
+}; + +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, + const char *dev_name, + struct qemuBlockStats *stats, + int nstats); + int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon, int *nparams);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..68c5cf8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1712,13 +1712,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; + struct qemuBlockStats stats; + int nstats = 1; + int ret = -1;
*rd_req = *rd_bytes = -1; *wr_req = *wr_bytes = *errs = -1; @@ -1732,9 +1728,45 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, if (flush_total_times) *flush_total_times = -1;
+ if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, &stats, nstats) != nstats) + goto cleanup; + + *rd_req = stats.rd_req; + *rd_bytes = stats.rd_bytes; + *rd_total_times = stats.rd_total_times; + *wr_req = stats.wr_req; + *wr_bytes = stats.wr_bytes; + *wr_total_times = stats.wr_total_times; + *flush_req = stats.flush_req; + *flush_total_times = stats.flush_total_times; + *errs = stats.errs;
Are you guaranteed that all of these pointers are non-NULL and that you can blindly assign into them? A quick grep shows that at least qemuDomainBlockStats passes in several NULLs.
+ +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
Style: return type on its own line.
- /* 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 (dev_name != NULL) {
Can be written as just 'if (dev_name) {'
} - if (virJSONValueObjectGetNumberLong(stats, "wr_operations", wr_req) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "wr_operations", &blockstats->wr_req) < 0) {
Some long lines, worth wrapping to keep under 80 columns.
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)) {
Over-parenthesized (here and several other places).
- if (!found) { + if (dev_name != NULL && !found) {
if (dev_name && !found) { -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
From: "Eric Blake" <eblake@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Wednesday, September 3, 2014 12:14:58 AM Subject: Re: [libvirt] [PATCHv2 03/11] qemu: add helper to get the block stats
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Add an helper function to get the block stats of a disk. This helper is meant to be used by the bulk stats API.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 41 +++++++++++++++ src/qemu/qemu_monitor.c | 23 +++++++++ src/qemu/qemu_monitor.h | 18 +++++++ src/qemu/qemu_monitor_json.c | 118 +++++++++++++++++++++++++++++-------------- src/qemu/qemu_monitor_json.h | 4 ++ 5 files changed, 165 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1842e60..39e2c1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -178,6 +178,12 @@ static int qemuDomainHelperGetVcpus(virDomainObjPtr vm, unsigned char *cpumaps, int maplen);
+static int qemuDomainGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + struct qemuBlockStats *stats, + int nstats); + +
Another forward declaration to be avoided.
Why do you need 'struct qemuBlockStats' in the declaration? Did you forget a typedef somewhere?
I saw an internal struct typedef-less (qemuAutostartData maybe?) and this somehow got stuck on my mind. But I believe modern libvirt style mandates typedef, is that correct? I'll amend my code accordingly.
+static int +qemuDomainGetBlockStats(virQEMUDriverPtr driver, + virDomainObjPtr vm, + struct qemuBlockStats *stats, + int nstats) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + priv = vm->privateData; + + qemuDomainObjEnterMonitor(driver, vm);
Missing a call to check if the domain is still running. The mere act of calling qemuDomainObjBeginJob causes us to temporarily drop locks, and while the locks are down, the VM can shutdown independently and that makes the monitor go away; but qemuDomainObjEnterMonitor is only safe to call if we know the monitor still exists. There should be plenty of examples to copy from.
Thanks for the explanation, will add the check.
+ + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL, + stats, nstats); + + qemuDomainObjExitMonitor(driver, vm); + + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm);
Another case of required transfer semantics.
Is this patch complete? I don't see any caller of the new qemuDomainGetBlockStats, and gcc generally gives a compiler warning (which then turns into a failed build due to -Werror) if you have an unused static function.
It does. I (wrongly) thought it is easier/better to have little patches so one which add code, one which makes use of it (10/11 in this series). Will squash with the dependent patch on the next submission.
+int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
Style: two blank lines between functions, return type on its own line.
Oops. Will fix.
+ const char *dev_name, + struct qemuBlockStats *stats, + int nstats) +{ + int ret; + VIR_DEBUG("mon=%p dev=%s", mon, dev_name); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + }
This if can be nuked if you'd just mark the mon parameter as ATTRIBUTE_NONNULL (all our callers use it correctly, after all).
Will do.
+ + if (mon->json) + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, + stats, nstats); + else + ret = -1; /* not supported */
Returning -1 without printing an error message is bad.
Will add.
+ +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; + long long errs; /* meaningless for QEMU */
Umm, why do we need an 'errs' parameter, if it is meaningless? I can see that this is sort of a superset of the public virDomainBlockStats struct, but that struct is generic to multiple hypervisors; and it also looks like the additional fields correspond to typed parameters available from virDomainBlockStatsFlags. But I see no point in reporting an 'errs' typed param if it makes no sense for qemu.
Will purge the 'errs' parameter. It was added to closely mimic the originating code (virDomainGetBlockInfo), but turns out I took this too strictly and become cargo cult-ish.
+ *rd_req = stats.rd_req; + *rd_bytes = stats.rd_bytes; + *rd_total_times = stats.rd_total_times; + *wr_req = stats.wr_req; + *wr_bytes = stats.wr_bytes; + *wr_total_times = stats.wr_total_times; + *flush_req = stats.flush_req; + *flush_total_times = stats.flush_total_times; + *errs = stats.errs;
Are you guaranteed that all of these pointers are non-NULL and that you can blindly assign into them? A quick grep shows that at least qemuDomainBlockStats passes in several NULLs.
Will add checks in this function to handle NULLs.
+ +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
Style: return type on its own line.
Will do.
- /* 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 (dev_name != NULL) {
Can be written as just 'if (dev_name) {'
Will do.
} - if (virJSONValueObjectGetNumberLong(stats, "wr_operations", wr_req) < 0) { + if (virJSONValueObjectGetNumberLong(stats, "wr_operations", &blockstats->wr_req) < 0) {
Some long lines, worth wrapping to keep under 80 columns.
Will fix here and everywhere else.
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)) {
Over-parenthesized (here and several other places).
Will fix everywhere.
- if (!found) { + if (dev_name != NULL && !found) {
if (dev_name && !found) {
Will fix. thanks, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
This patch adds the reporting of the highest block stats written to the block stats. This information was already there since the beginning, and doing so we gain the information provided by GetBlockInfo without entering the monitor again. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1b7d00b..8e16c7d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -357,6 +357,7 @@ struct qemuBlockStats { long long flush_req; long long flush_total_times; long long errs; /* meaningless for QEMU */ + 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 68c5cf8..31ffb6c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1880,6 +1880,13 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, "flush_total_time_ns"); goto cleanup; } + if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", &blockstats->wr_highest_offset) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot read %s statistic"), + "wr_highest_offset"); + goto cleanup; + } + blockstats->errs = -1; /* meaningless for QEMU */ ret++; -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
This patch adds the reporting of the highest block stats written to the block stats.
This information was already there since the beginning, and doing so we gain the information provided by GetBlockInfo without entering the monitor again.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 7 +++++++ 2 files changed, 8 insertions(+)
+++ b/src/qemu/qemu_monitor_json.c @@ -1880,6 +1880,13 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, "flush_total_time_ns"); goto cleanup; } + if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", &blockstats->wr_highest_offset) < 0) {
Long line; wrap at 80 columns. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Future patches which will implement more bulk stats groups for QEMU will need to access the connection object, so enrich the worker prototype. Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39e2c1b..a9f6821 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17282,7 +17282,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, static int -qemuDomainGetStatsState(virDomainObjPtr dom, +qemuDomainGetStatsState(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int privflags ATTRIBUTE_UNUSED) @@ -17304,9 +17305,9 @@ qemuDomainGetStatsState(virDomainObjPtr dom, return 0; } - typedef int -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, +(*qemuDomainGetStatsFunc)(virConnectPtr conn, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, unsigned int flags); @@ -17367,7 +17368,7 @@ qemuDomainGetStats(virConnectPtr conn, for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) { if (stats & qemuDomainGetStatsWorkers[i].stats) { - if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams, + if (qemuDomainGetStatsWorkers[i].func(conn, dom, tmp, &maxparams, flags) < 0) goto cleanup; } -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Future patches which will implement more bulk stats groups for QEMU will need to access the connection object, so enrich the worker prototype.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 | 8 +++++++ src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a64f597..69ad152 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 5d8f01c..c6556ea 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21546,6 +21546,14 @@ 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 informations. + * The typed parameter keys are in this format: + * "cpu.count" - number of physical cpu available to this domain. + * "cpu.time" - total cpu time spent for this domain + * "cpu.user" - user cpu time spent + * "cpu.system" - system cpu time spent + * + * * 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 a9f6821..2ced593 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 @@ -17305,6 +17306,60 @@ qemuDomainGetStatsState(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; } + +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; + + ncpus = nodeGetCPUCount(); + + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + "cpu.count", + ncpus) < 0) + return -1; + + if (virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time) < 0) + return -1; + + if (virCgroupGetCpuacctStat(priv->cgroup, &user_time, &sys_time) < 0) + return -1; + + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "cpu.time", + cpu_time) < 0) + return -1; + + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "cpu.user", + user_time) < 0) + return -1; + + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + "cpu.system", + sys_time) < 0) + return -1; + + return 0; +} + + typedef int (*qemuDomainGetStatsFunc)(virConnectPtr conn, virDomainObjPtr dom, @@ -17319,6 +17374,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, + { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL }, { NULL, 0 } }; -- 1.9.3
On 09/02/2014 06:31 AM, 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 | 8 +++++++ src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+)
+++ b/src/libvirt.c @@ -21546,6 +21546,14 @@ 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 informations. + * The typed parameter keys are in this format: + * "cpu.count" - number of physical cpu available to this domain.
s/cpu/cpus/ Missing what type to expect. Mention that this one is returned as an int (unsigned int would actually be better, but that means using AddUInt() down below).
+ * "cpu.time" - total cpu time spent for this domain + * "cpu.user" - user cpu time spent + * "cpu.system" - system cpu time spent
Mention that these three are typed as unsigned long long. rest of the patch looks okay, modulo the typing of cpu.count. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/02/2014 06:31 AM, 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 | 8 +++++++ src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+)
Oh, one more nit:
* + * VIR_DOMAIN_STATS_CPU_TOTAL: Return CPU statistics and usage informations.
s/informations/information/ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 | 4 ++++ src/qemu/qemu_driver.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 69ad152..7ec57cd 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 c6556ea..8b0f589 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21553,6 +21553,10 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.user" - user cpu time spent * "cpu.system" - system cpu time spent * + * VIR_DOMAIN_STATS_BALLOON: Return memory balloon device informations. + * The typed parameter keys are in this format: + * "balloon.current" - the memory in KBytes currently used + * "balloon.maximum" - the maximum memory in KBytes allowed * * 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 2ced593..98f1a31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17359,6 +17359,37 @@ 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; + int err = 0; + + err = qemuDomainGetBalloonMemory(driver, dom, &cur_balloon); + if (err) + return -1; + + 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, @@ -17375,6 +17406,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL }, + { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON }, { NULL, 0 } }; -- 1.9.3
On 09/02/2014 06:31 AM, 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 | 4 ++++ src/qemu/qemu_driver.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+)
* + * VIR_DOMAIN_STATS_BALLOON: Return memory balloon device informations.
s/informations/information/
+ * The typed parameter keys are in this format: + * "balloon.current" - the memory in KBytes currently used + * "balloon.maximum" - the maximum memory in KBytes allowed
Missing type information of unsigned long long. KBytes is not quite the right abbreviated unit name; kibibytes (kiB) would be more accurate. -- 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. Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 8 +++++ src/qemu/qemu_driver.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7ec57cd..46f4067 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 8b0f589..a5942bc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21558,6 +21558,14 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "balloon.current" - the memory in KBytes currently used * "balloon.maximum" - the maximum memory in KBytes allowed * + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics. + * The typed parameter keys are in this format: + * "vcpu.current" - current number of online virtual CPUs + * "vcpu.maximum" - maximum number of online virtual CPUs + * "vcpu.<num>.state" - state of the virtual CPU <num> + * "vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> + * "vcpu.<num>.cpu" - physical CPU pinned to virtual CPU <num> + * * 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 98f1a31..72ec284 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17391,6 +17391,77 @@ qemuDomainGetStatsBalloon(virConnectPtr conn, return 0; } + +static int +qemuDomainGetStatsVcpu(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + int ret = -1; + char param_name[NAME_MAX]; + virVcpuInfoPtr cpuinfo = NULL; + + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + "vcpu.current", + dom->def->vcpus) < 0) + return -1; + + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + "vcpu.maximum", + dom->def->maxvcpus) < 0) + return -1; + + if (VIR_ALLOC_N(cpuinfo, dom->def->vcpus) < 0) + return -1; + + if ((ret = qemuDomainHelperGetVcpus(dom, + cpuinfo, + dom->def->vcpus, + NULL, + 0)) < 0) + goto cleanup; + + for (i = 0; i < dom->def->vcpus; i++) { + snprintf(param_name, NAME_MAX, "vcpu.%u.state", cpuinfo[i].number); + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].state) < 0) + goto cleanup; + + snprintf(param_name, NAME_MAX, "vcpu.%u.time", cpuinfo[i].number); + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].cpuTime) < 0) + goto cleanup; + + snprintf(param_name, NAME_MAX, "vcpu.%u.cpu", cpuinfo[i].number); + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + param_name, + cpuinfo[i].cpu) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(cpuinfo); + return ret; +} + + typedef int (*qemuDomainGetStatsFunc)(virConnectPtr conn, virDomainObjPtr dom, @@ -17407,6 +17478,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON }, + { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU }, { NULL, 0 } }; -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
This patch implements the VIR_DOMAIN_STATS_VCPU group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 8 +++++ src/qemu/qemu_driver.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+)
* + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics. + * The typed parameter keys are in this format: + * "vcpu.current" - current number of online virtual CPUs + * "vcpu.maximum" - maximum number of online virtual CPUs + * "vcpu.<num>.state" - state of the virtual CPU <num>
Is this an int mapping to some particular enum?
+ * "vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> + * "vcpu.<num>.cpu" - physical CPU pinned to virtual CPU <num>
Missing types. Should there be a parameter vcpu.count that says how many vcpu.<num> entries to expect? Or is that vcpu.current? Or do we have a situation where if cpus 0 and 2 are online but 1 and 3 are offline, then we have vcpu.0.x and vcpu2.x but not vcpu1.x? A bit more documentation will help the user deciding which "array" entries to expect, and whether the "array" will be sparse if cpus are offline.
+{ + size_t i; + int ret = -1; + char param_name[NAME_MAX];
NAME_MAX (typically 256) is huge, compared to VIR_TYPED_PARAM_FIELD_LENGTH (80). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
From: "Eric Blake" <eblake@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Wednesday, September 3, 2014 1:00:53 AM Subject: Re: [libvirt] [PATCHv2 08/11] qemu: bulk stats: implement VCPU group
On 09/02/2014 06:31 AM, Francesco Romani wrote:
This patch implements the VIR_DOMAIN_STATS_VCPU group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 8 +++++ src/qemu/qemu_driver.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+)
* + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics. + * The typed parameter keys are in this format: + * "vcpu.current" - current number of online virtual CPUs + * "vcpu.maximum" - maximum number of online virtual CPUs + * "vcpu.<num>.state" - state of the virtual CPU <num>
Is this an int mapping to some particular enum?
Yep: virVcpuState. Will document this.
+ * "vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> + * "vcpu.<num>.cpu" - physical CPU pinned to virtual CPU <num>
Missing types.
Should there be a parameter vcpu.count that says how many vcpu.<num> entries to expect? Or is that vcpu.current? Or do we have a situation where if cpus 0 and 2 are online but 1 and 3 are offline, then we have vcpu.0.x and vcpu2.x but not vcpu1.x?
Yes, the latter, due to VCPU hot(un)plugging
A bit more documentation will help the user deciding which "array" entries to expect, and whether the "array" will be sparse if cpus are offline.
Will document that the array will be up to "vcpu.maximum" items, could be sparse and the actual size will be of "vcpu.current" items.
+{ + size_t i; + int ret = -1; + char param_name[NAME_MAX];
NAME_MAX (typically 256) is huge, compared to VIR_TYPED_PARAM_FIELD_LENGTH (80).
Will switch to VIR_TYPED_PARAM_FIELD_LENGTH here and everywhere I used NAME_MAX. Thanks, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
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 | 13 +++++++ src/qemu/qemu_driver.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 46f4067..33588d6 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 a5942bc..099404b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21566,6 +21566,19 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "vcpu.<num>.time" - virtual cpu time spent by virtual CPU <num> * "vcpu.<num>.cpu" - physical CPU pinned to virtual CPU <num> * + * 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. + * "net.<num>.name" - name of the interface <num>. + * "net.<num>.rx.bytes" - bytes received. + * "net.<num>.rx.pkts" - packets received. + * "net.<num>.rx.errs" - receive errors. + * "net.<num>.rx.drop" - receive packets dropped. + * "net.<num>.tx.bytes" - bytes transmitted. + * "net.<num>.tx.pkts" - packets transmitted. + * "net.<num>.tx.errs" - transmission errors. + * "net.<num>.tx.drop" - transmit packets dropped. + * * 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 72ec284..069a15d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17461,6 +17461,90 @@ qemuDomainGetStatsVcpu(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } +#define QEMU_ADD_COUNT_PARAM(RECORD, MAXPARAMS, TYPE, COUNT) \ +do { \ + char param_name[NAME_MAX]; \ + snprintf(param_name, NAME_MAX, "%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[NAME_MAX]; \ + snprintf(param_name, NAME_MAX, "%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[NAME_MAX]; \ + snprintf(param_name, NAME_MAX, "net.%lu.%s", NUM, NAME); \ + if (virTypedParamsAddLLong(&RECORD->params, \ + &RECORD->nparams, \ + MAXPARAMS, \ + param_name, \ + VALUE) < 0) \ + return -1; \ +} while (0) + +static int +qemuDomainGetStatsInterface(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + size_t i; + struct _virDomainInterfaceStats tmp; + + /* 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_COUNT_PARAM(record, maxparams, "net", dom->def->nnets); + + 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)(virConnectPtr conn, @@ -17479,6 +17563,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU }, + { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE }, { NULL, 0 } }; -- 1.9.3
On 09/02/2014 06:31 AM, 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 | 13 +++++++ src/qemu/qemu_driver.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+)
* + * 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. + * "net.<num>.name" - name of the interface <num>. + * "net.<num>.rx.bytes" - bytes received. + * "net.<num>.rx.pkts" - packets received. + * "net.<num>.rx.errs" - receive errors. + * "net.<num>.rx.drop" - receive packets dropped. + * "net.<num>.tx.bytes" - bytes transmitted. + * "net.<num>.tx.pkts" - packets transmitted. + * "net.<num>.tx.errs" - transmission errors. + * "net.<num>.tx.drop" - transmit packets dropped.
Missing types.
+#define QEMU_ADD_COUNT_PARAM(RECORD, MAXPARAMS, TYPE, COUNT) \ +do { \ + char param_name[NAME_MAX]; \
NAME_MAX is oversized.
+ snprintf(param_name, NAME_MAX, "%s.count", TYPE); \ + if (virTypedParamsAddUInt(&RECORD->params, \
For safety, I'd write $(RECORD)->params, and so forth. ALL_CAPS macro parameter names are unusual; it is sufficient that the macro name is all caps. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch implements the VIR_DOMAIN_STATS_BLOCK group of statistics. Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 13 +++++++++ src/qemu/qemu_driver.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 33588d6..1d90f5e 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 099404b..cabfb91 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21579,6 +21579,19 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "net.<num>.tx.errs" - transmission errors. * "net.<num>.tx.drop" - transmit packets dropped. * + * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics. + * The typed paramer keys are in this format: + * "block.count" - number of block devices on this domain. + * "block.<num>.name" - name of the block device <num>. + * "block.<num>.rd.reqs" - number of read requests. + * "block.<num>.rd.bytes" - number of read bytes. + * "block.<num>.rd.times" - total time (ns) spent on reads. + * "block.<num>.wr.reqs" - number of write requests + * "block.<num>.wr.bytes" - number of written bytes. + * "block.<num>.wr.times" - total time (ns) spent on writes. + * "block.<num>.fl.reqs" - total flush requests + * "block.<num>.fl.times" - total time (ns) spent on cache flushing + * * 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 069a15d..977e8c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17542,6 +17542,70 @@ 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[NAME_MAX]; \ + snprintf(param_name, NAME_MAX, "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 = dom->def->ndisks; + struct qemuBlockStats *stats = NULL; + virQEMUDriverPtr driver = conn->privateData; + + if (VIR_ALLOC_N(stats, nstats) < 0) + return -1; + + if (qemuDomainGetBlockStats(driver, dom, stats, nstats) != nstats) + goto cleanup; + + for (i = 0; i < dom->def->ndisks; i++) { + QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks); + + 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 @@ -17564,6 +17628,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU }, { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE }, + { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK }, { NULL, 0 } }; -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
This patch implements the VIR_DOMAIN_STATS_BLOCK group of statistics.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 13 +++++++++ src/qemu/qemu_driver.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 33588d6..1d90f5e 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 099404b..cabfb91 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21579,6 +21579,19 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "net.<num>.tx.errs" - transmission errors. * "net.<num>.tx.drop" - transmit packets dropped. * + * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics. + * The typed paramer keys are in this format:
s/paramer/parameter/
+ * "block.count" - number of block devices on this domain. + * "block.<num>.name" - name of the block device <num>.
Does this match up to the <target dev='vda'/> name?
+ * "block.<num>.rd.reqs" - number of read requests. + * "block.<num>.rd.bytes" - number of read bytes. + * "block.<num>.rd.times" - total time (ns) spent on reads. + * "block.<num>.wr.reqs" - number of write requests + * "block.<num>.wr.bytes" - number of written bytes. + * "block.<num>.wr.times" - total time (ns) spent on writes. + * "block.<num>.fl.reqs" - total flush requests + * "block.<num>.fl.times" - total time (ns) spent on cache flushing
Missing types. Inconsistent on whether you use a trailing '.' Just because qemu doesn't report a meaningful errs does not mean that the public API should omit it. Hypervisors need only return the subset of typed parameters that make sense, but we ought to document a typed parameter for all possible fields for the API calls that this is copying from.
+ * * 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 069a15d..977e8c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17542,6 +17542,70 @@ 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[NAME_MAX]; \
Oversized.
+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 = dom->def->ndisks;
ndisks may include such things as a cdrom drive with no media...
+ struct qemuBlockStats *stats = NULL; + virQEMUDriverPtr driver = conn->privateData; + + if (VIR_ALLOC_N(stats, nstats) < 0) + return -1; + + if (qemuDomainGetBlockStats(driver, dom, stats, nstats) != nstats)
...so it is possible that nstats might be less than ndisks. Is that really a reason to return failure, instead of at least reporting the information that was available? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Management software, want 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, in order to save a call to the QEMU monitor. --- src/libvirt.c | 1 + src/qemu/qemu_driver.c | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index cabfb91..81d71be 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21591,6 +21591,7 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "block.<num>.wr.times" - total time (ns) spent on writes. * "block.<num>.fl.reqs" - total flush requests * "block.<num>.fl.times" - total time (ns) spent on cache flushing + * "block.<num>.allocation" - offset of the highest written sector. * * 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 977e8c7..3fb54db 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17554,6 +17554,18 @@ do { \ goto cleanup; \ } while (0) +#define QEMU_ADD_BLOCK_PARAM_ULL(RECORD, MAXPARAMS, NUM, NAME, VALUE) \ +do { \ + char param_name[NAME_MAX]; \ + snprintf(param_name, NAME_MAX, "block.%lu.%s", NUM, NAME); \ + if (virTypedParamsAddULLong(&RECORD->params, \ + &RECORD->nparams, \ + MAXPARAMS, \ + param_name, \ + VALUE) < 0) \ + goto cleanup; \ +} while (0) + static int qemuDomainGetStatsBlock(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainObjPtr dom, @@ -17595,6 +17607,9 @@ qemuDomainGetStatsBlock(virConnectPtr conn ATTRIBUTE_UNUSED, "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; -- 1.9.3
On 09/02/2014 06:31 AM, Francesco Romani wrote:
Management software, want to be able to allocate disk space on demand.
s/software, want/software wants/
To support this, they need keep track of the space occupation of the block device.
(what's more, during a blockcopy or active blockcommit, they also need to track space allocation of secondary files - but we can add that support later)
This information is reported by qemu as part of block stats.
This patch extend the block information in the bulk stats with
s/extend/extends/
the allocation information, in order to save a call to the QEMU monitor. --- src/libvirt.c | 1 + src/qemu/qemu_driver.c | 15 +++++++++++++++ 2 files changed, 16 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index cabfb91..81d71be 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21591,6 +21591,7 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "block.<num>.wr.times" - total time (ns) spent on writes. * "block.<num>.fl.reqs" - total flush requests * "block.<num>.fl.times" - total time (ns) spent on cache flushing + * "block.<num>.allocation" - offset of the highest written sector.
Missing type.
* * 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 977e8c7..3fb54db 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17554,6 +17554,18 @@ do { \ goto cleanup; \ } while (0)
+#define QEMU_ADD_BLOCK_PARAM_ULL(RECORD, MAXPARAMS, NUM, NAME, VALUE) \ +do { \ + char param_name[NAME_MAX]; \
Oversized. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/02/2014 06:31 AM, Francesco Romani wrote:
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.
changes 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.
Missing is virsh exposure of the new stat groups (Li's series gave an example for adding --block). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
From: "Eric Blake" <eblake@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Wednesday, September 3, 2014 1:15:38 AM Subject: Re: [libvirt] [PATCHv2 00/11] bulk stats: QEMU implementation
On 09/02/2014 06:31 AM, Francesco Romani wrote:
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.
changes 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.
Missing is virsh exposure of the new stat groups (Li's series gave an example for adding --block).
Sure, will add with a followup patch. One question, though. Do we want default to expose nothing, and options to add groups, is this correct? So usage would be something like virsh domstats # equivalent to --all , I suppose virsh domstats --block # only block group virsh domstats --interface --vcpu # only vcpu and interface Thanks and bests, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
On 09/03/2014 01:04 AM, Francesco Romani wrote:
Missing is virsh exposure of the new stat groups (Li's series gave an example for adding --block).
Sure, will add with a followup patch. One question, though. Do we want default to expose nothing, and options to add groups, is this correct?
Correct - the API is already wired that way (stats == 0 implies list all stats; non-zero stats says list just the requested stats).
So usage would be something like
virsh domstats # equivalent to --all , I suppose
Yes, except I don't think we need an actual --all flag.
virsh domstats --block # only block group virsh domstats --interface --vcpu # only vcpu and interface
Yep, looks right to me :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Francesco Romani