[libvirt] [PATCH RFC 1/2] Resctrl: Add new xml element to support cache tune
Martin Kletzander
mkletzan at redhat.com
Wed Jun 21 13:53:05 UTC 2017
On Mon, Jun 12, 2017 at 05:48:40PM +0800, Eli Qiao wrote:
>This patch adds new xml element to support cache tune as:
>
><cputune>
> ...
> <cachetune id='0' cache_id='0' level='3' type='both' size='2816' unit='KiB'
> vcpus='1,2'/>
The cache_id automatically implies level and type. Either have one or
the other. I know we talked about this already (maybe multiple times),
but without any clear outcome. For me the sensible thing is to have
level and type as that doesn't need to be changed when moving between
hosts, and if it cannot be migrated, then it's properly checked.
> ...
></cputune>
>
>id: any non-minus number
>cache_id: reference of the host's cache banks id, it's from capabilities
>level: cache level
>type: cache bank type
>size: should be multiples of the min_size of the bank on host.
must be multiple of granularity, must be greater than or equal to minimum.
>vcpus: cache allocation on vcpu set, if empty, will apply the allocation
>on all vcpus
>
>Signed-off-by: Eli Qiao <liyong.qiao at intel.com>
>---
> docs/schemas/domaincommon.rng | 54 +++++++++++++++++
> src/conf/domain_conf.c | 131 ++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 21 +++++++
> 3 files changed, 206 insertions(+)
>
>diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>index 4950ddc..11dbb55 100644
>--- a/docs/schemas/domaincommon.rng
>+++ b/docs/schemas/domaincommon.rng
>@@ -834,6 +834,35 @@
> </attribute>
> </element>
> </zeroOrMore>
>+ <zeroOrMore>
>+ <element name="cachetune">
>+ <attribute name="id">
>+ <ref name="cachetuneid"/>
>+ </attribute>
>+ <attribute name="cache_id">
>+ <ref name="cacheid"/>
>+ </attribute>
>+ <attribute name="level">
>+ <ref name="cachelevel"/>
>+ </attribute>
>+ <attribute name="type">
>+ <ref name="cachetype"/>
>+ </attribute>
>+ <attribute name="size">
>+ <ref name="unsignedInt"/>
>+ </attribute>
>+ <optional>
>+ <attribute name="unit">
>+ <ref name="cacheunit"/>
>+ </attribute>
>+ </optional>
>+ <optional>
>+ <attribute name="vcpus">
>+ <ref name="cpuset"/>
>+ </attribute>
>+ </optional>
>+ </element>
>+ </zeroOrMore>
> <optional>
> <element name="emulatorpin">
> <attribute name="cpuset">
>@@ -5686,6 +5715,31 @@
> <param name="minInclusive">-1</param>
> </data>
> </define>
>+ <define name="cachetuneid">
>+ <data type="unsignedShort">
>+ <param name="pattern">[0-9]+</param>
>+ </data>
>+ </define>
>+ <define name="cacheid">
>+ <data type="unsignedShort">
>+ <param name="pattern">[0-9]+</param>
>+ </data>
>+ </define>
These are useless ^^
>+ <define name="cachelevel">
>+ <data type="unsignedShort">
>+ <param name="pattern">(3)</param>
Either don't restrict it at all or do the following:
<choice>
<!-- For now we only support allocation of L3 caches -->
<value>3</value>
<choice>
>+ </data>
>+ </define>
>+ <define name="cachetype">
>+ <data type="string">
>+ <param name="pattern">(both|code|data)</param>
>+ </data>
>+ </define>
<choice>
<value>both</value>
<value>code</value>
<value>data</value>
</choice>
>+ <define name="cacheunit">
>+ <data type="string">
>+ <param name="pattern">KiB</param>
>+ </data>
>+ </define>
This doesn't have to be in KiB, otherwise the unit doesn't make sense.
For all the units I'm sure there already is a define in the sources
somewhere.
> <!-- weight currently is in range [100, 1000] -->
> <define name="weight">
> <data type="unsignedInt">
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 5c32f93..93984bc 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -16250,6 +16250,104 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
> return ret;
> }
>
>+/* Parse the XML definition for cachetune
>+ * and a cachetune has the form
>+ * <cachetune id='0' cache_id='0' level='3' type='both' size='1024' unit='KiB'/>
>+ */
>+static int
>+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
>+ int n,
>+ xmlNodePtr* nodes)
>+{
>+ char* tmp = NULL;
>+ size_t i;
>+ int type = -1;
>+ virDomainCacheBankPtr bank = NULL;
>+
>+ if (VIR_ALLOC_N(bank, n) < 0)
>+ goto cleanup;
>+
>+ for (i = 0; i < n; i++) {
>+ if (!(tmp = virXMLPropString(nodes[i], "id"))) {
>+ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing id in cache tune"));
>+ goto cleanup;
>+ }
>+ if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].id)) < 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("invalid setting for cache id '%s'"), tmp);
>+ goto cleanup;
>+ }
>+ VIR_FREE(tmp);
>+
>+ if (!(tmp = virXMLPropString(nodes[i], "cache_id"))) {
>+ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache id in cache tune"));
>+ goto cleanup;
>+ }
>+ if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].cache_id)) < 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("invalid setting for cache host id '%s'"), tmp);
>+ goto cleanup;
>+ }
>+ VIR_FREE(tmp);
>+
>+ if (!(tmp = virXMLPropString(nodes[i], "level"))) {
>+ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache level in cache tune"));
>+ goto cleanup;
>+ }
>+ if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].level)) < 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("invalid setting for cache level '%s'"), tmp);
>+ goto cleanup;
>+ }
>+ VIR_FREE(tmp);
>+
>+
>+ if (!(tmp = virXMLPropString(nodes[i], "size"))) {
>+ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing size in cache tune"));
>+ goto cleanup;
>+ }
>+ if (virStrToLong_ull(tmp, NULL, 10, &(bank[i].size)) < 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("invalid setting for cache size '%s'"), tmp);
Your error messages until now were almost consistent
>+ goto cleanup;
>+ }
>+ /* convert to B */
>+ bank[i].size *= 1024;
>+ VIR_FREE(tmp);
>+
>+ if (!(tmp = virXMLPropString(nodes[i], "type"))) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+ _("missing cache type"));
>+ goto cleanup;
>+ }
>+ if ((type = virCacheTypeFromString(tmp)) < 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("'unsupported cache type '%s'"), tmp);
But now it's different, plus s/'unsupp/unsupp/
>+ goto cleanup;
>+ }
>+
>+ if ((tmp = virXMLPropString(nodes[i], "vcpus"))) {
>+ if (virBitmapParse(tmp, &bank[i].vcpus,
>+ VIR_DOMAIN_CPUMASK_LEN) < 0)
This can be one line
>+ goto cleanup;
>+
>+ if (virBitmapIsAllClear(bank[i].vcpus)) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+ _("Invalid value of 'vcpus': %s"), tmp);
And now there's a colon, apostrophes around 'vcpus', but tmp can be
empty string and that will not be very visible...
>+ goto cleanup;
>+ }
>+ }
>+ }
>+
>+ def->cachetune.cache_banks = bank;
>+ def->cachetune.n_banks = n;
>+ return 0;
>+
>+ cleanup:
>+ VIR_FREE(bank);
>+ VIR_FREE(tmp);
>+ return -1;
>+}
>
> /* Parse the XML definition for a iothreadpin
> * and an iothreadspin has the form
>@@ -17538,6 +17636,14 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
> VIR_FREE(nodes);
>
>+ if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0)
>+ goto error;
>+
>+ if (virDomainCacheTuneDefParseXML(def, n, nodes) < 0)
You are calling it even when n == 0
>+ goto error;
>+
>+ VIR_FREE(nodes);
>+
> if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot extract emulatorpin nodes"));
>@@ -24328,6 +24434,29 @@ virDomainSchedulerFormat(virBufferPtr buf,
> }
>
>
>+static void
>+virDomainCacheTuneDefFormat(virBufferPtr buf,
>+ virDomainCachetunePtr cache)
>+{
>+ size_t i;
Missing empty line
>+ for (i = 0; i < cache->n_banks; i ++) {
>+ virBufferAsprintf(buf, "<cachetune id='%u' cache_id='%u' "
>+ "level='%u' type='%s' size='%llu' unit='KiB'",
>+ cache->cache_banks[i].id,
>+ cache->cache_banks[i].cache_id,
>+ cache->cache_banks[i].level,
>+ virCacheTypeToString(cache->cache_banks[i].type),
>+ cache->cache_banks[i].size / 1024);
>+
>+ if (cache->cache_banks[i].vcpus)
>+ virBufferAsprintf(buf, " vcpus='%s'/>\n",
>+ virBitmapFormat(cache->cache_banks[i].vcpus));
>+ else
>+ virBufferAddLit(buf, "/>\n");
>+ }
>+}
>+
>+
> static int
> virDomainCputuneDefFormat(virBufferPtr buf,
> virDomainDefPtr def)
>@@ -24374,6 +24503,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
> "</iothread_quota>\n",
> def->cputune.iothread_quota);
>
>+ virDomainCacheTuneDefFormat(&childrenBuf, &def->cachetune);
>+
> for (i = 0; i < def->maxvcpus; i++) {
> char *cpumask;
> virDomainVcpuDefPtr vcpu = def->vcpus[i];
>diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>index e67b6fd..0a3566a 100644
>--- a/src/conf/domain_conf.h
>+++ b/src/conf/domain_conf.h
>@@ -2190,6 +2190,25 @@ struct _virDomainMemtune {
> int allocation; /* enum virDomainMemoryAllocation */
> };
>
>+typedef struct _virDomainCacheBank virDomainCacheBank;
>+typedef virDomainCacheBank *virDomainCacheBankPtr;
>+
>+struct _virDomainCacheBank {
>+ unsigned int id;
>+ unsigned int cache_id; /* cache id to be allocated on */
>+ unsigned int level; /* cache level */
>+ int type; /* enum virCacheType */
>+ unsigned long long size; /* in kibibytes */
>+ virBitmapPtr vcpus;
>+};
>+
>+typedef struct _virDomainCachetune virDomainCachetune;
>+typedef virDomainCachetune *virDomainCachetunePtr;
>+struct _virDomainCachetune {
>+ size_t n_banks;
>+ virDomainCacheBankPtr cache_banks;
>+};
>+
I don't think there's a real need for this intermediate structure, but
since you want to be passing it around everywhere...
> typedef struct _virDomainPowerManagement virDomainPowerManagement;
> typedef virDomainPowerManagement *virDomainPowerManagementPtr;
>
>@@ -2263,6 +2282,8 @@ struct _virDomainDef {
>
> virDomainCputune cputune;
>
>+ virDomainCachetune cachetune;
>+
It is part of cputune in the XML, why not here?
> virDomainNumaPtr numa;
> virDomainResourceDefPtr resource;
> virDomainIdMapDef idmap;
>--
>1.9.1
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170621/e73af934/attachment-0001.sig>
More information about the libvir-list
mailing list