[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




在 11/24/2018 1:33 AM, John Ferlan 写道:

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.


Good suggestion. Thanks.


+
+            /* 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).


Resctrl monitor ID will not be NULL at this place.

I am using VIR_STRDUP to make a copy of name and ensure it is freed at the end

of function in next version.



+                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.


OK. I'll use VIR_APPEND_ELEMENT to allocate and append new element

for @resdata.  That means the lines of 'if (VIR_ALLOC_N(resdata, nresdata) < 0)'

will be removed and the @nresdata will not be accumulated before calling

qemuDomainGetResctrlMonData.



+
+    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.


The patch wouldn't work. In virResctrlMonitorGetStats the @stats

is patched as a array of virResctrlMonitorStats not virResctrlMonitorStatsPtr,

so the line VIR_FREE(stats[i]) does not work. Element of *stats is not pointer

but a  virResctrlMonitorStats structure.

The VIR_APPEND_ELEMENTis:

             if (VIR_APPEND_ELEMENT(*stats, *nstats, *stat) < 0)

             and @stat is

             virResctrlMonitorStatsPtr stat = NULL;

So it appends the whole element to the end of * stats

If you think it is more common to append a pointer not the data

structure, I can change it.

Anyway I'll send a patch to append the pointer (virResctrlMonitorStatsPtr).

You can make the decision to use it or not.


John


Thanks for review.

Huaqiang


+
+    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:


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