[libvirt] [PATCHv5 01/19] docs: Refactor schemas to support default allocation
John Ferlan
jferlan at redhat.com
Tue Oct 9 20:36:22 UTC 2018
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 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.
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
>
>
More information about the libvir-list
mailing list