[libvirt] [PATCHv5 01/19] docs: Refactor schemas to support default allocation
Wang, Huaqiang
huaqiang.wang at intel.com
Wed Oct 17 06:47:14 UTC 2018
I think I have forget replying this review.
On 10/11/2018 5:28 AM, John Ferlan wrote:
>
> On 10/10/18 9:44 AM, Wang, Huaqiang wrote:
>>
>>> -----Original Message-----
>>> From: John Ferlan [mailto:jferlan at redhat.com]
>>> Sent: Wednesday, October 10, 2018 4:36 AM
>>> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
>>> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
>>> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
>>> 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 at intel.com>
>>>> ---
>>>> 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.
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'.
>
>> 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.
Boundary check between vcpus of <cachetune> and <memorytune> should be
considered.
As stated, will create resctrl->alloc at all <cachetune> element.
>
>> 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).
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.
>
>> 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 does not need to change since we decided to create @alloc when
code reach this place.
>> 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.
Understand.
Will remove 'default allocation' related things and create @->alloc
>
>> 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.
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.
>
>> 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.
Will remove 'default allocation'.
>
> 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.
Yes. In this case 'if (!alloc)' is no meaning, will be removed.
>
> John
Thanks for review and suggestions!
Huaqiang
>> 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
>>>>
More information about the libvir-list
mailing list