[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/5 V5] qemu driver for virDomainGetCPUstats using cpuacct cgroup.



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(&params[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 = &params[ (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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]