[libvirt] [PATCHv7 17/18] qemu: Report cache occupancy (CMT) with domstats

Wang Huaqiang posted 18 patches 5 years, 8 months ago
[libvirt] [PATCHv7 17/18] qemu: Report cache occupancy (CMT) with domstats
Posted by Wang Huaqiang 5 years, 8 months ago
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     |   9 ++
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   | 229 +++++++++++++++++++++++++++++++++++++++++++++++
 src/util/virresctrl.c    | 130 +++++++++++++++++++++++++++
 src/util/virresctrl.h    |  12 +++
 5 files changed, 381 insertions(+)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 7690339..4895f9f 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11345,6 +11345,15 @@ 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" - tocal cache monitoring groups
+ *     "cpu.cache.monitor.M.name" - name of cache monitoring group 'M'
+ *     "cpu.cache.monitor.M.vcpus" - vcpus for cache monitoring group 'M'
+ *     "cpu.cache.monitor.M.bank.count" - total bank number of cache monitoring
+ *                    group 'M'
+ *     "cpu.cache.monitor.M.bank.N.id" - OS assigned cache bank id for cache
+ *                    'N' in cache monitoring group 'M'
+ *     "cpu.cache.monitor.M.bank.N.bytes" - monitor's cache occupancy of cache
+ *                    bank 'N' in cache monitoring group 'M'
  *
  * VIR_DOMAIN_STATS_BALLOON:
  *     Return memory balloon device information.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 91801ed..4551767 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2683,6 +2683,7 @@ virResctrlInfoNew;
 virResctrlMonitorAddPID;
 virResctrlMonitorCreate;
 virResctrlMonitorDeterminePath;
+virResctrlMonitorGetCacheOccupancy;
 virResctrlMonitorGetID;
 virResctrlMonitorIsRunning;
 virResctrlMonitorNew;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 12a5f8f..9828118 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -102,6 +102,7 @@
 #include "virnuma.h"
 #include "dirname.h"
 #include "netdev_bandwidth_conf.h"
