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

Re: [libvirt] [PATCHv9 1/2] qemu: Report cache occupancy (CMT) with domstats




On 11/20/18 8:56 AM, Wang Huaqiang wrote:
> Adding the interface in qemu to report CMT statistic information
> through command 'virsh domstats --cpu-total'.
> 
> Below is a typical output:
> 
>          # virsh domstats 1 --cpu-total
>          Domain: 'ubuntu16.04-base'
>            ...
>            cpu.cache.monitor.count=2
>            cpu.cache.monitor.0.name=vcpus_1
>            cpu.cache.monitor.0.vcpus=1
>            cpu.cache.monitor.0.bank.count=2
>            cpu.cache.monitor.0.bank.0.id=0
>            cpu.cache.monitor.0.bank.0.bytes=4505600
>            cpu.cache.monitor.0.bank.1.id=1
>            cpu.cache.monitor.0.bank.1.bytes=5586944
>            cpu.cache.monitor.1.name=vcpus_4-6
>            cpu.cache.monitor.1.vcpus=4,5,6
>            cpu.cache.monitor.1.bank.count=2
>            cpu.cache.monitor.1.bank.0.id=0
>            cpu.cache.monitor.1.bank.0.bytes=17571840
>            cpu.cache.monitor.1.bank.1.id=1
>            cpu.cache.monitor.1.bank.1.bytes=29106176
> 
> Signed-off-by: Wang Huaqiang <huaqiang wang intel com>
> ---
>  src/libvirt-domain.c   |  12 ++++
>  src/qemu/qemu_driver.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  tools/virsh.pod        |  14 +++++
>  3 files changed, 185 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 5b76458..73d602e 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11415,6 +11415,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   *     "cpu.user" - user cpu time spent in nanoseconds as unsigned long long.
>   *     "cpu.system" - system cpu time spent in nanoseconds as unsigned long
>   *                    long.
> + *     "cpu.cache.monitor.count" - the number of cache monitors for this domain
> + *     "cpu.cache.monitor.<num>.name" - the name of cache monitor <num>
> + *     "cpu.cache.monitor.<num>.vcpus" - vcpu list of cache monitor <num>
> + *     "cpu.cache.monitor.<num>.bank.count" - the number of cache banks in
> + *                                            cache monitor <num>
> + *     "cpu.cache.monitor.<num>.bank.<index>.id" - host allocated cache id for
> + *                                                 bank <index> in cache
> + *                                                 monitor <num>
> + *     "cpu.cache.monitor.<num>.bank.<index>.bytes" - the number of bytes of
> + *                                                    last level cache that the
> + *                                                    domain is using on cache
> + *                                                    bank <index>
>   *
>   * VIR_DOMAIN_STATS_BALLOON:
>   *     Return memory balloon device information.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7fb9102..d9e216c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19929,6 +19929,158 @@ typedef enum {
>  #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB)
>  
>  
> +typedef struct _virQEMUResctrlMonData virQEMUResctrlMonData;
> +typedef virQEMUResctrlMonData *virQEMUResctrlMonDataPtr;
> +struct _virQEMUResctrlMonData {
> +    const char *name;
> +    char *vcpus;
> +    virResctrlMonitorStatsPtr stats;
> +    size_t nstats;
> +};
> +
> +
> +static int
> +qemuDomainGetResctrlMonData(virDomainObjPtr dom,
> +                            virQEMUResctrlMonDataPtr resdata)
> +{
> +    virDomainResctrlDefPtr resctrl = NULL;
> +    size_t i = 0;
> +    size_t j = 0;
> +    size_t k = 0;
> +
> +    for (i = 0; i < dom->def->nresctrls; i++) {
> +        resctrl = dom->def->resctrls[i];
> +
> +        for (j = 0; j < resctrl->nmonitors; j++) {
> +            virDomainResctrlMonDefPtr domresmon = NULL;
> +            virResctrlMonitorPtr monitor = NULL;
> +
> +            domresmon = resctrl->monitors[j];
> +            monitor = domresmon->instance;
> +
> +            if (domresmon->tag != VIR_RESCTRL_MONITOR_TYPE_CACHE)
> +                continue;

If you want to make this generic, then you could pass this tag from
qemuDomainGetStatsCpuCache as the rest would seemingly be useful for
VIR_RESCTRL_MONITOR_TYPE_MEMBW eventually, just different results.

> +
> +            /* If virBitmapFormat successfully returns an vcpu string, then
> +             * resdata[k].vcpus is assigned with an memory space holding it,
> +             * let this newly allocated memory buffer to be freed along with
> +             * the free of 'resdata' */
> +            if (!(resdata[k].vcpus = virBitmapFormat(domresmon->vcpus)))
> +                return -1;
> +
> +            if (!(resdata[k].name = virResctrlMonitorGetID(monitor))) {

Could this ever be NULL?  Perhaps we just assign directly and assume
we're good. Alternatively it's a VIR_STRDUP() w/ the corresponding
VIR_FREE(*->name).

> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("Could not get monitor ID"));
> +                return -1;
> +            }
> +
> +            if (virResctrlMonitorGetCacheOccupancy(monitor,
> +                                                   &resdata[k].stats,
> +                                                   &resdata[k].nstats) < 0)
> +                return -1;
> +
> +            k++;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
> +static int
> +qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
> +                           virDomainStatsRecordPtr record,
> +                           int *maxparams)
> +{
> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
> +    virQEMUResctrlMonDataPtr resdata = NULL;
> +    virDomainResctrlDefPtr resctrl = NULL;
> +    virDomainResctrlMonDefPtr domresmon = NULL;
> +    unsigned int nresdata = 0;
> +    size_t i = 0;
> +    size_t j = 0;
> +    int ret = -1;
> +
> +    if (!virDomainObjIsActive(dom))
> +        return 0;
> +
> +    for (i = 0; i < dom->def->nresctrls; i++) {
> +        resctrl = dom->def->resctrls[i];
> +        for (j = 0; j < resctrl->nmonitors; j++) {
> +            domresmon = resctrl->monitors[j];
> +            if (domresmon->tag == VIR_RESCTRL_MONITOR_TYPE_CACHE)
> +                nresdata++;
> +        }
> +    }
> +
> +    if (nresdata == 0)
> +        return 0;
> +
> +    if (VIR_ALLOC_N(resdata, nresdata) < 0)
> +        return -1;

Given below - perhaps none of the above really matters if you follow how
virResctrlMonitorGetStats was coded using VIR_APPEND_ELEMENT to append
on each @resdata.

> +
> +    if (qemuDomainGetResctrlMonData(dom, resdata) < 0)
> +        goto cleanup;
> +
> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +             "cpu.cache.monitor.count");
> +    if (virTypedParamsAddUInt(&record->params, &record->nparams,
> +                              maxparams, param_name, nresdata) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; i < nresdata; i++) {
> +        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                 "cpu.cache.monitor.%zu.name", i);
> +        if (virTypedParamsAddString(&record->params,
> +                                    &record->nparams,
> +                                    maxparams,
> +                                    param_name,
> +                                    resdata[i].name) < 0)
> +            goto cleanup;
> +
> +        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                 "cpu.cache.monitor.%zu.vcpus", i);
> +        if (virTypedParamsAddString(&record->params, &record->nparams,
> +                                    maxparams, param_name,
> +                                    resdata[i].vcpus) < 0)
> +            goto cleanup;
> +
> +        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                 "cpu.cache.monitor.%zu.bank.count", i);
> +        if (virTypedParamsAddUInt(&record->params, &record->nparams,
> +                                  maxparams, param_name,
> +                                  resdata[i].nstats) < 0)
> +            goto cleanup;
> +
> +        for (j = 0; j < resdata[i].nstats; j++) {
> +            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                     "cpu.cache.monitor.%zu.bank.%zu.id", i, j);
> +            if (virTypedParamsAddUInt(&record->params, &record->nparams,
> +                                      maxparams, param_name,
> +                                      resdata[i].stats[j].id) < 0)
> +                goto cleanup;
> +
> +            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                     "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j);
> +            if (virTypedParamsAddUInt(&record->params, &record->nparams,
> +                                      maxparams, param_name,
> +                                      resdata[i].stats[j].val) < 0)
> +                goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    for (i = 0; i < nresdata; i++) {
> +        VIR_FREE(resdata[i].vcpus);
> +        VIR_FREE(resdata[i].stats);
> +    }
> +    VIR_FREE(resdata);

