Refactor 'virResctrlMonitorStats' to track multiple statistical
records.
Signed-off-by: Wang Huaqiang <huaqiang.wang@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)
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;
};
virResctrlMonitorPtr
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
> Refactor 'virResctrlMonitorStats' to track multiple statistical
> records.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@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?
Also, I'm not sure @vals is going to be allocated at all times, will it?
> 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.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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'.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.