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

Re: [libvirt] [PATCH 3/3 V7] cpu-stats command shows cpu statistics information of a domain.



On 03/01/2012 07:54 PM, Lai Jiangshan wrote:
> From: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
> 
> 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 cn fujitsu com>
> ---
>  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, &params[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, &params[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, &params[pos]);
> +                        vshPrint(ctl, _("%s(ABI changed? ullong is expected)\n"), s);
> +                        VIR_FREE(s);
> +                    }
> +                } else {
> +                    const char *s = vshGetTypedParamValue(ctl, &params[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 redhat com>
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, &params[i]);
-                vshPrint(ctl, _("%s(ABI changed? ullong is
expected)\n"), s);
-                VIR_FREE(s);
-            }
-        } else {
-            const char *s = vshGetTypedParamValue(ctl, &params[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,
&params[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,
&params[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, &params[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 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]