3:36 p.m.
s/docs/conf,util/
It's more than just docs...
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.
It seems <memorytune> entries have their own unique "back door" of sorts
calling virResctrlAllocNew when there are no <cachetune> entries.
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.
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.
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
+ * @SetvirDomainResctrlDefPtr->alloc is set to NULL */
Not sure what ^^ this was...
+ 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
- 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
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
@@ -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.
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.
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. */
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.
> 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
John