Short, sweet, and to the point. Looks like this patch does nothing, eh?
:-)... Of course you realized that based on my patch 15 review. After
reading the cover letter I was expecting a least a few sentences here,
but nothing, sigh.
On 11/13/2017 03:50 AM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 24 ++
> docs/schemas/domaincommon.rng | 32 +++
> src/conf/domain_conf.c | 249 +++++++++++++++++++++
> src/conf/domain_conf.h | 21 ++
> src/util/virresctrl.c | 2 +-
> .../genericxml2xmlindata/generic-cachetune-cdp.xml | 36 +++
> .../generic-cachetune-colliding-allocs.xml | 30 +++
> .../generic-cachetune-colliding-tunes.xml | 32 +++
> .../generic-cachetune-colliding-types.xml | 30 +++
> .../generic-cachetune-small.xml | 29 +++
> tests/genericxml2xmlindata/generic-cachetune.xml | 33 +++
> tests/genericxml2xmltest.c | 10 +
> 12 files changed, 527 insertions(+), 1 deletion(-)
> create mode 100644 tests/genericxml2xmlindata/generic-cachetune-cdp.xml
> create mode 100644 tests/genericxml2xmlindata/generic-cachetune-colliding-allocs.xml
> create mode 100644 tests/genericxml2xmlindata/generic-cachetune-colliding-tunes.xml
> create mode 100644 tests/genericxml2xmlindata/generic-cachetune-colliding-types.xml
> create mode 100644 tests/genericxml2xmlindata/generic-cachetune-small.xml
> create mode 100644 tests/genericxml2xmlindata/generic-cachetune.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 92e14a919aba..b9d7f53b31f9 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -689,6 +689,10 @@
> <iothread_quota>-1</iothread_quota>
> <vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/>
> <iothreadsched iothreads='2' scheduler='batch'/>
> + <cachetune vcpus='0-3'>
> + <cache id='0' level='3' type='both' size='3' unit='MiB'/>
> + <cache id='1' level='3' type='both' size='3' unit='MiB'/>
> + </cachetune>
> </cputune>
> ...
> </domain>
> @@ -834,6 +838,26 @@
> <span class="since">Since 1.2.13</span>
> </dd>
>
> + <dt><code>cachetune</code></dt>
Perhaps this goes against norms, but I think that <span> should be
placed on the "cachetune" above to signify it's for all of these. If
some day later a new element or attribute is created it then gets it's
one <span> for the since of that specific version.
> + <dd>
> + Optional <code>cachetune</code> element can control allocations for CPU
> + caches using the resctrlfs on the host. Whether or not is this supported
> + can be gathered from capabilities where some limitations like minimum
> + size and required granularity are reported as well.
> + Attribute <code>vcpus</code> specifies what vCPUs this allocation
The required attribute...
s/what/to which/
> + applies to. One vCPU cannot have multiple <code>cachetune</code>
> + allocations.
A vCPU can only be a member of one <code>cachetune</code> element
allocation.
> + For now there is only one supported
> + sub-element, <code>cache</code>. That element must have
Each <code>vcputune</code> element must have at least one
<code>cache</code> subelement. The <code>cache</code> subelement must have
> + attributes <code>level</code> and <code>id</code> - host's cache level/id
> + to allocate from, <code>type</code> - whether to
> + allocate <code>code</code>, <code>data</code> or <code>both</code>
> + and <code>size</code> - size of the region to allocate. Size is by
> + default in bytes, but optional attribute <code>unit</code> can be used
> + to scale the size appropriately. Each <code>cachetune</code> can have
> + multiple <code>cache</code> elements. <span class="since">Since
> + 3.10.0</span>
Also I like it better when the subelements are in some sort of list
format as it's easier to read than a long sentence. So picking up from
where I left off before:
the following attributes:
<dl>
<dt><code>level</code></dt>
<dd>Host cache level from which to allocate.</dd>
<dt><code>id</code></dt>
<dd>Host cache id from which to allocate.</dd>
<dt><code>type</code></dt>
<dd>Whether to allocate <code>code</code>, <code>data</code>,
or <code>both</code> regions.</dd>
<dt><code>size</code></dt>
<dd>The size of the region to allocate. The value by default is
in bytes, but the <code>unit</code> attribute can be used to scale
the value.</dd>
<dt><code>unit</code></dt>
<dd>Scaled memory value such as KiB, MiB, GiB, or TiB
described in the <code>memory</code> element for
<a href="#elementsMemoryAllocation">Memory Allocation</a>.</dd>
</dl>
BTW: Whether you expound upon what data, code, and both are is up to
you. I have a good idea what that means, but it's not necessarily well
described as to the usefulness regarding why to select one or the other.
> +
> + </dd>
> </dl>
>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 9cec1a063724..5d90247799ad 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -900,6 +900,38 @@
> <ref name="schedparam"/>
> </element>
> </zeroOrMore>
> + <zeroOrMore>
> + <element name="cachetune">
> + <attribute name="vcpus">
> + <ref name='cpuset'/>
> + </attribute>
> + <oneOrMore>
> + <element name="cache">
> + <attribute name="id">
> + <ref name='unsignedInt'/>
> + </attribute>
> + <attribute name="level">
> + <ref name='unsignedInt'/>
> + </attribute>
> + <attribute name="type">
> + <choice>
> + <value>both</value>
> + <value>code</value>
> + <value>data</value>
> + </choice>
> + </attribute>
> + <attribute name="size">
> + <ref name='unsignedLong'/>
> + </attribute>
> + <optional>
> + <attribute name='unit'>
> + <ref name='unit'/>
> + </attribute>
> + </optional>
> + </element>
> + </oneOrMore>
> + </element>
> + </zeroOrMore>
> </interleave>
> </element>
> </define>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 400e900325f2..bf9e61efc8d2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2848,6 +2848,17 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
> VIR_FREE(loader);
> }
>
> +static void
> +virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune)
> +{
> + if (!cachetune)
> + return;
> +
> + virObjectUnref(cachetune->alloc);
> + virBitmapFree(cachetune->vcpus);
> + VIR_FREE(cachetune);
> +}
> +
> void virDomainDefFree(virDomainDefPtr def)
> {
> size_t i;
> @@ -3020,6 +3031,10 @@ void virDomainDefFree(virDomainDefPtr def)
> virDomainShmemDefFree(def->shmems[i]);
> VIR_FREE(def->shmems);
>
> + for (i = 0; i < def->ncachetunes; i++)
> + virDomainCachetuneDefFree(def->cachetunes[i]);
> + VIR_FREE(def->cachetunes);
> +
> VIR_FREE(def->keywrap);
>
> if (def->namespaceData && def->ns.free)
> @@ -18080,6 +18095,163 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
> }
>
>
> +static int
> +virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
> + xmlNodePtr node,
> + virResctrlAllocPtr *alloc)
> +{
> + xmlNodePtr oldnode = ctxt->node;
> + unsigned int level;
> + unsigned int cache;
> + int type;
> + unsigned long long size;
> + char *tmp = NULL;
> + int ret = -1;
> +
> + ctxt->node = node;
> +
> + tmp = virXMLPropString(node, "id");
> + if (!tmp) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing attribute id for cachetune/cache"));
'id' (and only because later on you added '' around id in a different
message ;-)
Just cachetune?? (consistency w/ other messages - IDC which way, but
similar point in this function and the folowing one)
> + goto cleanup;
> + }
> + if (virStrToLong_uip(tmp, NULL, 10, &cache) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid attribute id '%s' for cachetune/cache"),
Maybe it's just me, but the message just doesn't flow well, consider:
"Invalid cachetune attribute 'id' value '%s'"
It's not all that important though, but could be used in subsequent
messages too.
> + tmp);
> + goto cleanup;
> + }
> + VIR_FREE(tmp);
> +
> + tmp = virXMLPropString(node, "level");
> + if (!tmp) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing attribute level for cachetune/cache"));
'level'
> + goto cleanup;
> + }
> + if (virStrToLong_uip(tmp, NULL, 10, &level) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid attribute level '%s' for cachetune/cache"),
> + tmp);
> + goto cleanup;
> + }
> + VIR_FREE(tmp);
> +
> + tmp = virXMLPropString(node, "type");
> + if (!tmp) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing attribute type for cachetune/cache"));
'type'
> + goto cleanup;
> + }
> + type = virCacheTypeFromString(tmp);
> + if (type < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid attribute type '%s' for cachetune"),
> + tmp);
> + goto cleanup;
> + }
> + VIR_FREE(tmp);
> +
> + if (virDomainParseScaledValue("./@size", "./@unit",
> + ctxt, &size, 1024,
> + ULLONG_MAX, true) < 0)
> + goto cleanup;
> +
> + if (virResctrlAllocSetSize(alloc, level, type, cache, size) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + ctxt->node = oldnode;
> + VIR_FREE(tmp);
> + return ret;
> +}
> +
Two blank lines.
> +static int
> +virDomainCachetuneDefParse(virDomainDefPtr def,
> + xmlXPathContextPtr ctxt,
> + xmlNodePtr node)
> +{
> + xmlNodePtr oldnode = ctxt->node;
> + xmlNodePtr *nodes = NULL;
> + virBitmapPtr vcpus = NULL;
> + virResctrlAllocPtr alloc = NULL;
> + virDomainCachetuneDefPtr tmp_cachetune = NULL;
> + char *tmp = NULL;
> + char *vcpus_str = NULL;
> + ssize_t i = 0;
> + int n;
> + int ret = -1;
> +
> + ctxt->node = node;
> +
> + if (VIR_ALLOC(tmp_cachetune) < 0)
> + goto cleanup;
> +
> + vcpus_str = virXMLPropString(node, "vcpus");
> + if (!vcpus_str) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing attribute vcpus for cachetune"));
Here again w/ the cachetune/cache
> + goto cleanup;
> + }
> + if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid attribute vcpus '%s' for cachetune"),
> + vcpus_str);
> + goto cleanup;
> + }
> +
> + if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot extract cachetune/cache nodes"));
> + goto cleanup;
> + }
> +
> + for (i = 0; i < n; i++) {
> + if (virDomainCachetuneDefParseCache(ctxt, nodes[i], &alloc) < 0)
> + goto cleanup;
> + }
> +
> + if (!alloc) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + virBitmapShrink(vcpus, def->maxvcpus);
> +
> + for (i = 0; i < def->ncachetunes; i++) {
> + if (virBitmapOverlaps(def->cachetunes[i]->vcpus, vcpus)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Overlapping vcpus in cachetunes"));
> + goto cleanup;
> + }
> + }
> +
> + if (virResctrlAllocSetID(alloc, vcpus_str) < 0)
> + goto cleanup;
> +
> + tmp_cachetune->vcpus = vcpus;
> + tmp_cachetune->alloc = alloc;
> + vcpus = NULL;
> + alloc = NULL;
> +
> + if (VIR_APPEND_ELEMENT(def->cachetunes, def->ncachetunes, tmp_cachetune) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + ctxt->node = oldnode;
> + virDomainCachetuneDefFree(tmp_cachetune);
> + virObjectUnref(alloc);
> + virBitmapFree(vcpus);
> + VIR_FREE(vcpus_str);
> + VIR_FREE(nodes);
> + VIR_FREE(tmp);
> + return ret;
> +}
> +
> +
> static virDomainDefPtr
> virDomainDefParseXML(xmlDocPtr xml,
> xmlNodePtr root,
> @@ -18632,6 +18804,18 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
> VIR_FREE(nodes);
>
> + if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot extract cachetune nodes"));
> + goto error;
> + }
> +
> + for (i = 0; i < n; i++) {
> + if (virDomainCachetuneDefParse(def, ctxt, nodes[i]) < 0)
> + goto error;
> + }
> + VIR_FREE(nodes);
> +
> if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, &def->cpu) < 0)
> goto error;
>
> @@ -25444,6 +25628,68 @@ virDomainSchedulerFormat(virBufferPtr buf,
> }
>
>
> +struct virCachetuneHelperData {
> + virBufferPtr buf;
> + size_t vcpu_id;
> +};
> +
> +static int
> +virDomainCachetuneDefFormatHelper(unsigned int level,
> + virCacheType type,
> + unsigned int cache,
> + unsigned long long size,
> + void *opaque)
> +{
> + const char *unit;
> + virBufferPtr buf = opaque;
> + unsigned long long short_size = virPrettySize(size, &unit);
> +
> + virBufferAsprintf(buf,
> + "<cache id='%u' level='%u' type='%s' "
> + "size='%llu' unit='%s'/>\n",
> + cache, level, virCacheTypeToString(type),
> + short_size, unit);
> +
> + return 0;
> +}
> +
> +
> +static int
> +virDomainCachetuneDefFormat(virBufferPtr buf,
> + virDomainCachetuneDefPtr cachetune)
> +{
> + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
> + char *vcpus = NULL;
> + int ret = -1;
> +
> + virBufferSetChildIndent(&childrenBuf, buf);
> + virResctrlAllocForeachSize(cachetune->alloc,
> + virDomainCachetuneDefFormatHelper,
> + &childrenBuf);
> +
> +
> + if (virBufferCheckError(&childrenBuf) < 0)
> + goto cleanup;
> +
> + if (!virBufferUse(&childrenBuf)) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + vcpus = virBitmapFormat(cachetune->vcpus);
> +
> + virBufferAsprintf(buf, "<cachetune vcpus='%s'>\n", vcpus);
> + virBufferAddBuffer(buf, &childrenBuf);
> + virBufferAddLit(buf, "</cachetune>\n");
> +
> + ret = 0;
> + cleanup:
> + virBufferFreeAndReset(&childrenBuf);
> + VIR_FREE(vcpus);
> + return ret;
> +}
> +
> +
The brevity of the format is something all formatting functions should
aspire to become!
> static int
> virDomainCputuneDefFormat(virBufferPtr buf,
> virDomainDefPtr def)
> @@ -25545,6 +25791,9 @@ virDomainCputuneDefFormat(virBufferPtr buf,
> def->iothreadids[i]->iothread_id);
> }
>
> + for (i = 0; i < def->ncachetunes; i++)
> + virDomainCachetuneDefFormat(&childrenBuf, def->cachetunes[i]);
> +
> if (virBufferCheckError(&childrenBuf) < 0)
> return -1;
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index be38792c6942..bda83d72e7ed 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -56,6 +56,7 @@
> # include "virperf.h"
> # include "virtypedparam.h"
> # include "virsavecookie.h"
> +# include "virresctrl.h"
>
> /* forward declarations of all device types, required by
> * virDomainDeviceDef
> @@ -2167,6 +2168,15 @@ struct _virDomainCputune {
> };
>
>
> +typedef struct _virDomainCachetuneDef virDomainCachetuneDef;
> +typedef virDomainCachetuneDef *virDomainCachetuneDefPtr;
> +
> +struct _virDomainCachetuneDef {
> + virBitmapPtr vcpus;
> + virResctrlAllocPtr alloc;
> +};
> +
> +
> typedef struct _virDomainVcpuDef virDomainVcpuDef;
> typedef virDomainVcpuDef *virDomainVcpuDefPtr;
>
> @@ -2294,6 +2304,17 @@ struct _virDomainDef {
> virDomainIOThreadIDDefPtr *iothreadids;
>
> virDomainCputune cputune;
> + /*
> + * Both cachetune as well as runtime allocation (the same structure, but
> + * updated) for all threads is kept here. It is specified here instead of
> + * specifying it in virDomainVcpuDef _deliberately_ because it needs to be
> + * set at once under one flock() and not for each thread separately. Also
> + * if one cachetune is specified for multiple vCPUs, it must be created as
> + * one due to limited number of concurrent cachetune settings (number of
> + * CLoS IDs) provided by hardware.
> + */
Well that's lengthy! Still that last sentence makes me wonder if that
"limited number" is known or is it random and thus if enough were
created there would be random failures...
> + virDomainCachetuneDefPtr *cachetunes;
> + size_t ncachetunes;
>
> virDomainNumaPtr numa;
> virDomainResourceDefPtr resource;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index ac1b38436bb2..8753b1dca325 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -693,7 +693,7 @@ virResctrlAllocSetID(virResctrlAllocPtr alloc,
> {
> if (!id) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Resctrl allocation id cannot be NULL"));
> + _("Resctrl allocation 'id' cannot be NULL"));
This probably should go in some earlier patch (if it's not already
pushed ;-)).
I think the fixup's are essentially simple. I'm fine with the naming
scheme - although maybe someone else who has been thinking about this
longer, but didn't jump in to review would have a different opinion ;-)
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
> return -1;
> }
>
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
