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

[Michal Privoznik]

On 5/28/19 10:32 AM, Huaqiang,Wang wrote:
> 
> 
> 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 at intel.com>
>>> ---
>>>   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'.
> 

Yes, I kind of expected that. But the explanation was missing.

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

I'm not opposed this change, it's just that there was no justification 
for this change.

> 
>>
>> 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 at 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.

Fine by me.

Michal