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

Re: [libvirt] [PATCH 5/5 V5] cpu-accts command shows cpu accounting information of a domain.



On 02/09/2012 03:43 AM, Lai Jiangshan wrote:
> From: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
> 
> Total:
> 	cpu_time        15.8
> CPU0:
> 	cpu_time        8.6
> CPU1:
> 	cpu_time        3.5
> CPU2:
> 	cpu_time        2.4
> CPU3:
> 	cpu_time        1.3

We should align the output so all '.' are in the same column.  Given
that the kernel gives us more than just tenths of a second, should we be
exposing more granularity to the user?

>  
>  /*
> + * "cputime" command

Wrong comment, since you installed it under "cpu-accts".

> + */
> +static const vshCmdInfo info_cpu_accts[] = {

But I still don't like that name, either; since we're wrapping
virDomainGetCPUStats, I think a better name would be "cpu-stats".

> +    {"help", N_("show domain cpu accounting information")},

Here, I'd use "show domain cpu usage statistics",

> +    {"desc", N_("Returns information about the domain's cpu accounting")},

and for the longer text, maybe "Display information about the domain's
CPU usage, including per-CPU accounting"

> +    {NULL, NULL},
> +};
> +
> +static const vshCmdOptDef opts_cpu_accts[] = {
> +    {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> +    {NULL, 0, 0, NULL},

Should we expose the ability to pass in '--all' or an optional
'--start=n --count=n' to let the user limit how much input they want?
Given your above example,

virsh cpu-stats dom
=> Stats for all CPUs, and a total

virsh cpu-stats dom --all
=> Only the total stats

virsh cpu-stats dom --start=2
=> Only the per-cpu stats, for CPU 2 to the end

virsh cpu-stats dom --start=2 --count=1
=> Only the per-cpu stats of just CPU 2

> +};
> +
> +static bool
> +cmdCPUAccts(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom;
> +    virTypedParameterPtr params = NULL;
> +    bool ret = true;
> +    int i, j, pos, cpu, nparams;
> +    int max_id;
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn))
> +        return false;
> +
> +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    /* get max cpu id on the node */
> +    max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0);
> +    if (max_id < 0) {
> +        vshPrint(ctl, "max id %d\n", max_id);

If max_id is less than 0, then it will be -1 and the command failed; we
should print out the libvirt error at that point.

> +        goto cleanup;
> +    }
> +    /* get supported num of parameter for total statistics */
> +    nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0);
> +    if (nparams < 0)
> +        goto cleanup;
> +
> +    if (VIR_ALLOC_N(params, nparams)) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +    /* passing start_cpu == -1 gives us domain's total status */
> +    nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, 0);
> +    if (nparams < 0)
> +        goto cleanup;
> +
> +    vshPrint(ctl, "Total:\n");
> +    for (i = 0; i < nparams; i++) {
> +        vshPrint(ctl, "\t%-15s ", params[i].field);
> +        switch (params[i].type) {
> +        case VIR_TYPED_PARAM_ULLONG:
> +            /* Now all UULONG param is used for time accounting in ns */

That may not be true in the future.  I'd rather see this specifically do
a STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME) before
reformatting the output...

> +            vshPrint(ctl, "%.1lf\n",
> +                     params[i].value.ul / 1000000000.0);
> +            break;
> +        default:
> +            vshError(ctl, "%s", _("API mismatch?"));
> +            goto cleanup;

...and the default, rather than erroring out, should instead use
vshGetTypedParamValue and output the name/value pair for all statistics
that were unknown by this version of virsh but which may have been added
in the server.

> +        }
> +    }
> +    VIR_FREE(params);

Memory leak if you don't use virTypedParameterArrayClear first.  Just
because we don't pass strings back now doesn't mean we won't do it in
the future.

> +    /* get percpu information */
> +    nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0);
> +    if (nparams < 0)
> +        goto cleanup;
> +
> +    cpu = 0;
> +    while (cpu <= max_id) {
> +        int ncpus = 128;
> +
> +        if (cpu + ncpus - 1 > max_id) /* id starts from 0. */
> +            ncpus = max_id + 1 - cpu;
> +
> +        if (VIR_ALLOC_N(params, nparams * ncpus)) {

Why are we re-allocating this each time through the loop?  It should be
enough to allocate it once before the loop, and reuse it thereafter.

> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0)
> +            goto cleanup;
> +
> +        for (i = 0; i < ncpus; i++) {
> +            if (params[i * nparams].type == 0)
> +                continue;

This shouldn't be possible (if it is true, we have a bug in our server).

> +            vshPrint(ctl, "CPU%d:\n", cpu + i);
> +
> +            for (j = 0; j < nparams; j++) {
> +                pos = i * nparams + j;
> +                if (params[pos].type == 0)
> +                    continue;

This shouldn't be possible (if it is true, we have a bug in our server).

> +                vshPrint(ctl, "\t%-15s ", params[pos].field);
> +                switch(params[j].type) {
> +                case VIR_TYPED_PARAM_ULLONG:
> +                    vshPrint(ctl, "%.1lf\n",
> +                        params[pos].value.ul / 1000000000.0);
> +                    break;
> +                default:
> +                    vshError(ctl, "%s", _("API mismatch?"));
> +                    goto cleanup;

Same story about formatting - we want to format all name/value pairs,
even those that were added in the server after this version of virsh was
compiled; and we can only do the conversion from nanoseconds to
fractional seconds if the field name is known.

> +                }
> +            }
> +        }
> +        cpu += ncpus;
> +        /* Note: If we handle string type, it should be freed */
> +        VIR_FREE(params);

Your comment was right - you are missing a call to
virTypedParameterArrayClear.  And even if you hoist the array allocation
outside of the loop, you still need an array clear in the loop between
successive iterations.

> +    }
> +cleanup:
> +    VIR_FREE(params);
> +    virDomainFree(dom);
> +    return ret;
> +}
> +
> +/*
>   * "inject-nmi" command
>   */
>  static const vshCmdInfo info_inject_nmi[] = {
> @@ -16441,6 +16556,7 @@ static const vshCmdDef domManagementCmds[] = {
>  #endif
>      {"cpu-baseline", cmdCPUBaseline, opts_cpu_baseline, info_cpu_baseline, 0},
>      {"cpu-compare", cmdCPUCompare, opts_cpu_compare, info_cpu_compare, 0},
> +    {"cpu-accts", cmdCPUAccts, opts_cpu_accts, info_cpu_accts, 0},
>      {"create", cmdCreate, opts_create, info_create, 0},
>      {"define", cmdDefine, opts_define, info_define, 0},
>      {"desc", cmdDesc, opts_desc, info_desc, 0},
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index bd79b4c..2b2f70b 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -762,6 +762,11 @@ Provide the maximum number of virtual CPUs supported for a guest VM on
>  this connection.  If provided, the I<type> parameter must be a valid
>  type attribute for the <domain> element of XML.
>  
> +=item B<cpu-accts> I<domain>
> +
> +Provide cpu accounting information of a domain. The domain should
> +be running.
> +
>  =item B<migrate> [I<--live>] [I<--direct>] [I<--p2p> [I<--tunnelled>]]
>  [I<--persistent>] [I<--undefinesource>] [I<--suspend>] [I<--copy-storage-all>]
>  [I<--copy-storage-inc>] [I<--change-protection>] [I<--verbose>]

-- 
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]