On 03/01/2012 07:54 PM, Lai Jiangshan wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>
> Total:
> cpu_time 14.312
> CPU0:
> cpu_time 3.253
> CPU1:
> cpu_time 1.923
> CPU2:
> cpu_time 7.424
> CPU3:
> cpu_time 1.712
Personally, I like totals to appear last :) Meanwhile, since the API
returns nanoseconds, but we are printing in seconds, it might be nice to
output a unit.
>
> Changed from V5:
> add --all, --start, --count option
> Changed from V:
> rebase
Again, the 'changed from' lines are better after the ---.
>
> Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
> ---
> tools/virsh.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tools/virsh.pod | 8 +++
> 2 files changed, 162 insertions(+), 0 deletions(-)
>
> /*
> + * "cpu-stats" command
> + */
> +static const vshCmdInfo info_cpu_stats[] = {
> + {"help", N_("show domain cpu statistics")},
> + {"desc", N_("Display statistics about the domain's CPUs, including per-CPU statistics.")},
> + {NULL, NULL},
> +};
> +
> +static const vshCmdOptDef opts_cpu_stats[] = {
> + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> + {"all", VSH_OT_BOOL, 0, N_("Show total statistics only")},
After thinking about this a bit more, "total" works better for the name
of this option. That is, we default to per-cpu then total, --total
limits us to total only, and --start or --count limits to per-cpu only.
That means my squash below will be a bit hard to follow, since it does
a big block of code motion; oh well.
> + {"start", VSH_OT_INT, 0, N_("Show statistics from this CPU")},
> + {"count", VSH_OT_INT, 0, N_("Number of shown CPUs at most")},
> + {NULL, 0, 0, NULL},
> +};
> +
> +static bool
> +cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
> +{
> +
> + /* get supported num of parameter for total statistics */
> + if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0)
> + goto failed_stats;
> + if (VIR_ALLOC_N(params, nparams))
> + goto failed_params;
> +
> + /* passing start_cpu == -1 gives us domain's total status */
> + if ((nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, 0)) < 0)
> + goto failed_stats;
> +
> + vshPrint(ctl, "Total:\n");
This should be marked for translation.
> + for (i = 0; i < nparams; i++) {
> + vshPrint(ctl, "\t%-10s ", params[i].field);
> + if (STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) {
> + if (params[i].type == VIR_TYPED_PARAM_ULLONG) {
> + vshPrint(ctl, "%12.3lf\n",
> + params[i].value.ul / 1000000000.0);
We're losing information; both by chopping fractional digits, and also
by conversion to floating point. It's better to give the user
everything and let them round.
> + } else {
> + const char *s = vshGetTypedParamValue(ctl, ¶ms[i]);
> + vshPrint(ctl, _("%s(ABI changed? ullong is expected)\n"), s);
Not sure this message is worth it.
> + VIR_FREE(s);
> + }
> + } else {
> + const char *s = vshGetTypedParamValue(ctl, ¶ms[i]);
Again, malloc'd result strings should generally not be marked const.
> + vshPrint(ctl, _("%s\n"), s);
This string doesn't need translation.
> + VIR_FREE(s);
> + }
> + }
> + virTypedParameterArrayClear(params, nparams);
> + VIR_FREE(params);
> +
> + if (!show_per_cpu) /* show all stats only */
> + goto cleanup;
> +
> +do_show_per_cpu:
> + /* check cpu, show_count, and ignore wrong argument */
> + if (cpu < 0)
> + cpu = 0;
> + if (show_count < 0)
> + show_count = INT_MAX;
> +
> + /* get max cpu id on the node */
> + if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0)) < 0)
> + goto failed_stats;
> + /* get percpu information */
> + if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0)
This should be 0, not -1, as the total may have a different number of
stats than per-cpu.
It is feasible that some hypervisors might return 0 for one of the two
stats (that is, provide total but not per-cpu stats); we shouldn't error
out in those cases.
> + goto failed_stats;
> +
> + if (VIR_ALLOC_N(params, nparams * 128))
> + goto failed_params;
> +
> + while (cpu <= max_id) {
Per 2/3, max_id should be the array size, not the last index in the
array. We also want to stop iterating if show_count was specified...
> + int ncpus = 128;
> +
> + if (cpu + ncpus - 1 > max_id) /* id starts from 0. */
> + ncpus = max_id + 1 - cpu;
> +
> + if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0)
...and to fully test the underlying API, if the user passes show_count
of 1, we want ncpus to be 1, not 128. So rather than futzing around
with max_id, it's easier to base the entire loop on show_count.
> + goto failed_stats;
> +
> + for (i = 0; i < ncpus; i++) {
> + if (params[i * nparams].type == 0) /* this cpu is not in the map */
> + continue;
> + vshPrint(ctl, "CPU%d:\n", cpu + i);
> +
> + for (j = 0; j < nparams; j++) {
> + pos = i * nparams + j;
> + vshPrint(ctl, "\t%-10s ", params[pos].field);
> + if (STREQ(params[pos].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) {
> + if (params[j].type == VIR_TYPED_PARAM_ULLONG) {
> + vshPrint(ctl, "%12.3lf\n",
> + params[pos].value.ul / 1000000000.0);
Same comments as for totals.
> + } else {
> + const char *s = vshGetTypedParamValue(ctl, ¶ms[pos]);
> + vshPrint(ctl, _("%s(ABI changed? ullong is expected)\n"), s);
> + VIR_FREE(s);
> + }
> + } else {
> + const char *s = vshGetTypedParamValue(ctl, ¶ms[pos]);
> + vshPrint(ctl, _("%s\n"), s);
> + VIR_FREE(s);
> + }
> + }
> +
> + if (--show_count <= 0) /* mark done to exit the outmost loop */
s/outmost/outermost/
> +++ b/tools/virsh.pod
> @@ -790,6 +790,14 @@ 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-stats> I<domain> [I<--all>] [I<start>] [I<count>]
> +
> +Provide cpu statistics information of a domain. The domain should
> +be running. Default it shows stats for all CPUs, and a total. Use
s/Default it/By default, it/
> +I<--all> for only the total stats, I<start> for only the per-cpu
> +stats of the CPUs from I<start>, I<count> for only I<count> CPUs'
> +stats.
> +
ACK with these changes squashed in, so I'll push the series shortly once
I figure out how to properly handle the case where the driver truncates
the array.
From 6fb1f35cc283e08be3f62fc8d60b3297467fdafd Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@xxxxxxxxxx>
Date: Tue, 6 Mar 2012 17:24:39 -0700
Subject: [PATCH] fixup to 3/3
---
tools/virsh.c | 131
++++++++++++++++++++++++++++---------------------------
tools/virsh.pod | 4 +-
2 files changed, 68 insertions(+), 67 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index ab52b5b..70a932b 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -5541,13 +5541,14 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
*/
static const vshCmdInfo info_cpu_stats[] = {
{"help", N_("show domain cpu statistics")},
- {"desc", N_("Display statistics about the domain's CPUs, including
per-CPU statistics.")},
+ {"desc",
+ N_("Display per-CPU and total statistics about the domain's CPUs")},
{NULL, NULL},
};
static const vshCmdOptDef opts_cpu_stats[] = {
{"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
- {"all", VSH_OT_BOOL, 0, N_("Show total statistics only")},
+ {"total", VSH_OT_BOOL, 0, N_("Show total statistics only")},
{"start", VSH_OT_INT, 0, N_("Show statistics from this CPU")},
{"count", VSH_OT_INT, 0, N_("Number of shown CPUs at most")},
{NULL, 0, 0, NULL},
@@ -5559,7 +5560,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
virDomainPtr dom;
virTypedParameterPtr params = NULL;
int i, j, pos, max_id, cpu = -1, show_count = -1, nparams;
- bool show_all = false, show_per_cpu = false;
+ bool show_total = false, show_per_cpu = false;
if (!vshConnectionUsability(ctl, ctl->conn))
return false;
@@ -5567,77 +5568,45 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
return false;
- show_all = vshCommandOptBool(cmd, "all");
+ show_total = vshCommandOptBool(cmd, "total");
if (vshCommandOptInt(cmd, "start", &cpu) > 0)
show_per_cpu = true;
if (vshCommandOptInt(cmd, "count", &show_count) > 0)
show_per_cpu = true;
/* default show per_cpu and total */
- if (!show_all && !show_per_cpu) {
- show_all = true;
+ if (!show_total && !show_per_cpu) {
+ show_total = true;
show_per_cpu = true;
}
- if (!show_all)
- goto do_show_per_cpu;
-
- /* get supported num of parameter for total statistics */
- if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0)
- goto failed_stats;
- if (VIR_ALLOC_N(params, nparams))
- goto failed_params;
+ if (!show_per_cpu) /* show total stats only */
+ goto do_show_total;
- /* passing start_cpu == -1 gives us domain's total status */
- if ((nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1,
0)) < 0)
- goto failed_stats;
-
- vshPrint(ctl, "Total:\n");
- for (i = 0; i < nparams; i++) {
- vshPrint(ctl, "\t%-10s ", params[i].field);
- if (STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) {
- if (params[i].type == VIR_TYPED_PARAM_ULLONG) {
- vshPrint(ctl, "%12.3lf\n",
- params[i].value.ul / 1000000000.0);
- } else {
- const char *s = vshGetTypedParamValue(ctl, ¶ms[i]);
- vshPrint(ctl, _("%s(ABI changed? ullong is
expected)\n"), s);
- VIR_FREE(s);
- }
- } else {
- const char *s = vshGetTypedParamValue(ctl, ¶ms[i]);
- vshPrint(ctl, _("%s\n"), s);
- VIR_FREE(s);
- }
- }
- virTypedParameterArrayClear(params, nparams);
- VIR_FREE(params);
-
- if (!show_per_cpu) /* show all stats only */
- goto cleanup;
-
-do_show_per_cpu:
/* check cpu, show_count, and ignore wrong argument */
if (cpu < 0)
cpu = 0;
- if (show_count < 0)
- show_count = INT_MAX;
- /* get max cpu id on the node */
+ /* get number of cpus on the node */
if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0)) < 0)
goto failed_stats;
+ if (show_count < 0 || show_count > max_id)
+ show_count = max_id;
+
/* get percpu information */
- if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0)
+ if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0)) < 0)
goto failed_stats;
- if (VIR_ALLOC_N(params, nparams * 128))
- goto failed_params;
+ if (!nparams) {
+ vshPrint(ctl, "%s", _("No per-CPU stats available"));
+ goto do_show_total;
+ }
- while (cpu <= max_id) {
- int ncpus = 128;
+ if (VIR_ALLOC_N(params, nparams * MIN(show_count, 128)) < 0)
+ goto failed_params;
- if (cpu + ncpus - 1 > max_id) /* id starts from 0. */
- ncpus = max_id + 1 - cpu;
+ while (show_count) {
+ int ncpus = MIN(show_count, 128);
if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0)
goto failed_stats;
@@ -5650,29 +5619,61 @@ do_show_per_cpu:
for (j = 0; j < nparams; j++) {
pos = i * nparams + j;
vshPrint(ctl, "\t%-10s ", params[pos].field);
- if (STREQ(params[pos].field,
VIR_DOMAIN_CPU_STATS_CPUTIME)) {
- if (params[j].type == VIR_TYPED_PARAM_ULLONG) {
- vshPrint(ctl, "%12.3lf\n",
- params[pos].value.ul / 1000000000.0);
- } else {
- const char *s = vshGetTypedParamValue(ctl,
¶ms[pos]);
- vshPrint(ctl, _("%s(ABI changed? ullong is
expected)\n"), s);
- VIR_FREE(s);
- }
+ if (STREQ(params[pos].field,
VIR_DOMAIN_CPU_STATS_CPUTIME) &&
+ params[j].type == VIR_TYPED_PARAM_ULLONG) {
+ vshPrint(ctl, "%lld.%09lld seconds\n",
+ params[pos].value.ul / 1000000000,
+ params[pos].value.ul % 1000000000);
} else {
const char *s = vshGetTypedParamValue(ctl,
¶ms[pos]);
vshPrint(ctl, _("%s\n"), s);
VIR_FREE(s);
}
}
-
- if (--show_count <= 0) /* mark done to exit the outmost loop */
- cpu = max_id;
}
cpu += ncpus;
+ show_count -= ncpus;
virTypedParameterArrayClear(params, nparams * ncpus);
}
VIR_FREE(params);
+
+do_show_total:
+ if (!show_total)
+ goto cleanup;
+
+ /* get supported num of parameter for total statistics */
+ if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0)
+ goto failed_stats;
+
+ if (!nparams) {
+ vshPrint(ctl, "%s", _("No total stats available"));
+ goto cleanup;
+ }
+
+ if (VIR_ALLOC_N(params, nparams))
+ goto failed_params;
+
+ /* passing start_cpu == -1 gives us domain's total status */
+ if ((nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1,
0)) < 0)
+ goto failed_stats;
+
+ vshPrint(ctl, _("Total:\n"));
+ for (i = 0; i < nparams; i++) {
+ vshPrint(ctl, "\t%-10s ", params[i].field);
+ if (STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME) &&
+ params[i].type == VIR_TYPED_PARAM_ULLONG) {
+ vshPrint(ctl, "%llu.%09llu seconds\n",
+ params[i].value.ul / 1000000000,
+ params[i].value.ul % 1000000000);
+ } else {
+ char *s = vshGetTypedParamValue(ctl, ¶ms[i]);
+ vshPrint(ctl, "%s\n", s);
+ VIR_FREE(s);
+ }
+ }
+ virTypedParameterArrayClear(params, nparams);
+ VIR_FREE(params);
+
cleanup:
virDomainFree(dom);
return true;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index b23868d..1eb9499 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -790,11 +790,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-stats> I<domain> [I<--all>] [I<start>] [I<count>]
+=item B<cpu-stats> I<domain> [I<--total>] [I<start>] [I<count>]
Provide cpu statistics information of a domain. The domain should
be running. Default it shows stats for all CPUs, and a total. Use
-I<--all> for only the total stats, I<start> for only the per-cpu
+I<--total> for only the total stats, I<start> for only the per-cpu
stats of the CPUs from I<start>, I<count> for only I<count> CPUs'
stats.
--
1.7.7.6
--
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