Re: [PATCH 8/9] util: Extend virresctl API to retrieve multiple monitor statistics

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

 





On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Export virResctrlMonitorGetStats and make
virResctrlMonitorGetCacheOccupancy obsoleted.

Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
---
  src/libvirt_private.syms |  1 +
  src/qemu/qemu_driver.c   | 33 +++++++++++++++++++++++++--------
  src/util/virresctrl.c    | 46 +++++++++++++++++++++++++++-------------------
  src/util/virresctrl.h    |  6 ++++++
  4 files changed, 59 insertions(+), 27 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9099757..2e3d48c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2771,6 +2771,7 @@ virResctrlMonitorDeterminePath;
  virResctrlMonitorFreeStats;
  virResctrlMonitorGetCacheOccupancy;
  virResctrlMonitorGetID;
+virResctrlMonitorGetStats;
  virResctrlMonitorNew;
  virResctrlMonitorRemove;
  virResctrlMonitorSetAlloc;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ea346c..bc19171 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19987,6 +19987,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
    /**
   * qemuDomainGetResctrlMonData:
+ * @driver: Pointer to qemu driver
   * @dom: Pointer for the domain that the resctrl monitors reside in
   * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for receiving the    *            virQEMUResctrlMonDataPtr array. Caller is responsible for @@ -20005,16 +20006,29 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
   * Returns -1 on failure, or 0 on success.
   */
  static int
-qemuDomainGetResctrlMonData(virDomainObjPtr dom,
+qemuDomainGetResctrlMonData(virQEMUDriverPtr driver,
+                            virDomainObjPtr dom,
                              virQEMUResctrlMonDataPtr **resdata,
                              size_t *nresdata,
                              virResctrlMonitorType tag)
  {
      virDomainResctrlDefPtr resctrl = NULL;
      virQEMUResctrlMonDataPtr res = NULL;
+    char **features = NULL;
      size_t i = 0;
      size_t j = 0;
  +    if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
+        features = driver->caps->host.cache.monitor->features;


No, we have virQEMUDriverGetCapabilities() which ensures that driver->caps object doesn't go away while accessing this.


I can't refer 'driver->caps->host.cache.monitor->features' directly because this function (qemuDomainGetResctrlMonData)
will be used for 'memory bandwidth' monitors also.
at that time that the piece of code looks like:
  +    if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
   +      if (driver->caps->host.cache.monitor)
+            features = driver->caps->host.cache.monitor->features;
  +    if (tag == VIR_RESCTRL_MONITOR_TYPE_MEMBW) {
   +      if(driver->caps->host.membw)
+            features = driver->caps->host.membw.monitor->features;

Hope I understand your question correctly.

+    } else {
+        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                       _("Unsupported resctrl monitor type"));
+        return -1;
+    }
+
+    if (virStringListLength((const char * const *)features) == 0)
+        return 0;
+
      for (i = 0; i < dom->def->nresctrls; i++) {
          resctrl = dom->def->resctrls[i];
  @@ -20041,9 +20055,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,               if (VIR_STRDUP(res->name, virResctrlMonitorGetID(monitor)) < 0)
                  goto error;
  -            if (virResctrlMonitorGetCacheOccupancy(monitor,
- &res->stats,
- &res->nstats) < 0)
+            if (virResctrlMonitorGetStats(monitor, (const char **)features, +                                          &res->stats, &res->nstats) < 0)
                  goto error;
                if (VIR_APPEND_ELEMENT(*resdata, *nresdata, res) < 0)
@@ -20060,7 +20073,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
      static int
-qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
+qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
+                           virDomainObjPtr dom,
                             virDomainStatsRecordPtr record,
                             int *maxparams)
  {
@@ -20074,10 +20088,13 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
      if (!virDomainObjIsActive(dom))
          return 0;
  -    if (qemuDomainGetResctrlMonData(dom, &resdata, &nresdata,
+    if (qemuDomainGetResctrlMonData(driver, dom, &resdata, &nresdata,
VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0)
          goto cleanup;
  +    if (nresdata == 0)
+        return 0;
+
      snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
               "cpu.cache.monitor.count");
      if (virTypedParamsAddUInt(&record->params, &record->nparams,
@@ -20175,7 +20192,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
      static int
-qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+qemuDomainGetStatsCpu(virQEMUDriverPtr driver,
                        virDomainObjPtr dom,
                        virDomainStatsRecordPtr record,
                        int *maxparams,
@@ -20184,7 +20201,7 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
      if (qemuDomainGetStatsCpuCgroup(dom, record, maxparams) < 0)
          return -1;
  -    if (qemuDomainGetStatsCpuCache(dom, record, maxparams) < 0)
+    if (qemuDomainGetStatsCpuCache(driver, dom, record, maxparams) < 0)
          return -1;
        return 0;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index c2fe2ed..76a8d02 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2668,8 +2668,7 @@ virResctrlMonitorStatsSorter(const void *a,
   * 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.
+ * @resources: A string list for the monitor feature names.
   * @stats: Pointer of of virResctrlMonitorStatsPtr array for holding cache or
   * memory bandwidth usage data.
   * @nstats: A size_t pointer to hold the returned array length of @stats
@@ -2678,14 +2677,15 @@ virResctrlMonitorStatsSorter(const void *a,
   *
   * Returns 0 on success, -1 on error.
   */
-static int
+int
  virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
-                          const char *resource,
+                          const char **resources,
                            virResctrlMonitorStatsPtr **stats,
                            size_t *nstats)
  {
      int rv = -1;
      int ret = -1;
+    size_t i = 0;
      unsigned int val = 0;
      DIR *dirp = NULL;
      char *datapath = NULL;
@@ -2743,21 +2743,23 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
          if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0)
              goto cleanup;
  -        rv = virFileReadValueUint(&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;
+        for (i = 0; resources[i]; i++) {
+            rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
+                                      ent->d_name, resources[i]);
+            if (rv == -2) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("File '%s/%s/%s' does not exist."),
+                               datapath, ent->d_name, resources[i]);
+            }
+            if (rv < 0)
+                goto cleanup;
  -        if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
-            goto cleanup;
+            if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
+                goto cleanup;
  -        if (virStringListAdd(&stat->features, resource) < 0)
-            goto cleanup;
+            if (virStringListAdd(&stat->features, resources[i]) < 0)
+                goto cleanup;
+        }
            if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0)
              goto cleanup;
@@ -2813,6 +2815,12 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
                                     virResctrlMonitorStatsPtr **stats,
                                     size_t *nstats)
  {
-    return virResctrlMonitorGetStats(monitor, "llc_occupancy",
-                                     stats, nstats);
+    char **features = NULL;
+    int ret = -1;
+
+    virStringListAdd(&features, "llc_occupancy");
+    ret = virResctrlMonitorGetStats(monitor, (const char**)features, stats, nstats);
+    virStringListFree(features);

No need to dynamically create the string list.

const char *features = {"llc_occupancy", NULL};

is just fine. Also, this way you don't need to check if virStringListAdd() succeeded ;-) I know you're removing this function in next commit, but it still makes sense todo things right.


Agree. will be refined.

Michal

Thanks for review.
Huaqiang

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux