The resctrl default allocation is introduced in this patch, which refers to the root directory (/sys/fs/resctrl) and immediately be created after mounting, owns all the tasks and cpus in the system and can make full use of all resources. It does not intentionally allocate any dedicated amount of resource, either cache or memory bandwidth, for default allocation. If a system task has no resource control applied but you want to know task's cache or memroy bandwidth utilization information, the default allocation is meaningful. We create resctrl monitor under the default allocation for such kind of task. Refactoring schemas docs and APIs to create a default cache allocation by allowing the appearance of an <cachetune> with no <cache> element. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- docs/formatdomain.html.in | 4 ++-- docs/schemas/domaincommon.rng | 4 ++-- src/conf/domain_conf.c | 32 +++++++++++++++++++------------- src/util/virresctrl.c | 27 +++++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 17 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8189959..b1651e3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -943,8 +943,8 @@ <dl> <dt><code>cache</code></dt> <dd> - This element controls the allocation of CPU cache and has the - following attributes: + This optional element controls the allocation of CPU cache and has + the following attributes: <dl> <dt><code>level</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 099a949..5c533d6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -956,7 +956,7 @@ <attribute name="vcpus"> <ref name='cpuset'/> </attribute> - <oneOrMore> + <zeroOrMore> <element name="cache"> <attribute name="id"> <ref name='unsignedInt'/> @@ -980,7 +980,7 @@ </attribute> </optional> </element> - </oneOrMore> + </zeroOrMore> </element> </zeroOrMore> <zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9911d56..b77680e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19002,22 +19002,27 @@ virDomainCachetuneDefParse(virDomainDefPtr def, goto cleanup; } - if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) - goto cleanup; - - if (!alloc) { - alloc = virResctrlAllocNew(); - if (!alloc) + /* If 'n' equals 0, then no <cache> element found in <cachetune>, + * this means it is a default alloction. For default allocation, + * @SetvirDomainResctrlDefPtr->alloc is set to NULL */ + if (n != 0) { + if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) goto cleanup; - } else { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Identical vcpus in cachetunes found")); - goto cleanup; - } - for (i = 0; i < n; i++) { - if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) + if (!alloc) { + alloc = virResctrlAllocNew(); + if (!alloc) + goto cleanup; + } else { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Identical vcpus in cachetunes found")); goto cleanup; + } + + for (i = 0; i < n; i++) { + if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) + goto cleanup; + } } if (virResctrlAllocIsEmpty(alloc)) { @@ -19027,6 +19032,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def, if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0) goto cleanup; + vcpus = NULL; alloc = NULL; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index df5b512..697424c 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -234,6 +234,10 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon) * in case there is no allocation for that particular cache allocation (level, * cache, ...) or memory allocation for particular node). * + * Resctrl file system root directory, /sys/fs/sysctrl/, is called the default + * allocation, which is created, immediately after mounting, owns all the + * tasks and cpus in the system and can make full use of all resources. + * * =====Cache allocation technology (CAT)===== * * Since one allocation can be made for caches on different levels, the first @@ -1165,6 +1169,9 @@ virResctrlAllocSetCacheSize(virResctrlAllocPtr alloc, unsigned int cache, unsigned long long size) { + if (!alloc) + return 0; + if (virResctrlAllocCheckCollision(alloc, level, type, cache)) { virReportError(VIR_ERR_XML_ERROR, _("Colliding cache allocations for cache " @@ -1235,6 +1242,9 @@ virResctrlAllocSetMemoryBandwidth(virResctrlAllocPtr alloc, { virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw; + if (!alloc) + return 0; + if (memory_bandwidth > 100) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Memory Bandwidth value exceeding 100 is invalid.")); @@ -1304,6 +1314,11 @@ int virResctrlAllocSetID(virResctrlAllocPtr alloc, const char *id) { + /* If passed a default allocation in, @alloc will be NULL. This is + * a valid case, return normally. */ + if (!alloc) + return 0; + if (!id) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Resctrl allocation 'id' cannot be NULL")); @@ -1317,6 +1332,9 @@ virResctrlAllocSetID(virResctrlAllocPtr alloc, const char * virResctrlAllocGetID(virResctrlAllocPtr alloc) { + if (!alloc) + return NULL; + return alloc->id; } @@ -2209,6 +2227,9 @@ int virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, const char *machinename) { + if (!alloc) + return 0; + if (!alloc->id) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Resctrl Allocation ID must be set before creation")); @@ -2302,6 +2323,9 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, char *pidstr = NULL; int ret = 0; + if (!alloc) + return 0; + if (!alloc->path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot add pid to non-existing resctrl allocation")); @@ -2334,6 +2358,9 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc) { int ret = 0; + if (!alloc) + return 0; + if (!alloc->path) return 0; -- 2.7.4

Yes, the title will be changed accordingly. If no <monitor> and no <cache> parsed from XML, the <cachetune> element will not be shown. The <cachetune> element only could be seen if there is at least one <cache> or <monitor> element. Following layouts could not be seen in XML: <cputune> ... <cachetune vcpus='0'/> </cputune> 'Resctrl monitor' has not yet been introduced until this patch, for a better understanding of purpose of this patch, let me take an example to illustrate what will happen after applying whole series patches. Supposing we are going to create a monitor over vcpu 0 for getting cache utilization, and we haven't created any cache allocation for vcpu 0. This could happen if user want to know the cache usage of specific vcpu but don't want to allocate any dedicated amount of cache for it. The XML layout would be: <cputune> <cachetune vcpus='0'> <monitor level='3' vcpus='0'/> </cachetune> </cputune> To support above XML layout in future patches, we need to append a virDomainResctrlDef element to @(virDomainDefPtr*)def->resctrls even the virDomainResctrlDef.alloc is empty. This patch changes code implement this and also will not create any allocation for cache if @def->resctrls[i]->alloc is set to NULL. Also this monitor specified in above configuration will be created under '/sys/fs/resctrl/mon_groups'. This piece of code has been refactored for several times in this patch and subsequent patches, each patch works and does not break original functionality already implemented. But the functionality of resctrl monitor only works after whole series have been applied. Yes. memorytune creates separate entry in <cputune>. Not understand above paragraph too much. Supposing your 'cachetune' entry means an corresponding element in @def->resctrls array. Up to this patch, it is not allowed to append the virDomainResctrlDef element to @def->resctrls if there is no <cache> element. Later, a virDomainResctrlDef element could only be appended if there exists at least one <cache> element or one <monitor>. A <cachetune> entry without <cache>entries, and at same time, without <monitor> entries is not permitted here. What is created and no visibility? Not understand. If no <cachtune> entries, there will no virDomainResctrlDef element appended to @def->resctrls. Up to this patch, there will be no creation for either <cachtune> entry in later invocation of virDomainCachetuneDefFormat or an appending of element in @def->resctrls during XML parsing, even with following XML configuration: <cputune> ... <cachetune vcpus='0-1'/> </cputune> Even after an integration of the whole patch series, upper XML configuration will not create any @def->resctrls elements in configuration file parsing or any <cachetune> XML element in later call of virDomainCachetuneDefFormat. My mistake. will be fixed. Should be @virDomainResctrlDefPtr->alloc. Your advising changes works here, but will conflict with later logic I will introduce in patch 13. This part of code will be modified in later patch (patch 13 of 19), adding some code parsing configuration for monitor. At that time, if n==0 then letting this function return without error is not a reasonable logic, and need to check if <monitor> entries exists under <cachetune> entry. Paste the code here for your reference: 19180 if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) { 19181 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 19182 _("Cannot extract cache nodes under cachetune")); 19183 goto cleanup; 19184 } 19185 19186 /* If 'n' equals 0, then no <cache> element found in <cachetune>, 19187 * this means it is a default alloction. For default allocation, 19188 * @virDomainResctrlDefPtr->alloc is set to NULL */ 19189 if (n != 0) { 19190 if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0) 19191 goto cleanup; 19192 19193 if (!alloc) { 19194 alloc = virResctrlAllocNew(); 19195 if (!alloc) 19196 goto cleanup; 19197 } else { 19198 virReportError(VIR_ERR_XML_ERROR, "%s", 19199 _("Identical vcpus in cachetunes found")); 19200 goto cleanup; 19201 } 19202 19203 for (i = 0; i < n; i++) { 19204 if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) 19205 goto cleanup; 19206 } 19207 } 19208 19209 resctrl = virDomainResctrlNew(alloc, vcpus); 19210 if (!resctrl) 19211 goto cleanup; 19212 19213 if (virDomainResctrlMonDefParse(def, ctxt, node, 19214 VIR_RESCTRL_MONITOR_TYPE_CACHE, 19215 resctrl) < 0) 19216 goto cleanup; 19217 19218 if (virResctrlAllocIsEmpty(alloc) && !resctrl->nmonitors) { --> If there is no 'cache' entry and no 'monitor' entry in current <cachetune> entry, code will go to this place, and function will return normally without error, and virDomainResctrlAppend will not be executed. 19219 ret = 0; 19220 goto cleanup; 19221 } 19222 19223 if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) 19224 goto cleanup; 19225 19226 resctrl = NULL; 19227 ret = 0; 19228 cleanup: 19229 ctxt->node = oldnode; 19230 virDomainResctrlDefFree(resctrl); 19231 virObjectUnref(alloc); 19232 virBitmapFree(vcpus); 19233 VIR_FREE(nodes); 19234 return ret; This blank line is involved by mistake, will be removed :) Yes. and I also double checked for these functions. I am focus on cache monitor entries in these patches, will be further checked when introducing memoryBW monitor later. I don't realize the @alloc has already been used! Will remove the pointer checking for @alloc. Let me add a test for this case in next update. Will remove the comment. I'll try to add this comment to the CAT and MBA comments. There is a subsequent patch, patch 16, handling this. Up to now, no monitor introduced, there will not there is no way to pass in an empty @alloc, so the code will not introduce any trouble. In patch 16, a 'virDomainResctrl.id' is introduced, and later code will use 'virDomainResctrlDef.id' to track the id of cachetune. I did this, because I have extended the scope of virDomainResctrlDef in later patches by adding monitors, and one virDomainResctrlDef is the abstraction of one <cachetune> entry, so logically 'id' of <cachetune> should be kept in structure virDomainResctrlDef. Will be combined. Thanks for review. Huaqiang

