On 02/09/2012 03:43 AM, Lai Jiangshan wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list