All of this should be replaced by a call to qemuDomainFreeResctrlMonData
which would do the above, but replace the VIR_FREE(resdata[i].stats)
with a call to virResctrlMonitorFreeStats which would essentially:

    if (!stats)
        return;

    for (i = 0; i < nstats; i++)
        VIR_FREE(stats[i]);

    VIR_FREE(stats);

This being the opposing action of virResctrlMonitorGetStats.


See and test if the attached patch works for you.

John

> +
> +    return ret;
> +}
> +
> +
>  static int
>  qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
>                              virDomainStatsRecordPtr record,
> @@ -19976,7 +20128,13 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                        int *maxparams,
>                        unsigned int privflags ATTRIBUTE_UNUSED)
>  {
> -    return qemuDomainGetStatsCpuCgroup(dom, record, maxparams);
> +    if (qemuDomainGetStatsCpuCgroup(dom, record, maxparams) < 0)
> +        return -1;
> +
> +    if (qemuDomainGetStatsCpuCache(dom, record, maxparams) < 0)
> +        return -1;
> +
> +    return 0;
>  }
>  
>  
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 4876656..86a4996 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1012,6 +1012,20 @@ I<--cpu-total> returns:
>   "cpu.time" - total cpu time spent for this domain in nanoseconds
>   "cpu.user" - user cpu time spent in nanoseconds
>   "cpu.system" - system cpu time spent in nanoseconds
> + "cpu.cache.monitor.count" - the number of cache monitors for this
> +                             domain
> + "cpu.cache.monitor.<num>.name" - the name of cache monitor <num>
> + "cpu.cache.monitor.<num>.vcpus" - vcpu list of cache monitor <num>
> + "cpu.cache.monitor.<num>.bank.count" - the number of cache banks
> +                                        in cache monitor <num>
> + "cpu.cache.monitor.<num>.bank.<index>.id" - host allocated cache id
> +                                             for bank <index> in
> +                                             cache monitor <num>
> + "cpu.cache.monitor.<num>.bank.<index>.bytes" - the number of bytes
> +                                                of last level cache
> +                                                that the domain is
> +                                                using on cache bank
> +                                                <index>
>  
>  I<--balloon> returns:
>  
> 
>From b55341d7c9516a531dafa7396d60cb86af5c3255 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan redhat com>
Date: Fri, 23 Nov 2018 12:26:49 -0500
Subject: [PATCH] Fixes to CMT v9