I think I have forget replying this review. On 10/11/2018 5:28 AM, John Ferlan wrote: So your suggestions is create @resctrl->alloc whenever seeing a <cachetune>, while my solution is don't create it, and leaving it as NULL, if an empty <cachetune> element found. Your suggestion should work if we do not let it to create any actual directory in '/sys/fs/resctrl'. Boundary check between vcpus of <cachetune> and <memorytune> should be considered. As stated, will create resctrl->alloc at all <cachetune> element. This because memorytune and cachetune shares same data structure 'virResctrlAlloc' they are called 'allocation' but refer to different sub-field. cachetune and memorytune works independently from the interface. This part does not need to change since we decided to create @alloc when code reach this place. Understand. Will remove 'default allocation' related things and create @->alloc I can do that but with a long parameter list for ResctrlAppend, I have to pass all element of resctrl into this function, because this is the place that all information will be submitted to @def->resctrls. Will remove 'default allocation'. Yes. In this case 'if (!alloc)' is no meaning, will be removed. Thanks for review and suggestions! Huaqiang

On 10/9/18 6:30 AM, Wang Huaqiang wrote: "if no specific CAT or MBA entries are provided for the guest" The rest seems reasonable at least for now, so Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]

The code of adding PID to the allocation could be reused, refactor it for later reusing. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 18ee560..41c7e51 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2359,24 +2359,21 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, } -int -virResctrlAllocAddPID(virResctrlAllocPtr alloc, - pid_t pid) +static int +virResctrlAddPID(const char *path, + pid_t pid) { char *tasks = NULL; char *pidstr = NULL; int ret = 0; - if (!alloc) - return 0; - - if (!alloc->path) { + if (!path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot add pid to non-existing resctrl allocation")); + _("Cannot add pid to non-existing resctrl group")); return -1; } - if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0) + if (virAsprintf(&tasks, "%s/tasks", path) < 0) return -1; if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0) @@ -2398,6 +2395,17 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, int +virResctrlAllocAddPID(virResctrlAllocPtr alloc, + pid_t pid) +{ + if (!alloc) + return 0; + + return virResctrlAddPID(alloc->path, pid); +} + + +int virResctrlAllocRemove(virResctrlAllocPtr alloc) { int ret = 0; -- 2.7.4

OK. How about changes like these? if (!id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl resource ID must be set before creation")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl ID must be set before determining resctrl " + "path under '%s'"), + parentpath); return NULL; } Will be renamed. Will be changed. if (alloc->path) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Resctrl group path is expected to be NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl allocation path is already set to '%s'"), + alloc->path); return -1; } Thanks for review. Huaqiang

The code for determining resctrl allocation path could be reused for monitor. Refactor it for reusing. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index c9a79f7..03001cc 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, } +static char * +virResctrlDeterminePath(const char *pathparent, + const char *prefix, + const char *id) +{ + char *path = NULL; + + if (!id) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resctrl resource ID must be set before creation")); + return NULL; + } + + if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0) + return NULL; + + return path; +} + + int virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, const char *machinename) @@ -2274,15 +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, if (!alloc) return 0; - if (!alloc->id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl Allocation ID must be set before creation")); + if (alloc->path) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Resctrl group path is expected to be NULL")); return -1; } - if (!alloc->path && - virAsprintf(&alloc->path, "%s/%s-%s", - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH, + machinename, + alloc->id); + if (!alloc->path) return -1; return 0; -- 2.7.4

OK. How about changes like these? if (!id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl resource ID must be set before creation")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl ID must be set before determining resctrl " + "path under '%s'"), + parentpath); return NULL; } Will be renamed. Will be changed. if (alloc->path) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Resctrl group path is expected to be NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl allocation path is already set to '%s'"), + alloc->path); return -1; } Thanks for review. Huaqiang

Thanks for advice, will be fixed.

On 10/9/18 6:30 AM, Wang Huaqiang wrote: const char VIR_AUTOFREE(char *) parentpath = NULL; (thus the VIR_FREE later isn't necessary) Shouldn't there be a monitor->path check here like there is in virResctrlAllocDeterminePath? s/(char *)// see above... John

On 10/10/18 9:56 AM, Wang, Huaqiang wrote: Yeah it's "newer" stuff, but it hasn't always gotten into my review cadence so sometimes I remember, sometimes I don't. We had a Google summer of code student working through those changes, but there's still many more places to change. John

The code for creating resctrl allocation group could be reused for monitoring group, refactor it for reusing in the later patch. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/util/virresctrl.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 1a5578e..8b617a6 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2310,6 +2310,26 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, } +/* This function creates a resctrl directory in resource control file system, + * and the directory path is specified by @path. */ +static int +virResctrlCreateGroupPath(const char *path) +{ + /* Directory exists, return */ + if (virFileExists(path)) + return 0; + + if (virFileMakePath(path) < 0) { + virReportSystemError(errno, + _("Cannot create resctrl directory '%s'"), + path); + return -1; + } + + return 0; +} + + /* This checks if the directory for the alloc exists. If not it tries to create * it and apply appropriate alloc settings. */ int @@ -2334,13 +2354,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virResctrlAllocDeterminePath(alloc, machinename) < 0) return -1; - if (virFileExists(alloc->path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Path '%s' for resctrl allocation exists"), - alloc->path); - goto cleanup; - } - lockfd = virResctrlLockWrite(); if (lockfd < 0) goto cleanup; @@ -2348,6 +2361,9 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virResctrlAllocAssign(resctrl, alloc) < 0) goto cleanup; + if (virResctrlCreateGroupPath(alloc->path) < 0) + goto cleanup; + alloc_str = virResctrlAllocFormat(alloc); if (!alloc_str) goto cleanup; @@ -2355,13 +2371,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virAsprintf(&schemata_path, "%s/schemata", alloc->path) < 0) goto cleanup; - if (virFileMakePath(alloc->path) < 0) { - virReportSystemError(errno, - _("Cannot create resctrl directory '%s'"), - alloc->path); - goto cleanup; - } - VIR_DEBUG("Writing resctrl schemata '%s' into '%s'", alloc_str, schemata_path); if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) { rmdir(alloc->path); -- 2.7.4

Add interface for creating the resource monitoring group according to '@virResctrlMonitor->path'. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt_private.syms | 1 + src/util/virresctrl.c | 28 ++++++++++++++++++++++++++++ src/util/virresctrl.h | 6 ++++++ 3 files changed, 35 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e175c8b..a878083 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2681,6 +2681,7 @@ virResctrlInfoGetMonitorPrefix; virResctrlInfoMonFree; virResctrlInfoNew; virResctrlMonitorAddPID; +virResctrlMonitorCreate; virResctrlMonitorDeterminePath; virResctrlMonitorNew; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 8b617a6..b3d20cc 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2475,6 +2475,7 @@ virResctrlMonitorAddPID(virResctrlMonitorPtr monitor, return virResctrlAddPID(monitor->path, pid); } + int virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor, const char *machinename) @@ -2506,3 +2507,30 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor, return 0; } + + +int +virResctrlMonitorCreate(virResctrlAllocPtr alloc, + virResctrlMonitorPtr monitor, + const char *machinename) +{ + int lockfd = -1; + int ret = -1; + + if (!monitor) + return 0; + + monitor->alloc = virObjectRef(alloc); + + if (virResctrlMonitorDeterminePath(monitor, machinename) < 0) + return -1; + + lockfd = virResctrlLockWrite(); + if (lockfd < 0) + return -1; + + ret = virResctrlCreateGroupPath(monitor->path); + + virResctrlUnlock(lockfd); + return ret; +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 69b6b1d..1efe394 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -196,7 +196,13 @@ virResctrlMonitorNew(void); int virResctrlMonitorAddPID(virResctrlMonitorPtr monitor, pid_t pid); + int virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor, const char *machinename); + +int +virResctrlMonitorCreate(virResctrlAllocPtr alloc, + virResctrlMonitorPtr monitor, + const char *machinename); #endif /* __VIR_RESCTRL_H__ */ -- 2.7.4

