Re: [PATCH 7/9] util: Refactor 'virResctrlMonitorStats'

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

On 2019年05月27日 23:26, Michal Privoznik wrote:
> On 5/23/19 11:34 AM, Wang Huaqiang wrote:
> > Refactor 'virResctrlMonitorStats' to track multiple statistical
> > records.
> >
> > Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> > ---
> >   src/qemu/qemu_driver.c |  2 +-
> >   src/util/virresctrl.c  | 17 ++++++++++++++---
> >   src/util/virresctrl.h  | 12 ++++++++++--
> >   3 files changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 2abed86..4ea346c 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -20120,7 +20120,7 @@ qemuDomainGetStatsCpuCache(virDomainObjPtr dom,
> >                        "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j);
> >               if (virTypedParamsAddUInt(&record->params, 
> > &record->nparams,
> >                                         maxparams, param_name,
> > - resdata[i]->stats[j]->val) < 0)
> > + resdata[i]->stats[j]->vals[0]) < 0)
>
> So why undergo the whole torture of 8/9 and 9/9 if we will report one 
> value only?


The changes of patch7 and 8 give the code the capability for pass more 
than one @vals
from util/resctrl to qemu. This will be used in later MBM patches, at 
that time, @vals[0]
and @vals[1] will be used for passing the 'local memory bandwidth' and 
'total memory
bandwidth'.

If change not make, then we have to add some other function
like 'qemuDomainGetStatsCpuCache' but for memory bandwidth make the call 
twice,
it is very inefficient.

> Also, I'm not sure @vals is going to be allocated at all times, will it?

Yes. @vals should never be NULL for code in qemu_driver.c, and it is 
allocated by
util/resctrl.

> >                   goto cleanup;
> >           }
> >       }
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > index 0f18d2b..c2fe2ed 100644
> > --- a/src/util/virresctrl.c
> > +++ b/src/util/virresctrl.c
> > @@ -2686,6 +2686,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr 
> > monitor,
> >   {
> >       int rv = -1;
> >       int ret = -1;
> > +    unsigned int val = 0;
> >       DIR *dirp = NULL;
> >       char *datapath = NULL;
> >       char *filepath = NULL;
> > @@ -2742,7 +2743,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr 
> > monitor,
> >           if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0)
> >               goto cleanup;
> >   -        rv = virFileReadValueUint(&stat->val, "%s/%s/%s", datapath,
> > +        rv = virFileReadValueUint(&val, "%s/%s/%s", datapath,
> >                                     ent->d_name, resource);
> >           if (rv == -2) {
> >               virReportError(VIR_ERR_INTERNAL_ERROR,
> > @@ -2752,6 +2753,12 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr 
> > monitor,
> >           if (rv < 0)
> >               goto cleanup;
> >   +        if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0)
> > +            goto cleanup;
> > +
> > +        if (virStringListAdd(&stat->features, resource) < 0)
> > +            goto cleanup;
> > +
> >           if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0)
> >               goto cleanup;
> >       }
> > @@ -2779,8 +2786,12 @@ 
> > virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats,
> >       if (!stats)
> >           return;
> >   -    for (i = 0; i < nstats; i++)
> > -        VIR_FREE(stats[i]);
> > +    for (i = 0; i < nstats; i++) {
> > +        if (stats[i]) {
> > +            VIR_FREE(stats[i]->vals);
> > +            virStringListFree(stats[i]->features);
> > +        }
> > +    }
> >   }
> >     diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> > index abdeb59..97205bc 100644
> > --- a/src/util/virresctrl.h
> > +++ b/src/util/virresctrl.h
> > @@ -194,8 +194,16 @@ typedef virResctrlMonitor *virResctrlMonitorPtr;
> >   typedef struct _virResctrlMonitorStats virResctrlMonitorStats;
> >   typedef virResctrlMonitorStats *virResctrlMonitorStatsPtr;
> >   struct _virResctrlMonitorStats {
> > -    unsigned int id;
> > -    unsigned int val;
> > +    /* The system assigned cache ID associated with statistical 
> > record */
> > +     unsigned int id;
> > +    /* @features is a NULL terminal string list tracking the 
> > statistical record
> > +     * name.*/
> > +    char **features;
> > +    /* @vals store the statistical record values and @val[0] is the 
> > value for
> > +     * @features[0], @val[1] for@features[1] ... respectively */
> > +    unsigned int *vals;
> > +    /* The length of @vals array */
> > +    size_t nvals;
>
> We like the following style more:
>
> struct X {
>   int memberA;  /* some description of this member
>                    split into two lines */
>   bool memberB; /* one line description */
> };
>
> But on the other hand, this is consistent with the rest of resctrl 
> code. Your call which style to use.

Yes. That's my think for why using this kind of coding style. If you 
permit, I'd like to using
the current comment coding style, it looks consistent with virresctrl.c 
and virresctrl.h files.

> Michal

Thanks for review.
Huaqiang


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