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

["Huaqiang,Wang" <huaqiang.wang@xxxxxxxxx>]

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