On 10/11/2018 3:13 AM, John Ferlan wrote: OK. Two lines between each function. @alloc could be NULL. In virResctrlAllocCreate, if the passed in @alloc is NULL, the virResctrlAllocCreate function will return 0, this is not considered as an error. Following the code invoking virResctrlMonitorCreate, if @vm->def->resctrls[i]->alloc is NULL, then the virResctrlAllocCreate function will return 0, this is not an error, code going on, then virResctrlMonitorCreate will be invoked if @nmonitors is not 0, in this case, the first argument,the @alloc, of virResctrlMonitorCreate is NULL. 2613 for (i = 0; i < vm->def->nresctrls; i++) { 2614 size_t j = 0; 2615 if (virResctrlAllocCreate(caps->host.resctrl, 2616 vm->def->resctrls[i]->alloc, 2617 priv->machineName) < 0) 2618 goto cleanup; 2619 2620 for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) { 2621 virDomainResctrlMonDefPtr mon = NULL; 2622 2623 mon = vm->def->resctrls[i]->monitors[j]; 2624 if (virResctrlMonitorCreate(vm->def->resctrls[i]->alloc, 2625 mon->instance, 2626 priv->machineName) < 0) 2627 goto cleanup; 2628 2629 } 2630 } If in later steps error happens and returns a -1, then it will go through the resource releasing functions by calling virResctrlMonitorDispose and virResctrlAllocDispose. The unref of @monitor->alloc is done in virResctrlMonitorDispose (function introduced in patch2): 401 static void 402 virResctrlMonitorDispose(void *obj) 403 { 404 virResctrlMonitorPtr monitor = obj; 405 406 virObjectUnref(monitor->alloc); 407 VIR_FREE(monitor->id); 408 VIR_FREE(monitor->path); 409 } Maybe combine virResctrlMonitorDispose and virResctrlMonitorCreate in one patch? Will that make you look better for understanding how @monitor->alloc is used. Thanks for review. Huaqiang

On 10/9/18 6:30 AM, Wang Huaqiang wrote: I would think that perhaps the comments changing here would go earlier when virResctrlMonitor was introduced. The comment with the single virResctrlAlloc could be changed to "virResctrlAlloc and virResctrlMonitor", then merge in what you've typed above. The eventual only caller never checks status... So unlike Alloc, @monitor cannot be NULL... Either virReportSystemError if you're going to handle the returned status or VIR_ERROR if you're not (and are just ignoring), but not both. Based on usage, maybe we should just give up on API's like this. Create a VIR_RESCTRL_MONITOR_CACHE_LEVEL (or something like it)... Then use it at least for now when reading/supplying the level. Thus we leave it to future developer to create the API's and handle the new levels... If when we Parse we don't find the constant, then error. Usually just GetStats is fine or GetOneStat occupancy mem_total_bytes Although the actual names could go below when describing [1] [1] e.g. here - what you'd expect @resource to be... I think all of the above could be replaced by: if (!(str_id = STRSKIP(ent->d_name, "mon_L"))) continue; I personally am not a fan of pre-auto-incr. I get particularly uncomfortable when it involves pointer arithmetic... But I think it's unnecessary if you use STRSKIP If you had a single structure for id and val, then... ...qsort would be a much better option than the above open code... I think the above two may be a case for waiting until they're needed, but I haven't got that far yet. IOW: Some extraction and reordering. John

[...] Understood, but that doesn't check status either. Maybe it needs to change to a void since it uses VIR_ERROR. I can agree to that along with it being void since it's a best effort. Yeah, I think I've come to that conclusion. In the Parse code you'd still parse the value, but then compare against the constant. In the Format code, you can just format what you have. Whomever creates a different level in the future can have the fun of managing range and of course managing if whatever is being fetched is in the particular cache level. [...] So MON_L3DATA_00 would do the same as would MON_L3FUTURE_00? Then let's STRSKIP(str_id, "_") - IOW: Skip to the next The first STRSKIP is to get to the "level"#, right? The second to the "id"#. Maybe the variables should be named that way to make it clear along with some comments. Your call how you do it, but that self created sort (below) is more what is objectionable. I remember peeking quickly - there are plenty of qsort examples to choose from in libvirt sources. John [...]

In resctrl file system, more than one monitoring groups could be created within one allocation group, along with the creation of allocation group, a monitoring group is created at the same, which monitors the resource utilization information of whole allocation group. This patch is introducing the concept of default monitor, which represents the particular monitoring group that created along with the creation of allocation group. Default monitor shares the common 'vcpu' list with the allocation. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt_private.syms | 1 + src/util/virresctrl.c | 23 +++++++++++++++++++++++ src/util/virresctrl.h | 2 ++ 3 files changed, 26 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c93d19f..4b22ed4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2689,6 +2689,7 @@ virResctrlMonitorGetID; virResctrlMonitorNew; virResctrlMonitorRemove; virResctrlMonitorSetCacheLevel; +virResctrlMonitorSetDefault; virResctrlMonitorSetID; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index fca1f6f..41e8d48 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -340,6 +340,13 @@ struct _virResctrlAlloc { * bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal * features. The monitor will be created under the scope of default allocation * if no CAT or MBA supported in the system. + * + * In resctrl file sytem, more than one monitoring groups could be created + * within one allocation group, along with the creation of allocation group, + * a monitoring group is created at the same, which monitors the resource + * utilization information of whole allocation group. + * A virResctrlMonitor with @default_monitor marked as 'true' is representing + * the monitoring group created along with the creation of allocation group. */ struct _virResctrlMonitor { virObject parent; @@ -355,6 +362,8 @@ struct _virResctrlMonitor { /* libvirt-generated path in /sys/fs/resctrl for this particular * monitor */ char *path; + /* Boolean flag for default monitor */ + bool default_monitor; /* The cache 'level', special for cache monitor */ unsigned int cache_level; }; @@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor, return -1; } + if (monitor->default_monitor) { + if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0) + return -1; + + return 0; + } + if (monitor->alloc) alloc_path = monitor->alloc->path; else @@ -2739,3 +2755,10 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, return virResctrlMonitorGetStatistic(monitor, "llc_occupancy", nbank, bankids, bankcaches); } + + +void +virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor) +{ + monitor->default_monitor = true; +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 6137fee..371df8a 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -228,4 +228,6 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, size_t *nbank, unsigned int **bankids, unsigned int **bankcaches); +void +virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor); #endif /* __VIR_RESCTRL_H__ */ -- 2.7.4

On 10/11/2018 3:14 AM, John Ferlan wrote: Got. Got Rewording like these: * There are two type of monitors, the default monitor and the non-default * monitor. Within one allocation, up to one default monitor and more than * one non-default monitors could be created. * * The flag 'default_monitor' defined in structure virResctrlMonitor denotes * if a monitor is the default one or not. Make you confused, my bad. I was trying to introducing the situation of '/sys/fs/resctrl' filesystem and what I was done here at the same time. Regarding the XML layout for default monitor, following XML lines represents a default monitor, the key rule is the default monitor's <monitor> entry has the same 'vcpus' setting as <cachetune>. <cachetune vcpus='0-1'> <monitor level='3' vcpus='0-1'/> </cachetune> and following example illustrates a <cachetune> with two monitors, one of them is a default monitor <cachetune vcpus='0-1'> <monitor level='3' vcpus='0-1'/> <monitor level='3' vcpus='0'/> </cachetune> Particularly, following XML layout doesn't define any monitor or allocation. Following XML lines does not have any effect, and does not indicate an error. <cachetune vcpus='0-1'/> or <cachetune vcpus='0-1'> </cachetune> Thanks for catching my bug. I did not run the code on my machine with default monitor, because I have trouble in run libvirt with CAT, creating non-default resctrl allocation. I am working on it, and will more tests for monitor. Using following code to fix it: (also changed next lines....) if (monitor->alloc) alloc_path = monitor->alloc->path; else alloc_path = SYSFS_RESCTRL_PATH; if (monitor->default_monitor) { if (VIR_STRDUP(monitor->path, alloc_path) < 0) return -1; return 0; } Yes. Two defaults. But I think the <monitor .. > entry should be placed under <cachetune> entry. I'd like to rewrite your configuration in following way: <cachetune vcpus='0-1'> <monitor levels='3' vcpus='0-1'/> <cachetune/> <cachetune vcpus='2-3'> <monitor levels='3' vcpus='2-3'/> <cachetune/> upper configuration creates two allocations and one default monitor for each allocation. By the way <cachetune vcpus='0-1' /> has no effect for resctrl but not be considered as an error. Unlike default allocation, default allocation could be created for multiple times in host, but with a limitation that you can only create one default monitor for one allocation at most. There is only one default allocation in whole system. Every monitor is linked with some specific allocation, either default allocation or non-default allocation (or just 'allocation'). The upper manner of monitor is determined by kernel's 'resctrl' file system. If I created an allocation by creating a directory 'p0' under '/sys/fs/resctrl/', and after adding two vcpus' PID to file '/sys/fs/resctrl/p0/tasks' and making some changes to 'sys/fs/resctrl/p0/schemata' file, then the allocation is functional for resource limitation. The libvirt CAT code is doing similar operations for VM in an automatic way. Let me show the files under '/sys/fs/resctrl/p0': . ├── cpus ├── cpus_list ├── mon_data │ ├── mon_L3_00 │ │ ├── llc_occupancy │ │ ├── mbm_local_bytes │ │ └── mbm_total_bytes │ └── mon_L3_01 │ ├── llc_occupancy │ ├── mbm_local_bytes │ └── mbm_total_bytes ├── mon_groups ├── schemata └── tasks We can find some files for the monitoring role, the 'llc_occupancy' file, the 'mbm_local_bytes' file and the 'mbm_total_bytes' file. The truth is 'resctrl' fs create a monitoring group for each allocation group, and this is I called default monitor referring to. Since this monitoring group is created whenever creating a libvirt allocation, in the design of libvirt resctrl monitor, I choose to make it shown not shown according to the XML configuration. To create other monitoring groups just making sub-directories under the 'mon_group' directory, and adding corresponding vcpu PID to that sub-directory's 'tasks' file. The virResctrlMonitorCreate function creates this kind of sub-directory under this 'mon_group' directory for non-default monitor. For an allocation with one default monitor and a non-default monitor (or just 'monitor' in wording), the file layout is like these: . ├── cpus ├── cpus_list ├── mon_data <--- default monitor interface │ ├── mon_L3_00 │ │ ├── llc_occupancy │ │ ├── mbm_local_bytes │ │ └── mbm_total_bytes │ └── mon_L3_01 │ ├── llc_occupancy │ ├── mbm_local_bytes │ └── mbm_total_bytes ├── mon_groups │ └── mon1 │ ├── cpus │ ├── cpus_list │ ├── mon_data <--- non-default monitor interface │ │ ├── mon_L3_00 │ │ │ ├── llc_occupancy │ │ │ ├── mbm_local_bytes │ │ │ └── mbm_total_bytes │ │ └── mon_L3_01 │ │ ├── llc_occupancy │ │ ├── mbm_local_bytes │ │ └── mbm_total_bytes │ └── tasks ├── schemata └── tasks This patch is trying to let monitor has the capability to mark a monitor as a default monitor, and the default monitor is 'physically' existed in kernel 'resctrl' file system, and which has a different manner with other monitors. default monitor has a different role with other monitors, hope I have documented it clearly. Thanks for review. Huaqiang