+#include "c-ctype.h"
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -19698,6 +19699,230 @@ typedef enum {
 #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB)
 
 
+/* In terms of the output of virBitmapFormat, both '1-3' and '1,3' are valid
+ * outputs and represent different vcpu set. But it is not easy to
+ * differentiate these two vcpu set formats at first glance.
+ *
+ * This function could be used to clear this ambiguity, it substitutes all '-'
+ * with ',' while generates semantically correct vcpu set.
+ * e.g. vcpu set string '1-3' will be replaced by string '1,2,3'. */
+static char *
+qemuDomainVcpuFormatHelper(const char *vcpus)
+{
+    size_t i = 0;
+    int last = 0;
+    int start = 0;
+    char * tmp = NULL;
+    bool firstnum = true;
+    const char *cur = vcpus;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    char *ret = NULL;
+
+    if (virStringIsEmpty(cur))
+        return NULL;
+
+    while (*cur != '\0') {
+        if (!c_isdigit(*cur))
+            goto cleanup;
+
+        if (virStrToLong_i(cur, &tmp, 10, &start) < 0)
+            goto cleanup;
+        if (start < 0)
+            goto cleanup;
+
+        cur = tmp;
+
+        virSkipSpaces(&cur);
+
+        if (*cur == ',' || *cur == 0) {
+            if (!firstnum)
+                virBufferAddChar(&buf, ',');
+            virBufferAsprintf(&buf, "%d", start);
+            firstnum = false;
+        } else if (*cur == '-') {
+            cur++;
+            virSkipSpaces(&cur);
+
+            if (virStrToLong_i(cur, &tmp, 10, &last) < 0)
+                goto cleanup;
+
+            if (last < start)
+                goto cleanup;
+            cur = tmp;
+
+            for (i = start; i <= last; i++) {
+                if (!firstnum)
+
+                    virBufferAddChar(&buf, ',');
+                virBufferAsprintf(&buf, "%ld", i);
+                firstnum = 0;
+            }
+
+            virSkipSpaces(&cur);
+        }
+
+        if (*cur == ',') {
+            cur++;
+            virSkipSpaces(&cur);
+        } else if (*cur == 0) {
+            break;
+        } else {
+            goto cleanup;
+        }
+    }
+
+    ret = virBufferContentAndReset(&buf);
+ cleanup:
+    virBufferFreeAndReset(&buf);
+    return ret;
+}
+
+
+static int
+qemuDomainGetStatsCpuResource(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+                              virDomainObjPtr dom,
+                              virDomainStatsRecordPtr record,
+                              int *maxparams,
+                              unsigned int privflags ATTRIBUTE_UNUSED,
+                              virResctrlMonitorType restag)
+{
+    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
+    virDomainResctrlMonDefPtr domresmon = NULL;
+    virResctrlMonitorStatsPtr stats = NULL;
+    size_t nstats = 0;
+    virDomainResctrlDefPtr resctrl = NULL;
+    unsigned int nmonitors = 0;
+    unsigned int imonitor = 0;
+    const char *restype = NULL;
+    char *rawvcpus = NULL;
+    char *vcpus = NULL;
+    size_t i = 0;
+    size_t j = 0;
+    int ret = -1;
+
+    if (!virDomainObjIsActive(dom))
+        return 0;
+
+    if (restag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
+        restype = "cache";
+    } else {
+        VIR_DEBUG("Invalid CPU resource type");
+        return -1;
+    }
+
+    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 (virResctrlMonitorIsRunning(domresmon->instance) &&
+                domresmon->tag == restag)
+                nmonitors++;
+        }
+    }
+
+    if (nmonitors) {
+        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                 "cpu.%s.monitor.count", restype);
+        if (virTypedParamsAddUInt(&record->params,
+                                  &record->nparams,
+                                  maxparams,
+                                  param_name,
+                                  nmonitors) < 0)
+            goto cleanup;
+    }
+
+    for (i = 0; i < dom->def->nresctrls; i++) {
+        resctrl = dom->def->resctrls[i];
+
+        for (j = 0; j < resctrl->nmonitors; j++) {
+            size_t l = 0;
+            virResctrlMonitorPtr monitor = resctrl->monitors[j]->instance;
+            const char *id = virResctrlMonitorGetID(monitor);
+
+            if (!id)
+                goto cleanup;
+
+            domresmon = resctrl->monitors[j];
+
+            if (!virResctrlMonitorIsRunning(domresmon->instance))
+                continue;
+
+            if (!(rawvcpus = virBitmapFormat(domresmon->vcpus)))
+                goto cleanup;
+
+            vcpus = qemuDomainVcpuFormatHelper(rawvcpus);
+            if (!vcpus)
+                goto cleanup;
+
+            if (virResctrlMonitorGetCacheOccupancy(monitor, &stats,
+                                                   &nstats) < 0)
+                goto cleanup;
+
+            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                     "cpu.%s.monitor.%d.name", restype, imonitor);
+            if (virTypedParamsAddString(&record->params,
+                                        &record->nparams,
+                                        maxparams,
+                                        param_name,
+                                        id) < 0)
+                goto cleanup;
+
+            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                     "cpu.%s.monitor.%d.vcpus", restype, imonitor);
+
+            if (virTypedParamsAddString(&record->params,
+                                        &record->nparams,
+                                        maxparams,
+                                        param_name,
+                                        vcpus) < 0)
+                goto cleanup;
+
+            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                     "cpu.%s.monitor.%d.bank.count", restype, imonitor);
+            if (virTypedParamsAddUInt(&record->params,
+                                      &record->nparams,
+                                      maxparams,
+                                      param_name,
+                                      nstats) < 0)
+                goto cleanup;
+
+            for (l = 0; l < nstats; l++) {
+                snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                         "cpu.%s.monitor.%d.bank.%ld.id",
+                         restype, imonitor, l);
+                if (virTypedParamsAddUInt(&record->params,
+                                          &record->nparams,
+                                          maxparams,
+                                          param_name,
+                                          stats[l].id) < 0)
+                    goto cleanup;
+
+
+                snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+                         "cpu.%s.monitor.%d.bank.%ld.bytes",
+                         restype, imonitor, l);
+                if (virTypedParamsAddUInt(&record->params,
+                                          &record->nparams,
+                                          maxparams,
+                                          param_name,
+                                          stats[l].val) < 0)
+                    goto cleanup;
+            }
+
+            VIR_FREE(stats);
+            VIR_FREE(vcpus);
+            imonitor++;
+        }
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(vcpus);
+    return ret;
+}
+
+
 static int
 qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
                       virDomainObjPtr dom,
