This patch adds new xml element to support cache tune as:
<cputune>
...
<cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB'
vcpus='1,2'/>
...
</cputune>
id: any non-minus number
host_id: reference of the host's cache banks id, it's from capabilities
type: cache bank type
size: should be multiples of the min_size of the bank on host.
vcpus: cache allocation on vcpu set, if empty, will apply the allocation
on all vcpus
---
docs/schemas/domaincommon.rng | 46 +++++++++++++
src/conf/domain_conf.c | 152 ++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 19 ++++++
src/util/virresctrl.c | 29 ++++++--
src/util/virresctrl.h | 4 +-
5 files changed, 244 insertions(+), 6 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index c64544a..efc84c5 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -825,6 +825,32 @@
</attribute>
</element>
</zeroOrMore>
+ <zeroOrMore>
+ <element name="cachetune">
+ <attribute name="id">
+ <ref name="cacheid"/>
+ </attribute>
+ <attribute name="host_id">
+ <ref name="hostid"/>
+ </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">
@@ -5490,6 +5516,26 @@
<param name="minInclusive">-1</param>
</data>
</define>
+ <define name="cacheid">
+ <data type="unsignedShort">
+ <param name="pattern">[0-9]+</param>
+ </data>
+ </define>
+ <define name="hostid">
+ <data type="unsignedShort">
+ <param name="pattern">[0-9]+</param>
+ </data>
+ </define>
+ <define name="cachetype">
+ <data type="string">
+ <param name="pattern">(l3)</param>
+ </data>
+ </define>
+ <define name="cacheunit">
+ <data type="string">
+ <param name="pattern">KiB</param>
+ </data>
+ </define>
<!-- 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 97d42fe..652f4ca 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -56,6 +56,7 @@
#include "virstring.h"
#include "virnetdev.h"
#include "virhostdev.h"
+#include "virresctrl.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -15745,6 +15746,127 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
return ret;
}
+/* Parse the XML definition for cachetune
+ * and a cachetune has the form
+ * <cachetune id='0' host_id='0' type='l3' size='1024' unit='KiB'/>
+ */
+static int
+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
+ int n,
+ xmlNodePtr* nodes)
+{
+ char* tmp = NULL;
+ size_t i, j;
+ int type = -1;
+ virDomainCacheBankPtr bank = NULL;
+ virResCtrlPtr resctrl;
+
+ 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], "host_id"))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing host id in cache tune"));
+ goto cleanup;
+ }
+ if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].host_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], "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);
+ goto cleanup;
+ }
+ VIR_FREE(tmp);
+
+ if (!(tmp = virXMLPropString(nodes[i], "type"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing cache type"));
+ goto cleanup;
+ }
+
+ if ((type = virResCtrlTypeFromString(tmp)) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("'unsupported cache type '%s'"), tmp);
+ goto cleanup;
+ }
+
+ resctrl = virResCtrlGet(type);
+
+ if (resctrl == NULL || !resctrl->enabled) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("'host doesn't enabled cache type '%s'"), tmp);
+ goto cleanup;
+ }
+
+ bool found_host_id = false;
+ /* Loop for banks to search host_id */
+ for (j = 0; j < resctrl->num_banks; j++) {
+ if (resctrl->cache_banks[j].host_id == bank[i].host_id) {
+ found_host_id = true;
+ break;
+ }
+ }
+
+ if (! found_host_id) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("'cache bank's host id %u not found on the host"),
+ bank[i].host_id);
+ goto cleanup;
+ }
+
+ if (bank[i].size == 0 ||
+ bank[i].size % resctrl->cache_banks[j].cache_min != 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("'the size should be multiples of '%llu'"),
+ resctrl->cache_banks[j].cache_min);
+ goto cleanup;
+ }
+
+ if (VIR_STRDUP(bank[i].type, tmp) < 0)
+ goto cleanup;
+
+ if ((tmp = virXMLPropString(nodes[i], "vcpus"))) {
+ if (virBitmapParse(tmp, &bank[i].vcpus,
+ VIR_DOMAIN_CPUMASK_LEN) < 0)
+ goto cleanup;
+
+ if (virBitmapIsAllClear(bank[i].vcpus)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'vcpus': %s"), tmp);
+ 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
@@ -17033,6 +17155,14 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
+ if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0)
+ goto error;
+
+ if (virDomainCacheTuneDefParseXML(def, n, nodes) < 0)
+ goto error;
+
+ VIR_FREE(nodes);
+
if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot extract emulatorpin nodes"));
@@ -23554,6 +23684,26 @@ virDomainSchedulerFormat(virBufferPtr buf,
}
+static void
+virDomainCacheTuneDefFormat(virBufferPtr buf,
+ virDomainCachetunePtr cache)
+{
+ size_t i;
+ for (i = 0; i < cache->n_banks; i ++) {
+ virBufferAsprintf(buf, "<cachetune id='%u' host_id='%u' "
+ "type='%s' size='%llu' unit='KiB'",
+ cache->cache_banks[i].id,
+ cache->cache_banks[i].host_id,
+ cache->cache_banks[i].type,
+ cache->cache_banks[i].size);
+
+ 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,
@@ -23617,6 +23767,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
VIR_FREE(cpumask);
}
+ virDomainCacheTuneDefFormat(&childrenBuf, &def->cachetune);
+
if (def->cputune.emulatorpin) {
char *cpumask;
virBufferAddLit(&childrenBuf, "<emulatorpin ");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1e53cc3..3da5d61 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2153,6 +2153,23 @@ struct _virDomainMemtune {
int allocation; /* enum virDomainMemoryAllocation */
};
+typedef struct _virDomainCacheBank virDomainCacheBank;
+typedef virDomainCacheBank *virDomainCacheBankPtr;
+struct _virDomainCacheBank {
+ unsigned int id;
+ unsigned int host_id;
+ unsigned long long size;
+ char *type;
+ virBitmapPtr vcpus;
+};
+
+typedef struct _virDomainCachetune virDomainCachetune;
+typedef virDomainCachetune *virDomainCachetunePtr;
+struct _virDomainCachetune {
+ size_t n_banks;
+ virDomainCacheBankPtr cache_banks;
+};
+
typedef struct _virDomainPowerManagement virDomainPowerManagement;
typedef virDomainPowerManagement *virDomainPowerManagementPtr;
@@ -2216,6 +2233,8 @@ struct _virDomainDef {
virDomainCputune cputune;
+ virDomainCachetune cachetune;
+
virDomainNumaPtr numa;
virDomainResourceDefPtr resource;
virDomainIdMapDef idmap;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 44a47cc..eee6675 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -32,15 +32,34 @@
VIR_LOG_INIT("util.resctrl");
#define VIR_FROM_THIS VIR_FROM_RESCTRL
-
-#define RESCTRL_DIR "/sys/fs/resctrl"
-#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
-#define SYSFS_SYSTEM_PATH "/sys/devices/system"
-
#define MAX_CPU_SOCKET_NUM 8
#define MAX_CBM_BIT_LEN 32
#define MAX_SCHEMATA_LEN 1024
#define MAX_FILE_LEN (10 * 1024 * 1024)
+#define RESCTRL_DIR "/sys/fs/resctrl"
+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
+VIR_ENUM_IMPL(virResCtrl, VIR_RDT_RESOURCE_LAST,
+ "l3", "l3data", "l3code", "l2");
+
+#define CONSTRUCT_RESCTRL_PATH(domain_name, item_name) \
+do { \
+ if (NULL == domain_name) { \
+ if (virAsprintf(&path, "%s/%s", RESCTRL_DIR, item_name) < 0)\
+ return -1; \
+ } else { \
+ if (virAsprintf(&path, "%s/%s/%s", RESCTRL_DIR, \
+ domain_name, \
+ item_name) < 0) \
+ return -1; \
+ } \
+} while (0)
+
+#define VIR_RESCTRL_ENABLED(type) \
+ resctrlall[type].enabled
+
+#define VIR_RESCTRL_GET_SCHEMATA(count) ((1 << count) - 1)
static unsigned int host_id;
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 5a6a344..074d307 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -25,6 +25,7 @@
# define __VIR_RESCTRL_H__
# include "virbitmap.h"
+# include "virutil.h"
enum {
VIR_RDT_RESOURCE_L3,
@@ -32,9 +33,10 @@ enum {
VIR_RDT_RESOURCE_L3CODE,
VIR_RDT_RESOURCE_L2,
/* Must be the last */
- VIR_RDT_RESOURCE_LAST,
+ VIR_RDT_RESOURCE_LAST
};
+VIR_ENUM_DECL(virResCtrl);
typedef struct _virResCacheBank virResCacheBank;
typedef virResCacheBank *virResCacheBankPtr;
--
1.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote:
>This patch adds new xml element to support cache tune as:
>
><cputune>
> ...
> <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB'
Again, this was already discussed, probably, I just can't find the
source of it. But host_id actually already selects what cache is
supposed to be used, so instead of type='l3' we only need scope='both'
(or data/instruction) there. Or am I missing something? What I'm
concerned about is that the host_id is mostly stable on one host (when
the previously mentioned problems are fixed), but it will make no sense
when the VM is migrated to another one. Unfortunately, the only
solution I can think of is using multiple keys to precisely describe the
bank we want (e.g. host's cpu id, cache level and scope), but that seems
very unclean.
> vcpus='1,2'/>
> ...
></cputune>
>
>id: any non-minus number
>host_id: reference of the host's cache banks id, it's from capabilities
>type: cache bank type
>size: should be multiples of the min_size of the bank on host.
>vcpus: cache allocation on vcpu set, if empty, will apply the allocation
>on all vcpus
>---
> docs/schemas/domaincommon.rng | 46 +++++++++++++
> src/conf/domain_conf.c | 152 ++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 19 ++++++
> src/util/virresctrl.c | 29 ++++++--
> src/util/virresctrl.h | 4 +-
> 5 files changed, 244 insertions(+), 6 deletions(-)
>
>diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>index c64544a..efc84c5 100644
>--- a/docs/schemas/domaincommon.rng
>+++ b/docs/schemas/domaincommon.rng
>@@ -825,6 +825,32 @@
> </attribute>
> </element>
> </zeroOrMore>
>+ <zeroOrMore>
>+ <element name="cachetune">
>+ <attribute name="id">
>+ <ref name="cacheid"/>
>+ </attribute>
this should be optional when defining a domain, we can fill in the ids
after (or during) parsing.
>+ <attribute name="host_id">
>+ <ref name="hostid"/>
>+ </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">
>@@ -5490,6 +5516,26 @@
> <param name="minInclusive">-1</param>
> </data>
> </define>
>+ <define name="cacheid">
>+ <data type="unsignedShort">
>+ <param name="pattern">[0-9]+</param>
unsignedShort doesn't need pattern, wherever you copied that from, it
should be cleaned up there as well (of course not needed to be part of
this series). I'll include such things with other silly one-line fixes
in the series so that we get rid of it.
>+ </data>
>+ </define>
>+ <define name="hostid">
>+ <data type="unsignedShort">
>+ <param name="pattern">[0-9]+</param>
>+ </data>
>+ </define>
>+ <define name="cachetype">
>+ <data type="string">
>+ <param name="pattern">(l3)</param>
>+ </data>
What's the difference to <value>l3</value> ???
>+ </define>
>+ <define name="cacheunit">
>+ <data type="string">
>+ <param name="pattern">KiB</param>
>+ </data>
>+ </define>
> <!-- 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 97d42fe..652f4ca 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -56,6 +56,7 @@
> #include "virstring.h"
> #include "virnetdev.h"
> #include "virhostdev.h"
>+#include "virresctrl.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>
>@@ -15745,6 +15746,127 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
> return ret;
> }
>
>+/* Parse the XML definition for cachetune
>+ * and a cachetune has the form
>+ * <cachetune id='0' host_id='0' type='l3' size='1024' unit='KiB'/>
>+ */
>+static int
>+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
>+ int n,
>+ xmlNodePtr* nodes)
>+{
>+ char* tmp = NULL;
>+ size_t i, j;
>+ int type = -1;
>+ virDomainCacheBankPtr bank = NULL;
>+ virResCtrlPtr resctrl;
>+
>+ 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;
>+ }
>+
As said before, ids can be calculated later, we don't require
referencing them in the user's provided XML anywhere, or do we?
>+ VIR_FREE(tmp);
>+ if (!(tmp = virXMLPropString(nodes[i], "host_id"))) {
>+ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing host id in cache tune"));
>+ goto cleanup;
>+ }
>+ if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].host_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], "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);
>+ goto cleanup;
>+ }
>+ VIR_FREE(tmp);
>+
>+ if (!(tmp = virXMLPropString(nodes[i], "type"))) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+ _("missing cache type"));
>+ goto cleanup;
>+ }
>+
>+ if ((type = virResCtrlTypeFromString(tmp)) < 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("'unsupported cache type '%s'"), tmp);
>+ goto cleanup;
>+ }
>+
>+ resctrl = virResCtrlGet(type);
>+
>+ if (resctrl == NULL || !resctrl->enabled) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("'host doesn't enabled cache type '%s'"), tmp);
s/'host/host/, s/enabled/support/??
This should be in PostParse callback, though. Or even better in
qemuProcessStartValidate() so that the domain doesn't disappear when the
host changes kernels or kernel config or anything else.
>+ goto cleanup;
>+ }
>+
>+ bool found_host_id = false;
>+ /* Loop for banks to search host_id */
>+ for (j = 0; j < resctrl->num_banks; j++) {
>+ if (resctrl->cache_banks[j].host_id == bank[i].host_id) {
>+ found_host_id = true;
>+ break;
>+ }
>+ }
>+
>+ if (! found_host_id) {
no spaces after exclamation mark, please
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("'cache bank's host id %u not found on the host"),
>+ bank[i].host_id);
Indentation, ...
>+ goto cleanup;
>+ }
>+
>+ if (bank[i].size == 0 ||
>+ bank[i].size % resctrl->cache_banks[j].cache_min != 0) {
>+ virReportError(VIR_ERR_XML_ERROR,
>+ _("'the size should be multiples of '%llu'"),
>+ resctrl->cache_banks[j].cache_min);
... indentation everywhere!
>@@ -23554,6 +23684,26 @@ virDomainSchedulerFormat(virBufferPtr buf,
>
> }
>
>+static void
>+virDomainCacheTuneDefFormat(virBufferPtr buf,
>+ virDomainCachetunePtr cache)
>+{
>+ size_t i;
>+ for (i = 0; i < cache->n_banks; i ++) {
>+ virBufferAsprintf(buf, "<cachetune id='%u' host_id='%u' "
>+ "type='%s' size='%llu' unit='KiB'",
>+ cache->cache_banks[i].id,
>+ cache->cache_banks[i].host_id,
>+ cache->cache_banks[i].type,
>+ cache->cache_banks[i].size);
>+
>+ 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,
>@@ -23617,6 +23767,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
> VIR_FREE(cpumask);
> }
>
>+ virDomainCacheTuneDefFormat(&childrenBuf, &def->cachetune);
>+
> if (def->cputune.emulatorpin) {
> char *cpumask;
> virBufferAddLit(&childrenBuf, "<emulatorpin ");
>diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>index 1e53cc3..3da5d61 100644
>--- a/src/conf/domain_conf.h
>+++ b/src/conf/domain_conf.h
>@@ -2153,6 +2153,23 @@ struct _virDomainMemtune {
> int allocation; /* enum virDomainMemoryAllocation */
> };
>
>+typedef struct _virDomainCacheBank virDomainCacheBank;
>+typedef virDomainCacheBank *virDomainCacheBankPtr;
>+struct _virDomainCacheBank {
>+ unsigned int id;
>+ unsigned int host_id;
>+ unsigned long long size;
>+ char *type;
>+ virBitmapPtr vcpus;
>+};
>+
>+typedef struct _virDomainCachetune virDomainCachetune;
>+typedef virDomainCachetune *virDomainCachetunePtr;
>+struct _virDomainCachetune {
>+ size_t n_banks;
>+ virDomainCacheBankPtr cache_banks;
>+};
>+
> typedef struct _virDomainPowerManagement virDomainPowerManagement;
> typedef virDomainPowerManagement *virDomainPowerManagementPtr;
>
>@@ -2216,6 +2233,8 @@ struct _virDomainDef {
>
> virDomainCputune cputune;
>
>+ virDomainCachetune cachetune;
>+
> virDomainNumaPtr numa;
> virDomainResourceDefPtr resource;
> virDomainIdMapDef idmap;
>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>index 44a47cc..eee6675 100644
>--- a/src/util/virresctrl.c
>+++ b/src/util/virresctrl.c
>@@ -32,15 +32,34 @@
> VIR_LOG_INIT("util.resctrl");
>
> #define VIR_FROM_THIS VIR_FROM_RESCTRL
>-
>-#define RESCTRL_DIR "/sys/fs/resctrl"
>-#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
>-#define SYSFS_SYSTEM_PATH "/sys/devices/system"
>-
> #define MAX_CPU_SOCKET_NUM 8
> #define MAX_CBM_BIT_LEN 32
> #define MAX_SCHEMATA_LEN 1024
> #define MAX_FILE_LEN (10 * 1024 * 1024)
>+#define RESCTRL_DIR "/sys/fs/resctrl"
>+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
>+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
>+
your diffs are weird, why does it show like this?
>+VIR_ENUM_IMPL(virResCtrl, VIR_RDT_RESOURCE_LAST,
>+ "l3", "l3data", "l3code", "l2");
>+
>+#define CONSTRUCT_RESCTRL_PATH(domain_name, item_name) \
>+do { \
>+ if (NULL == domain_name) { \
>+ if (virAsprintf(&path, "%s/%s", RESCTRL_DIR, item_name) < 0)\
>+ return -1; \
>+ } else { \
>+ if (virAsprintf(&path, "%s/%s/%s", RESCTRL_DIR, \
>+ domain_name, \
>+ item_name) < 0) \
>+ return -1; \
>+ } \
For macros like this, it's more readable when the backslashes are
aligned (your editor should be able to do that pretty automatically)
But for such function it's not very nice to use macro. Just use
function (if needed after reworking the patches).
Also you'r enot using the macro here, but in later patch, so it doesn't
make sense to add it in this commit. Each commit should be somehow one
isolated change with all things in one commit somehow related to each
other.
>+} while (0)
>+
>+#define VIR_RESCTRL_ENABLED(type) \
>+ resctrlall[type].enabled
>+
>+#define VIR_RESCTRL_GET_SCHEMATA(count) ((1 << count) - 1)
>
I thought we have BITS or something similar for this, but I can't find
it, but BITS feels more descriptive.
> static unsigned int host_id;
>
>diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
>index 5a6a344..074d307 100644
>--- a/src/util/virresctrl.h
>+++ b/src/util/virresctrl.h
>@@ -25,6 +25,7 @@
> # define __VIR_RESCTRL_H__
>
> # include "virbitmap.h"
>+# include "virutil.h"
>
> enum {
> VIR_RDT_RESOURCE_L3,
>@@ -32,9 +33,10 @@ enum {
> VIR_RDT_RESOURCE_L3CODE,
> VIR_RDT_RESOURCE_L2,
> /* Must be the last */
>- VIR_RDT_RESOURCE_LAST,
>+ VIR_RDT_RESOURCE_LAST
What's the point of this being in this commit?
> };
>
>+VIR_ENUM_DECL(virResCtrl);
>
> typedef struct _virResCacheBank virResCacheBank;
> typedef virResCacheBank *virResCacheBankPtr;
>--
>1.9.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote: > On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote: > > This patch adds new xml element to support cache tune as: > > > > <cputune> > > ... > > <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB' > > Again, this was already discussed, probably, I just can't find the > source of it. But host_id actually already selects what cache is > supposed to be used, so instead of type='l3' we only need scope='both' > (or data/instruction) there. Or am I missing something? What I'm > concerned about is that the host_id is mostly stable on one host (when > the previously mentioned problems are fixed), but it will make no sense > when the VM is migrated to another one. Unfortunately, the only > solution I can think of is using multiple keys to precisely describe the > bank we want (e.g. host's cpu id, cache level and scope), but that seems > very unclean. I tend to view use of this cachetune setting as being similar to using host CPU passthrough - you're intentionally trading off migratability of your guest to get a perf boost. Even without the host_id bit, this is still non-portable, as you might be requesting separate regions for code + data, but the target host of migration may only support shared regions. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 15, 2017 at 02:23:26PM +0000, Daniel P. Berrange wrote: >On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote: >> On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote: >> > This patch adds new xml element to support cache tune as: >> > >> > <cputune> >> > ... >> > <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB' >> >> Again, this was already discussed, probably, I just can't find the >> source of it. But host_id actually already selects what cache is >> supposed to be used, so instead of type='l3' we only need scope='both' >> (or data/instruction) there. Or am I missing something? What I'm >> concerned about is that the host_id is mostly stable on one host (when >> the previously mentioned problems are fixed), but it will make no sense >> when the VM is migrated to another one. Unfortunately, the only >> solution I can think of is using multiple keys to precisely describe the >> bank we want (e.g. host's cpu id, cache level and scope), but that seems >> very unclean. > >I tend to view use of this cachetune setting as being similar to >using host CPU passthrough - you're intentionally trading off >migratability of your guest to get a perf boost. > >Even without the host_id bit, this is still non-portable, as you >might be requesting separate regions for code + data, but the >target host of migration may only support shared regions. > Sure, but those are things we can check during migration. I'd be OK with disabling migration (or making it --unsafe) when migrating with cachetune. >Regards, >Daniel >-- >|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| >|: http://libvirt.org -o- http://virt-manager.org :| >|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| > >-- >libvir-list mailing list >libvir-list@redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 15, 2017 at 03:59:31PM +0100, Martin Kletzander wrote: > On Wed, Mar 15, 2017 at 02:23:26PM +0000, Daniel P. Berrange wrote: > >On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote: > >>On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote: > >>> This patch adds new xml element to support cache tune as: > >>> > >>> <cputune> > >>> ... > >>> <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB' > >> > >>Again, this was already discussed, probably, I just can't find the > >>source of it. But host_id actually already selects what cache is > >>supposed to be used, so instead of type='l3' we only need scope='both' > >>(or data/instruction) there. Or am I missing something? What I'm > >>concerned about is that the host_id is mostly stable on one host (when > >>the previously mentioned problems are fixed), but it will make no sense > >>when the VM is migrated to another one. This is the same conditions as pinning a vcpu to a pcpu. So yes, it might be that you want to migrate to a host where a different "host ID" number is used (which might or might not be a different socket). > Unfortunately, the only > >>solution I can think of is using multiple keys to precisely describe the > >>bank we want (e.g. host's cpu id, cache level and scope), but that seems > >>very unclean. > > > >I tend to view use of this cachetune setting as being similar to > >using host CPU passthrough - you're intentionally trading off > >migratability of your guest to get a perf boost. > > > >Even without the host_id bit, this is still non-portable, as you > >might be requesting separate regions for code + data, but the > >target host of migration may only support shared regions. Then migration should fail. > Sure, but those are things we can check during migration. I'd be OK > with disabling migration (or making it --unsafe) when migrating with > cachetune. Migration support is required (for NFV usecase they want to migrate VMs around). -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 15, 2017 at 02:11:23PM -0300, Marcelo Tosatti wrote: >On Wed, Mar 15, 2017 at 03:59:31PM +0100, Martin Kletzander wrote: >> On Wed, Mar 15, 2017 at 02:23:26PM +0000, Daniel P. Berrange wrote: >> >On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote: >> >>On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote: >> >>> This patch adds new xml element to support cache tune as: >> >>> >> >>> <cputune> >> >>> ... >> >>> <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB' >> >> >> >>Again, this was already discussed, probably, I just can't find the >> >>source of it. But host_id actually already selects what cache is >> >>supposed to be used, so instead of type='l3' we only need scope='both' >> >>(or data/instruction) there. Or am I missing something? What I'm >> >>concerned about is that the host_id is mostly stable on one host (when >> >>the previously mentioned problems are fixed), but it will make no sense >> >>when the VM is migrated to another one. > >This is the same conditions as pinning a vcpu to a pcpu. > >So yes, it might be that you want to migrate to a host where >a different "host ID" number is used (which might or might not >be a different socket). > >> Unfortunately, the only >> >>solution I can think of is using multiple keys to precisely describe the >> >>bank we want (e.g. host's cpu id, cache level and scope), but that seems >> >>very unclean. >> > >> >I tend to view use of this cachetune setting as being similar to >> >using host CPU passthrough - you're intentionally trading off >> >migratability of your guest to get a perf boost. >> > >> >Even without the host_id bit, this is still non-portable, as you >> >might be requesting separate regions for code + data, but the >> >target host of migration may only support shared regions. > >Then migration should fail. > Yes, it will automatically fail when you can't do the same allocations, that's easy. The difference with host_id is that you cannot foresee whether keeping the host_id makes sense on the destination as well. >> Sure, but those are things we can check during migration. I'd be OK >> with disabling migration (or making it --unsafe) when migrating with >> cachetune. > >Migration support is required (for NFV usecase they want to migrate >VMs around). > > So we at least need a check for the fact that either the caches are the somehow same or destination XML is supplied. The other option would be what I hinted above, that is to change host_id= to something like level, scope, and pcpu(s)/socket(s). That is fairly easy change from the code point of view, but I'm trying to think more about the user experience. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 15, 2017 at 08:49:57PM +0100, Martin Kletzander wrote:
> On Wed, Mar 15, 2017 at 02:11:23PM -0300, Marcelo Tosatti wrote:
> >On Wed, Mar 15, 2017 at 03:59:31PM +0100, Martin Kletzander wrote:
> >>On Wed, Mar 15, 2017 at 02:23:26PM +0000, Daniel P. Berrange wrote:
> >>>On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote:
> >>>>On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote:
> >>>>> This patch adds new xml element to support cache tune as:
> >>>>>
> >>>>> <cputune>
> >>>>> ...
> >>>>> <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB'
> >>>>
> >>>>Again, this was already discussed, probably, I just can't find the
> >>>>source of it. But host_id actually already selects what cache is
> >>>>supposed to be used, so instead of type='l3' we only need scope='both'
> >>>>(or data/instruction) there. Or am I missing something? What I'm
> >>>>concerned about is that the host_id is mostly stable on one host (when
> >>>>the previously mentioned problems are fixed), but it will make no sense
> >>>>when the VM is migrated to another one.
> >
> >This is the same conditions as pinning a vcpu to a pcpu.
> >
> >So yes, it might be that you want to migrate to a host where
> >a different "host ID" number is used (which might or might not
> >be a different socket).
> >
> >> Unfortunately, the only
> >>>>solution I can think of is using multiple keys to precisely describe the
> >>>>bank we want (e.g. host's cpu id, cache level and scope), but that seems
> >>>>very unclean.
> >>>
> >>>I tend to view use of this cachetune setting as being similar to
> >>>using host CPU passthrough - you're intentionally trading off
> >>>migratability of your guest to get a perf boost.
> >>>
> >>>Even without the host_id bit, this is still non-portable, as you
> >>>might be requesting separate regions for code + data, but the
> >>>target host of migration may only support shared regions.
> >
> >Then migration should fail.
> >
>
> Yes, it will automatically fail when you can't do the same allocations,
> that's easy. The difference with host_id is that you cannot foresee
> whether keeping the host_id makes sense on the destination as well.
But on the destination, the user can use a different vcpu<->pcpu
configuration, right?
That is, you do not enforce that if the source has say vcpu0 <-> pcpu2,
the destination must have vcpu0 <-> pcpu2 (the configuration for that
part is different).
So, the same thing can be applied to cache configuration.
> >>Sure, but those are things we can check during migration. I'd be OK
> >>with disabling migration (or making it --unsafe) when migrating with
> >>cachetune.
> >
> >Migration support is required (for NFV usecase they want to migrate
> >VMs around).
> >
> >
>
> So we at least need a check for the fact that either the caches are the
> somehow same or destination XML is supplied.
>
> The other option would be what I hinted above, that is to change
> host_id= to something like level, scope, and pcpu(s)/socket(s).
host_id maps to L3 socket via:
commit d57e3ab7e34c51a8badeea1b500bfb738d0af66e
Author: Fenghua Yu <fenghua.yu@intel.com>
Date: Sat Oct 22 06:19:50 2016 -0700
x86/intel_cacheinfo: Enable cache id in cache info
Cache id is retrieved from APIC ID and CPUID leaf 4 on x86.
For more details please see the section on "Cache ID Extraction
Parameters" in "Intel 64 Architecture Processor Topology
Enumeration".
Also the documentation of the CPUID instruction in the "Intel 64 and
IA-32 Architectures Software Developer's Manual"
So on the destination the user has to configure host_id= of the PCPU
which the guest is pinned to.
> That is
> fairly easy change from the code point of view, but I'm trying to think
> more about the user experience.
>
> Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 15, 2017 at 05:48:09PM -0300, Marcelo Tosatti wrote: >On Wed, Mar 15, 2017 at 08:49:57PM +0100, Martin Kletzander wrote: >> On Wed, Mar 15, 2017 at 02:11:23PM -0300, Marcelo Tosatti wrote: >> >On Wed, Mar 15, 2017 at 03:59:31PM +0100, Martin Kletzander wrote: >> >>On Wed, Mar 15, 2017 at 02:23:26PM +0000, Daniel P. Berrange wrote: >> >>>On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote: >> >>>>On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote: >> >>>>> This patch adds new xml element to support cache tune as: >> >>>>> >> >>>>> <cputune> >> >>>>> ... >> >>>>> <cachetune id='1' host_id='0' type='l3' size='2816' unit='KiB' >> >>>> >> >>>>Again, this was already discussed, probably, I just can't find the >> >>>>source of it. But host_id actually already selects what cache is >> >>>>supposed to be used, so instead of type='l3' we only need scope='both' >> >>>>(or data/instruction) there. Or am I missing something? What I'm >> >>>>concerned about is that the host_id is mostly stable on one host (when >> >>>>the previously mentioned problems are fixed), but it will make no sense >> >>>>when the VM is migrated to another one. >> > >> >This is the same conditions as pinning a vcpu to a pcpu. >> > >> >So yes, it might be that you want to migrate to a host where >> >a different "host ID" number is used (which might or might not >> >be a different socket). >> > >> >> Unfortunately, the only >> >>>>solution I can think of is using multiple keys to precisely describe the >> >>>>bank we want (e.g. host's cpu id, cache level and scope), but that seems >> >>>>very unclean. >> >>> >> >>>I tend to view use of this cachetune setting as being similar to >> >>>using host CPU passthrough - you're intentionally trading off >> >>>migratability of your guest to get a perf boost. >> >>> >> >>>Even without the host_id bit, this is still non-portable, as you >> >>>might be requesting separate regions for code + data, but the >> >>>target host of migration may only support shared regions. >> > >> >Then migration should fail. >> > >> >> Yes, it will automatically fail when you can't do the same allocations, >> that's easy. The difference with host_id is that you cannot foresee >> whether keeping the host_id makes sense on the destination as well. > >But on the destination, the user can use a different vcpu<->pcpu >configuration, right? > >That is, you do not enforce that if the source has say vcpu0 <-> pcpu2, >the destination must have vcpu0 <-> pcpu2 (the configuration for that >part is different). > Yes, you can change it by supplying different destination XML to the migration API. >So, the same thing can be applied to cache configuration. > Yes, it can. However it is way easier to do when you select the particular cache bank by socket/pcpu, level and scope, then just host_id. >> >>Sure, but those are things we can check during migration. I'd be OK >> >>with disabling migration (or making it --unsafe) when migrating with >> >>cachetune. >> > >> >Migration support is required (for NFV usecase they want to migrate >> >VMs around). >> > >> > >> >> So we at least need a check for the fact that either the caches are the >> somehow same or destination XML is supplied. >> >> The other option would be what I hinted above, that is to change >> host_id= to something like level, scope, and pcpu(s)/socket(s). > >host_id maps to L3 socket via: > >commit d57e3ab7e34c51a8badeea1b500bfb738d0af66e >Author: Fenghua Yu <fenghua.yu@intel.com> >Date: Sat Oct 22 06:19:50 2016 -0700 > > x86/intel_cacheinfo: Enable cache id in cache info > > Cache id is retrieved from APIC ID and CPUID leaf 4 on x86. > > For more details please see the section on "Cache ID Extraction > Parameters" in "Intel 64 Architecture Processor Topology >Enumeration". > > Also the documentation of the CPUID instruction in the "Intel 64 and > IA-32 Architectures Software Developer's Manual" > >So on the destination the user has to configure host_id= of the PCPU >which the guest is pinned to. > Oh, OK, I missed that it is actually taken from the host. that's because the code generates host_id numbers by using and incrementing global variable, so it does not correspond to the one kernel knows about. Is cache/index*/id the one from APIC? >> That is >> fairly easy change from the code point of view, but I'm trying to think >> more about the user experience. >> >> Martin > > >-- >libvir-list mailing list >libvir-list@redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.