On 10/11/18 8:02 AM, Wang, Huaqiang wrote: I assume the two responses to my review are very similar, but I'll use this second one since it's timewise later... [...] Starting with "Within one allocation,..." - doesn't make much sense to me as a reader, sorry. This gets tagged as a default monitor just because the vcpus match? and this is not a default monitor because the vcpus don't match I understand that, but that wasn't my point. If someone modified their XML and added just that, then add the resctrl->alloc as soon as you see it. Then when you see a <cache>, that gets added to some cache list. When you see a <monitor> that gets added to some monitor list. The whole concept of calling some a default something just isn't working for me. It's a cache or monitor for either all or some subset of the vcpus for the cachetune (or resctrl->alloc). Nothing more, nothing less. and a default monitor vs. a non-default monitor has no specific meaning afaict. Simplify the code - default allocation means no <cache> lines true? Is monitor the related to the cache or the cachetune? If you had: <cachetune 0-4> <cache 0> <cache 1-2> <cache 3> <cache 4> ... Then is : <monitor 0-4> <monitor 2> acceptible? If so, then I'm not seeing the need for default monitor and/or default allocation. The next hunk I'll need to look at later - too many other things cycling through my head right now. Nice picture, but correlating this to code is not clicking. My quick look though - I see the same files in both the default and non-default pictures - the path to get there is different, but that path afaik is generated as part of the processing and shouldn't know whether it's default or non-default. It's just a path to data based on some other "base" path. John > Let me show the files under '/sys/fs/resctrl/p0': > > . > ├── cpus > ├── cpus_list > ├── mon_data > │ ├── mon_L3_00 > │ │ ├── llc_occupancy > │ │ ├── mbm_local_bytes > │ │ └── mbm_total_bytes > │ └── mon_L3_01 > │ ├── llc_occupancy > │ ├── mbm_local_bytes > │ └── mbm_total_bytes > ├── mon_groups > ├── schemata > └── tasks > > We can find some files for the monitoring role, the 'llc_occupancy' > file, the 'mbm_local_bytes' file and the 'mbm_total_bytes' file. > The truth is 'resctrl' fs create a monitoring group for each allocation > group along with its creation, and this monitoring group is what I > introduced default monitor. > The default monitor shares the same 'tasks' file with allocation, so > it monitors the resource utilization for all pids existing in 'tasks' file. > > Since this monitoring group is created whenever creating a libvirt > allocation, in the design of libvirt resctrl monitor, I choose to make it > shown not shown according to the XML configuration. > > To create other monitoring groups just making sub-directories under > the 'mon_group' directory, and adding corresponding vcpu PID to > that sub-directory's 'tasks' file. > The virResctrlMonitorCreate function creates this kind of sub-directory > under this 'mon_group' directory for non-default monitor. > For non-default monitor, you can specify a subset of pids of that in > allocation 'tasks' file, and no pid overlap allowed between non-default > monitors. > > For an allocation with one default monitor and a non-default monitor > (or just 'monitor' in wording), the files layout are like these: > . > ├── cpus > ├── cpus_list > ├── mon_data <--- default monitor interface > │ ├── mon_L3_00 > │ │ ├── llc_occupancy > │ │ ├── mbm_local_bytes > │ │ └── mbm_total_bytes > │ └── mon_L3_01 > │ ├── llc_occupancy > │ ├── mbm_local_bytes > │ └── mbm_total_bytes > ├── mon_groups > │ └── mon1 > │ ├── cpus > │ ├── cpus_list > │ ├── mon_data <--- non-default monitor interface > │ │ ├── mon_L3_00 > │ │ │ ├── llc_occupancy > │ │ │ ├── mbm_local_bytes > │ │ │ └── mbm_total_bytes > │ │ └── mon_L3_01 > │ │ ├── llc_occupancy > │ │ ├── mbm_local_bytes > │ │ └── mbm_total_bytes > │ └── tasks > ├── schemata > └── tasks > > This patch is trying to let monitor has the capability to mark a monitor > as a default monitor, and the default monitor is 'physically' existed in > kernel 'resctrl' file system, and which has a different manner with > other monitors. > >> >> Is all this being done to save a few steps in >> virResctrlMonitorDeterminePath? If so, then I see no value. It only adds >> confusion. > > default monitor has a different role with other monitors, hope I have > documented it clearly. > > Without identifying the default monitor, all monitors will be create under > allocation's 'mon_group' directory, the following configuration will not be > supported due to overlap between monitors. > > <cachetune vcpus='0-1'> > <monitor level='3' vcpus='0-1'/> > </cachetune> > > So default monitor is valuable if you get to know the backend mechanisim > of kernel resctrl file system. > [...]