-> Introduced virResctrlMonitorFreeStats to be the corollary of the
   virResctrlMonitorGetStats

-> Altered _virQEMUResctrlMonData to follow similar definitions that
   use VIR_APPEND_ELEMENT and then need to free each entry in a loop.

-> Altered qemuDomainGetResctrlMonData to build up @resdata on the fly
   using VIR_APPEND_ELEMENT

-> Added qemuDomainFreeResctrlMonData to undo

Signed-off-by: John Ferlan <jferlan redhat com>
---
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_driver.c   | 86 +++++++++++++++++++++-------------------
 src/util/virresctrl.c    | 13 ++++++
 src/util/virresctrl.h    |  4 ++
 4 files changed, 63 insertions(+), 41 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8889aaa379..5018a13e9c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2692,6 +2692,7 @@ virResctrlInfoNew;
 virResctrlMonitorAddPID;
 virResctrlMonitorCreate;
 virResctrlMonitorDeterminePath;
+virResctrlMonitorFreeStats;
 virResctrlMonitorGetCacheOccupancy;
 virResctrlMonitorGetID;
 virResctrlMonitorNew;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d9e216ccf4..a8e0480ae1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19934,19 +19934,20 @@ typedef virQEMUResctrlMonData *virQEMUResctrlMonDataPtr;
 struct _virQEMUResctrlMonData {
     const char *name;
     char *vcpus;
-    virResctrlMonitorStatsPtr stats;
+    virResctrlMonitorStatsPtr *stats;
     size_t nstats;
 };
 
 
 static int
 qemuDomainGetResctrlMonData(virDomainObjPtr dom,
-                            virQEMUResctrlMonDataPtr resdata)
+                            virQEMUResctrlMonDataPtr *ret_resdata,
+                            size_t *nresdata)
 {
+    virQEMUResctrlMonDataPtr resdata;
     virDomainResctrlDefPtr resctrl = NULL;
     size_t i = 0;
     size_t j = 0;
-    size_t k = 0;
 
     for (i = 0; i < dom->def->nresctrls; i++) {
         resctrl = dom->def->resctrls[i];
@@ -19961,29 +19962,50 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
             if (domresmon->tag != VIR_RESCTRL_MONITOR_TYPE_CACHE)
                 continue;
 
+            if (VIR_ALLOC(resdata) < 0)
+                return -1;
+
             /* If virBitmapFormat successfully returns an vcpu string, then
-             * resdata[k].vcpus is assigned with an memory space holding it,
+             * resdata->vcpus is assigned with an memory space holding it,
              * let this newly allocated memory buffer to be freed along with
              * the free of 'resdata' */
-            if (!(resdata[k].vcpus = virBitmapFormat(domresmon->vcpus)))
-                return -1;
+            if (!(resdata->vcpus = virBitmapFormat(domresmon->vcpus)))
+                goto error;
 
-            if (!(resdata[k].name = virResctrlMonitorGetID(monitor))) {
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("Could not get monitor ID"));
-                return -1;
-            }
+            resdata->name = virResctrlMonitorGetID(monitor);
 
-            if (virResctrlMonitorGetCacheOccupancy(monitor,
-                                                   &resdata[k].stats,
-                                                   &resdata[k].nstats) < 0)
-                return -1;
+            if (virResctrlMonitorGetCacheOccupancy(monitor, resdata->stats,
+                                                   &resdata->nstats) < 0)
+                goto error;
 
-            k++;
+            if (VIR_APPEND_ELEMENT(ret_resdata, *nresdata, resdata) < 0)
+                goto error;
         }
     }
 
     return 0;
