10:31 p.m.
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
10:31 p.m.
New subject: [libvirt] [PATCHv2 01/11] util, conf: Handle default monitor group of an allocation properly
'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(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 02/11] conf: code cleanup, remove empty line and one space
Remove some redundant space and line.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 03/11] conf: code cleanup for return error code directly
code cleanup for 'virDomainCachetuneDefParse' and
'virDomainMemorytuneDefParse'.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 04/11] conf: some code cleanup
Creating object and judging if it is successfully created in fewer
lines.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 05/11] conf: refactor 'virDomainResctrlVcpuMatch' and some code cleanup
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(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 06/11] conf: Append 'resctrl' object according to number of monitor group directly
'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(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 07/11] util: Refactor and rename 'virResctrlMonitorFreeStats'
Refactor and rename 'virResctrlMonitorFreeStats' to
'virResctrlMonitorStatsFree' to free one
'virResctrlMonitorStatsPtr' object.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 08/11] util: Refactor 'virResctrlMonitorStats'
Refactor 'virResctrlMonitorStats' to track multiple statistical
records.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 09/11] util: Extend virresctl API to retrieve multiple monitor statistics
Export virResctrlMonitorGetStats and make
virResctrlMonitorGetCacheOccupancy obsoleted.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
5:48 a.m.
New subject: [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(a)intel.com> wrote:
Export virResctrlMonitorGetStats and make
virResctrlMonitorGetCacheOccupancy obsoleted.
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
6:18 a.m.
New subject: [libvirt] [PATCHv2 09/11] util: Extend virresctl API to retrieve multiple monitor statistics
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(a)intel.com>; Michal Privoznik
<mprivozn(a)redhat.com>
Cc: libvir-list(a)redhat.com; Su, Tao <tao.su(a)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(a)intel.com> wrote:
>
> Export virResctrlMonitorGetStats and make
> virResctrlMonitorGetCacheOccupancy obsoleted.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
10:31 p.m.
New subject: [libvirt] [PATCHv2 10/11] util: Remove unused virResctrlMonitorGetCacheOccupancy
Signed-off-by: Wang Huaqiang <huaqiang.wang(a)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
10:31 p.m.
New subject: [libvirt] [PATCHv2 11/11] conf: Refactor and rename the function to validate a new resctrl monitor
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(a)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
4:01 a.m.
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(a)redhat.com
Cc: Wang, Huaqiang <huaqiang.wang(a)intel.com>; Su, Tao
<tao.su(a)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
12:22 a.m.
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(a)redhat.com; Michal Privoznik <mprivozn(a)redhat.com>
Cc: Su, Tao <tao.su(a)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(a)redhat.com
> Cc: Wang, Huaqiang <huaqiang.wang(a)intel.com>; Su, Tao
> <tao.su(a)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
7:37 a.m.
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(a)redhat.com>
I'll push these once the freeze is over.
Michal
8:21 a.m.
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(a)intel.com>; libvir-list(a)redhat.com
Cc: Su, Tao <tao.su(a)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(a)redhat.com>
I'll push these once the freeze is over.
Michal
5:30 p.m.
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(a)redhat.com>
I'll push these once the freeze is over.
- Cole
4:05 a.m.
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(a)redhat.com>; Wang, Huaqiang
<huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com
Cc: Su, Tao <tao.su(a)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(a)redhat.com>
>
> I'll push these once the freeze is over.
>
- Cole
1921
days inactive
1991
days old
19 comments
5 participants
participants (5)
-
Cole Robinson -
Marc-André Lureau -
Michal Privoznik -
Wang Huaqiang -
Wang, Huaqiang