Refactoring the code of matching the new resctrl with existing resctrl groups. Add the virObjectRef action into function virDomainResctrlVcpuMatch. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/conf/domain_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b77680e..e2b4701 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18833,7 +18833,7 @@ virDomainResctrlVcpuMatch(virDomainDefPtr def, * Just updating memory allocation information of that group */ if (virBitmapEqual(def->resctrls[i]->vcpus, vcpus)) { - *alloc = def->resctrls[i]->alloc; + *alloc = virObjectRef(def->resctrls[i]->alloc); break; } if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) { @@ -19225,8 +19225,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def, if (!alloc) goto cleanup; new_alloc = true; - } else { - alloc = virObjectRef(alloc); } for (i = 0; i < n; i++) { -- 2.7.4

Yes. I will make this patch as a standalone patch for pushing and change commit message accordingly. Thanks for review. Huaqiang

This is more "Introduce virDomainResctrlNew" On 10/9/18 6:30 AM, Wang Huaqiang wrote: and then this says something like: "Rather than rely on virDomainResctrlAppend to perform the allocation, move the onus to the caller and make use of virBitmapNewCopy for @vcpus and virObjectRef for @alloc, thus removing the need to set each to NULL after the call." I'd prefer: if (!(resctrl->vcpus = virBitmapNewCopy(vcpus))) { Based on below, I think this is where you call the virDomainResctrlNew w/ @cpus and @alloc You'd keep the above by use @resctrl for a parameter. On success, VIR_APPEND_ELEMENT will set resctrl = NULL so the *DefFree won't do anything. Without that, then the @resctrl would be leaked if the APPEND failed for any reason. Of course this is where it gets tricky - although you could pass &resctrl and then use (*resctrl)-> in the function - that way upon successful return resctrl is either added or free'd already. Alternatively, since both areas are changing to first alloc and then append, is there any specific reason the virDomainResctrlNew has to be outside of virDomainResctrlAppend? I do see the future does some other virDomainResctrlMonDefParse and virResctrlAllocIsEmpty calls before virDomainResctrlAppend - may have to rethink all that or just go with the &resctrl logic. Maybe I'll have a different thought later - let's see what happens when I get there. John

Introducing <monitor> element under <cachetune> to represent a cache monitor. Supports two kind of monitors, which are, monitor under default allocation or monitor under particular allocation. Monitor supervises the cache or memory bandwidth usage for interested vcpu thread set, if the vcpu thread set is belong to some resctrl allocation, then the monitor will be created under this allocation, that is, creating a resctrl monitoring group directory under the directory of '@alloc->path/mon_group'. Otherwise, the monitor will be created under default allocation. For default allocation monitor, it will have such kind of XML layout: <cachetune vcpus='1'> <monitor level=3 vcpus='1'/> </cachetune> For other type monitor, the XML layout will be something like: <cachetune vcpus='2-4'> <cache id='0' level='3' type='both' size='3' unit='MiB'/> <cache id='1' level='3' type='both' size='3' unit='MiB'/> <monitor level=3 vcpus='2'/> </cachetune> Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- docs/formatdomain.html.in | 26 +++ docs/schemas/domaincommon.rng | 10 + src/conf/domain_conf.c | 217 ++++++++++++++++++++- src/conf/domain_conf.h | 11 ++ tests/genericxml2xmlindata/cachetune-cdp.xml | 3 + .../cachetune-colliding-monitor.xml | 30 +++ tests/genericxml2xmlindata/cachetune-small.xml | 7 + tests/genericxml2xmltest.c | 2 + 8 files changed, 301 insertions(+), 5 deletions(-) create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-monitor.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b1651e3..2fd665c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -759,6 +759,12 @@ &lt;cachetune vcpus='0-3'&gt; &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt; &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt; + &lt;monitor level='3' vcpus='1'/&gt; + &lt;monitor level='3' vcpus='0-3'/&gt; + &lt;/cachetune&gt; + &lt;cachetune vcpus='4-5'&gt; + &lt;monitor level='3' vcpus='4'/&gt; + &lt;monitor level='3' vcpus='5'/&gt; &lt;/cachetune&gt; &lt;memorytune vcpus='0-3'&gt; &lt;node id='0' bandwidth='60'/&gt; @@ -978,6 +984,26 @@ </dd> </dl> </dd> + <dt><code>monitor</code></dt> + <dd> + The optional element <code>monitor</code> creates the cache + monitor(s) for current cache allocation and has the following + required attributes: + <dl> + <dt><code>level</code></dt> + <dd> + Host cache level the monitor belongs to. + </dd> + <dt><code>vcpus</code></dt> + <dd> + vCPU list the monitor applies to. A monitor's vCPU list + can only be the member(s) of the vCPU list of associating + allocation. The default monitor has the same vCPU list as the + associating allocation. For non-default monitors, there + are no vCPU overlap permitted. + </dd> + </dl> + </dd> </dl> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5c533d6..7ce49d3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -981,6 +981,16 @@ </optional> </element> </zeroOrMore> + <zeroOrMore> + <element name="monitor"> + <attribute name="level"> + <ref name='unsignedInt'/> + </attribute> + <attribute name="vcpus"> + <ref name='cpuset'/> + </attribute> + </element> + </zeroOrMore> </element> </zeroOrMore> <zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9a514a6..4f4604f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2955,13 +2955,30 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader) static void +virDomainResctrlMonDefFree(virDomainResctrlMonDefPtr domresmon) +{ + if (!domresmon) + return; + + virBitmapFree(domresmon->vcpus); + virObjectUnref(domresmon->instance); +} + + +static void virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl) { + size_t i = 0; + if (!resctrl) return; + for (i = 0; i < resctrl->nmonitors; i++) + virDomainResctrlMonDefFree(resctrl->monitors[i]); + virObjectUnref(resctrl->alloc); virBitmapFree(resctrl->vcpus); + VIR_FREE(resctrl->monitors); VIR_FREE(resctrl); } @@ -18919,6 +18936,154 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, return ret; } +/* Checking if the monitor's vcpus is conflicted with existing allocation + * and monitors. + * + * Returns 1 if @vcpus equals to @resctrl->vcpus, means it is a default + * monitor. Returns - 1 if a conflict found. Returns 0 if no conflict and + * @vcpus is not equal to @resctrl->vcpus. + * */ +static int +virDomainResctrlMonValidateVcpu(virDomainResctrlDefPtr resctrl, + virBitmapPtr vcpus) +{ + size_t i = 0; + int vcpu = -1; + + if (virBitmapIsAllClear(vcpus)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("vcpus is empty")); + return -1; + } + + while ((vcpu = virBitmapNextSetBit(vcpus, vcpu)) >= 0) { + if (!virBitmapIsBitSet(resctrl->vcpus, vcpu)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Monitor vcpus conflicts with allocation")); + return -1; + } + } + + if (resctrl->alloc && virBitmapEqual(vcpus, resctrl->vcpus)) + return 1; + + for (i = 0; i < resctrl->nmonitors; i++) { + if (virBitmapEqual(resctrl->vcpus, resctrl->monitors[i]->vcpus)) + continue; + + if (virBitmapOverlaps(vcpus, resctrl->monitors[i]->vcpus)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Monitor vcpus conflicts with monitors")); + + return -1; + } + } + + return 0; +} + + +static int +virDomainResctrlMonDefParse(virDomainDefPtr def, + xmlXPathContextPtr ctxt, + xmlNodePtr node, + virResctrlMonitorType tag, + virDomainResctrlDefPtr resctrl) +{ + virDomainResctrlMonDefPtr domresmon = NULL; + xmlNodePtr oldnode = ctxt->node; + xmlNodePtr *nodes = NULL; + unsigned int level = 0; + char * tmp = NULL; + char * id = NULL; + size_t i = 0; + int n = 0; + int rv = -1; + int ret = -1; + + ctxt->node = node; + + if ((n = virXPathNodeSet("./monitor", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot extract monitor nodes")); + goto cleanup; + } + + for (i = 0; i < n; i++) { + + if (VIR_ALLOC(domresmon) < 0) + goto cleanup; + + domresmon->tag = tag; + + domresmon->instance = virResctrlMonitorNew(); + if (!domresmon->instance) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not create monitor")); + goto cleanup; + } + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + tmp = virXMLPropString(nodes[i], "level"); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing monitor attribute 'level'")); + goto cleanup; + } + + if (virStrToLong_uip(tmp, NULL, 10, &level) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid monitor attribute 'level' value '%s'"), + tmp); + goto cleanup; + } + + if (virResctrlMonitorSetCacheLevel(domresmon->instance, level) < 0) + goto cleanup; + + VIR_FREE(tmp); + } + + if (virDomainResctrlParseVcpus(def, nodes[i], &domresmon->vcpus) < 0) + goto cleanup; + + rv = virDomainResctrlMonValidateVcpu(resctrl, domresmon->vcpus); + + /* If monitor's vcpu list is identical to allocation's vcpu list, + * set as default monitor */ + if (rv == 1 && resctrl->alloc) + virResctrlMonitorSetDefault(domresmon->instance); + else if (rv < 0) + goto cleanup; + + if (!(tmp = virBitmapFormat(domresmon->vcpus))) + goto cleanup; + + if (virAsprintf(&id, "vcpus_%s", tmp) < 0) + goto cleanup; + + if (virResctrlMonitorSetID(domresmon->instance, id) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(resctrl->monitors, + resctrl->nmonitors, + domresmon) < 0) + goto cleanup; + + VIR_FREE(id); + VIR_FREE(tmp); + domresmon = NULL; + } + + ret = 0; + cleanup: + ctxt->node = oldnode; + VIR_FREE(id); + VIR_FREE(tmp); + virDomainResctrlMonDefFree(domresmon); + return ret; +} + static virDomainResctrlDefPtr virDomainResctrlNew(virResctrlAllocPtr alloc, @@ -19041,15 +19206,20 @@ virDomainCachetuneDefParse(virDomainDefPtr def, } } - if (virResctrlAllocIsEmpty(alloc)) { - ret = 0; - goto cleanup; - } - resctrl = virDomainResctrlNew(alloc, vcpus); if (!resctrl) goto cleanup; + if (virDomainResctrlMonDefParse(def, ctxt, node, + VIR_RESCTRL_MONITOR_TYPE_CACHE, + resctrl) < 0) + goto cleanup; + + if (virResctrlAllocIsEmpty(alloc) && !resctrl->nmonitors) { + ret = 0; + goto cleanup; + } + if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) goto cleanup; @@ -27085,12 +27255,42 @@ virDomainCachetuneDefFormatHelper(unsigned int level, static int +virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDefPtr domresmon, + virResctrlMonitorType tag, + virBufferPtr buf) +{ + char *vcpus = NULL; + unsigned int level = 0; + + if (domresmon->tag != tag) + return 0; + + virBufferAddLit(buf, "<monitor "); + + if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + level = virResctrlMonitorGetCacheLevel(domresmon->instance); + virBufferAsprintf(buf, "level='%u' ", level); + } + + vcpus = virBitmapFormat(domresmon->vcpus); + if (!vcpus) + return -1; + + virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus); + + VIR_FREE(vcpus); + return 0; +} + + +static int virDomainCachetuneDefFormat(virBufferPtr buf, virDomainResctrlDefPtr resctrl, unsigned int flags) { virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; char *vcpus = NULL; + size_t i = 0; int ret = -1; virBufferSetChildIndent(&childrenBuf, buf); @@ -27099,6 +27299,13 @@ virDomainCachetuneDefFormat(virBufferPtr buf, &childrenBuf) < 0) goto cleanup; + for (i = 0; i < resctrl->nmonitors; i ++) { + if (virDomainResctrlMonDefFormatHelper(resctrl->monitors[i], + VIR_RESCTRL_MONITOR_TYPE_CACHE, + &childrenBuf) < 0) + goto cleanup; + } + if (virBufferCheckError(&childrenBuf) < 0) goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e30a4b2..60f6464 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2236,12 +2236,23 @@ struct _virDomainCputune { }; +typedef struct _virDomainResctrlMonDef virDomainResctrlMonDef; +typedef virDomainResctrlMonDef *virDomainResctrlMonDefPtr; +struct _virDomainResctrlMonDef { + virBitmapPtr vcpus; + virResctrlMonitorType tag; + virResctrlMonitorPtr instance; +}; + typedef struct _virDomainResctrlDef virDomainResctrlDef; typedef virDomainResctrlDef *virDomainResctrlDefPtr; struct _virDomainResctrlDef { virBitmapPtr vcpus; virResctrlAllocPtr alloc; + + virDomainResctrlMonDefPtr *monitors; + size_t nmonitors; }; diff --git a/tests/genericxml2xmlindata/cachetune-cdp.xml b/tests/genericxml2xmlindata/cachetune-cdp.xml index 9718f06..9f4c139 100644 --- a/tests/genericxml2xmlindata/cachetune-cdp.xml +++ b/tests/genericxml2xmlindata/cachetune-cdp.xml @@ -8,9 +8,12 @@ <cachetune vcpus='0-1'> <cache id='0' level='3' type='code' size='7680' unit='KiB'/> <cache id='1' level='3' type='data' size='3840' unit='KiB'/> + <monitor level='3' vcpus='0'/> + <monitor level='3' vcpus='1'/> </cachetune> <cachetune vcpus='2'> <cache id='1' level='3' type='code' size='6' unit='MiB'/> + <monitor level='3' vcpus='2'/> </cachetune> <cachetune vcpus='3'> <cache id='1' level='3' type='data' size='6912' unit='KiB'/> diff --git a/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml b/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml new file mode 100644 index 0000000..d481fb5 --- /dev/null +++ b/tests/genericxml2xmlindata/cachetune-colliding-monitor.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>4</vcpu> + <cputune> + <cachetune vcpus='0-1'> + <cache id='0' level='3' type='both' size='768' unit='KiB'/> + <monitor level='3' vcpus='2'/> + </cachetune> + </cputune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/genericxml2xmlindata/cachetune-small.xml b/tests/genericxml2xmlindata/cachetune-small.xml index ab2d9cf..748be08 100644 --- a/tests/genericxml2xmlindata/cachetune-small.xml +++ b/tests/genericxml2xmlindata/cachetune-small.xml @@ -7,6 +7,13 @@ <cputune> <cachetune vcpus='0-1'> <cache id='0' level='3' type='both' size='768' unit='KiB'/> + <monitor level='3' vcpus='0'/> + <monitor level='3' vcpus='1'/> + <monitor level='3' vcpus='0-1'/> + </cachetune> + <cachetune vcpus='2-3'> + <monitor level='3' vcpus='2'/> + <monitor level='3' vcpus='3'/> </cachetune> </cputune> <os> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index fa941f0..4393d44 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -137,6 +137,8 @@ mymain(void) TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); DO_TEST_FULL("cachetune-colliding-types", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); + DO_TEST_FULL("cachetune-colliding-monitor", false, true, + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); DO_TEST("memorytune"); DO_TEST_FULL("memorytune-colliding-allocs", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); -- 2.7.4