@@ -19735,6 +19960,10 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
             return -1;
     }
 
+    if (qemuDomainGetStatsCpuResource(driver, dom, record, maxparams, privflags,
+                                      VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0)
+        return -1;
+
     return 0;
 }
 
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index fa3e6e9..02e7095 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2713,3 +2713,133 @@ virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor)
 
     return ret;
 }
+
+
+static int
+virResctrlMonitorStatsSorter(const void *a,
+                             const void *b)
+{
+    return ((virResctrlMonitorStatsPtr)a)->id
+        - ((virResctrlMonitorStatsPtr)b)->id;
+}
+
+
+/*
+ * virResctrlMonitorGetStats
+ *
+ * @monitor: The monitor that the statistic data will be retrieved from.
+ * @resource: The name for resource name. 'llc_occupancy' for cache resource.
+ * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth resource.
+ * @stats: Array of virResctrlMonitorStatsPtr for holding cache or memory
+ * bandwidth usage data.
+ * @nstats: A size_t pointer to hold the returned array length of @stats
+ *
+ * Get cache or memory bandwidth utilization information.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+static int
+virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
+                          const char *resource,
+                          virResctrlMonitorStatsPtr *stats,
+                          size_t *nstats)
+{
+    int rv = -1;
+    int ret = -1;
+    DIR *dirp = NULL;
+    char *datapath = NULL;
+    struct dirent *ent = NULL;
+    virResctrlMonitorStatsPtr stat = NULL;
+
+    if (!monitor) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Invalid resctrl monitor"));
+        return -1;
+    }
+
+    if (virAsprintf(&datapath, "%s/mon_data", monitor->path) < 0)
+        return -1;
+
+    if (virDirOpen(&dirp, datapath) < 0)
+        goto cleanup;
+
+    *nstats = 0;
+    while (virDirRead(dirp, &ent, datapath) > 0) {
+        char *node_id = NULL;
+
+        if (VIR_ALLOC(stat) < 0)
+            goto cleanup;
+
+        /* Looking for directory that contains resource utilization
+         * information file. The directory name is arranged in format
+         * "mon_<node_name>_<node_id>". For example, "mon_L3_00" and
+         * "mon_L3_01" are two target directories for a two nodes system
+         * with resource utilization data file for each node respectively.
+         */
+        if (ent->d_type != DT_DIR)
+            continue;
+
+        /* Looking for directory has a prefix 'mon_L' */
+        if (!(node_id = STRSKIP(ent->d_name, "mon_L")))
+            continue;
+
+        /* Looking for directory has another '_' */
+        node_id = strchr(node_id, '_');
+        if (!node_id)
+            continue;
+
+        /* Skip the character '_' */
+        if (!(node_id = STRSKIP(node_id, "_")))
+            continue;
+
+        /* The node ID number should be here, parsing it. */
+        if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0)
+            goto cleanup;
+
+        rv = virFileReadValueUint(&stat->val, "%s/%s/%s", datapath,
+                                  ent->d_name, resource);
+        if (rv == -2) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("File '%s/%s/%s' does not exist."),
+                           datapath, ent->d_name, resource);
+        }
+        if (rv < 0)
+            goto cleanup;
+
+        if (VIR_APPEND_ELEMENT(*stats, *nstats, *stat) < 0)
+            goto cleanup;
+    }
+
+    /* Sort in id's ascending order */
+    qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter);
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(datapath);
+    VIR_FREE(stat);
+    VIR_DIR_CLOSE(dirp);
+    return ret;
+}
+
+
+/*
+ * virResctrlMonitorGetCacheOccupancy
+ *
+ * @monitor: The monitor that the statistic data will be retrieved from.
+ * @caches: Array of virResctrlMonitorStatsPtr for receiving cache occupancy
+ * data. Caller is responsible to free this array.
+ * @ncaches: A size_t pointer to hold the returned array length of @caches
+ *
+ * Get cache or memory bandwidth utilization information.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+
+int
+virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
+                                   virResctrlMonitorStatsPtr *caches,
+                                   size_t *ncaches)
+{
+    return virResctrlMonitorGetStats(monitor, "llc_occupancy",
+                                     caches, ncaches);
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 8d8fdc2..004c40e 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -191,6 +191,13 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
 typedef struct _virResctrlMonitor virResctrlMonitor;
 typedef virResctrlMonitor *virResctrlMonitorPtr;
 
+typedef struct _virResctrlMonitorStats virResctrlMonitorStats;
+typedef virResctrlMonitorStats *virResctrlMonitorStatsPtr;
+struct _virResctrlMonitorStats {
+    unsigned int id;
+    unsigned int val;
+};
+
 virResctrlMonitorPtr
 virResctrlMonitorNew(void);
 
@@ -222,4 +229,9 @@ virResctrlMonitorRemove(virResctrlMonitorPtr monitor);
 
 bool
 virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor);
+
+int
+virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
+                                   virResctrlMonitorStatsPtr *caches,
+                                   size_t *ncaches);
 #endif /*  __VIR_RESCTRL_H__ */
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 17/18] qemu: Report cache occupancy (CMT) with domstats
Posted by John Ferlan 5 years, 8 months ago

