[libvirt] [PATCH 0/9] util/resctrl cleanups and refactors
Plan to support Resctrl Control Monitors, which is a feature introduced by kernel 'resctrl' sub-model. Submit some cleanup and refactoring patches for upcoming memory bandwidth resource monitoring (MBM) monitors. Related MBM RFC is https://www.redhat.com/archives/libvir-list/2019-April/msg01409.html. This RFC is not actively discussed since libvirt already implemented similar resctrl cache monitoring (CMT), and lots details have been discussed and implemented during the work of CMT. The cleanups and refactoring includes: 1. Removing some reluctant lines and white spaces that is existing in current code and not meet the libvirt coding style. 2. Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file. 3. Private API changes, removed 'virResctrlMonitorGetCacheOccupancy' and exported a new API named 'virResctrlMonitorGetStats' with similar functionality, but with capability to be used for retrieving MBM statistical information. 4. Refactoring 'virResctrlMonitorFreeStats' for more reusing in code. 5. Extend data structure 'virResctrlMonitorStats' with the capability to carry multiple statistical information from monitor. Wang Huaqiang (9): conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: check if vcpus matches with any 'existing' allocaiton conf: Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file conf: refactor 'virDomainResctrlVcpuMatch' util: Refactor 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy src/conf/domain_conf.c | 89 +++++++++++++++++++++++------------------------- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++----- src/util/virresctrl.c | 68 +++++++++++++++--------------------- src/util/virresctrl.h | 19 ++++++++--- 5 files changed, 113 insertions(+), 101 deletions(-) -- 2.7.4
Remove some redundant space and line. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3a5141..e3c8aa0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19519,7 +19519,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, VIR_AUTOPTR(virBitmap) vcpus = NULL; VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; VIR_AUTOUNREF(virResctrlAllocPtr) alloc = NULL; - ssize_t i = 0; int n; int ret = -1; @@ -27395,7 +27394,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf, &childrenBuf) < 0) goto cleanup; - for (i = 0; i < resctrl->nmonitors; i ++) { + for (i = 0; i < resctrl->nmonitors; i++) { if (virDomainResctrlMonDefFormatHelper(resctrl->monitors[i], VIR_RESCTRL_MONITOR_TYPE_CACHE, &childrenBuf) < 0) -- 2.7.4
code cleanup for 'virDomainCachetuneDefParse' and 'virDomainMemorytuneDefParse'. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 46 +++++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e3c8aa0..db25c1f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19321,40 +19321,38 @@ virDomainCachetuneDefParse(virDomainDefPtr def, ctxt->node = node; if (virDomainResctrlParseVcpus(def, node, &vcpus) < 0) - goto cleanup; + return -1; - if (virBitmapIsAllClear(vcpus)) { - ret = 0; - goto cleanup; - } + if (virBitmapIsAllClear(vcpus)) + return 0; if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot extract cache nodes under cachetune")); - goto cleanup; + return -1; } if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) - goto cleanup; + return -1; if (!alloc) { alloc = virResctrlAllocNew(); if (!alloc) - goto cleanup; + return -1; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("Identical vcpus in cachetunes found")); - goto cleanup; + return -1; } for (i = 0; i < n; i++) { if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) - goto cleanup; + return -1; } resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); if (!resctrl) - goto cleanup; + return -1; if (virDomainResctrlMonDefParse(def, ctxt, node, VIR_RESCTRL_MONITOR_TYPE_CACHE, @@ -19527,37 +19525,35 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, ctxt->node = node; if (virDomainResctrlParseVcpus(def, node, &vcpus) < 0) - goto cleanup; + return -1; - if (virBitmapIsAllClear(vcpus)) { - ret = 0; - goto cleanup; - } + if (virBitmapIsAllClear(vcpus)) + return 0; if ((n = virXPathNodeSet("./node", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot extract memory nodes under memorytune")); - goto cleanup; + return -1; } if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) - goto cleanup; + return -1; if (!alloc) { alloc = virResctrlAllocNew(); if (!alloc) - goto cleanup; + return -1; new_alloc = true; } for (i = 0; i < n; i++) { if (virDomainMemorytuneDefParseMemory(ctxt, nodes[i], alloc) < 0) - goto cleanup; - } - if (virResctrlAllocIsEmpty(alloc)) { - ret = 0; - goto cleanup; + return -1; } + + if (virResctrlAllocIsEmpty(alloc)) + return 0; + /* * If this is a new allocation, format ID and append to resctrl, otherwise * just update the existing alloc information, which is done in above @@ -19565,7 +19561,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, if (new_alloc) { resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); if (!resctrl) - goto cleanup; + return -1; if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) goto cleanup; -- 2.7.4
In checking if vcpus matches with any exisitng resctrl allocation it should consider if the allocation exists or not. '@resctrl->alloc != NULL' means an allocation exists. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index db25c1f..9c95467 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19088,10 +19088,9 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, /* Checking if the monitor's vcpus is conflicted with existing allocation * and monitors. * - * Returns 1 if @vcpus equals to @resctrl->vcpus, then the monitor will - * share the underlying resctrl group with @resctrl->alloc. Returns - 1 - * if any conflict found. Returns 0 if no conflict and @vcpus is not equal - * to @resctrl->vcpus. + * Returns 1 if @vcpus equals to vcpus of an existing resctrl allocation group. + * Returns -1 if any conflict found. Returns 0 if no conflict and @vcpus is + * not equal to any created resctrl allocation. */ static int virDomainResctrlMonValidateVcpus(virDomainResctrlDefPtr resctrl, @@ -19115,7 +19114,7 @@ virDomainResctrlMonValidateVcpus(virDomainResctrlDefPtr resctrl, } } - if (virBitmapEqual(vcpus, resctrl->vcpus)) + if (virBitmapEqual(vcpus, resctrl->vcpus) && resctrl->alloc != NULL) return 1; for (i = 0; i < resctrl->nmonitors; i++) { -- 2.7.4
@n denotes the number of <cache> element under <cputune/cachetune> element in function 'virDomainCachetuneDefParse' or the number of <node> element under <cputune/memorytune> element in function virDomainMemorytuneDefParse'. Originally it is using 'virResctrlAllocIsEmpty' function to judge if no resctrl allocation defined in <cputune/cachetune> or <cputune/memorytune> element, this role could be replaced with checking if @n is zero or not. This replacement is more efficient and avoiding a long function calling path. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9c95467..dcfd2dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19360,7 +19360,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, /* If no <cache> element or <monitor> element in <cachetune>, do not * append any resctrl element */ - if (!resctrl->nmonitors && virResctrlAllocIsEmpty(alloc)) { + if (!resctrl->nmonitors && n == 0) { ret = 0; goto cleanup; } @@ -19550,7 +19550,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; } - if (virResctrlAllocIsEmpty(alloc)) + if (n == 0) return 0; /* -- 2.7.4
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
@n denotes the number of <cache> element under <cputune/cachetune> element in function 'virDomainCachetuneDefParse' or the number of <node> element under <cputune/memorytune> element in function virDomainMemorytuneDefParse'. Originally it is using 'virResctrlAllocIsEmpty' function to judge if no resctrl allocation defined in <cputune/cachetune> or <cputune/memorytune> element, this role could be replaced with checking if @n is zero or not.
This replacement is more efficient and avoiding a long function calling path.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9c95467..dcfd2dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19360,7 +19360,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
/* If no <cache> element or <monitor> element in <cachetune>, do not * append any resctrl element */ - if (!resctrl->nmonitors && virResctrlAllocIsEmpty(alloc)) { + if (!resctrl->nmonitors && n == 0) { ret = 0; goto cleanup; } @@ -19550,7 +19550,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; }
- if (virResctrlAllocIsEmpty(alloc)) + if (n == 0) return 0;
/*
Is this a good idea? virResctrlAllocIsEmpty does more than plain n == 0. Michal
On 2019年05月27日 23:27, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
@n denotes the number of <cache> element under <cputune/cachetune> element in function 'virDomainCachetuneDefParse' or the number of <node> element under <cputune/memorytune> element in function virDomainMemorytuneDefParse'. Originally it is using 'virResctrlAllocIsEmpty' function to judge if no resctrl allocation defined in <cputune/cachetune> or <cputune/memorytune> element, this role could be replaced with checking if @n is zero or not.
This replacement is more efficient and avoiding a long function calling path.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9c95467..dcfd2dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19360,7 +19360,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, /* If no <cache> element or <monitor> element in <cachetune>, do not * append any resctrl element */ - if (!resctrl->nmonitors && virResctrlAllocIsEmpty(alloc)) { + if (!resctrl->nmonitors && n == 0) { ret = 0; goto cleanup; } @@ -19550,7 +19550,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; } - if (virResctrlAllocIsEmpty(alloc)) + if (n == 0) return 0; /*
Is this a good idea? virResctrlAllocIsEmpty does more than plain n == 0.
Yes, I have omitted some details, actually using virResctrlAllocIsEmpty will cause some logic error in original code in processing memory bandwidth groups which is not fully test When detecting the configurations from XML file, the cache allocation and monitor settings are first parsed and then the memory bandwidth allocation settings. The desired logic when paring resource control group settings are: ** For cache resctrl groups ** Cache resctrl groups, both allocation group and monitor groups, are firstly parsed, if any cache allocation group (the <cputune>/<cachetune>/<cache> element) or monitor group (the <cputune>/<cachetune>/<monitor> element) is parsed, it requires to create and register a @def->resctrls element. So, in this case, @n represents the number of <cputune>/<cachetune>/<cache> element, and @resctrl->nmonitors denotes the number of <cputune>/<cachetune>/<monitor> element. If @n==0 and @resctrl->nmonitors==0, then it tells no cache allocation and cache monitor groups are defined in XML file, it is not necessary to create any @def->resctrls objects. We need to ensure that one or more allocation groups will be created in underlying util/resctrl module if value of @n is not zero, this could be guaranteed by current XML schemas rules. If we do not consider the performance, calling virResctrlAllocIsEmpty here is logically correct because each cache allocation group will register an 'allocation' object in util/resctrl module. If codes runs without error, then @n represents the allocation objects in util/resctrl module. So, for cache resctrl groups, there is no need to call virResctrlAllocIsEmpty. ** For memory bandwidth resctrl groups ** It hopes to append a @resctrl element to @def->resctrls array if some valid <cputune>/<memorytune>/<bank> elements have been found, and the @n in this part of code represents the number of elements. It is no need to call virResctrlAllocIsEmpty if we make sure each <cputune>/<memorytune>/<bank> item in XML file is valid and will create some allocation group. Another thing I haven't mentioned that using virResctrlAllocIsEmpty here will make error. If some cache allocations are created, the function will return 'NOT empty' regardless the number of memory bandwidth allocations ordered in the XML file. This is logically wrong.
Michal
Thanks for comments. Huaqiang
Let 'virDomainResctrlVcpuMatch' return a pointer of @virDomainResctrlDefPtr in its third parameter. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dcfd2dd..b0f5d80 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18992,7 +18992,7 @@ virDomainResctrlParseVcpus(virDomainDefPtr def, static int virDomainResctrlVcpuMatch(virDomainDefPtr def, virBitmapPtr vcpus, - virResctrlAllocPtr *alloc) + virDomainResctrlDefPtr *resctrl) { ssize_t i = 0; @@ -19001,7 +19001,7 @@ virDomainResctrlVcpuMatch(virDomainDefPtr def, * Just updating memory allocation information of that group */ if (virBitmapEqual(def->resctrls[i]->vcpus, vcpus)) { - *alloc = virObjectRef(def->resctrls[i]->alloc); + *resctrl = def->resctrls[i]; break; } if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) { @@ -19331,19 +19331,19 @@ virDomainCachetuneDefParse(virDomainDefPtr def, return -1; } - if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) + if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; - if (!alloc) { - alloc = virResctrlAllocNew(); - if (!alloc) - return -1; - } else { + if (resctrl) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Identical vcpus in cachetunes found")); return -1; } + alloc = virResctrlAllocNew(); + if (!alloc) + return -1; + for (i = 0; i < n; i++) { if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) return -1; @@ -19519,7 +19519,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, ssize_t i = 0; int n; int ret = -1; - bool new_alloc = false; ctxt->node = node; @@ -19535,14 +19534,18 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; } - if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) + if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; - if (!alloc) { + if (resctrl && resctrl->alloc) { + alloc = virObjectRef(resctrl->alloc); + } else { alloc = virResctrlAllocNew(); - if (!alloc) - return -1; - new_alloc = true; + if (resctrl) { + alloc = virResctrlAllocNew(); + if (!alloc) + return -1; + } } for (i = 0; i < n; i++) { @@ -19557,7 +19560,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, * If this is a new allocation, format ID and append to resctrl, otherwise * just update the existing alloc information, which is done in above * virDomainMemorytuneDefParseMemory */ - if (new_alloc) { + if (!resctrl) { resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); if (!resctrl) return -1; -- 2.7.4
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Let 'virDomainResctrlVcpuMatch' return a pointer of @virDomainResctrlDefPtr in its third parameter.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dcfd2dd..b0f5d80 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18992,7 +18992,7 @@ virDomainResctrlParseVcpus(virDomainDefPtr def, static int virDomainResctrlVcpuMatch(virDomainDefPtr def, virBitmapPtr vcpus, - virResctrlAllocPtr *alloc) + virDomainResctrlDefPtr *resctrl) { ssize_t i = 0;
@@ -19001,7 +19001,7 @@ virDomainResctrlVcpuMatch(virDomainDefPtr def, * Just updating memory allocation information of that group */ if (virBitmapEqual(def->resctrls[i]->vcpus, vcpus)) { - *alloc = virObjectRef(def->resctrls[i]->alloc); + *resctrl = def->resctrls[i]; break; } if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) { @@ -19331,19 +19331,19 @@ virDomainCachetuneDefParse(virDomainDefPtr def, return -1; }
- if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) + if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1;
- if (!alloc) { - alloc = virResctrlAllocNew(); - if (!alloc) - return -1; - } else { + if (resctrl) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Identical vcpus in cachetunes found")); return -1; }
+ alloc = virResctrlAllocNew(); + if (!alloc) + return -1;
Or simply: if (!(alloc = virResctrlAllocNew())) return -1; Here and in the rest of the patches.
+ for (i = 0; i < n; i++) { if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) return -1;
Michal
On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Let 'virDomainResctrlVcpuMatch' return a pointer of @virDomainResctrlDefPtr in its third parameter.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dcfd2dd..b0f5d80 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18992,7 +18992,7 @@ virDomainResctrlParseVcpus(virDomainDefPtr def, static int virDomainResctrlVcpuMatch(virDomainDefPtr def, virBitmapPtr vcpus, - virResctrlAllocPtr *alloc) + virDomainResctrlDefPtr *resctrl) { ssize_t i = 0; @@ -19001,7 +19001,7 @@ virDomainResctrlVcpuMatch(virDomainDefPtr def, * Just updating memory allocation information of that group */ if (virBitmapEqual(def->resctrls[i]->vcpus, vcpus)) { - *alloc = virObjectRef(def->resctrls[i]->alloc); + *resctrl = def->resctrls[i]; break; } if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) { @@ -19331,19 +19331,19 @@ virDomainCachetuneDefParse(virDomainDefPtr def, return -1; } - if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) + if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; - if (!alloc) { - alloc = virResctrlAllocNew(); - if (!alloc) - return -1; - } else { + if (resctrl) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Identical vcpus in cachetunes found")); return -1; } + alloc = virResctrlAllocNew(); + if (!alloc) + return -1;
Or simply:
if (!(alloc = virResctrlAllocNew())) return -1;
Here and in the rest of the patches.
Got. Thanks!
+ for (i = 0; i < n; i++) { if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) return -1;
Michal
Thanks for review. Huaqiang
Refactor 'virResctrlMonitorFreeStats' to let it available to free the 'virResctrlMonitorStatsPtr' variable. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/qemu/qemu_driver.c | 1 + src/util/virresctrl.c | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2..2abed86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19980,6 +19980,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) VIR_FREE(resdata->name); VIR_FREE(resdata->vcpus); virResctrlMonitorFreeStats(resdata->stats, resdata->nstats); + VIR_FREE(resdata->stats); VIR_FREE(resdata); } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 90532cf..0f18d2b 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2764,7 +2764,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, cleanup: VIR_FREE(datapath); VIR_FREE(filepath); - VIR_FREE(stat); + virResctrlMonitorFreeStats(&stat, 1); VIR_DIR_CLOSE(dirp); return ret; } @@ -2781,8 +2781,6 @@ virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats, for (i = 0; i < nstats; i++) VIR_FREE(stats[i]); - - VIR_FREE(stats); } -- 2.7.4
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Refactor 'virResctrlMonitorFreeStats' to let it available to free the 'virResctrlMonitorStatsPtr' variable.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/qemu/qemu_driver.c | 1 + src/util/virresctrl.c | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2..2abed86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19980,6 +19980,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) VIR_FREE(resdata->name); VIR_FREE(resdata->vcpus); virResctrlMonitorFreeStats(resdata->stats, resdata->nstats); + VIR_FREE(resdata->stats); VIR_FREE(resdata); }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 90532cf..0f18d2b 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2764,7 +2764,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, cleanup: VIR_FREE(datapath); VIR_FREE(filepath); - VIR_FREE(stat); + virResctrlMonitorFreeStats(&stat, 1);
How about creating a function that frees exactly one virResctrlMonitorStatsPtr and then (possibly) have a wrapper that would call it 1 to n times? We can then get rid of this ugly call.
VIR_DIR_CLOSE(dirp); return ret; } @@ -2781,8 +2781,6 @@ virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats,
for (i = 0; i < nstats; i++) VIR_FREE(stats[i]); - - VIR_FREE(stats);
This means that virResctrlMonitorGetStats() is now leaking @stat. For instance, if virStrToLong_uip() fails. Michal
On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Refactor 'virResctrlMonitorFreeStats' to let it available to free the 'virResctrlMonitorStatsPtr' variable.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/qemu/qemu_driver.c | 1 + src/util/virresctrl.c | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2..2abed86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19980,6 +19980,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) VIR_FREE(resdata->name); VIR_FREE(resdata->vcpus); virResctrlMonitorFreeStats(resdata->stats, resdata->nstats); + VIR_FREE(resdata->stats); VIR_FREE(resdata); } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 90532cf..0f18d2b 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2764,7 +2764,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, cleanup: VIR_FREE(datapath); VIR_FREE(filepath); - VIR_FREE(stat); + virResctrlMonitorFreeStats(&stat, 1);
How about creating a function that frees exactly one virResctrlMonitorStatsPtr and then (possibly) have a wrapper that would call it 1 to n times? We can then get rid of this ugly call.
How about change virResctrlMonitorFreeStats to: (function name have been changed to virResctrlMonitorStatsFree) ```code void virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stat) { if (!stat) return; VIR_FREE(stat->vals); virStringListFree(stat->features); VIR_FREE(stat); /* Free virResctrlMonitorStats object */ } ``` And following code to is the way to free a list of @virResctrlMonitorStatsPtr objects: ```code static void qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) { ... size_t i = 0; for ( i = 0; i < resdata->nstats; i++) virResctrlMonitorStatsFree(resdata->stats[i]); virResctrlMonitorStatsFree ... } ```
VIR_DIR_CLOSE(dirp); return ret; } @@ -2781,8 +2781,6 @@ virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats, for (i = 0; i < nstats; i++) VIR_FREE(stats[i]); - - VIR_FREE(stats);
This means that virResctrlMonitorGetStats() is now leaking @stat. For instance, if virStrToLong_uip() fails.
Got. Thanks.
Michal
Thanks for review. Huaqiang
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 'virResctrlMonitorFreeStats' to let it available to free the 'virResctrlMonitorStatsPtr' variable.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/qemu/qemu_driver.c | 1 + src/util/virresctrl.c | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2..2abed86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19980,6 +19980,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) VIR_FREE(resdata->name); VIR_FREE(resdata->vcpus); virResctrlMonitorFreeStats(resdata->stats, resdata->nstats); + VIR_FREE(resdata->stats); VIR_FREE(resdata); } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 90532cf..0f18d2b 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2764,7 +2764,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, cleanup: VIR_FREE(datapath); VIR_FREE(filepath); - VIR_FREE(stat); + virResctrlMonitorFreeStats(&stat, 1);
How about creating a function that frees exactly one virResctrlMonitorStatsPtr and then (possibly) have a wrapper that would call it 1 to n times? We can then get rid of this ugly call.
How about change virResctrlMonitorFreeStats to: (function name have been changed to virResctrlMonitorStatsFree)
```code
void virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stat) { if (!stat) return;
VIR_FREE(stat->vals); virStringListFree(stat->features); VIR_FREE(stat); /* Free virResctrlMonitorStats object */ }
``` And following code to is the way to free a list of @virResctrlMonitorStatsPtr objects:
```code
static void qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) { ... size_t i = 0;
for ( i = 0; i < resdata->nstats; i++) virResctrlMonitorStatsFree(resdata->stats[i]);
virResctrlMonitorStatsFree ... }
Yes, this is exactly what I had on mind. Michal
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
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
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
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
Export virResctrlMonitorGetStats and make virResctrlMonitorGetCacheOccupancy obsoleted. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- 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; + } 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); + + return ret; } diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 97205bc..d1905b0 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -236,6 +236,12 @@ int virResctrlMonitorRemove(virResctrlMonitorPtr monitor); int +virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, + const char **resources, + virResctrlMonitorStatsPtr **stats, + size_t *nstats); + +int virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, virResctrlMonitorStatsPtr **stats, size_t *nstats); -- 2.7.4
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Export virResctrlMonitorGetStats and make virResctrlMonitorGetCacheOccupancy obsoleted.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- 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
On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Export virResctrlMonitorGetStats and make virResctrlMonitorGetCacheOccupancy obsoleted.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- 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.
I can't refer 'driver->caps->host.cache.monitor->features' directly because this function (qemuDomainGetResctrlMonData) will be used for 'memory bandwidth' monitors also. at that time that the piece of code looks like: + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + if (driver->caps->host.cache.monitor) + features = driver->caps->host.cache.monitor->features; + if (tag == VIR_RESCTRL_MONITOR_TYPE_MEMBW) { + if(driver->caps->host.membw) + features = driver->caps->host.membw.monitor->features; Hope I understand your question correctly.
+ } 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.
Agree. will be refined.
Michal
Thanks for review. Huaqiang
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:
Export virResctrlMonitorGetStats and make virResctrlMonitorGetCacheOccupancy obsoleted.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- 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.
I can't refer 'driver->caps->host.cache.monitor->features' directly because this function (qemuDomainGetResctrlMonData) will be used for 'memory bandwidth' monitors also. at that time that the piece of code looks like: + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + if (driver->caps->host.cache.monitor) + features = driver->caps->host.cache.monitor->features; + if (tag == VIR_RESCTRL_MONITOR_TYPE_MEMBW) { + if(driver->caps->host.membw) + features = driver->caps->host.membw.monitor->features;
What I meant is that you can obtain virCaps object and then access it instead: virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false); if (tag == VIR_RES..) features = caps->host.cache.monitor->features; It's not safe to access driver->caps directly because another thread might refresh those meanwhile. What may happen for instance is the following: Thread1: fetches driver pointer adds ->caps displacement fetches value from that address (which is a long way of saying 'fetch driver->caps') Thread2: Executes virQEMUDriverGetCapabilities(), which boils down to: virObjectUnref(driver->caps); driver->caps = caps; Thread1: resumes execution and tries to access ->host member. This is where Thread1 would be accessing invalid memory because the memory it's trying to access was freed meanwhile by Thread2. Michal
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/libvirt_private.syms | 1 - src/util/virresctrl.c | 29 ----------------------------- src/util/virresctrl.h | 5 ----- 3 files changed, 35 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2e3d48c..41733cc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2769,7 +2769,6 @@ virResctrlMonitorAddPID; virResctrlMonitorCreate; virResctrlMonitorDeterminePath; virResctrlMonitorFreeStats; -virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; virResctrlMonitorGetStats; virResctrlMonitorNew; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 76a8d02..640941c 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2795,32 +2795,3 @@ virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats, } } } - - -/* - * virResctrlMonitorGetCacheOccupancy - * - * @monitor: The monitor that the statistic data will be retrieved from. - * @stats: Array of virResctrlMonitorStatsPtr for receiving cache occupancy - * data. Caller is responsible to free this array. - * @nstats: A size_t pointer to hold the returned array length of @caches - * - * Get cache or memory bandwidth utilization information. - * - * Returns 0 on success, -1 on error. - */ - -int -virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, - virResctrlMonitorStatsPtr **stats, - size_t *nstats) -{ - char **features = NULL; - int ret = -1; - - virStringListAdd(&features, "llc_occupancy"); - ret = virResctrlMonitorGetStats(monitor, (const char**)features, stats, nstats); - virStringListFree(features); - - return ret; -} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index d1905b0..ab12ebf 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -241,11 +241,6 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, virResctrlMonitorStatsPtr **stats, size_t *nstats); -int -virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, - virResctrlMonitorStatsPtr **stats, - size_t *nstats); - void virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats, size_t nstats); -- 2.7.4
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Plan to support Resctrl Control Monitors, which is a feature introduced by kernel 'resctrl' sub-model. Submit some cleanup and refactoring patches for upcoming memory bandwidth resource monitoring (MBM) monitors.
Related MBM RFC is https://www.redhat.com/archives/libvir-list/2019-April/msg01409.html. This RFC is not actively discussed since libvirt already implemented similar resctrl cache monitoring (CMT), and lots details have been discussed and implemented during the work of CMT.
The cleanups and refactoring includes: 1. Removing some reluctant lines and white spaces that is existing in current code and not meet the libvirt coding style. 2. Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file. 3. Private API changes, removed 'virResctrlMonitorGetCacheOccupancy' and exported a new API named 'virResctrlMonitorGetStats' with similar functionality, but with capability to be used for retrieving MBM statistical information. 4. Refactoring 'virResctrlMonitorFreeStats' for more reusing in code. 5. Extend data structure 'virResctrlMonitorStats' with the capability to carry multiple statistical information from monitor.
Wang Huaqiang (9): conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: check if vcpus matches with any 'existing' allocaiton conf: Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file conf: refactor 'virDomainResctrlVcpuMatch' util: Refactor 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy
src/conf/domain_conf.c | 89 +++++++++++++++++++++++------------------------- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++----- src/util/virresctrl.c | 68 +++++++++++++++--------------------- src/util/virresctrl.h | 19 ++++++++--- 5 files changed, 113 insertions(+), 101 deletions(-)
Patches look good, but there are some small issues that need fixing before I'd be able to push these. Looking forward to v2. Michal
On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Plan to support Resctrl Control Monitors, which is a feature introduced by kernel 'resctrl' sub-model. Submit some cleanup and refactoring patches for upcoming memory bandwidth resource monitoring (MBM) monitors.
Related MBM RFC is https://www.redhat.com/archives/libvir-list/2019-April/msg01409.html. This RFC is not actively discussed since libvirt already implemented similar resctrl cache monitoring (CMT), and lots details have been discussed and implemented during the work of CMT.
The cleanups and refactoring includes: 1. Removing some reluctant lines and white spaces that is existing in current code and not meet the libvirt coding style. 2. Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file. 3. Private API changes, removed 'virResctrlMonitorGetCacheOccupancy' and exported a new API named 'virResctrlMonitorGetStats' with similar functionality, but with capability to be used for retrieving MBM statistical information. 4. Refactoring 'virResctrlMonitorFreeStats' for more reusing in code. 5. Extend data structure 'virResctrlMonitorStats' with the capability to carry multiple statistical information from monitor.
Wang Huaqiang (9): conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: check if vcpus matches with any 'existing' allocaiton conf: Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file conf: refactor 'virDomainResctrlVcpuMatch' util: Refactor 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy
src/conf/domain_conf.c | 89 +++++++++++++++++++++++------------------------- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++----- src/util/virresctrl.c | 68 +++++++++++++++--------------------- src/util/virresctrl.h | 19 ++++++++--- 5 files changed, 113 insertions(+), 101 deletions(-)
Patches look good, but there are some small issues that need fixing before I'd be able to push these. Looking forward to v2.
Michal
Hi Michal, Thanks for your kindly comments. I'll submit v2 patches shortly once we achieved the consistence on the code changes. Br Huaqiang
On 2019年05月28日 16:32, Huaqiang,Wang wrote:
On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
Plan to support Resctrl Control Monitors, which is a feature introduced by kernel 'resctrl' sub-model. Submit some cleanup and refactoring patches for upcoming memory bandwidth resource monitoring (MBM) monitors.
Related MBM RFC is https://www.redhat.com/archives/libvir-list/2019-April/msg01409.html. This RFC is not actively discussed since libvirt already implemented similar resctrl cache monitoring (CMT), and lots details have been discussed and implemented during the work of CMT.
The cleanups and refactoring includes: 1. Removing some reluctant lines and white spaces that is existing in current code and not meet the libvirt coding style. 2. Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file. 3. Private API changes, removed 'virResctrlMonitorGetCacheOccupancy' and exported a new API named 'virResctrlMonitorGetStats' with similar functionality, but with capability to be used for retrieving MBM statistical information. 4. Refactoring 'virResctrlMonitorFreeStats' for more reusing in code. 5. Extend data structure 'virResctrlMonitorStats' with the capability to carry multiple statistical information from monitor.
Wang Huaqiang (9): conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: check if vcpus matches with any 'existing' allocaiton conf: Replace 'virResctrlAllocIsEmpty' with @n==0 for indicating no resctrl allocation in configuration file conf: refactor 'virDomainResctrlVcpuMatch' util: Refactor 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy
src/conf/domain_conf.c | 89 +++++++++++++++++++++++------------------------- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 36 +++++++++++++++----- src/util/virresctrl.c | 68 +++++++++++++++--------------------- src/util/virresctrl.h | 19 ++++++++--- 5 files changed, 113 insertions(+), 101 deletions(-)
Patches look good, but there are some small issues that need fixing before I'd be able to push these. Looking forward to v2.
Michal
Hi Michal,
Thanks for your kindly comments. I'll submit v2 patches shortly once we achieved the consistence on the code changes.
Br Huaqiang
Hi Michal, I know we have achieved agreements on most of existing problems/comments for this series, but I could not submit with v2 patches shortly, I hope I can make more tests on the functionality of all libvirt resctrl including the MBM patches before I submit them. Just for your information. BR Huaqiang
participants (3)
-
Huaqiang,Wang -
Michal Privoznik -
Wang Huaqiang