[libvirt] [PATCH v5 2/4] xenconfig: add domxml conversions for xen-xl
Jim Fehlig
jfehlig at suse.com
Thu Oct 26 22:46:33 UTC 2017
On 10/12/2017 01:31 PM, Wim Ten Have wrote:
> From: Wim ten Have <wim.ten.have at oracle.com>
>
> 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 at oracle.com>
> ---
> 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
More information about the libvir-list
mailing list