+
+ error:
+    VIR_FREE(resdata->vcpus);
+    VIR_FREE(resdata);
+    return -1;
+}
+
+
+static void
+qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata,
+                            unsigned int nresdata)
+{
+    size_t i;
+
+    if (!resdata)
+        return;
+
+    for (i = 0; i < nresdata; i++) {
+        VIR_FREE(resdata[i].vcpus);
+        virResctrlMonitorFreeStats(resdata[i].stats, resdata[i].nstats);
+    }
+    VIR_FREE(resdata);
 }
 
 
@@ -19994,9 +20016,7 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
 {
     char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
     virQEMUResctrlMonDataPtr resdata = NULL;
-    virDomainResctrlDefPtr resctrl = NULL;
-    virDomainResctrlMonDefPtr domresmon = NULL;
-    unsigned int nresdata = 0;
+    size_t nresdata = 0;
     size_t i = 0;
     size_t j = 0;
     int ret = -1;
@@ -20004,24 +20024,12 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
     if (!virDomainObjIsActive(dom))
         return 0;
 
-    for (i = 0; i < dom->def->nresctrls; i++) {
-        resctrl = dom->def->resctrls[i];
-        for (j = 0; j < resctrl->nmonitors; j++) {
-            domresmon = resctrl->monitors[j];
-            if (domresmon->tag == VIR_RESCTRL_MONITOR_TYPE_CACHE)
-                nresdata++;
-        }
-    }
+    if (qemuDomainGetResctrlMonData(dom, &resdata, &nresdata) < 0)
+        goto cleanup;
 
     if (nresdata == 0)
         return 0;
 
-    if (VIR_ALLOC_N(resdata, nresdata) < 0)
-        return -1;
-
-    if (qemuDomainGetResctrlMonData(dom, resdata) < 0)
-        goto cleanup;
-
     snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
              "cpu.cache.monitor.count");
     if (virTypedParamsAddUInt(&record->params, &record->nparams,
@@ -20057,26 +20065,22 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
                      "cpu.cache.monitor.%zu.bank.%zu.id", i, j);
             if (virTypedParamsAddUInt(&record->params, &record->nparams,
                                       maxparams, param_name,
-                                      resdata[i].stats[j].id) < 0)
+                                      resdata[i].stats[j]->id) < 0)
                 goto cleanup;
 
             snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
                      "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j);
             if (virTypedParamsAddUInt(&record->params, &record->nparams,
                                       maxparams, param_name,
-                                      resdata[i].stats[j].val) < 0)
+                                      resdata[i].stats[j]->val) < 0)
                 goto cleanup;
         }
     }
 
     ret = 0;
- cleanup:
-    for (i = 0; i < nresdata; i++) {
-        VIR_FREE(resdata[i].vcpus);
-        VIR_FREE(resdata[i].stats);
-    }
-    VIR_FREE(resdata);
 
+ cleanup:
+    qemuDomainFreeResctrlMonData(resdata, nresdata);
     return ret;
 }
 
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b32eedc3b1..3319aa6b2d 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2747,6 +2747,19 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
 }
 
 
+void
+virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats,
+                           size_t nstats)
+{
+    size_t i;
+
+    for (i = 0; i < nstats; i++)
+        VIR_FREE(stats[i]);
+
+    VIR_FREE(stats);
+}
+
+
 /*
  * virResctrlMonitorGetCacheOccupancy
  *
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 45ec967630..d75ea8e823 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -227,6 +227,10 @@ virResctrlMonitorSetAlloc(virResctrlMonitorPtr monitor,
 int
 virResctrlMonitorRemove(virResctrlMonitorPtr monitor);
 
+void
+virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats,
+                           size_t nstats);
+
 int
 virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
                                    virResctrlMonitorStatsPtr *caches,
-- 
2.17.2


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