On 10/12/2017 01:31 PM, Wim Ten Have wrote:
From: Wim ten Have <wim.ten.have@xxxxxxxxxx>
This patch converts NUMA configurations between the Xen libxl
configuration file format and libvirt's XML format.
XML HVM domain on a 4 node (2 cores/socket) configuration:
<cpu>
<numa>
<cell id='0' cpus='0-1' memory='2097152' unit='KiB'>
<distances>
<sibling id='0' value='10'/>
<sibling id='1' value='21'/>
<sibling id='2' value='31'/>
<sibling id='3' value='21'/>
</distances>
</cell>
<cell id='1' cpus='2-3' memory='2097152' unit='KiB'>
<distances>
<sibling id='0' value='21'/>
<sibling id='1' value='10'/>
<sibling id='2' value='21'/>
<sibling id='3' value='31'/>
</distances>
</cell>
<cell id='2' cpus='3-4' memory='2097152' unit='KiB'>
<distances>
<sibling id='0' value='31'/>
<sibling id='1' value='21'/>
<sibling id='2' value='10'/>
<sibling id='3' value='21'/>
</distances>
</cell>
<cell id='3' cpus='5-6' memory='2097152' unit='KiB'>
<distances>
<sibling id='0' value='21'/>
<sibling id='1' value='31'/>
<sibling id='2' value='21'/>
<sibling id='3' value='10'/>
</distances>
</cell>
</numa>
</cpu>
Xen xl.cfg domain configuration:
vnuma = [["pnode=0","size=2048","vcpus=0-1","vdistances=10,21,31,21"],
["pnode=1","size=2048","vcpus=2-3","vdistances=21,10,21,31"],
["pnode=2","size=2048","vcpus=4-5","vdistances=31,21,10,21"],
["pnode=3","size=2048","vcpus=6-7","vdistances=21,31,21,10"]]
If there is no XML <distances> description amongst the <cell> data the
conversion schema from xml to native will generate 10 for local and 20
for all remote instances.
Does xl have the same behavior? E.g. with
vnuma = [["pnode=0","size=2048","vcpus=0-1"],
["pnode=1","size=2048","vcpus=2-3"],
["pnode=2","size=2048","vcpus=4-5"],
["pnode=3","size=2048","vcpus=6-7"]]
will distances be 10 for local and 20 for remote nodes?
Signed-off-by: Wim ten Have <wim.ten.have@xxxxxxxxxx>
---
Changes on v2:
- Reduce the indentation level under xenParseXLVnuma().
Changes on v3:
- Add the Numa core split functions required to interface.
---
src/conf/numa_conf.c | 138 ++++++++++++++++++++
src/conf/numa_conf.h | 20 +++
src/libvirt_private.syms | 5 +
src/xenconfig/xen_xl.c | 333 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 496 insertions(+)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 00a3343fb..b513b1de1 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1144,6 +1144,133 @@ virDomainNumaGetNodeCount(virDomainNumaPtr numa)
}
+size_t
+virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
+{
+ if (!nmem_nodes) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot set an empty mem_nodes set"));
+ return 0;
+ }
+
+ if (numa->mem_nodes) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot alter an existing mem_nodes set"));
+ return 0;
+ }
+
+ if (VIR_ALLOC_N(numa->mem_nodes, nmem_nodes) < 0)
+ return 0;
+
+ numa->nmem_nodes = nmem_nodes;
+
+ return numa->nmem_nodes;
+}
+
+size_t
+virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
+ size_t node,
+ size_t cellid)
+{
+ virDomainNumaDistancePtr distances = NULL;
+
+ if (node < numa->nmem_nodes)
+ distances = numa->mem_nodes[node].distances;
+
+ /*
+ * Present the configured distance value. If
+ * out of range or not available set the platform
+ * defined default for local and remote nodes.
+ */
+ if (!distances ||
+ !distances[cellid].value ||
+ !numa->mem_nodes[node].ndistances)
+ return (node == cellid) ? \
+ LOCAL_DISTANCE : REMOTE_DISTANCE;
This return statement will fit within 80 columns on a single line. (Side note:
lines <= 80 columns is not strictly enforced in libvirt.)
+
+ return distances[cellid].value;
+}
+
+
+int
+virDomainNumaSetNodeDistance(virDomainNumaPtr numa,
+ size_t node,
+ size_t cellid,
+ unsigned int value)
+{
+ virDomainNumaDistancePtr distances;
+
+ if (node >= numa->nmem_nodes) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Argument 'node' %zu outranges "
+ "defined number of NUMA nodes"),
+ node);
+ return -1;
+ }
+
+ distances = numa->mem_nodes[node].distances;
+ if (!distances ||
+ cellid >= numa->mem_nodes[node].ndistances) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Arguments under memnode element do not "
+ "correspond with existing guest's NUMA cell"));
+ return -1;
+ }
+
+ /*
+ * Advanced Configuration and Power Interface
+ * Specification version 6.1. Chapter 5.2.17
+ * System Locality Distance Information Table
+ * ... Distance values of 0-9 are reserved.
+ */
+ if (value < LOCAL_DISTANCE ||
+ value > UNREACHABLE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Distance value of %d is in 0-9 reserved range"),
Should the '0-9' be dropped? E.g. value could be > UNREACHABLE.
+ value);
+ return -1;
+ }
+
+ if (value == LOCAL_DISTANCE && node != cellid) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Distance value %d under node %zu seems "
s/seems/is/ ?
+ "LOCAL_DISTANCE and should be set to 10"),
+ value, node);
+ return -1;
+ }
+
+ distances[cellid].cellid = cellid;
+ distances[cellid].value = value;
+
+ return distances[cellid].value;
+}
+
+
+size_t
+virDomainNumaSetNodeDistanceCount(virDomainNumaPtr numa,
+ size_t node,
+ size_t ndistances)
+{
+ virDomainNumaDistancePtr distances;
+
+ distances = numa->mem_nodes[node].distances;
+ if (distances) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot alter an existing nmem_nodes distances set for node: %zu"),
+ node);
+ return 0;
+ }
+
+ if (VIR_ALLOC_N(distances, ndistances) < 0)
+ return 0;
+
+ numa->mem_nodes[node].distances = distances;
+ numa->mem_nodes[node].ndistances = ndistances;
+
+ return numa->mem_nodes[node].ndistances;
+}
+
+
virBitmapPtr
virDomainNumaGetNodeCpumask(virDomainNumaPtr numa,
size_t node)
@@ -1152,6 +1279,17 @@ virDomainNumaGetNodeCpumask(virDomainNumaPtr numa,
}
+virBitmapPtr
+virDomainNumaSetNodeCpumask(virDomainNumaPtr numa,
+ size_t node,
+ virBitmapPtr cpumask)
+{
+ numa->mem_nodes[node].cpumask = cpumask;
+
+ return numa->mem_nodes[node].cpumask;
+}
+
+
virDomainMemoryAccess
virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa,
size_t node)
diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
index 378b772e4..8184b528b 100644
--- a/src/conf/numa_conf.h
+++ b/src/conf/numa_conf.h
@@ -87,12 +87,32 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune,
size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa);
+size_t virDomainNumaSetNodeCount(virDomainNumaPtr numa,
+ size_t nmem_nodes)
+ ATTRIBUTE_NONNULL(1);
+size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
+ size_t node,
+ size_t sibling)
+ ATTRIBUTE_NONNULL(1);
+int virDomainNumaSetNodeDistance(virDomainNumaPtr numa,
+ size_t node,
+ size_t sibling,
+ unsigned int value)
+ ATTRIBUTE_NONNULL(1);
+size_t virDomainNumaSetNodeDistanceCount(virDomainNumaPtr numa,
+ size_t node,
+ size_t ndistances)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
Is ATTRIBUTE_NONNULL needed for ndistances?
virBitmapPtr virDomainNumaGetNodeCpumask(virDomainNumaPtr numa,
size_t node)
ATTRIBUTE_NONNULL(1);
virDomainMemoryAccess virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa,
size_t node)
ATTRIBUTE_NONNULL(1);
+virBitmapPtr virDomainNumaSetNodeCpumask(virDomainNumaPtr numa,
+ size_t node,
+ virBitmapPtr cpumask)
+ ATTRIBUTE_NONNULL(1);
This file contains sections for the function types. E.g. all the getters are in
'Getters' section, setters in the 'Setters' section, etc.
unsigned long long virDomainNumaGetNodeMemorySize(virDomainNumaPtr numa,
size_t node)
ATTRIBUTE_NONNULL(1);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 26c5ddb40..861660b94 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -713,9 +713,14 @@ virDomainNumaGetMaxCPUID;
virDomainNumaGetMemorySize;
virDomainNumaGetNodeCount;
virDomainNumaGetNodeCpumask;
+virDomainNumaGetNodeDistance;
virDomainNumaGetNodeMemoryAccessMode;
virDomainNumaGetNodeMemorySize;
virDomainNumaNew;
+virDomainNumaSetNodeCount;
+virDomainNumaSetNodeCpumask;
+virDomainNumaSetNodeDistance;
+virDomainNumaSetNodeDistanceCount;
virDomainNumaSetNodeMemorySize;
virDomainNumatuneFormatNodeset;
virDomainNumatuneFormatXML;
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 8acbfe3f6..54b2ac650 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -309,6 +309,205 @@ xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
return -1;
}
+#ifdef LIBXL_HAVE_VNUMA
+static int
+xenParseXLVnuma(virConfPtr conf, virDomainDefPtr def)
+{
+ int ret = -1;
+ char *tmp = NULL;
+ char **token = NULL;
+ size_t vcpus = 0;
+ size_t nr_nodes = 0;
+ size_t vnodeCnt = 0;
+ virCPUDefPtr cpu = NULL;
+ virConfValuePtr list;
+ virConfValuePtr vnode;
+ virDomainNumaPtr numa;
+
+ numa = def->numa;
+ if (numa == NULL)
+ return -1;
+
+ list = virConfGetValue(conf, "vnuma");
+ if (!list || list->type != VIR_CONF_LIST)
+ return 0;
+
+ vnode = list->list;
+ while (vnode && vnode->type == VIR_CONF_LIST) {
+ vnode = vnode->next;
+ nr_nodes++;
+ }
+
+ if (!virDomainNumaSetNodeCount(numa, nr_nodes))
+ goto cleanup;
+
+ if (VIR_ALLOC(cpu) < 0)
+ goto cleanup;
+
+ list = list->list;
+ while (list) {
+ int pnode = -1;
+ virBitmapPtr cpumask = NULL;
+ unsigned long long kbsize = 0;
+
+ /* Is there a sublist (vnode)? */
+ if (list && list->type == VIR_CONF_LIST) {
+ vnode = list->list;
+
+ while (vnode && vnode->type == VIR_CONF_STRING) {
+ const char *data;
+ const char *str = vnode->str;
+
+ if (!str ||
+ !(data = strrchr(str, '='))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode invalid format '%s'"),
+ str);
+ goto cleanup;
+ }
+ data++;
+
+ if (*data) {
+ size_t len;
+ char vtoken[64];
+
+ if (STRPREFIX(str, "pnode")) {
+ unsigned int cellid;
+
+ len = strlen(data);
+ if (!virStrncpy(vtoken, data,
+ len, sizeof(vtoken))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu pnode '%s' too long for destination"),
+ vnodeCnt, data);
+ goto cleanup;
+ }
+
+ if ((virStrToLong_ui(vtoken, NULL, 10, &cellid) < 0) ||
+ (cellid >= nr_nodes)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("vnuma vnode %zu invalid value pnode '%s'"),
Maybe better worded as "vnuma vnode %zu contains invalid pnode value '%s'"?
+ vnodeCnt, data);
+ goto cleanup;
+ }
+ pnode = cellid;
+ } else if (STRPREFIX(str, "size")) {
+ len = strlen(data);
+ if (!virStrncpy(vtoken, data,
+ len, sizeof(vtoken))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu size '%s' too long for destination"),
+ vnodeCnt, data);
+ goto cleanup;
+ }
+
+ if (virStrToLong_ull(vtoken, NULL, 10, &kbsize) < 0)
+ goto cleanup;
+
+ virDomainNumaSetNodeMemorySize(numa, vnodeCnt, (kbsize * 1024));
+
+ } else if (STRPREFIX(str, "vcpus")) {
+ len = strlen(data);
+ if (!virStrncpy(vtoken, data,
+ len, sizeof(vtoken))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu vcpus '%s' too long for destination"),
+ vnodeCnt, data);
+ goto cleanup;
+ }
+
+ if ((virBitmapParse(vtoken, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) ||
+ (virDomainNumaSetNodeCpumask(numa, vnodeCnt, cpumask) == NULL))
+ goto cleanup;
+
+ vcpus += virBitmapCountBits(cpumask);
+
+ } else if (STRPREFIX(str, "vdistances")) {
+ size_t i, ndistances;
+ unsigned int value;
+
+ len = strlen(data);
+ if (!virStrncpy(vtoken, data,
+ len, sizeof(vtoken))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu vdistances '%s' too long for destination"),
+ vnodeCnt, data);
+ goto cleanup;
+ }
+
+ if (VIR_STRDUP(tmp, vtoken) < 0)
+ goto cleanup;
+
+ if (!(token = virStringSplitCount(tmp, ",", 0, &ndistances)))
+ goto cleanup;
+
+ if (ndistances != nr_nodes) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("vnuma pnode %d configured '%s' (count %zu) doesn't fit the number of specified vnodes %zu"),
+ pnode, str, ndistances, nr_nodes);
+ goto cleanup;
+ }
+
+ if (virDomainNumaSetNodeDistanceCount(numa, vnodeCnt, ndistances) != ndistances)
+ goto cleanup;
+
+ for (i = 0; i < ndistances; i++) {
+ if ((virStrToLong_ui(token[i], NULL, 10, &value) < 0) ||
+ (virDomainNumaSetNodeDistance(numa, vnodeCnt, i, value) != value))
+ goto cleanup;
+ }
+
+ } else {
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("vnuma vnode %zu invalid token '%s'"),
How about "Invalid vnuma configuration for vnode %zu"?
+ vnodeCnt, str);
+ goto cleanup;
+ }
+ }
+ vnode = vnode->next;
+ }
+ }
+
+ if ((pnode < 0) ||
+ (cpumask == NULL) ||
+ (kbsize == 0)) {
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("vnuma vnode %zu incomplete token. Missing%s%s%s"),
This could be tough for translation too. Maybe the message I suggested above is
sufficient for here too.
+ vnodeCnt,
+ (pnode < 0) ? " \'pnode\'":"",
+ (cpumask == NULL) ? " \'vcpus\'":"",
+ (kbsize == 0) ? " \'size\'":"");
+ goto cleanup;
+ }
+
+ list = list->next;
+ vnodeCnt++;
+ }
+
+ if (def->maxvcpus == 0)
+ def->maxvcpus = vcpus;
+
+ if (def->maxvcpus < vcpus) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("vnuma requested vcpus %zu fails available maxvcpus %zu"),
How about "vnuma configuration contains %zu vcpus, which is greater than %zu
maxvcpus"?
+ vcpus, def->maxvcpus);
+ goto cleanup;
+ }
+
+ cpu->type = VIR_CPU_TYPE_GUEST;
+ def->cpu = cpu;
+
+ ret = 0;
+
+ cleanup:
+ if (ret)
+ VIR_FREE(cpu);
+ virStringListFree(token);
+ VIR_FREE(tmp);
+
+ return ret;
+}
+#endif
Wow, that's a hairy function :-). I probably missed something that coverity will
complain about once this code hits libvirt.git.
static int
xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
@@ -863,6 +1062,11 @@ xenParseXL(virConfPtr conf,
if (xenParseXLOS(conf, def, caps) < 0)
goto cleanup;
+#ifdef LIBXL_HAVE_VNUMA
+ if (xenParseXLVnuma(conf, def) < 0)
+ goto cleanup;
+#endif
+
if (xenParseXLDisk(conf, def) < 0)
goto cleanup;
@@ -1005,6 +1209,130 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)
return 0;
}
+#ifdef LIBXL_HAVE_VNUMA
+static int
+xenFormatXLVnode(virConfValuePtr list, virBufferPtr buf)
+{
+ int ret = -1;
+ virConfValuePtr numaPnode, tmp;
+
+ if (virBufferCheckError(buf) < 0)
+ goto cleanup;
+
+ if (VIR_ALLOC(numaPnode) < 0)
+ goto cleanup;
+
+ /* Place VNODE directive */
+ numaPnode->type = VIR_CONF_STRING;
+ numaPnode->str = virBufferContentAndReset(buf);
+
+ tmp = list->list;
+ while (tmp && tmp->next)
+ tmp = tmp->next;
+ if (tmp)
+ tmp->next = numaPnode;
+ else
+ list->list = numaPnode;
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(buf);
+ return ret;
+}
+
+static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
I don't think these are needed on this internal function.
+xenFormatXLVnuma(virConfValuePtr list,
+ virDomainNumaPtr numa, size_t node, size_t nr_nodes)
One parameter per line.
+{
+ int ret = -1;
+ size_t i;
+
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ virConfValuePtr numaVnode, tmp;
+
+ size_t nodeSize = virDomainNumaGetNodeMemorySize(numa, node) / 1024;
+ char *nodeVcpus = virBitmapFormat(virDomainNumaGetNodeCpumask(numa, node));
+
+ if (VIR_ALLOC(numaVnode) < 0)
+ goto cleanup;
+
+ numaVnode->type = VIR_CONF_LIST;
+ numaVnode->list = NULL;
+
+ /* pnode */
+ virBufferAsprintf(&buf, "pnode=%ld", node);
+ xenFormatXLVnode(numaVnode, &buf);
+
+ /* size */
+ virBufferAsprintf(&buf, "size=%ld", nodeSize);
+ xenFormatXLVnode(numaVnode, &buf);
+
+ /* vcpus */
+ virBufferAsprintf(&buf, "vcpus=%s", nodeVcpus);
+ xenFormatXLVnode(numaVnode, &buf);
+
+ /* distances */
+ virBufferAddLit(&buf, "vdistances=");
+ for (i = 0; i < nr_nodes; i++) {
+ virBufferAsprintf(&buf, "%zu",
+ virDomainNumaGetNodeDistance(numa, node, i));
+ if ((nr_nodes - i) > 1)
+ virBufferAddLit(&buf, ",");
+ }
+ xenFormatXLVnode(numaVnode, &buf);
+
+ tmp = list->list;
+ while (tmp && tmp->next)
+ tmp = tmp->next;
+ if (tmp)
+ tmp->next = numaVnode;
+ else
+ list->list = numaVnode;
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(nodeVcpus);
+ return ret;
+}
+
+static int
+xenFormatXLDomainVnuma(virConfPtr conf, virDomainDefPtr def)
+{
+ virDomainNumaPtr numa = def->numa;
+ virConfValuePtr vnumaVal;
+ size_t i;
+ size_t nr_nodes;
+
+ if (numa == NULL)
+ return -1;
+
+ if (VIR_ALLOC(vnumaVal) < 0)
+ return -1;
+
+ vnumaVal->type = VIR_CONF_LIST;
+ vnumaVal->list = NULL;
+
+ nr_nodes = virDomainNumaGetNodeCount(numa);
+ for (i = 0; i < nr_nodes; i++) {
+ if (xenFormatXLVnuma(vnumaVal, numa, i, nr_nodes) < 0)
+ goto cleanup;
+ }
+
+ if (vnumaVal->list != NULL) {
+ int ret = virConfSetValue(conf, "vnuma", vnumaVal);
+ vnumaVal = NULL;
+ if (ret < 0)
+ return -1;
+ }
+ VIR_FREE(vnumaVal);
+
+ return 0;
+
+ cleanup:
+ virConfFreeValue(vnumaVal);
+ return -1;
+}
+#endif
the "formaters" are easier to read :-).
static char *
xenFormatXLDiskSrcNet(virStorageSourcePtr src)
@@ -1642,6 +1970,11 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn)
if (xenFormatXLOS(conf, def) < 0)
goto cleanup;
+#ifdef LIBXL_HAVE_VNUMA
+ if (xenFormatXLDomainVnuma(conf, def) < 0)
+ goto cleanup;
+#endif
+
if (xenFormatXLDomainDisks(conf, def) < 0)
goto cleanup;
Other than the nits, looks good.
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