Yes. Will remove the memory BW related words. Monitor in default allocation could be commonly used when we want to observe the resource usage for VM vcpu threads that are not specified with any dedicated resource limitation through CAT or MBA technology. And another common case is it will be used if CAT/MBA is not supported but CMT/MBM is enabled in libvirt. The reason I have introduced the monitor in previous patch and further introducing the monitor in default allocation in this patch is the monitor n two different allocations has different behavior. Let's focus on CAT and CMT technology in my below lines, since MBA and MBM are very similar cases to them. As we know, before the CAT technology was introduced, any process in Linux is sharing the L3 cache along with all other active processes. After CAT is enabled in libvirt, it has the capability to apply cache isolation and assign dedicated amount of cache to some key VM vcpu threads. If we want to observe the real L3 cache usage information, then we need the help of monitor. == Monitor usage case 1: monitor in default allocation == If you want to get the cache utilization data before applying any cache isolation to the VM vcpu threads, you need to create a monitor in the default allocation, because you haven't create any cache allocation. == Monitor usage case 2: monitor in non-default allocation == If you have created a cache allocation for specific VM vcpu treads and not sure how many cache-lines these VM vcpu threads are really used, you need to create a monitor under the this allocation to get real cache usage information. If you find your VM vcpu threads only used a small part of cache that have allocated, you might think about to reduce its allocation. == Usage for default monitor and non-default monitor == Since we have introduced the 'default monitor' and 'non-default monitor' concepts in previous patches, and now, you can monitor the cache usage for all VM vcpu threads that added to this allocation, and also you has the capability to monitor a subset of vcpu list of this allocation. Without 'default monitor', it is impossible to get the cache usage for all vcpu threads in the allocation and at the same time get the cache usage for some highly interested vcpu threads of allocation. == Monitor usage case 3: allocating dedicated cache and monitoring its usage of one VM, and getting its influence over another VM== Think about the scenario that there are two VMs, it is a known information that one VM is very cache sensitive and don't want to share cache with other workloads, and for another VM, we have no knowledge about cache requirement, but it is required to monitor the cache usage for the two VMs. With the concepts introduced until now. We need to create an allocation and for this VM, then create a default-monitor in this allocation for monitoring. For another VM, it is required to create a non-default monitor under default allocation. With introduced concepts, the allocation, the default allocation, the monitor and the default monitor, it is possible to fulfill requirement all scenarios. The creation of monitor does not have too much flexibility, as I stated in my reply of v5patch0's review comments, the monitors need to follow below rules: 1. In each <cachetune> entry more than one monitors could be specified. 2. In each <cachetune> entry up to one allocation could be specified. 3. The allocation is using the vcpu list specified in <cachetune> attribute 'vcpus'. 4. A monitor has the same vcpu list as allocation is allowed, and this monitor is allocation's default monitor. 5. A monitor has a subset vcpu list of allocation is allowed. 6. For non-default monitors, any vcpu list overlap is not permitted. And also, the number of monitors could be generated is subject to the hardware RMID numbers. About the behavior of underlying '/sys/fs/resctrl' file system, you can get more detail from this link: https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt <cache> does not matter for monitor. Even in case of no CAT in system, the <cache> entry will never be shown under <cachetune>. If valid <cache> entries exist, means allocating some part of cache-lines to VM vcpu threads specified in attribute 'vcpus'. If there is no <cache> entry existed in <cachetune>, it does not mean these vcpu threads does not use any cache resource, it means it will use the cache resource specified in default allocation. Normally the default allocation's cache resource is shared by a lot of cache insensitive workloads. cache_occupancy is measuring based on cache bank. For Intel 2 socket xeon CPU, it is considered as two cache banks, one cache bank per socket. The typical output for each monitor of this case is: cpu.cache.0.name=vcpus_1 cpu.cache.0.vcpus=1 cpu.cache.0.bank.count=2 <--- 2 cache banks cpu.cache.0.bank.0.id=0 <--- bank.id.0 cache_occypancy cpu.cache.0.bank.0.bytes=9371648 _| cpu.cache.0.bank.1.id=1 <--- bank.id.1 cache_occypancy cpu.cache.0.bank.1.bytes=1081344 _| If you want to know the total cache occupancy for VM vcpu threads of this monitor, you need to add them up. In this case, the monitor you created only monitors the specific vcpus you added for monitor. Following two configurations satisfy your scenario, and the only monitor will detect the cache usage of thread of vcpu 2. <cachetune vcpus='2-4'> <cache id='0' level='3' type='both' size='3' unit='MiB'/> <cache id='1' level='3' type='both' size='3' unit='MiB'/> <monitor level=3 vcpus='2'/> </cachetune> <cachetune vcpus='2-4'> <monitor level=3 vcpus='2'/> </cachetune> If no cache entry in cachetune, it will also get vcpu threads' cache utilization information based on cache bank. No cache entry specified for the cachetune, means it will use the cache allocating policy of default cache allocation, which is file /sys/fs/resctrl/schemata. If valid cache entries are provided in cachetune, then an allocation will be created for the threads of vcpu listed in <cachetune> 'vcpus' attribute. Supposing the allocation is the directory /sys/fs/resctrl/p0, then the cache resource limitation was applied on these threads. For monitor, it does not care if vcpu threads are allowed or not alloowed to access a limit amount of cache-lines. Monitor only reports the amount of cache has been accesses. Yes. supposing you have this vcpus setting for <cachetune> <cachetune vcpus='0-4,8' ..../> and you choose to monitor the cache usage for vcpu 0,3,8, then you create following monitor entry inside the cachetune entry, with the output of monitor, you will get an aggregative cache occupancy information for threads of vcpu 0,3,8. <cachetune vcpus='0-4,8'/> <monitor level='3' vcpus='0,3,8'/> </cachetune> Sorry for make you confused, I'll try to refine the descriptions. I forget to free monitor itself. Will be fixed. OK. OK. I am looking cachetune as an entity with the capability to do the performance tuning tasks by a better cache resource arrangement. In a general performance tuning taks, first we need to get to know the resource utilization information and then applies some resource arrangement, such as cache isolation or allocation. @resctrl->alloc represents the role for resource arranging, @resctrl->monitors are for the performance detecting/monitoring part. Performance monitoring belongs to the scope of performance tuning, just like the part doing the resource limitation. Based on this understanding, I combined @alloc and @monitors, and let them as a child of @resctrl. If you still think it is strange to put @monitors under @resctrl, then I have to change it, creating a data structure at the level of @def->resctrls, then the new _virDomainDef structure would be: struct _virDomainDef { ... virDomainResctrlDefPtr *resctrls; size_t nresctrls; virDomainResctrlMonDefPtr *monitors; size_t nmonitors; ... } It seems the "virDomainResctrlDefPtr" should be refactored by reducing its scope that the name implies. Further, even have refactored like this, I have to add a lot of code to maintain the relationship between @resctrls->vcpus and each one of @monitors->vcpus. As I stated, default monitor, monitor, and monitor in default allocation has different character and behavior in resctrl, they are necessary. If we don't support default monitor, we could not monitor cache usage of whole allocation and also monitor some the cache usage of a subset of allocation vcpus at the same time. If we don't support default allocation, we will have to create many duplicated allocations that has same cache settings, that is sharing same cache resources. In that case, we'll lose the flexibility that kernel resctrl fs provided. You are right, it is double checked, changes to if (rv == 1) virResctrlMonitorSetDefault(domresmon->instance); I forget to free it. Will be added. Your understanding is right. The downside, what you mean is the 'churn on the internal hierarchy though'? If no <cache> entry, then 'n = virXPathNodeSet("./cache", ctxt, &nodes)' statement will assign n to 0. If n is 0, then virDomainResctrlVcpuMatch will not be called and its subsequent steps for creating @alloc will not be executed. Correct, this is how code works. Yes. Got. Will be done. I use a lot of paragraph in introducing the usage of 'default monitor', 'default allocation' and 'monitor in non-default allocation', and states that these 'default' are necessary for purpose of saving RMIDs, removing allocation duplications and keeping the flexibility of monitor that kernel interface provided. Hope I tell them clearly. Thanks for review. Huaqiang