On 10/22/18 4:01 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     |   9 ++
>  src/libvirt_private.syms |   1 +
>  src/qemu/qemu_driver.c   | 229 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virresctrl.c    | 130 +++++++++++++++++++++++++++
>  src/util/virresctrl.h    |  12 +++
>  5 files changed, 381 insertions(+)
> 

I have a feeling I'll be asking for this to be split up a bit, but let's
see...  There's *util, *qemu, and *API code in the same patch.

> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 7690339..4895f9f 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11345,6 +11345,15 @@ 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" - tocal cache monitoring groups
> + *     "cpu.cache.monitor.M.name" - name of cache monitoring group 'M'
> + *     "cpu.cache.monitor.M.vcpus" - vcpus for cache monitoring group 'M'
> + *     "cpu.cache.monitor.M.bank.count" - total bank number of cache monitoring
> + *                    group 'M'
> + *     "cpu.cache.monitor.M.bank.N.id" - OS assigned cache bank id for cache
> + *                    'N' in cache monitoring group 'M'
> + *     "cpu.cache.monitor.M.bank.N.bytes" - monitor's cache occupancy of cache
> + *                    bank 'N' in cache monitoring group 'M'
>   *
>   * VIR_DOMAIN_STATS_BALLOON:
>   *     Return memory balloon device information.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 91801ed..4551767 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2683,6 +2683,7 @@ virResctrlInfoNew;
>  virResctrlMonitorAddPID;
>  virResctrlMonitorCreate;
>  virResctrlMonitorDeterminePath;
> +virResctrlMonitorGetCacheOccupancy;
>  virResctrlMonitorGetID;
>  virResctrlMonitorIsRunning;
>  virResctrlMonitorNew;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 12a5f8f..9828118 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -102,6 +102,7 @@
>  #include "virnuma.h"
>  #include "dirname.h"
>  #include "netdev_bandwidth_conf.h"
> +#include "c-ctype.h"

Ahh.. this one's a red flag...  Says to me that something should move to
a util function...

