8:44 a.m.
-----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>
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>
> ---
> 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
>
>