On 10/13/2018 6:29 AM, John Ferlan wrote: Agree. As stated in reply of patch10, 'default monitor' and will be removed, and related code will be cleaned. Thanks for suggestion. Then no things will be mentioned as 'default', code and document will be cleaned accordingly. Agree. Agree. Will be done. Only 'allocation' and 'monitor'. After going through the code, I think the 'default allocation' and 'default monitor' could be removed, as I stated, without any lose of functionality currently implemented. Yes. Output is sum of two vcpus. for cache bank 0     vcpus_0-1.bank.0.bytes =   vcpus_0.bank.0.bytes + vcpus_1.bank.0.bytes for cache bank 1     vcpus_0-1.bank.1.bytes =   vcpus_0.bank.1.bytes + vcpus_1.bank.1.bytes "I honestly think this just needs to be simplified as much as possible." I reconsidered your comment ( in above line), do you mean the XML configuration for 'monitor' need to be simplified also? What I think is, even after the removal of 'default monitor' and 'default allocation' concepts, the XML configuration for monitors (with type 'all', 'm-to-n', 'one to one') still need such kind of arrangement. Take an example, a VM has 4 vcpus, vcpu 0 and 1 run cache sensitive workload, and wants to hold private L3 caches, and there is no specific requirement for left vcpus but still need a monitoring on the cache usage. Then we could create an cache allocation for vcpu 0 and 1 as well as a monitor on getting the actual cache that these two vcpus used. For vcpu 2 and 3, create a monitor for it. The XML configurations are: (no change in general rules comparing to my previous examples) <cachetune vcpus='0-1'> <cache id='0' level='3' type='both' size='3' unit='MiB'/> <cache id='1' level='3' type='both' size='3' unit='MiB'/> <monitor level=3 vcpus='0-1'/> </cachetune> <cachetune vcpus='2-3'> <monitor level=3 vcpus='2-3'/> </cachetune> Any suggestion from you is welcome. I perhaps still not understand you well ... There will have significant influence for the output of monitor if <cache> entry exist and if vcpu2-4 demands much more caches that allocation can offer; If the cache that the allocation offers is much bigger than vcpu2-4 actually used, the influence will be tiny. But in another case, that, if there is no 'cache' entries, just showing in the second example, it still influenced by the cache that the 'allocation' offers. Its difference with the first example is: the top example is using the cache resources allocated by the allocation of itself, while the second example uses the allocation of resources defined in /sys/fs/resctrl/schemata, and this cache is shared by multiple system tasks. Yes. Agree with the conclusion that 'default xxx' is a confusing things. But hope you understand that, a monitor has same vcpu list with the allocation is created along with the creation of allocation, no matter you defined a <monitor> in <cachetune> and has a same 'vcpus' setting with allocation in the XML configuration or not. This is the behavior of kernel resctrl fs. To get the cache utilization information for whole allocation, enable this system created monitor is most economic way in terms of saving RMID. Still thank you. Thank you again. Hope someday I can hold the power of Coverity ... Yes. virresctrl.c could determine a monitor is a default one by checking @resctrl->alloc. As stated in prior paragraph. Will remove 'default  monitor' and 'default allocation' and make cleaning for code and comments. Do I miss anything? BTW, I find the 'virsh domstats --cpu-total' output for monitors, introduced in patch18, is not good enough. current is " Domain: 'ubuntu16.04-base'   cpu.cache.monitor.count=2   cpu.cache.0.name=vcpus_0   cpu.cache.0.vcpus=0   cpu.cache.0.bank.count=2   cpu.cache.0.bank.0.id=0   cpu.cache.0.bank.0.bytes=9371648   cpu.cache.0.bank.1.id=1   cpu.cache.0.bank.1.bytes=1081344   cpu.cache.1.name=vcpus_3   cpu.cache.1.vcpus=3   cpu.cache.1.bank.count=2   cpu.cache.1.bank.0.id=0   cpu.cache.1.bank.0.bytes=630784   cpu.cache.1.bank.1.id=1   cpu.cache.1.bank.1.bytes=10452992 " I may change the output to following by adding 'monitor' for each line: Domain: 'ubuntu16.04-base'   cpu.cache.monitor.count=2   cpu.cache.monitor.0.name=vcpus_0   cpu.cache.monitor.0.vcpus=0   cpu.cache.monitor.0.bank.count=2   cpu.cache.monitor.0.bank.0.id=0   cpu.cache.monitor.0.bank.0.bytes=9371648   cpu.cache.monitor.0.bank.1.id=1   cpu.cache.monitor.0.bank.1.bytes=1081344   cpu.cache.monitor.1.name=vcpus_3   cpu.cache.monitor.1.vcpus=3   cpu.cache.monitor.1.bank.count=2   cpu.cache.monitor.1.bank.0.id=0   cpu.cache.monitor.1.bank.0.bytes=630784   cpu.cache.monitor.1.bank.1.id=1   cpu.cache.monitor.1.bank.1.bytes=10452992 Please take this change in consideration when you make review for patch 18. Thanks for review. Huaqiang

Check monitor status by checking the PIDs are in file 'task' or not. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt_private.syms | 1 + src/util/virresctrl.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virresctrl.h | 4 +++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4b22ed4..c90e48a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2686,6 +2686,7 @@ virResctrlMonitorDeterminePath; virResctrlMonitorGetCacheLevel; virResctrlMonitorGetCacheOccupancy; virResctrlMonitorGetID; +virResctrlMonitorIsRunning; virResctrlMonitorNew; virResctrlMonitorRemove; virResctrlMonitorSetCacheLevel; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 41e8d48..67dfbb8 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -364,6 +364,9 @@ struct _virResctrlMonitor { char *path; /* Boolean flag for default monitor */ bool default_monitor; + /* Tracking the tasks' PID associated with this monitor */ + pid_t *pids; + size_t npids; /* The cache 'level', special for cache monitor */ unsigned int cache_level; }; @@ -425,6 +428,7 @@ virResctrlMonitorDispose(void *obj) virObjectUnref(monitor->alloc); VIR_FREE(monitor->id); VIR_FREE(monitor->path); + VIR_FREE(monitor->pids); } @@ -2491,7 +2495,13 @@ int virResctrlMonitorAddPID(virResctrlMonitorPtr monitor, pid_t pid) { - return virResctrlAddPID(monitor->path, pid); + if (virResctrlAddPID(monitor->path, pid) < 0) + return -1; + + if (VIR_APPEND_ELEMENT(monitor->pids, monitor->npids, pid) < 0) + return -1; + + return 0; } @@ -2762,3 +2772,75 @@ virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor) { monitor->default_monitor = true; } + + +static int +virResctrlPIDCompare(const void *pida, const void *pidb) +{ + return *(pid_t*)pida - *(pid_t*)pidb; +} + + +bool +virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor) +{ + char *pidstr = NULL; + char **spids = NULL; + size_t nspids = 0; + pid_t *pids = NULL; + size_t npids = 0; + size_t i = 0; + int rv = -1; + bool ret = false; + + if (!monitor->path) + return false; + + if (monitor->npids == 0) + return false; + + rv = virFileReadValueString(&pidstr, "%s/tasks", monitor->path); + if (rv == -2) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Task file '%s/tasks' does not exist"), + monitor->path); + if (rv < 0) + goto cleanup; + + /* no PID in task file */ + if (!*pidstr) + goto cleanup; + + spids = virStringSplitCount(pidstr, "\n", 0, &nspids); + if (nspids != monitor->npids) + return false; + + for (i = 0; i < nspids; i++) { + unsigned int val = 0; + pid_t pid = 0; + + if (virStrToLong_uip(spids[i], NULL, 0, &val) < 0) + goto cleanup; + + pid = (pid_t)val; + + if (VIR_APPEND_ELEMENT(pids, npids, pid) < 0) + goto cleanup; + } + + qsort(pids, npids, sizeof(pid_t), virResctrlPIDCompare); + qsort(monitor->pids, monitor->npids, sizeof(pid_t), virResctrlPIDCompare); + + for (i = 0; i < monitor->npids; i++) { + if (monitor->pids[i] != pids[i]) + goto cleanup; + } + + ret = true; + cleanup: + virStringListFree(spids); + VIR_FREE(pids); + VIR_FREE(pidstr); + + return ret; +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index 371df8a..c5794cb 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -230,4 +230,8 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor, unsigned int **bankcaches); void virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor); + +bool +virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor); + #endif /* __VIR_RESCTRL_H__ */ -- 2.7.4

My bad. Will be corrected. Got. Will add some comments for each return and each 'goto cleanup', and will refine the error message. Thanks for review. Huaqiang

On 10/9/18 6:30 AM, Wang Huaqiang wrote: Rogue edit? Caused an issue w/ my Coverity environment because there's a false positive surrounding the data.vnc.websocket value being -1 and thus passing that to virPortAllocatorRelease and using it without checking > 0 in a few lines. Anyway this hunk needs to be removed. John (OK, so I didn't look too hard at these last two, but I'm going back to the start to read what you've written today).

