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