>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -19698,6 +19699,230 @@ typedef enum {
>  #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB)
>  
>  
> +/* In terms of the output of virBitmapFormat, both '1-3' and '1,3' are valid
> + * outputs and represent different vcpu set. But it is not easy to
> + * differentiate these two vcpu set formats at first glance.
> + *
> + * This function could be used to clear this ambiguity, it substitutes all '-'
> + * with ',' while generates semantically correct vcpu set.
> + * e.g. vcpu set string '1-3' will be replaced by string '1,2,3'. */
> +static char *
> +qemuDomainVcpuFormatHelper(const char *vcpus)
> +{
> +    size_t i = 0;
> +    int last = 0;
> +    int start = 0;
> +    char * tmp = NULL;
> +    bool firstnum = true;
> +    const char *cur = vcpus;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    char *ret = NULL;
> +
> +    if (virStringIsEmpty(cur))
> +        return NULL;
> +
> +    while (*cur != '\0') {
> +        if (!c_isdigit(*cur))

Explains the #include, but in the long run, I don't think this method is
worth the effort since all you're doing is printing the format in the
output. Is there other libvirt generated output for cpu stats that does
a similar conversion?  If so, share code, if not drop this.

No need to rearrange the range someone else entered and we've
succesfully parsed in some manner. I think the output should look like
the XML output unless there's some compelling reason to change it which
I don't see.

>From above the:

>            cpu.cache.monitor.1.name=vcpus_4-6
>            cpu.cache.monitor.1.vcpus=4,5,6

would then be:

>            cpu.cache.monitor.1.name=vcpus_4-6
>            cpu.cache.monitor.1.vcpus=4-6

right?

I don't even want to think about someone who does "7,4-6"...

> +            goto cleanup;
> +
> +        if (virStrToLong_i(cur, &tmp, 10, &start) < 0)
> +            goto cleanup;
> +        if (start < 0)
> +            goto cleanup;
> +
> +        cur = tmp;
> +
> +        virSkipSpaces(&cur);
> +
> +        if (*cur == ',' || *cur == 0) {
> +            if (!firstnum)
> +                virBufferAddChar(&buf, ',');
> +            virBufferAsprintf(&buf, "%d", start);
> +            firstnum = false;
> +        } else if (*cur == '-') {
> +            cur++;
> +            virSkipSpaces(&cur);
> +
> +            if (virStrToLong_i(cur, &tmp, 10, &last) < 0)
> +                goto cleanup;
> +
> +            if (last < start)
> +                goto cleanup;
> +            cur = tmp;
> +
> +            for (i = start; i <= last; i++) {
> +                if (!firstnum)
> +
> +                    virBufferAddChar(&buf, ',');
> +                virBufferAsprintf(&buf, "%ld", i);
> +                firstnum = 0;
> +            }
> +
> +            virSkipSpaces(&cur);
> +        }
> +
> +        if (*cur == ',') {
> +            cur++;
> +            virSkipSpaces(&cur);
> +        } else if (*cur == 0) {
> +            break;
> +        } else {
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = virBufferContentAndReset(&buf);
> + cleanup:
> +    virBufferFreeAndReset(&buf);
> +    return ret;
> +}
> +
> +
> +static int
> +qemuDomainGetStatsCpuResource(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +                              virDomainObjPtr dom,
> +                              virDomainStatsRecordPtr record,
> +                              int *maxparams,
> +                              unsigned int privflags ATTRIBUTE_UNUSED,
> +                              virResctrlMonitorType restag)

Why bother passing ATTRIBUTE_UNUSED ?

> +{
> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
> +    virDomainResctrlMonDefPtr domresmon = NULL;
> +    virResctrlMonitorStatsPtr stats = NULL;
> +    size_t nstats = 0;

since these are local to the:

+        for (j = 0; j < resctrl->nmonitors; j++) {

loop and there's local's in that for loop, maybe these two should move
there too...

> +    virDomainResctrlDefPtr resctrl = NULL;
> +    unsigned int nmonitors = 0;
> +    unsigned int imonitor = 0;
> +    const char *restype = NULL;
> +    char *rawvcpus = NULL;
> +    char *vcpus = NULL;
> +    size_t i = 0;
> +    size_t j = 0;
> +    int ret = -1;
> +
> +    if (!virDomainObjIsActive(dom))
> +        return 0;
> +
> +    if (restag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
> +        restype = "cache";
> +    } else {
> +        VIR_DEBUG("Invalid CPU resource type");

VIR_DEBUG? virReportError would seem to be more appropriate. As I've
stated before there will be an error for some reason generated.

> +        return -1;
> +    }
> +
> +    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 (virResctrlMonitorIsRunning(domresmon->instance) &&
> +                domresmon->tag == restag)

Knowing how heavy wait *IsRunning is, the order of checking should be
reversed at the very least.

> +                nmonitors++;
> +        }
> +    }
> +
> +    if (nmonitors) {
> +        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                 "cpu.%s.monitor.count", restype);
> +        if (virTypedParamsAddUInt(&record->params,
> +                                  &record->nparams,
> +                                  maxparams,
> +                                  param_name,
> +                                  nmonitors) < 0)
> +            goto cleanup;
> +    }

All that above to ensure nmonitors == resctrl->nmonitors??? I think
there's a lot of unnecessary stuff.
> +
> +    for (i = 0; i < dom->def->nresctrls; i++) {
> +        resctrl = dom->def->resctrls[i];
> +
> +        for (j = 0; j < resctrl->nmonitors; j++) {
> +            size_t l = 0;
> +            virResctrlMonitorPtr monitor = resctrl->monitors[j]->instance;
> +            const char *id = virResctrlMonitorGetID(monitor);
> +
> +            if (!id)
> +                goto cleanup;

I would think you have other problems in this case. Besides there's no
error message associated with this.

> +
> +            domresmon = resctrl->monitors[j];
> +
> +            if (!virResctrlMonitorIsRunning(domresmon->instance))
> +                continue;

Oh my one call for each iteration, ouch - highly inefficient.

> +
> +            if (!(rawvcpus = virBitmapFormat(domresmon->vcpus)))
> +                goto cleanup;
> +
> +            vcpus = qemuDomainVcpuFormatHelper(rawvcpus);
> +            if (!vcpus)
> +                goto cleanup;
> +
> +            if (virResctrlMonitorGetCacheOccupancy(monitor, &stats,
> +                                                   &nstats) < 0)
> +                goto cleanup;
> +
> +            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                     "cpu.%s.monitor.%d.name", restype, imonitor);
> +            if (virTypedParamsAddString(&record->params,
> +                                        &record->nparams,
> +                                        maxparams,
> +                                        param_name,
> +                                        id) < 0)
> +                goto cleanup;
> +
> +            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                     "cpu.%s.monitor.%d.vcpus", restype, imonitor);
> +
> +            if (virTypedParamsAddString(&record->params,
> +                                        &record->nparams,
> +                                        maxparams,
> +                                        param_name,
> +                                        vcpus) < 0)
> +                goto cleanup;
> +
> +            snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                     "cpu.%s.monitor.%d.bank.count", restype, imonitor);
> +            if (virTypedParamsAddUInt(&record->params,
> +                                      &record->nparams,
> +                                      maxparams,
> +                                      param_name,
> +                                      nstats) < 0)
> +                goto cleanup;
> +
> +            for (l = 0; l < nstats; l++) {
> +                snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                         "cpu.%s.monitor.%d.bank.%ld.id",
> +                         restype, imonitor, l);
> +                if (virTypedParamsAddUInt(&record->params,
> +                                          &record->nparams,
> +                                          maxparams,
> +                                          param_name,
> +                                          stats[l].id) < 0)
> +                    goto cleanup;
> +
> +
> +                snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> +                         "cpu.%s.monitor.%d.bank.%ld.bytes",
> +                         restype, imonitor, l);
> +                if (virTypedParamsAddUInt(&record->params,
> +                                          &record->nparams,
> +                                          maxparams,
> +                                          param_name,
> +                                          stats[l].val) < 0)
> +                    goto cleanup;
> +            }
> +
> +            VIR_FREE(stats);

               nstats = 0;