Adding element 'id' to virDomainResctrlDef. This 'id' reflects the attribute 'id' of of element 'cachetune in XML. virResctrlAlloc.id is a copy of virDomanResctrlDef.id. Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/conf/domain_conf.c | 20 ++++++++------------ src/conf/domain_conf.h | 1 + 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4f4604f..6da9dd4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2979,6 +2979,7 @@ virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl) virObjectUnref(resctrl->alloc); virBitmapFree(resctrl->vcpus); VIR_FREE(resctrl->monitors); + VIR_FREE(resctrl->id); VIR_FREE(resctrl); } @@ -19138,6 +19139,9 @@ virDomainResctrlAppend(virDomainDefPtr def, goto cleanup; } + if (VIR_STRDUP(resctrl->id, alloc_id) < 0) + goto cleanup; + if (virResctrlAllocSetID(resctrl->alloc, alloc_id) < 0) goto cleanup; @@ -27320,13 +27324,9 @@ virDomainCachetuneDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<cachetune vcpus='%s'", vcpus); - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc); - if (!alloc_id) - goto cleanup; + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAsprintf(buf, " id='%s'", resctrl->id); - virBufferAsprintf(buf, " id='%s'", alloc_id); - } virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); @@ -27383,13 +27383,9 @@ virDomainMemorytuneDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<memorytune vcpus='%s'", vcpus); - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc); - if (!alloc_id) - goto cleanup; + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAsprintf(buf, " id='%s'", resctrl->id); - virBufferAsprintf(buf, " id='%s'", alloc_id); - } virBufferAddLit(buf, ">\n"); virBufferAddBuffer(buf, &childrenBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 60f6464..e190aa2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2248,6 +2248,7 @@ typedef struct _virDomainResctrlDef virDomainResctrlDef; typedef virDomainResctrlDef *virDomainResctrlDefPtr; struct _virDomainResctrlDef { + char *id; virBitmapPtr vcpus; virResctrlAllocPtr alloc; -- 2.7.4

Adding the interface in qemu to report CMT statistic information through command 'virsh domstats --cpu-total'. Below is a typical output: # virsh domstats 1 --cpu-total Domain: 'ubuntu16.04-base' ... cpu.cache.monitor.count=2 cpu.cache.0.name=vcpus_1 cpu.cache.0.vcpus=1 cpu.cache.0.bank.count=2 cpu.cache.0.bank.0.id=0 cpu.cache.0.bank.0.bytes=4505600 cpu.cache.0.bank.1.id=1 cpu.cache.0.bank.1.bytes=5586944 cpu.cache.1.name=vcpus_4-6 cpu.cache.1.vcpus=4,5,6 cpu.cache.1.bank.count=2 cpu.cache.1.bank.0.id=0 cpu.cache.1.bank.0.bytes=17571840 cpu.cache.1.bank.1.id=1 cpu.cache.1.bank.1.bytes=29106176 Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com&gt; --- src/libvirt-domain.c | 9 ++ src/qemu/qemu_driver.c | 230 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 239 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 7690339..218c93a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11345,6 +11345,15 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "cpu.user" - user cpu time spent in nanoseconds as unsigned long long. * "cpu.system" - system cpu time spent in nanoseconds as unsigned long * long. + * "cpu.cache.monitor.count" - tocal cache monitoring groups + * "cpu.cache.M.name" - name of cache monitoring group 'M' + * "cpu.cache.M.vcpus" - vcpus for cache monitoring group 'M' + * "cpu.cache.M.bank.count" - total bank number of cache monitoring + * group 'M' + * "cpu.cache.M.bank.N.id" - OS assigned cache bank id for cache 'N' in + * cache monitoring group 'M' + * "cpu.cache.M.bank.N.bytes" - monitor's cache occupancy of cache bank + * 'N' in cache monitoring group 'M' * * VIR_DOMAIN_STATS_BALLOON: * Return memory balloon device information. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 12a5f8f..7510a62 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -102,6 +102,7 @@ #include "virnuma.h" #include "dirname.h" #include "netdev_bandwidth_conf.h" +#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -19698,6 +19699,231 @@ typedef enum { #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB) +/* In terms of the output of virBitmapFormat, both '1-3' and '1,3' are valid + * outputs and represent different vcpu set. + * + * It is not easy to differentiate these two vcpu set formats at first glance. + * This function could be used to clear this ambiguity, it substitutes all '-' + * with ',' while generates semantically correct vcpu set. + * e.g. vcpu set string '1-3' will be replaced by string '1,2,3'. */ +static char * +qemuDomainVcpuFormatHelper(const char *vcpus) +{ + size_t i = 0; + int last = 0; + int start = 0; + char * tmp = NULL; + bool firstnum = true; + const char *cur = vcpus; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *ret = NULL; + + if (virStringIsEmpty(cur)) + return NULL; + + while (*cur != '\0') { + if (!c_isdigit(*cur)) + goto cleanup; + + if (virStrToLong_i(cur, &tmp, 10, &start) < 0) + goto cleanup; + if (start < 0) + goto cleanup; + + cur = tmp; + + virSkipSpaces(&cur); + + if (*cur == ',' || *cur == 0) { + if (!firstnum) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%d", start); + firstnum = false; + } else if (*cur == '-') { + cur++; + virSkipSpaces(&cur); + + if (virStrToLong_i(cur, &tmp, 10, &last) < 0) + goto cleanup; + + if (last < start) + goto cleanup; + cur = tmp; + + for (i = start; i <= last; i++) { + if (!firstnum) + + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%ld", i); + firstnum = 0; + } + + virSkipSpaces(&cur); + } + + if (*cur == ',') { + cur++; + virSkipSpaces(&cur); + } else if (*cur == 0) { + break; + } else { + goto cleanup; + } + } + + ret = virBufferContentAndReset(&buf); + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + + +static int +qemuDomainGetStatsCpuResource(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED, + virResctrlMonitorType restag) +{ + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; + virDomainResctrlMonDefPtr domresmon = NULL; + virDomainResctrlDefPtr resctrl = NULL; + unsigned int nmonitors = NULL; + const char *restype = NULL; + unsigned int *vals = NULL; + unsigned int *ids = NULL; + size_t nvals = 0; + char *rawvcpus = NULL; + char *vcpus = NULL; + size_t i = 0; + size_t j = 0; + int ret = -1; + + if (!virDomainObjIsActive(dom)) + return 0; + + if (restag == VIR_RESCTRL_MONITOR_TYPE_CACHE) { + restype = "cache"; + } else { + VIR_DEBUG("Invalid CPU resource type"); + return -1; + } + + for (i = 0; i < dom->def->nresctrls; i++) { + resctrl = dom->def->resctrls[i]; + + for (j = 0; j < resctrl->nmonitors; j++) { + domresmon = resctrl->monitors[j]; + if (virResctrlMonitorIsRunning(domresmon->instance) && + domresmon->tag == restag) + nmonitors++; + } + } + + if (nmonitors) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.monitor.count", restype); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + nmonitors) < 0) + goto cleanup; + } + + for (i = 0; i < dom->def->nresctrls; i++) { + resctrl = dom->def->resctrls[i]; + + for (j = 0; j < resctrl->nmonitors; j++) { + size_t l = 0; + virResctrlMonitorPtr monitor = resctrl->monitors[j]->instance; + const char *id = virResctrlMonitorGetID(monitor); + + if (!id) + goto cleanup; + + domresmon = resctrl->monitors[j]; + + if (!virResctrlMonitorIsRunning(domresmon->instance)) + continue; + + if (!(rawvcpus = virBitmapFormat(domresmon->vcpus))) + goto cleanup; + + vcpus = qemuDomainVcpuFormatHelper(rawvcpus); + if (!vcpus) + goto cleanup; + + if (virResctrlMonitorGetCacheOccupancy(monitor, &nvals, + &ids, &vals) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.%ld.name", restype, i); + if (virTypedParamsAddString(&record->params, + &record->nparams, + maxparams, + param_name, + id) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.%ld.vcpus", restype, i); + + if (virTypedParamsAddString(&record->params, + &record->nparams, + maxparams, + param_name, + vcpus) < 0) + goto cleanup; + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.%ld.bank.count", restype, i); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + nvals) < 0) + goto cleanup; + + for (l = 0; l < nvals; l++) { + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.%ld.bank.%ld.id", restype, i, l); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + ids[l]) < 0) + goto cleanup; + + + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "cpu.%s.%ld.bank.%ld.bytes", restype, i, l); + if (virTypedParamsAddUInt(&record->params, + &record->nparams, + maxparams, + param_name, + vals[l]) < 0) + goto cleanup; + } + + VIR_FREE(ids); + VIR_FREE(vals); + VIR_FREE(vcpus); + nvals = 0; + } + } + + ret = 0; + cleanup: + VIR_FREE(ids); + VIR_FREE(vals); + VIR_FREE(vcpus); + return ret; +} + + static int qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainObjPtr dom, @@ -19735,6 +19961,10 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, return -1; } + if (qemuDomainGetStatsCpuResource(driver, dom, record, maxparams, privflags, + VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0) + return -1; + return 0; } -- 2.7.4