[libvirt] [PATCHv2 00/11] util/resctrl cleanups and refactors
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes. 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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001). v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor src/conf/domain_conf.c | 145 ++++++++++++++++++++++++----------------------- src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-) -- 2.7.4
'default monitor of an allocation' is defined as the resctrl monitor group that created along with an resctrl allocation, which is created by resctrl file system. If the monitor group specified in domain configuration file is happened to be a default monitor group of an allocation, then it is not necessary to create monitor group since it is already created. But if an monitor group is not an allocation default group, you should create the group under folder '/sys/fs/resctrl/mon_groups' and fill the vcpu PIDs to 'tasks' file. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 3 ++- src/util/virresctrl.c | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9099757..b95f958 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2756,6 +2756,7 @@ virResctrlAllocForeachMemory; virResctrlAllocFormat; virResctrlAllocGetID; virResctrlAllocGetUnused; +virResctrlAllocIsEmpty; virResctrlAllocNew; virResctrlAllocRemove; virResctrlAllocSetCacheSize; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9046677..ed3b94c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5561,7 +5561,8 @@ qemuProcessSetupVcpu(virDomainObjPtr vm, for (j = 0; j < ct->nmonitors; j++) { mon = ct->monitors[j]; - if (virBitmapEqual(ct->vcpus, mon->vcpus)) + if (virBitmapEqual(ct->vcpus, mon->vcpus) && + !virResctrlAllocIsEmpty(ct->alloc)) continue; if (virBitmapIsBitSet(mon->vcpus, vcpuid)) { diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 90532cf..fb66ea3 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2560,7 +2560,8 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor, return -1; } - if (STREQ_NULLABLE(monitor->id, monitor->alloc->id)) { + if (!virResctrlAllocIsEmpty(monitor->alloc) && + STREQ_NULLABLE(monitor->id, monitor->alloc->id)) { if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0) return -1; return 0; -- 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
Creating object and judging if it is successfully created in fewer lines. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index db25c1f..e4a6dfb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19336,8 +19336,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, return -1; if (!alloc) { - alloc = virResctrlAllocNew(); - if (!alloc) + if (!(alloc = virResctrlAllocNew())) return -1; } else { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -19350,8 +19349,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, return -1; } - resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); - if (!resctrl) + if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, flags))) return -1; if (virDomainResctrlMonDefParse(def, ctxt, node, @@ -19540,8 +19538,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; if (!alloc) { - alloc = virResctrlAllocNew(); - if (!alloc) + if (!(alloc = virResctrlAllocNew())) return -1; new_alloc = true; } @@ -19559,8 +19556,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, * just update the existing alloc information, which is done in above * virDomainMemorytuneDefParseMemory */ if (new_alloc) { - resctrl = virDomainResctrlNew(node, alloc, vcpus, flags); - if (!resctrl) + if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, flags))) return -1; if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) -- 2.7.4
Let 'virDomainResctrlVcpuMatch' to retrieve a pointer of virDomainResctrlDefPtr in its third parameter instead of virResctrlAllocPtr, if @vcpus is matched with the vcpus of some resctrl allocation in list of @def->resctrls. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e4a6dfb..ca2dba9 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)) { @@ -19332,18 +19332,18 @@ virDomainCachetuneDefParse(virDomainDefPtr def, return -1; } - if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) + if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; - if (!alloc) { - if (!(alloc = virResctrlAllocNew())) - return -1; - } else { + if (resctrl) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Identical vcpus in cachetunes found")); return -1; } + if (!(alloc = virResctrlAllocNew())) + return -1; + for (i = 0; i < n; i++) { if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) return -1; @@ -19518,7 +19518,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, ssize_t i = 0; int n; int ret = -1; - bool new_alloc = false; ctxt->node = node; @@ -19534,13 +19533,14 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; } - if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) + if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; - if (!alloc) { + if (resctrl) { + alloc = virObjectRef(resctrl->alloc); + } else { if (!(alloc = virResctrlAllocNew())) return -1; - new_alloc = true; } for (i = 0; i < n; i++) { @@ -19555,7 +19555,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) { if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, flags))) return -1; -- 2.7.4
'virResctrlAllocIsEmpty' checks if cache allocation or memory bandwidth allocation settings are specified in configuration file. It is not proper to be used in checking memory bandwidth allocation is specified in XML settings because this function could not distinguish memory bandwidth allocations from cache allocations. Here using the local variable @n, which indicates the cache allocation groups or memory bandwidth groups depending on the context it is in, to decide if append a new @resctrl object. If @n is zero and no monitors groups specified in XML, then we should not append a new @resctrl object to @def->resctrls. This kind of replacement is also 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 ca2dba9..676a2ac 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19359,7 +19359,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; } @@ -19548,7 +19548,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, return -1; } - if (virResctrlAllocIsEmpty(alloc)) + if (n == 0) return 0; /* -- 2.7.4
Refactor and rename 'virResctrlMonitorFreeStats' to 'virResctrlMonitorStatsFree' to free one 'virResctrlMonitorStatsPtr' object. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 6 +++++- src/util/virresctrl.c | 14 ++++---------- src/util/virresctrl.h | 4 ++-- 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b95f958..1d949b3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2769,13 +2769,13 @@ virResctrlInfoNew; virResctrlMonitorAddPID; virResctrlMonitorCreate; virResctrlMonitorDeterminePath; -virResctrlMonitorFreeStats; virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; virResctrlMonitorNew; virResctrlMonitorRemove; virResctrlMonitorSetAlloc; virResctrlMonitorSetID; +virResctrlMonitorStatsFree; # util/virrotatingfile.h diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2..85fbe21 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19977,9 +19977,13 @@ struct _virQEMUResctrlMonData { static void qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) { + size_t i = 0; + VIR_FREE(resdata->name); VIR_FREE(resdata->vcpus); - virResctrlMonitorFreeStats(resdata->stats, resdata->nstats); + for (i = 0; i < resdata->nstats; i++) + virResctrlMonitorStatsFree(resdata->stats[i]); + VIR_FREE(resdata->stats); VIR_FREE(resdata); } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index fb66ea3..af0e5c0 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2765,25 +2765,19 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, cleanup: VIR_FREE(datapath); VIR_FREE(filepath); - VIR_FREE(stat); + virResctrlMonitorStatsFree(stat); VIR_DIR_CLOSE(dirp); return ret; } void -virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats, - size_t nstats) +virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stat) { - size_t i = 0; - - if (!stats) + if (!stat) return; - for (i = 0; i < nstats; i++) - VIR_FREE(stats[i]); - - VIR_FREE(stats); + VIR_FREE(stat); } diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index abdeb59..d46e533 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -233,6 +233,6 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, size_t *nstats); void -virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats, - size_t nstats); +virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stats); + #endif /* LIBVIRT_VIRRESCTRL_H */ -- 2.7.4
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 | 11 ++++++++++- src/util/virresctrl.h | 12 ++++++++++-- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 85fbe21..6b7d3ea 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20123,7 +20123,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 af0e5c0..0117b8f 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2687,6 +2687,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, { int rv = -1; int ret = -1; + unsigned int val = 0; DIR *dirp = NULL; char *datapath = NULL; char *filepath = NULL; @@ -2743,7 +2744,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, @@ -2753,6 +2754,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; } @@ -2777,6 +2784,8 @@ virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stat) if (!stat) return; + virStringListFree(stat->features); + VIR_FREE(stat->vals); VIR_FREE(stat); } diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index d46e533..4dabb9c 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
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 | 44 +++++++++++++++++++++++++------------------- src/util/virresctrl.h | 6 ++++++ 4 files changed, 57 insertions(+), 27 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1d949b3..3a4f526 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2771,6 +2771,7 @@ virResctrlMonitorCreate; virResctrlMonitorDeterminePath; virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; +virResctrlMonitorGetStats; virResctrlMonitorNew; virResctrlMonitorRemove; virResctrlMonitorSetAlloc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6b7d3ea..300a5de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19991,6 +19991,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) /** * qemuDomainGetResctrlMonData: * @dom: Pointer for the domain that the resctrl monitors reside in + * @driver: Pointer to qemu driver * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for receiving the * virQEMUResctrlMonDataPtr array. Caller is responsible for * freeing the array. @@ -20008,16 +20009,32 @@ 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; + virCapsPtr caps = NULL; size_t i = 0; size_t j = 0; + caps = virQEMUDriverGetCapabilities(driver, false); + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + features = 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]; @@ -20044,9 +20061,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) @@ -20063,7 +20079,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom, static int -qemuDomainGetStatsCpuCache(virDomainObjPtr dom, +qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams) { @@ -20077,7 +20094,7 @@ 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; @@ -20178,7 +20195,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom, static int -qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainGetStatsCpu(virQEMUDriverPtr driver, virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, @@ -20187,7 +20204,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 0117b8f..80acb70 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2669,8 +2669,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 @@ -2679,14 +2678,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; @@ -2744,21 +2744,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; @@ -2808,6 +2810,10 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, virResctrlMonitorStatsPtr **stats, size_t *nstats) { - return virResctrlMonitorGetStats(monitor, "llc_occupancy", - stats, nstats); + int ret = -1; + const char *features[2] = {"llc_occupancy", NULL}; + + ret = virResctrlMonitorGetStats(monitor, features, stats, nstats); + + return ret; } diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 4dabb9c..9ef51b7 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
Hi On Tue, Jun 11, 2019 at 7:34 AM Wang Huaqiang <huaqiang.wang@intel.com> 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 | 44 +++++++++++++++++++++++++------------------- src/util/virresctrl.h | 6 ++++++ 4 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1d949b3..3a4f526 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2771,6 +2771,7 @@ virResctrlMonitorCreate; virResctrlMonitorDeterminePath; virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; +virResctrlMonitorGetStats; virResctrlMonitorNew; virResctrlMonitorRemove; virResctrlMonitorSetAlloc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6b7d3ea..300a5de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19991,6 +19991,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) /** * qemuDomainGetResctrlMonData: * @dom: Pointer for the domain that the resctrl monitors reside in + * @driver: Pointer to qemu driver * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for receiving the * virQEMUResctrlMonDataPtr array. Caller is responsible for * freeing the array. @@ -20008,16 +20009,32 @@ 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; + virCapsPtr caps = NULL; size_t i = 0; size_t j = 0;
+ caps = virQEMUDriverGetCapabilities(driver, false); + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + features = caps->host.cache.monitor->features;
This makes libvirt crash for me, because caps->host.cache.monitor is NULL. Any idea? thanks
+ } 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];
@@ -20044,9 +20061,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) @@ -20063,7 +20079,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
static int -qemuDomainGetStatsCpuCache(virDomainObjPtr dom, +qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams) { @@ -20077,7 +20094,7 @@ 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;
@@ -20178,7 +20195,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
static int -qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainGetStatsCpu(virQEMUDriverPtr driver, virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, @@ -20187,7 +20204,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 0117b8f..80acb70 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2669,8 +2669,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 @@ -2679,14 +2678,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; @@ -2744,21 +2744,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; @@ -2808,6 +2810,10 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, virResctrlMonitorStatsPtr **stats, size_t *nstats) { - return virResctrlMonitorGetStats(monitor, "llc_occupancy", - stats, nstats); + int ret = -1; + const char *features[2] = {"llc_occupancy", NULL}; + + ret = virResctrlMonitorGetStats(monitor, features, stats, nstats); + + return ret; } diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 4dabb9c..9ef51b7 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
I'll fix it soon.
-----Original Message----- From: Marc-André Lureau [mailto:marcandre.lureau@gmail.com] Sent: Tuesday, August 6, 2019 6:48 PM To: Wang, Huaqiang <huaqiang.wang@intel.com>; Michal Privoznik <mprivozn@redhat.com> Cc: libvir-list@redhat.com; Su, Tao <tao.su@intel.com> Subject: Re: [libvirt] [PATCHv2 09/11] util: Extend virresctl API to retrieve multiple monitor statistics
Hi
On Tue, Jun 11, 2019 at 7:34 AM Wang Huaqiang <huaqiang.wang@intel.com> 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 | 44 +++++++++++++++++++++++++------------------- src/util/virresctrl.h | 6 ++++++ 4 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1d949b3..3a4f526 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2771,6 +2771,7 @@ virResctrlMonitorCreate; virResctrlMonitorDeterminePath; virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; +virResctrlMonitorGetStats; virResctrlMonitorNew; virResctrlMonitorRemove; virResctrlMonitorSetAlloc; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6b7d3ea..300a5de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19991,6 +19991,7 @@ qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata) /** * qemuDomainGetResctrlMonData: * @dom: Pointer for the domain that the resctrl monitors reside in + * @driver: Pointer to qemu driver * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for receiving
the
* virQEMUResctrlMonDataPtr array. Caller is responsible for * freeing the array. @@ -20008,16 +20009,32 @@ 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; + virCapsPtr caps = NULL; size_t i = 0; size_t j = 0;
+ caps = virQEMUDriverGetCapabilities(driver, false); + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + features = caps->host.cache.monitor->features;
This makes libvirt crash for me, because caps->host.cache.monitor is NULL. Any idea?
thanks
+ } 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];
@@ -20044,9 +20061,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) @@ -20063,7 +20079,8 @@ qemuDomainGetResctrlMonData(virDomainObjPtr dom,
static int -qemuDomainGetStatsCpuCache(virDomainObjPtr dom, +qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver, + virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams) { @@ -20077,7 +20094,7 @@ 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;
@@ -20178,7 +20195,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
static int -qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainGetStatsCpu(virQEMUDriverPtr driver, virDomainObjPtr dom, virDomainStatsRecordPtr record, int *maxparams, @@ -20187,7 +20204,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 0117b8f..80acb70 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2669,8 +2669,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 @@ -2679,14 +2678,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; @@ -2744,21 +2744,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; @@ -2808,6 +2810,10 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, virResctrlMonitorStatsPtr **stats, size_t *nstats) { - return virResctrlMonitorGetStats(monitor, "llc_occupancy", - stats, nstats); + int ret = -1; + const char *features[2] = {"llc_occupancy", NULL}; + + ret = virResctrlMonitorGetStats(monitor, features, stats, + nstats); + + return ret; } diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 4dabb9c..9ef51b7 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/libvirt_private.syms | 1 - src/util/virresctrl.c | 27 --------------------------- src/util/virresctrl.h | 5 ----- 3 files changed, 33 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3a4f526..96de17e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2769,7 +2769,6 @@ virResctrlInfoNew; virResctrlMonitorAddPID; virResctrlMonitorCreate; virResctrlMonitorDeterminePath; -virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; virResctrlMonitorGetStats; virResctrlMonitorNew; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 80acb70..90b77e6 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2790,30 +2790,3 @@ virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stat) VIR_FREE(stat->vals); VIR_FREE(stat); } - - -/* - * 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) -{ - int ret = -1; - const char *features[2] = {"llc_occupancy", NULL}; - - ret = virResctrlMonitorGetStats(monitor, features, stats, nstats); - - return ret; -} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 9ef51b7..cdfe205 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 virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stats); -- 2.7.4
A new algorithm for detecting the vcpus and monitor type conflicts between new monitor an existing allocation and monitor groups. After refactoring, since we are verifying both @vcpus and monitor type @tag at the same time, the validating function name has been renamed from virDomainResctrlMonValidateVcpus to virDomainResctrlValidateMonitor. Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/conf/domain_conf.c | 62 +++++++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 676a2ac..2af81c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19085,29 +19085,32 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, } -/* Checking if the monitor's vcpus is conflicted with existing allocation - * and monitors. +/* Checking if the monitor's vcpus and tag 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 @monitor->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 @monitor->vcpus is + * not equal to @resctrl->vcpus. */ static int -virDomainResctrlMonValidateVcpus(virDomainResctrlDefPtr resctrl, - virBitmapPtr vcpus) +virDomainResctrlValidateMonitor(virDomainResctrlDefPtr resctrl, + virDomainResctrlMonDefPtr monitor) { size_t i = 0; int vcpu = -1; - size_t mons_same_alloc_vcpus = 0; + bool vcpus_overlap_any = false; + bool vcpus_equal_to_resctrl = false; + bool vcpus_overlap_no_resctrl = false; + bool default_alloc_monitor = virResctrlAllocIsEmpty(resctrl->alloc); - if (virBitmapIsAllClear(vcpus)) { + if (virBitmapIsAllClear(monitor->vcpus)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("vcpus is empty")); return -1; } - while ((vcpu = virBitmapNextSetBit(vcpus, vcpu)) >= 0) { + while ((vcpu = virBitmapNextSetBit(monitor->vcpus, vcpu)) >= 0) { if (!virBitmapIsBitSet(resctrl->vcpus, vcpu)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Monitor vcpus conflicts with allocation")); @@ -19115,29 +19118,40 @@ virDomainResctrlMonValidateVcpus(virDomainResctrlDefPtr resctrl, } } - if (virBitmapEqual(vcpus, resctrl->vcpus)) - return 1; + vcpus_equal_to_resctrl = virBitmapEqual(monitor->vcpus, resctrl->vcpus); for (i = 0; i < resctrl->nmonitors; i++) { - if (virBitmapEqual(resctrl->vcpus, resctrl->monitors[i]->vcpus)) { - mons_same_alloc_vcpus++; - continue; + if (virBitmapEqual(monitor->vcpus, resctrl->monitors[i]->vcpus)) { + if (monitor->tag != resctrl->monitors[i]->tag) { + continue; + } else { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Identical vcpus found in same type monitors")); + return -1; + } } - if (virBitmapOverlaps(vcpus, resctrl->monitors[i]->vcpus)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Monitor vcpus conflicts with monitors")); + if (virBitmapOverlaps(monitor->vcpus, resctrl->monitors[i]->vcpus)) + vcpus_overlap_any = true; - return -1; - } + if (vcpus_equal_to_resctrl || + virBitmapEqual(resctrl->monitors[i]->vcpus, resctrl->vcpus)) + continue; + + if (virBitmapOverlaps(monitor->vcpus, resctrl->monitors[i]->vcpus)) + vcpus_overlap_no_resctrl = true; } - if (mons_same_alloc_vcpus > 1) { + if (vcpus_overlap_no_resctrl || + (default_alloc_monitor && vcpus_overlap_any)) { virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Too many monitors have the same vcpu as allocation")); + _("vcpus overlaps in resctrl groups")); return -1; } + if (vcpus_equal_to_resctrl && !default_alloc_monitor) + return 1; + return 0; } @@ -19211,7 +19225,7 @@ virDomainResctrlMonDefParse(virDomainDefPtr def, if (virDomainResctrlParseVcpus(def, nodes[i], &domresmon->vcpus) < 0) goto cleanup; - rv = virDomainResctrlMonValidateVcpus(resctrl, domresmon->vcpus); + rv = virDomainResctrlValidateMonitor(resctrl, domresmon); if (rv < 0) goto cleanup; -- 2.7.4
Hi Michal, I think I make a mistake by not adding you into the mail receiver list since you are the reviewer of V1, and these patches have been submitted for more than two weeks. The v2 patches locates in this link: https://www.redhat.com/archives/libvir-list/2019-June/thread.html#00288 I wonder if you have time to have a review. Actually I aimed to submit the MBM patches, Which is ready based on these patches, and this series of patches for cleanups and fixes to existing codes to make the upcoming MBM patches to be more reasonable. If you think it is necessary to review the MBM patches as a whole , I can send them out shortly. Br Huaqiang
-----Original Message----- From: Wang, Huaqiang Sent: Tuesday, June 11, 2019 11:31 AM To: libvir-list@redhat.com Cc: Wang, Huaqiang <huaqiang.wang@intel.com>; Su, Tao <tao.su@intel.com> Subject: [PATCHv2 00/11] util/resctrl cleanups and refactors
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes.
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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001).
v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor
src/conf/domain_conf.c | 145 ++++++++++++++++++++++++----------------- ------ src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-)
-- 2.7.4
Ping. I am also prepared the MBM(memory bandwidth monitor) patches based on these cleanups, I wonder if it is proper to submit it along with these patches, if yes, I'd like to send out the whole patches in another thread. Thanks Huaqiang
-----Original Message----- From: Wang, Huaqiang Sent: Monday, July 1, 2019 5:01 PM To: libvir-list@redhat.com; Michal Privoznik <mprivozn@redhat.com> Cc: Su, Tao <tao.su@intel.com> Subject: RE: [PATCHv2 00/11] util/resctrl cleanups and refactors
Hi Michal,
I think I make a mistake by not adding you into the mail receiver list since you are the reviewer of V1, and these patches have been submitted for more than two weeks. The v2 patches locates in this link: https://www.redhat.com/archives/libvir- list/2019-June/thread.html#00288
I wonder if you have time to have a review. Actually I aimed to submit the MBM patches, Which is ready based on these patches, and this series of patches for cleanups and fixes to existing codes to make the upcoming MBM patches to be more reasonable. If you think it is necessary to review the MBM patches as a whole , I can send them out shortly.
Br Huaqiang
-----Original Message----- From: Wang, Huaqiang Sent: Tuesday, June 11, 2019 11:31 AM To: libvir-list@redhat.com Cc: Wang, Huaqiang <huaqiang.wang@intel.com>; Su, Tao <tao.su@intel.com> Subject: [PATCHv2 00/11] util/resctrl cleanups and refactors
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes.
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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001).
v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor
src/conf/domain_conf.c | 145 ++++++++++++++++++++++++--------------
------ src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-)
-- 2.7.4
On 6/11/19 5:31 AM, Wang Huaqiang wrote:
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes.
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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001).
v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor
src/conf/domain_conf.c | 145 ++++++++++++++++++++++++----------------------- src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> I'll push these once the freeze is over. Michal
Hi Michal, Thanks for review, have a good day! Br Huaqiang
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Wednesday, July 31, 2019 8:38 PM To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Su, Tao <tao.su@intel.com> Subject: Re: [libvirt] [PATCHv2 00/11] util/resctrl cleanups and refactors
On 6/11/19 5:31 AM, Wang Huaqiang wrote:
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes.
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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001).
v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor
src/conf/domain_conf.c | 145 ++++++++++++++++++++++++--------------
src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
I'll push these once the freeze is over.
Michal
On 7/31/19 8:37 AM, Michal Privoznik wrote:
On 6/11/19 5:31 AM, Wang Huaqiang wrote:
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes.
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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001).
v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup
I think this patch introduced an issue. resctrl returned from VcpuMatch should not be free'd, but that can happen in virDomainMemorytuneDefParse Use the attached XML and do: ./tools/virsh --connect test:///default define bad.xml
conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor
src/conf/domain_conf.c | 145 ++++++++++++++++++++++++----------------------- src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
I'll push these once the freeze is over.
- Cole
Hi Cole, Good catch, I'll fix and make test soon. Thanks
-----Original Message----- From: Cole Robinson [mailto:crobinso@redhat.com] Sent: Tuesday, August 20, 2019 6:30 AM To: Michal Privoznik <mprivozn@redhat.com>; Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com Cc: Su, Tao <tao.su@intel.com> Subject: Re: [libvirt] [PATCHv2 00/11] util/resctrl cleanups and refactors
On 7/31/19 8:37 AM, Michal Privoznik wrote:
On 6/11/19 5:31 AM, Wang Huaqiang wrote:
Patches submitted for purpose of refactoring existing 'resctrl' related source code, including some code cleanups as well as some fixes. This is also a preparation for memory bandwidth monitor codes.
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: v2 changes: 1. Addressed comments of v1. 2. Introduce a new algorithm for verifying new monitor vcpus and existing monitors and allocations. 3. Fixes for creating default-allocation-monitor in 'resctrl' file system.(patch 0001).
v1 changes: 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 (11): util,conf: Handle default monitor group of an allocation properly conf: code cleanup, remove empty line and one space conf: code cleanup for return error code directly conf: some code cleanup conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup
I think this patch introduced an issue. resctrl returned from VcpuMatch should not be free'd, but that can happen in virDomainMemorytuneDefParse
Use the attached XML and do:
./tools/virsh --connect test:///default define bad.xml
conf: Append 'resctrl' object according to number of monitor group directly util: Refactor and rename 'virResctrlMonitorFreeStats' util: Refactor 'virResctrlMonitorStats' util: Extend virresctl API to retrieve multiple monitor statistics util: Remove unused virResctrlMonitorGetCacheOccupancy conf: Refactor and rename the function to validate a new resctrl monitor
src/conf/domain_conf.c | 145 ++++++++++++++++++++++++----------------------- src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 41 ++++++++++---- src/qemu/qemu_process.c | 3 +- src/util/virresctrl.c | 75 ++++++++++-------------- src/util/virresctrl.h | 23 +++++--- 6 files changed, 156 insertions(+), 136 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
I'll push these once the freeze is over.
- Cole
participants (5)
-
Cole Robinson -
Marc-André Lureau -
Michal Privoznik -
Wang Huaqiang -
Wang, Huaqiang