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@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 memorybandwidth'.
Yes, I kind of expected that. But the explanation was missing.
If change not make, then we have to add some other functionlike '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 byutil/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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
