Re: [libvirt] [PATCHv5 01/19] docs: Refactor schemas to support default allocation

> -----Original Message----- > From: John Ferlan [mailto:jferlan@redhat.com] > Sent: Wednesday, October 10, 2018 4:36 AM > To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com > Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; > Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; > Subject: Re: [libvirt] [PATCHv5 01/19] docs: Refactor schemas to support default > allocation > > > s/docs/conf,util/ > > It's more than just docs... Yes, the title will be changed accordingly. > > On 10/9/18 6:30 AM, Wang Huaqiang wrote: >> 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(-) > > How would this look in XML output - supply a <cachetune> w/o the <cache> > element? Probably also need the <monitor> there as well in at least one entry > just prove it works. 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. > > It seems <memorytune> entries have their own unique "back door" of sorts > calling virResctrlAllocNew when there are no <cachetune> entries. Yes. memorytune creates separate entry in <cputune>. > Similar to what happens if there were entries cachetune for vcpus of "0-1" and > "2-5", but nothing for "6-7". The assumption always being <memorytune> read > after <cachetune> and as long as there's no overlap, just create the > <memorytune> entry, by supplying a <cachetune> entry without <cache> entries. > 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>. > supplying a <cachetune> entry without <cache> entries. A <cachetune> entry without <cache>entries, and at same time, without <monitor> entries is not permitted here. > It's a little awkward to read, but now makes me think about all the > existing/strange linkages. In a way I suppose having no <cachetune> entries is > proven to work by tests/genericxml2xmlindata/memorytune.xml. > The reality is they get created, but without visibility. 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. > >> >> 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, > > s/alloction/allocation My mistake. will be fixed. > >> + * @SetvirDomainResctrlDefPtr->alloc is set to NULL */ > > Not sure what ^^ this was... Should be @virDomainResctrlDefPtr->alloc. > >> + 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; >> - } > > diff is perhaps easier to read if you: > > if (n == 0) { > ret = 0; > goto cleanup; > } > > then none of the indent is needed for n != 0 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; > >> >> - 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; >> + > > Superfluous This blank line is involved by mistake, will be removed :) > >> 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 > > I assume you searched on: > > virResctrlAllocGetType (w/ callers:) > virResctrlAllocUpdateMask > virResctrlAllocUpdateSize > virResctrlAllocCopyMasks > > It's kind of "painful" to back trace all the callers and determine if any/each of > them does the if (!alloc) check "originally" somewhere. I took a quick look and > they seem OK 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. > >> @@ -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; >> > > ^^ This wouldn't have been too happy would it if alloc == NULL; however, > > >> + if (!alloc) >> + return 0; >> + > > I don't think it'll matter since the only caller is virDomainMemorytuneDefParse > which will allocate an @alloc if one didn't exist *and* pass that through to here, > so this check shouldn't be necessary. I don't realize the @alloc has already been used! Will remove the pointer checking for @alloc.

> > In researching this I realized that although we have a memorytune-colliding- > allocs.xml and memorytune.xml, there is no <memorytune> example that > includes <cachetune> entries as well. Let me add a test for this case in next update. > >> 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. */ Will remove the comment. I'll try to add this comment to the CAT and MBA comments. > > This is the only one to get that type of comment... Probably something that > should instead be more clearly indicated perhaps in the CAT and MBA comments > at the top of the module. > >> + 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; >> + > > Probably need to consider current callers... I see that both > virDomainCachetuneDefFormat and virDomainMemorytuneDefFormat would > return -1 for some unknown reason. Although perhaps the latter would work > fine since it'd create it's own @alloc if resctrl->alloc == NULL. > > Hence why I asked for an XML example above. 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. > >> 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; > > These two could be combined Will be combined. > > John Thanks for review. Huaqiang >> >>