too!

> +            VIR_FREE(vcpus);
> +            imonitor++;
> +        }
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(vcpus);
> +    return ret;
> +}
> +
> +
>  static int
>  qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                        virDomainObjPtr dom,
> @@ -19735,6 +19960,10 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>              return -1;
>      }
>  
> +    if (qemuDomainGetStatsCpuResource(driver, dom, record, maxparams, privflags,
> +                                      VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0)
> +        return -1;
> +
>      return 0;
>  }
>  
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index fa3e6e9..02e7095 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2713,3 +2713,133 @@ virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor)
>  
>      return ret;
>  }
> +
> +
> +static int
> +virResctrlMonitorStatsSorter(const void *a,
> +                             const void *b)
> +{
> +    return ((virResctrlMonitorStatsPtr)a)->id
> +        - ((virResctrlMonitorStatsPtr)b)->id;
> +}
> +
> +
> +/*
> + * virResctrlMonitorGetStats
> + *
> + * @monitor: The monitor that the statistic data will be retrieved from.
> + * @resource: The name for resource name. 'llc_occupancy' for cache resource.
> + * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth resource.
> + * @stats: Array of virResctrlMonitorStatsPtr for holding cache or memory
> + * bandwidth usage data.
> + * @nstats: A size_t pointer to hold the returned array length of @stats
> + *
> + * Get cache or memory bandwidth utilization information.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +static int
> +virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
> +                          const char *resource,
> +                          virResctrlMonitorStatsPtr *stats,
> +                          size_t *nstats)
> +{
> +    int rv = -1;
> +    int ret = -1;
> +    DIR *dirp = NULL;
> +    char *datapath = NULL;
> +    struct dirent *ent = NULL;
> +    virResctrlMonitorStatsPtr stat = NULL;
> +
> +    if (!monitor) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Invalid resctrl monitor"));
> +        return -1;
> +    }
> +
> +    if (virAsprintf(&datapath, "%s/mon_data", monitor->path) < 0)
> +        return -1;
> +
> +    if (virDirOpen(&dirp, datapath) < 0)
> +        goto cleanup;
> +
> +    *nstats = 0;
> +    while (virDirRead(dirp, &ent, datapath) > 0) {
> +        char *node_id = NULL;
> +
> +        if (VIR_ALLOC(stat) < 0)
> +            goto cleanup;
> +
> +        /* Looking for directory that contains resource utilization
> +         * information file. The directory name is arranged in format
> +         * "mon_<node_name>_<node_id>". For example, "mon_L3_00" and
> +         * "mon_L3_01" are two target directories for a two nodes system
> +         * with resource utilization data file for each node respectively.
> +         */
> +        if (ent->d_type != DT_DIR)
> +            continue;
> +
> +        /* Looking for directory has a prefix 'mon_L' */
> +        if (!(node_id = STRSKIP(ent->d_name, "mon_L")))
> +            continue;
> +
> +        /* Looking for directory has another '_' */
> +        node_id = strchr(node_id, '_');
> +        if (!node_id)
> +            continue;
> +
> +        /* Skip the character '_' */
> +        if (!(node_id = STRSKIP(node_id, "_")))
> +            continue;
> +
> +        /* The node ID number should be here, parsing it. */
> +        if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0)
> +            goto cleanup;
> +
> +        rv = virFileReadValueUint(&stat->val, "%s/%s/%s", datapath,
> +                                  ent->d_name, resource);
> +        if (rv == -2) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("File '%s/%s/%s' does not exist."),
> +                           datapath, ent->d_name, resource);
> +        }
> +        if (rv < 0)
> +            goto cleanup;
> +
> +        if (VIR_APPEND_ELEMENT(*stats, *nstats, *stat) < 0)
> +            goto cleanup;
> +    }
> +
> +    /* Sort in id's ascending order */
> +    qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter);

