2:30 p.m.
On 02/09/2012 03:43 AM, Lai Jiangshan wrote:
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org