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

On 5/23/19 11:34 AM, Wang Huaqiang wrote: > Export virResctrlMonitorGetStats and make > virResctrlMonitorGetCacheOccupancy obsoleted. > > Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; > --- >   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.

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

Michal