Coverity notes that *stats could be NULL - if the above while loop gets
nothing...

> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(datapath);
> +    VIR_FREE(stat);
> +    VIR_DIR_CLOSE(dirp);
> +    return ret;
> +}
> +
> +
> +/*
> + * virResctrlMonitorGetCacheOccupancy
> + *
> + * @monitor: The monitor that the statistic data will be retrieved from.
> + * @caches: Array of virResctrlMonitorStatsPtr for receiving cache occupancy
> + * data. Caller is responsible to free this array.
> + * @ncaches: A size_t pointer to hold the returned array length of @caches

They're @stats and @nstats elsewhere, be consistent.

> + *
> + * Get cache or memory bandwidth utilization information.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +
> +int
> +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
> +                                   virResctrlMonitorStatsPtr *caches,
> +                                   size_t *ncaches)

They're @stats and @nstats elsewhere, be consistent.

> +{
> +    return virResctrlMonitorGetStats(monitor, "llc_occupancy",
> +                                     caches, ncaches);
> +}
> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index 8d8fdc2..004c40e 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -191,6 +191,13 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
>  typedef struct _virResctrlMonitor virResctrlMonitor;
>  typedef virResctrlMonitor *virResctrlMonitorPtr;
>  
> +typedef struct _virResctrlMonitorStats virResctrlMonitorStats;
> +typedef virResctrlMonitorStats *virResctrlMonitorStatsPtr;
> +struct _virResctrlMonitorStats {
> +    unsigned int id;
> +    unsigned int val;
> +};
> +
>  virResctrlMonitorPtr
>  virResctrlMonitorNew(void);
>  
> @@ -222,4 +229,9 @@ virResctrlMonitorRemove(virResctrlMonitorPtr monitor);
>  
>  bool
>  virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor);
> +
> +int
> +virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
> +                                   virResctrlMonitorStatsPtr *caches,
> +                                   size_t *ncaches);

They're @stats and @nstats elsewhere, be consistent.

>  #endif /*  __VIR_RESCTRL_H__ */
> 

I'm sure there's more I'll pick up later, but you should have plenty for
now. As to splitting - I think some splitting could be done, I'll leave
it up to you... Lots of change across multiple areas - ask yourself -
can something be separated out?

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list