On 02/09/2012 03:43 AM, Lai Jiangshan wrote:
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
* Now, only "cpu_time" is supported. * cpuacct cgroup is used for providing percpu cputime information.
* include/libvirt/libvirt.h.in - defines VIR_DOMAIN_CPU_STATS_CPUTIME * src/qemu/qemu.conf - take care of cpuacct cgroup. * src/qemu/qemu_conf.c - take care of cpuacct cgroup. * src/qemu/qemu_driver.c - added an interface * src/util/cgroup.c/h - added interface for getting percpu cputime
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/qemu/qemu.conf | 3 +- src/qemu/qemu_conf.c | 3 +- src/qemu/qemu_driver.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 6 ++ src/util/cgroup.h | 1 + 5 files changed, 168 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 95428c1..db07b8a 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -166,6 +166,7 @@ # - 'memory' - use for memory tunables # - 'blkio' - use for block devices I/O tunables # - 'cpuset' - use for CPUs and memory nodes +# - cpuacct - use for CPUs statistics.
Needs consistent quoting with lines above.
+ +/* qemuDomainGetCPUStats() with start_cpu == -1 */ +static int qemuDomainGetTotalcpuStats(virDomainPtr domain ATTRIBUTE_UNUSED, + virCgroupPtr group, + virTypedParameterPtr params, + int nparams, + unsigned int flags)
Formatting nit: these days, we are preferring to start new function names on column 1, with the return type on the previous line, as in: static int qemuDomainGetTotalCpuStats(...) Why are you passing an unused parameter through to a static function? The only time that should happen is in a callback interface, but you are directly calling this function, so I'd prune the parameter instead.
+{ + unsigned long long cpu_time; + int param_idx = 0; + int ret; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
No need to re-check flags; we already checked it up front in the primary entry point. At which point, you probably don't need the flags parameter in this function.
+ + if (nparams == 0) /* return supprted number of params */
s/supprted/supported/
+ return 1; + /* entry 0 is cputime */ + ret = virCgroupGetCpuacctUsage(group, &cpu_time); + if (ret < 0) { + virReportSystemError(-ret, "%s", _("unable to get cpu account")); + return -1; + } + + virTypedParameterAssign(¶ms[param_idx], VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + return param_idx + 1;
Why are you using the 'param_idx' variable, which is always 0? I'd simplify this to just 'return 1;', and worry about things if we ever add additional parameters later.
+} + +static int qemuDomainGetPercpuStats(virDomainPtr domain, + virCgroupPtr group, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + virBitmapPtr map = NULL; + int rv = -1; + int i, max_id; + char *pos; + char *buf = NULL; + virTypedParameterPtr ent; + int param_idx; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
Again, why check flags here? It should have already been done in the caller.
+ /* return the number of supported params ? */ + if (nparams == 0 && ncpus != 0) + return 1; /* only cpu_time is supported */ + + /* return percpu cputime in index 0 */ + param_idx = 0; + /* to parse account file, we need "present" cpu map */ + map = nodeGetCPUmap(domain->conn, &max_id, "present");
I'm wondering if you can just use virDomainCpuSetParse to get a cpumap, and then use macros like VIR_CPU_USABLE in parsing that map, rather than going through virBitmap.
+ if (!map) + return rv; + + if (ncpus == 0) { /* returns max cpu ID */ + rv = max_id; + goto cleanup; + } + /* we get percpu cputime accoutning info. */
s/accoutning/accounting/
+ if (virCgroupGetCpuacctPercpuUsage(group, &buf)) + goto cleanup; + pos = buf; + + if (max_id > start_cpu + ncpus - 1) + max_id = start_cpu + ncpus - 1; + + for (i = 0; i <= max_id; i++) { + bool exist; + unsigned long long cpu_time; + + if (virBitmapGetBit(map, i, &exist) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, _("bitmap parse error")); + goto cleanup; + } + if (!exist) + continue; + if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cpuacct parse error")); + goto cleanup; + } + if (i < start_cpu) + continue; + ent = ¶ms[ (i - start_cpu) * nparams + param_idx]; + virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + } + rv = param_idx + 1;
This part looks right, even if we aren't incrementing param_idx until some future date when we add a second parameter.
+cleanup: + VIR_FREE(buf); + virBitmapFree(map); + return rv; +} + + +static int +qemuDomainGetCPUStats(virDomainPtr domain, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + struct qemud_driver *driver = domain->conn->privateData; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + bool isActive; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + qemuDriverLock(driver); + + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (vm == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), domain->uuid); + goto cleanup; + } + + isActive = virDomainObjIsActive(vm); + if (!isActive) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUACCT)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup CPUACCT controller is not mounted")); + goto cleanup; + } + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), vm->def->name); + goto cleanup; + } + + if (nparams == 0 && ncpus == 0) /* returns max cpu id */ + ret = qemuDomainGetPercpuStats(domain, group, NULL, 0, 0, 0, flags); + else if (start_cpu == -1) /* get total */ + ret = qemuDomainGetTotalcpuStats(domain, group, params, nparams, flags); + else + ret = qemuDomainGetPercpuStats(domain, group, params, nparams, + start_cpu, ncpus ,flags);
Formatting nit: s/ ,/, /
+cleanup: + virCgroupFree(&group); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = "QEMU", @@ -12193,6 +12349,7 @@ static virDriver qemuDriver = { .domainGetDiskErrors = qemuDomainGetDiskErrors, /* 0.9.10 */ .domainSetMetadata = qemuDomainSetMetadata, /* 0.9.10 */ .domainGetMetadata = qemuDomainGetMetadata, /* 0.9.10 */ + .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.10 */
We've now missed 0.9.10, so this should be 0.9.11. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org