Re: [libvirt] [PATCH 06/10] conf: Add support for cputune/cachetune

On Tue, Jan 23, 2018 at 07:05:15PM +0100, Martin Kletzander wrote: > More info in the documentation, this is basically the XML parsing/formatting > support, schemas, tests and documentation for the new cputune/cachetune element > that will get used by following patches. > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 54 ++++ > docs/schemas/domaincommon.rng | 32 +++ > src/conf/domain_conf.c | 295 ++++++++++++++++++++- > src/conf/domain_conf.h | 13 + > tests/genericxml2xmlindata/cachetune-cdp.xml | 36 +++ > .../cachetune-colliding-allocs.xml | 30 +++ > .../cachetune-colliding-tunes.xml | 32 +++ > .../cachetune-colliding-types.xml | 30 +++ > tests/genericxml2xmlindata/cachetune-small.xml | 29 ++ > tests/genericxml2xmlindata/cachetune.xml | 33 +++ > tests/genericxml2xmltest.c | 10 + > 11 files changed, 592 insertions(+), 2 deletions(-) > create mode 100644 tests/genericxml2xmlindata/cachetune-cdp.xml > create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-allocs.xml > create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-tunes.xml > create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-types.xml > create mode 100644 tests/genericxml2xmlindata/cachetune-small.xml > create mode 100644 tests/genericxml2xmlindata/cachetune.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index d272cc1ba698..7b4d9051a551 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -689,6 +689,10 @@ > &lt;iothread_quota&gt;-1&lt;/iothread_quota&gt; > &lt;vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/&gt; > &lt;iothreadsched iothreads='2' scheduler='batch'/&gt; > + &lt;cachetune vcpus='0-3'&gt; > + &lt;cache id='0' level='3' type='both' size='3' unit='MiB'/&gt; > + &lt;cache id='1' level='3' type='both' size='3' unit='MiB'/&gt; > + &lt;/cachetune&gt; > &lt;/cputune&gt; > ... > &lt;/domain&gt; > @@ -834,6 +838,56 @@ > <span class="since">Since 1.2.13</span> > </dd> > > + <dt><code>cachetune</code><span class="since">Since 4.1.0</span></dt> > + <dd> > + Optional <code>cachetune</code> element can control allocations for CPU > + caches using the resctrl 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. The required s/ / / > + attribute <code>vcpus</code> specifies to which vCPUs this allocation > + applies. A vCPU can only be member of one <code>cachetune</code> element > + allocations. Supported subelements are: > + <dl> > + <dt><code>cache</code></dt> > + <dd> > + This element controls the allocation of CPU cache and has 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> > + Type of allocation. Can be <code>code</code> for code s/ / / > + (instructions), <code>data</code> for data or <code>both</code> > + for both code and data (unified). Currently the allocation can s/ / /

> + be done only with the same type as the host supports, meaning > + you cannot request <code>both</code> for host with CDP > + (code/data prioritization) enabled. > + </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> (optional)</dt> > + <dd> > + If specified it is the unit such as KiB, MiB, GiB, or TiB > + (described in the <code>memory</code> element > + for <a href="#elementsMemoryAllocation">Memory Allocation</a>) > + in which <code>size</code> is specified, defaults to bytes. > + </dd> > + </dl> > + </dd> > + </dl> > + > + </dd> > </dl> > > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index f22c932f6c09..252f58f4379c 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 a1c25060f9e9..27665d0372a7 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -2883,6 +2883,19 @@ 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; > @@ -3055,6 +3068,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) > @@ -18247,6 +18264,194 @@ 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 cachetune attribute 'id'")); > + goto cleanup; > + } > + if (virStrToLong_uip(tmp, NULL, 10, &cache) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid cachetune attribute 'id' value '%s'"), > + tmp); > + goto cleanup; > + } > + VIR_FREE(tmp); > + > + tmp = virXMLPropString(node, "level"); > + if (!tmp) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Missing cachetune attribute 'level'")); > + goto cleanup; > + } > + if (virStrToLong_uip(tmp, NULL, 10, &level) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid cachetune attribute 'level' value '%s'"), > + tmp); > + goto cleanup; > + } > + VIR_FREE(tmp); > + > + tmp = virXMLPropString(node, "type"); > + if (!tmp) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Missing cachetune attribute 'type'")); > + goto cleanup; > + } > + type = virCacheTypeFromString(tmp); > + if (type < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid cachetune attribute 'type' value '%s'"), > + 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; > +} > + > + > +static int > +virDomainCachetuneDefParse(virDomainDefPtr def, > + xmlXPathContextPtr ctxt, > + xmlNodePtr node, > + unsigned int flags) > +{ > + xmlNodePtr oldnode = ctxt->node; > + xmlNodePtr *nodes = NULL; > + virBitmapPtr vcpus = NULL; > + virResctrlAllocPtr alloc = virResctrlAllocNew(); > + virDomainCachetuneDefPtr tmp_cachetune = NULL; > + char *tmp = NULL; > + char *vcpus_str = NULL; > + char *alloc_id = NULL; > + ssize_t i = 0; > + int n; > + int ret = -1; > + > + ctxt->node = node; > + > + if (!alloc) > + goto cleanup; > + > + if (VIR_ALLOC(tmp_cachetune) < 0) > + goto cleanup; > + > + vcpus_str = virXMLPropString(node, "vcpus"); > + if (!vcpus_str) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Missing cachetune attribute 'vcpus'")); > + goto cleanup; > + } > + if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid cachetune attribute 'vcpus' value '%s'"), > + vcpus_str); > + goto cleanup; > + } > + > + /* We need to limit the bitmap to number of vCPUs. If there's nothing left, > + * then we can just clean up and return 0 immediately */ > + virBitmapShrink(vcpus, def->maxvcpus); > + if (virBitmapIsAllClear(vcpus)) { > + ret = 0; > + goto cleanup; > + } > + > + if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot extract cache nodes under cachetune")); > + goto cleanup; > + } > + > + for (i = 0; i < n; i++) { > + if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0) > + goto cleanup; > + } > + > + if (virResctrlAllocIsEmpty(alloc)) { > + ret = 0; > + goto cleanup; > + } Can this ever happen? The

> + > + 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; > + } > + } > + > + /* We need to format it back because we need to be consistent in the naming > + * even when users specify some "sub-optimal" string there. */ > + VIR_FREE(vcpus_str); > + vcpus_str = virBitmapFormat(vcpus); > + if (!vcpus_str) > + goto cleanup; > + > + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) > + alloc_id = virXMLPropString(node, "id"); > + > + if (!alloc_id) { > + /* The number of allocatios is limited and the directory structure is flat, s/allocatios/allocations/

Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt;