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@xxxxxxxxxx]
Sent: Wednesday, October 10, 2018 4:36 AM
To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx
Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>;
Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx>
Subject: Re: [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@xxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
