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

On 10/10/18 9:44 AM, Wang, Huaqiang wrote: > >> -----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. > In a way this has become obvious as I've gone through things, but after really thinking through 13 patches, I'm not sure it matters if a <cachetune> entry exists without <cache> or <monitor>. It does nothing and can be documented that way. Far too much work and effort goes into concerning ourselves with concepts that in the end don't seem to matter and perhaps would never be done. But if they are done (e.g. <cachetune> without <cache> and <monitor>), so what it does nothing and can be documented that way. > 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. If someone has a <cachetune> element, then they get an resctrl->alloc. If it doesn't have <cache> elements (all that matters at this point), who cares.

> 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. This probably crossed boundaries, but the point was if <memorytune> didn't find a <cachetune> for the 'vcpus' it had, then it creates one. But this patch goes through a few handstands to not create ->alloc when as I've come to realize later it really doesn't seem to need to do.

> 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. It's that memorytune path I noted above. Nothing is ever saved with them, but they do exist 'internally' (virDomainMemorytuneDefParse calls virResctrlAllocNew and will eventually virDomainResctrlAppend because virResctrlAllocIsEmpty is false since memorytune would have a @bandwidth (and virResctrlAllocSetMemoryBandwidth fills in alloc->mem_bw).

> 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. Yeah about where I stopped and started thinking when a <cachetune> is see we create the @alloc

> 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) { But from here... > 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 ...to here has nothing to do with whether <cache> elements exist, so why would we restrict creation of @alloc based on whether <cache> entries existed. So what if no <cache> entries exist. This is perhaps less "default allocation" and more don't require <cache> elements. In that case, ... <whatever it means>... Later we're going to add <monitor> elements and they don't require <cache> elements, so having ->alloc predicated on whether <cache> entries exists complicates a lot of code. Simplify things.

> 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. > But a <memorytune> can allocate and insert too. As I pointed out later I think the ResctrlNew and ResctrlAppend don't need to be separate either.

> 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. > And yes, this is where/why I think you shouldn't have a concept of default allocation... It should just be an allocation (aka cachetune) without specific cache entries. Later that allows monitor entries, but it may allow something else now, who knows.

Again, as I pointed out you can have a domain with <memorytune> only entries and have the same "thing", so there's absolutely no reason to not just allow <cachetune> without <cache>. >> 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. I never got there, heap overflowed stack. I'm currently of the opinion that all of the "if (!alloc)" checks won't be necessary if you create the resctrl->alloc once a <cachetune> entry is seen. Similarly, if a <memorytune> references 'vcpus' that don't already have a <cachetune> entry, then one is created - whether it's formatted is a different story (currently it's not, which is fine). I think that'll simplify things.

John

> 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 >>>