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

On Wed, Jan 24, 2018 at 03:04:37PM +0100, Pavel Hrdina wrote: > 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/ / / > Fixed all three. /me wonders why people still insist on this. Is someone editing these files with proportional font? Do we have any other output than HTML? I guess this will be yet another thing I have to get used to and surrender.

> > + 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 > Sure, for example: <cachetune/>

> > + > > + 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/ > Fixed. > Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt;