[libvirt] [PATCH 00/24] Move all NUMA related configuration into one structure
Due to historical madne^Wreasons the NUMA configuration is split between /domain/cpu and /domain/numatune. Internally the data was also split into two places. We can't do anything about the external representation but we certainly can store all the definitions in one place internally. This series does that. Peter Krempa (24): conf: Move numatune_conf to numa_conf conf: Move NUMA cell parsing code from cpu conf to numa conf conf: Refactor virDomainNumaDefCPUParseXML conf: numa: Don't duplicate NUMA cell cpumask conf: Move NUMA cell formatter to numa_conf conf: numa: Rename virDomainNumatune to virDomainNuma conf: Move enum virMemAccess to the NUMA code and rename it conf: numa: Recalculate rather than remember total NUMA cpu count conf: numa: Improve error message in case a numa node doesn't have cpus conf: numa: Reformat virDomainNumatuneParseXML conf: numa: Refactor logic in virDomainNumatuneParseXML conf: numa: Format <numatune> XML only if necessary conf: Separate helper for creating domain objects conf: Allocate domain definition with the new helper conf: numa: Always allocate the NUMA config conf: numa: Avoid re-allocation of the NUMA conf numa: conf: Tweak parameters of virDomainNumatuneSet conf: numa: Don't pass double pointer to virDomainNumatuneParseXML qemu: command: Unify retrieval of NUMA cell count in qemuBuildNumaArgStr conf: numa: Add helper to get guest NUMA node count and refactor users conf: numa: Add accesor for the NUMA node cpu mask conf: numa: Add accessor to NUMA node's memory access mode conf: numa: Add setter/getter for NUMA node memory size conf: Move all NUMA configuration to virDomainNuma po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/conf/cpu_conf.c | 151 +------- src/conf/cpu_conf.h | 25 +- src/conf/domain_conf.c | 59 ++- src/conf/domain_conf.h | 11 +- src/conf/{numatune_conf.c => numa_conf.c} | 429 +++++++++++++++------ src/conf/{numatune_conf.h => numa_conf.h} | 77 ++-- src/cpu/cpu.c | 2 +- src/libvirt_private.syms | 13 +- src/lxc/lxc_cgroup.c | 4 +- src/lxc/lxc_controller.c | 6 +- src/lxc/lxc_native.c | 4 +- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_sdk.c | 4 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_cgroup.c | 12 +- src/qemu/qemu_command.c | 52 +-- src/qemu/qemu_driver.c | 20 +- src/qemu/qemu_process.c | 4 +- src/vbox/vbox_common.c | 8 +- src/vmx/vmx.c | 2 +- src/xen/xen_hypervisor.c | 8 +- src/xen/xend_internal.c | 4 +- src/xen/xm_internal.c | 4 +- src/xenconfig/xen_sxpr.c | 2 +- src/xenconfig/xen_xl.c | 2 +- src/xenconfig/xen_xm.c | 2 +- tests/cputest.c | 2 +- tests/openvzutilstest.c | 2 +- .../qemuxml2argv-numatune-memnode.xml | 2 +- .../qemuxml2xmlout-numatune-memnode.xml | 2 +- tests/securityselinuxtest.c | 2 +- tests/testutilsqemu.c | 4 - 34 files changed, 503 insertions(+), 424 deletions(-) rename src/conf/{numatune_conf.c => numa_conf.c} (60%) rename src/conf/{numatune_conf.h => numa_conf.h} (50%) -- 2.2.2
For a while now there are two places that gather information about NUMA related guest configuration. While the XML can't be changed we can at least store the data in one place in the definition. Rename the numatune_conf.[ch] files to numa_conf as later patches will move the rest of the definitions from the cpu definition to this one. --- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/conf/domain_conf.h | 2 +- src/conf/{numatune_conf.c => numa_conf.c} | 6 +++--- src/conf/{numatune_conf.h => numa_conf.h} | 10 +++++----- src/libvirt_private.syms | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) rename src/conf/{numatune_conf.c => numa_conf.c} (99%) rename src/conf/{numatune_conf.h => numa_conf.h} (96%) diff --git a/po/POTFILES.in b/po/POTFILES.in index 3064037..c25bfce 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -27,7 +27,7 @@ src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c src/conf/networkcommon_conf.c src/conf/node_device_conf.c -src/conf/numatune_conf.c +src/conf/numa_conf.c src/conf/nwfilter_conf.c src/conf/nwfilter_params.c src/conf/object_event.c diff --git a/src/Makefile.am b/src/Makefile.am index b41c6d4..85624be 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -273,7 +273,7 @@ DOMAIN_CONF_SOURCES = \ conf/domain_audit.c conf/domain_audit.h \ conf/domain_nwfilter.c conf/domain_nwfilter.h \ conf/snapshot_conf.c conf/snapshot_conf.h \ - conf/numatune_conf.c conf/numatune_conf.h + conf/numa_conf.c conf/numa_conf.h OBJECT_EVENT_SOURCES = \ conf/object_event.c conf/object_event.h \ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 325afa8..c45e303 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -38,7 +38,7 @@ # include "virsocketaddr.h" # include "networkcommon_conf.h" # include "nwfilter_params.h" -# include "numatune_conf.h" +# include "numa_conf.h" # include "virnetdevmacvlan.h" # include "virsysinfo.h" # include "virnetdevvportprofile.h" diff --git a/src/conf/numatune_conf.c b/src/conf/numa_conf.c similarity index 99% rename from src/conf/numatune_conf.c rename to src/conf/numa_conf.c index 7f322ea..f6a8248 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numa_conf.c @@ -1,7 +1,7 @@ /* - * numatune_conf.c + * numa_conf.c * - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -22,7 +22,7 @@ #include <config.h> -#include "numatune_conf.h" +#include "numa_conf.h" #include "domain_conf.h" #include "viralloc.h" diff --git a/src/conf/numatune_conf.h b/src/conf/numa_conf.h similarity index 96% rename from src/conf/numatune_conf.h rename to src/conf/numa_conf.h index 28c4ce2..bcc8c8a 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numa_conf.h @@ -1,7 +1,7 @@ /* - * numatune_conf.h + * numa_conf.h * - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -20,8 +20,8 @@ * Author: Martin Kletzander <mkletzan@redhat.com> */ -#ifndef __NUMATUNE_CONF_H__ -# define __NUMATUNE_CONF_H__ +#ifndef __NUMA_CONF_H__ +# define __NUMA_CONF_H__ # include <libxml/xpath.h> @@ -111,4 +111,4 @@ int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, virBitmapPtr auto_nodeset); -#endif /* __NUMATUNE_CONF_H__ */ +#endif /* __NUMA_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c07a561..edd54b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -625,7 +625,7 @@ virNodeDeviceObjRemove; virNodeDeviceObjUnlock; -# conf/numatune_conf.h +# conf/numa_conf.h virDomainNumatuneEquals; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
For a while now there are two places that gather information about NUMA related guest configuration. While the XML can't be changed we can at least store the data in one place in the definition.
Rename the numatune_conf.[ch] files to numa_conf as later patches will move the rest of the definitions from the cpu definition to this one. --- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/conf/domain_conf.h | 2 +- src/conf/{numatune_conf.c => numa_conf.c} | 6 +++--- src/conf/{numatune_conf.h => numa_conf.h} | 10 +++++----- src/libvirt_private.syms | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) rename src/conf/{numatune_conf.c => numa_conf.c} (99%) rename src/conf/{numatune_conf.h => numa_conf.h} (96%)
ACK John (Good news the whole series has no Coverity regressions too)
For weird historical reasons NUMA cells are added as a subelement of <cpu> while the actual configuration is done in <numatune>. This patch splits out the cell parser code from cpu config to NUMA config. Note that the changes to the code are minimal just to make it work and the function will be refactored in the next patch. --- src/conf/cpu_conf.c | 90 --------------------------------------- src/conf/domain_conf.c | 17 +++++--- src/conf/numa_conf.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 4 ++ 4 files changed, 126 insertions(+), 96 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 4a367a1..f14b37a 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -426,96 +426,6 @@ virCPUDefParseXML(xmlNodePtr node, def->features[i].policy = policy; } - if (virXPathNode("./numa[1]", ctxt)) { - VIR_FREE(nodes); - n = virXPathNodeSet("./numa[1]/cell", ctxt, &nodes); - if (n <= 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("NUMA topology defined without NUMA cells")); - goto error; - } - - if (VIR_RESIZE_N(def->cells, def->ncells_max, - def->ncells, n) < 0) - goto error; - - def->ncells = n; - - for (i = 0; i < n; i++) { - char *cpus, *memAccessStr; - int ret, ncpus = 0; - unsigned int cur_cell; - char *tmp = NULL; - - tmp = virXMLPropString(nodes[i], "id"); - if (!tmp) { - cur_cell = i; - } else { - ret = virStrToLong_ui(tmp, NULL, 10, &cur_cell); - if (ret == -1) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid 'id' attribute in NUMA cell: %s"), - tmp); - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); - } - - if (cur_cell >= n) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Exactly one 'cell' element per guest " - "NUMA cell allowed, non-contiguous ranges or " - "ranges not starting from 0 are not allowed")); - goto error; - } - - if (def->cells[cur_cell].cpustr) { - virReportError(VIR_ERR_XML_ERROR, - _("Duplicate NUMA cell info for cell id '%u'"), - cur_cell); - goto error; - } - - cpus = virXMLPropString(nodes[i], "cpus"); - if (!cpus) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'cpus' attribute in NUMA cell")); - goto error; - } - def->cells[cur_cell].cpustr = cpus; - - ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask, - VIR_DOMAIN_CPUMASK_LEN); - if (ncpus <= 0) - goto error; - def->cells_cpus += ncpus; - - ctxt->node = nodes[i]; - if (virDomainParseMemory("./@memory", "./@unit", ctxt, - &def->cells[cur_cell].mem, true, false) < 0) - goto cleanup; - - memAccessStr = virXMLPropString(nodes[i], "memAccess"); - if (memAccessStr) { - int rc = virMemAccessTypeFromString(memAccessStr); - - if (rc <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid 'memAccess' attribute " - "value '%s'"), - memAccessStr); - VIR_FREE(memAccessStr); - goto error; - } - - def->cells[cur_cell].memAccess = rc; - - VIR_FREE(memAccessStr); - } - } - } - cleanup: ctxt->node = oldnode; VIR_FREE(fallback); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b13cae8..06ed0fd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13495,12 +13495,17 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if (def->cpu->cells_cpus > def->maxvcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Number of CPUs in <numa> exceeds the" - " <vcpu> count")); - goto error; - } + } + + if (virDomainNumaDefCPUParseXML(def->cpu, ctxt) < 0) + goto error; + + if (def->cpu && + def->cpu->cells_cpus > def->maxvcpus) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of CPUs in <numa> exceeds the" + " <vcpu> count")); + goto error; } if (virDomainNumatuneParseXML(&def->numatune, diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index f6a8248..e36a4be 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -680,3 +680,114 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, return true; } + + +int +virDomainNumaDefCPUParseXML(virCPUDefPtr def, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr *nodes = NULL; + xmlNodePtr oldNode = ctxt->node; + int n; + size_t i; + int ret = -1; + + if (virXPathNode("/domain/cpu/numa[1]", ctxt)) { + VIR_FREE(nodes); + + n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes); + if (n <= 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NUMA topology defined without NUMA cells")); + goto error; + } + + if (VIR_RESIZE_N(def->cells, def->ncells_max, + def->ncells, n) < 0) + goto error; + + def->ncells = n; + + for (i = 0; i < n; i++) { + char *cpus, *memAccessStr; + int rc, ncpus = 0; + unsigned int cur_cell; + char *tmp = NULL; + + tmp = virXMLPropString(nodes[i], "id"); + if (!tmp) { + cur_cell = i; + } else { + rc = virStrToLong_ui(tmp, NULL, 10, &cur_cell); + if (rc == -1) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid 'id' attribute in NUMA cell: %s"), + tmp); + VIR_FREE(tmp); + goto error; + } + VIR_FREE(tmp); + } + + if (cur_cell >= n) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Exactly one 'cell' element per guest " + "NUMA cell allowed, non-contiguous ranges or " + "ranges not starting from 0 are not allowed")); + goto error; + } + + if (def->cells[cur_cell].cpustr) { + virReportError(VIR_ERR_XML_ERROR, + _("Duplicate NUMA cell info for cell id '%u'"), + cur_cell); + goto error; + } + + cpus = virXMLPropString(nodes[i], "cpus"); + if (!cpus) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'cpus' attribute in NUMA cell")); + goto error; + } + def->cells[cur_cell].cpustr = cpus; + + ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask, + VIR_DOMAIN_CPUMASK_LEN); + if (ncpus <= 0) + goto error; + def->cells_cpus += ncpus; + + ctxt->node = nodes[i]; + if (virDomainParseMemory("./@memory", "./@unit", ctxt, + &def->cells[cur_cell].mem, true, false) < 0) + goto cleanup; + + memAccessStr = virXMLPropString(nodes[i], "memAccess"); + if (memAccessStr) { + rc = virMemAccessTypeFromString(memAccessStr); + + if (rc <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid 'memAccess' attribute " + "value '%s'"), + memAccessStr); + VIR_FREE(memAccessStr); + goto error; + } + + def->cells[cur_cell].memAccess = rc; + + VIR_FREE(memAccessStr); + } + } + } + + ret = 0; + + error: + cleanup: + ctxt->node = oldNode; + VIR_FREE(nodes); + return ret; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index bcc8c8a..276d25a 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -29,6 +29,7 @@ # include "virutil.h" # include "virbitmap.h" # include "virbuffer.h" +# include "cpu_conf.h" typedef struct _virDomainNumatune virDomainNumatune; @@ -111,4 +112,7 @@ int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, virBitmapPtr auto_nodeset); + +int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt); + #endif /* __NUMA_CONF_H__ */ -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
For weird historical reasons NUMA cells are added as a subelement of <cpu> while the actual configuration is done in <numatune>.
This patch splits out the cell parser code from cpu config to NUMA config. Note that the changes to the code are minimal just to make it work and the function will be refactored in the next patch. --- src/conf/cpu_conf.c | 90 --------------------------------------- src/conf/domain_conf.c | 17 +++++--- src/conf/numa_conf.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 4 ++ 4 files changed, 126 insertions(+), 96 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 4a367a1..f14b37a 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -426,96 +426,6 @@ virCPUDefParseXML(xmlNodePtr node, def->features[i].policy = policy; }
- if (virXPathNode("./numa[1]", ctxt)) { - VIR_FREE(nodes); - n = virXPathNodeSet("./numa[1]/cell", ctxt, &nodes); - if (n <= 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("NUMA topology defined without NUMA cells")); - goto error; - } - - if (VIR_RESIZE_N(def->cells, def->ncells_max, - def->ncells, n) < 0) - goto error; - - def->ncells = n; - - for (i = 0; i < n; i++) { - char *cpus, *memAccessStr; - int ret, ncpus = 0; - unsigned int cur_cell; - char *tmp = NULL; - - tmp = virXMLPropString(nodes[i], "id"); - if (!tmp) { - cur_cell = i; - } else { - ret = virStrToLong_ui(tmp, NULL, 10, &cur_cell); - if (ret == -1) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid 'id' attribute in NUMA cell: %s"), - tmp); - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); - } - - if (cur_cell >= n) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Exactly one 'cell' element per guest " - "NUMA cell allowed, non-contiguous ranges or " - "ranges not starting from 0 are not allowed")); - goto error; - } - - if (def->cells[cur_cell].cpustr) { - virReportError(VIR_ERR_XML_ERROR, - _("Duplicate NUMA cell info for cell id '%u'"), - cur_cell); - goto error; - } - - cpus = virXMLPropString(nodes[i], "cpus"); - if (!cpus) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'cpus' attribute in NUMA cell")); - goto error; - } - def->cells[cur_cell].cpustr = cpus; - - ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask, - VIR_DOMAIN_CPUMASK_LEN); - if (ncpus <= 0) - goto error; - def->cells_cpus += ncpus; - - ctxt->node = nodes[i]; - if (virDomainParseMemory("./@memory", "./@unit", ctxt, - &def->cells[cur_cell].mem, true, false) < 0) - goto cleanup; - - memAccessStr = virXMLPropString(nodes[i], "memAccess"); - if (memAccessStr) { - int rc = virMemAccessTypeFromString(memAccessStr); - - if (rc <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid 'memAccess' attribute " - "value '%s'"), - memAccessStr); - VIR_FREE(memAccessStr); - goto error; - } - - def->cells[cur_cell].memAccess = rc; - - VIR_FREE(memAccessStr); - } - } - } - cleanup: ctxt->node = oldnode; VIR_FREE(fallback); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b13cae8..06ed0fd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13495,12 +13495,17 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; }
- if (def->cpu->cells_cpus > def->maxvcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Number of CPUs in <numa> exceeds the" - " <vcpu> count")); - goto error; - } + } + + if (virDomainNumaDefCPUParseXML(def->cpu, ctxt) < 0) + goto error; + + if (def->cpu && + def->cpu->cells_cpus > def->maxvcpus) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Number of CPUs in <numa> exceeds the" + " <vcpu> count")); + goto error; }
if (virDomainNumatuneParseXML(&def->numatune, diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index f6a8248..e36a4be 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -680,3 +680,114 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune,
return true; } + + +int +virDomainNumaDefCPUParseXML(virCPUDefPtr def, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr *nodes = NULL; + xmlNodePtr oldNode = ctxt->node; + int n; + size_t i; + int ret = -1; + + if (virXPathNode("/domain/cpu/numa[1]", ctxt)) { + VIR_FREE(nodes); + + n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes); + if (n <= 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NUMA topology defined without NUMA cells")); + goto error; + } + + if (VIR_RESIZE_N(def->cells, def->ncells_max, + def->ncells, n) < 0) + goto error; + + def->ncells = n; + + for (i = 0; i < n; i++) { + char *cpus, *memAccessStr; + int rc, ncpus = 0; + unsigned int cur_cell; + char *tmp = NULL; + + tmp = virXMLPropString(nodes[i], "id"); + if (!tmp) { + cur_cell = i; + } else { + rc = virStrToLong_ui(tmp, NULL, 10, &cur_cell); + if (rc == -1) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid 'id' attribute in NUMA cell: %s"), + tmp); + VIR_FREE(tmp); + goto error; + } + VIR_FREE(tmp); + } + + if (cur_cell >= n) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Exactly one 'cell' element per guest " + "NUMA cell allowed, non-contiguous ranges or " + "ranges not starting from 0 are not allowed")); + goto error; + } + + if (def->cells[cur_cell].cpustr) { + virReportError(VIR_ERR_XML_ERROR, + _("Duplicate NUMA cell info for cell id '%u'"), + cur_cell); + goto error; + } + + cpus = virXMLPropString(nodes[i], "cpus"); + if (!cpus) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'cpus' attribute in NUMA cell")); + goto error; + } + def->cells[cur_cell].cpustr = cpus; + + ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask, + VIR_DOMAIN_CPUMASK_LEN); + if (ncpus <= 0) + goto error; + def->cells_cpus += ncpus; + + ctxt->node = nodes[i]; + if (virDomainParseMemory("./@memory", "./@unit", ctxt, + &def->cells[cur_cell].mem, true, false) < 0) + goto cleanup; + + memAccessStr = virXMLPropString(nodes[i], "memAccess"); + if (memAccessStr) { + rc = virMemAccessTypeFromString(memAccessStr); + + if (rc <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid 'memAccess' attribute " + "value '%s'"), + memAccessStr); + VIR_FREE(memAccessStr); + goto error; + } + + def->cells[cur_cell].memAccess = rc; + + VIR_FREE(memAccessStr); + } + } + } + + ret = 0; + + error: + cleanup:
Although this is mostly a cut'n'paste, why not just change the one "goto cleanup;" to goto error and then not have two labels? ACK - since I'm sure you'll do the right thing... John
+ ctxt->node = oldNode; + VIR_FREE(nodes); + return ret; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index bcc8c8a..276d25a 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -29,6 +29,7 @@ # include "virutil.h" # include "virbitmap.h" # include "virbuffer.h" +# include "cpu_conf.h"
typedef struct _virDomainNumatune virDomainNumatune; @@ -111,4 +112,7 @@ int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune);
bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, virBitmapPtr auto_nodeset); + +int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt); + #endif /* __NUMA_CONF_H__ */
On Thu, Feb 19, 2015 at 16:12:28 -0500, John Ferlan wrote:
On 02/16/2015 01:51 PM, Peter Krempa wrote:
For weird historical reasons NUMA cells are added as a subelement of <cpu> while the actual configuration is done in <numatune>.
This patch splits out the cell parser code from cpu config to NUMA config. Note that the changes to the code are minimal just to make it work and the function will be refactored in the next patch. --- src/conf/cpu_conf.c | 90 --------------------------------------- src/conf/domain_conf.c | 17 +++++--- src/conf/numa_conf.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 4 ++ 4 files changed, 126 insertions(+), 96 deletions(-)
...
+ VIR_FREE(memAccessStr); + } + } + } + + ret = 0; + + error: + cleanup:
Although this is mostly a cut'n'paste, why not just change the one "goto cleanup;" to goto error and then not have two labels?
I wanted to separate cleanup/refactor from the move :)
ACK - since I'm sure you'll do the right thing...
John
Peter
Rewrite the function to save a few local variables and reorder the code to make more sense. Additionally the ncells_max member of the virCPUDef structure is used only for tracking alocation when parsing the numa definition, which can be avoided by switching to VIR_ALLOC_N as the array is not resized after initial allocation. --- src/conf/cpu_conf.c | 1 - src/conf/cpu_conf.h | 1 - src/conf/numa_conf.c | 129 +++++++++++++++++++++++--------------------------- tests/testutilsqemu.c | 1 - 4 files changed, 58 insertions(+), 74 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index f14b37a..98b309b 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -154,7 +154,6 @@ virCPUDefCopy(const virCPUDef *cpu) if (cpu->ncells) { if (VIR_ALLOC_N(copy->cells, cpu->ncells) < 0) goto error; - copy->ncells_max = copy->ncells = cpu->ncells; for (i = 0; i < cpu->ncells; i++) { copy->cells[i].mem = cpu->cells[i].mem; diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 46fce25..e201656 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -127,7 +127,6 @@ struct _virCPUDef { size_t nfeatures_max; virCPUFeatureDefPtr features; size_t ncells; - size_t ncells_max; virCellDefPtr cells; unsigned int cells_cpus; }; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index e36a4be..c50369d 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -688,45 +688,36 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, { xmlNodePtr *nodes = NULL; xmlNodePtr oldNode = ctxt->node; + char *tmp = NULL; int n; size_t i; int ret = -1; - if (virXPathNode("/domain/cpu/numa[1]", ctxt)) { - VIR_FREE(nodes); + /* check if NUMA definition is present */ + if (!virXPathNode("/domain/cpu/numa[1]", ctxt)) + return 0; - n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes); - if (n <= 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("NUMA topology defined without NUMA cells")); - goto error; - } + if ((n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NUMA topology defined without NUMA cells")); + goto cleanup; + } + + if (VIR_ALLOC_N(def->cells, n) < 0) + goto cleanup; + def->ncells = n; - if (VIR_RESIZE_N(def->cells, def->ncells_max, - def->ncells, n) < 0) - goto error; - - def->ncells = n; - - for (i = 0; i < n; i++) { - char *cpus, *memAccessStr; - int rc, ncpus = 0; - unsigned int cur_cell; - char *tmp = NULL; - - tmp = virXMLPropString(nodes[i], "id"); - if (!tmp) { - cur_cell = i; - } else { - rc = virStrToLong_ui(tmp, NULL, 10, &cur_cell); - if (rc == -1) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid 'id' attribute in NUMA cell: %s"), - tmp); - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); + for (i = 0; i < n; i++) { + int rc, ncpus = 0; + unsigned int cur_cell = i; + + /* cells are in order of parsing or explicitly numbered */ + if ((tmp = virXMLPropString(nodes[i], "id"))) { + if (virStrToLong_uip(tmp, NULL, 10, &cur_cell) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid 'id' attribute in NUMA cell: '%s'"), + tmp); + goto cleanup; } if (cur_cell >= n) { @@ -734,60 +725,56 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, _("Exactly one 'cell' element per guest " "NUMA cell allowed, non-contiguous ranges or " "ranges not starting from 0 are not allowed")); - goto error; - } - - if (def->cells[cur_cell].cpustr) { - virReportError(VIR_ERR_XML_ERROR, - _("Duplicate NUMA cell info for cell id '%u'"), - cur_cell); - goto error; + goto cleanup; } + } + VIR_FREE(tmp); - cpus = virXMLPropString(nodes[i], "cpus"); - if (!cpus) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'cpus' attribute in NUMA cell")); - goto error; - } - def->cells[cur_cell].cpustr = cpus; + if (def->cells[cur_cell].cpumask) { + virReportError(VIR_ERR_XML_ERROR, + _("Duplicate NUMA cell info for cell id '%u'"), + cur_cell); + goto cleanup; + } - ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask, - VIR_DOMAIN_CPUMASK_LEN); - if (ncpus <= 0) - goto error; - def->cells_cpus += ncpus; + if (!(tmp = virXMLPropString(nodes[i], "cpus"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'cpus' attribute in NUMA cell")); + goto cleanup; + } - ctxt->node = nodes[i]; - if (virDomainParseMemory("./@memory", "./@unit", ctxt, - &def->cells[cur_cell].mem, true, false) < 0) - goto cleanup; + ncpus = virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask, + VIR_DOMAIN_CPUMASK_LEN); - memAccessStr = virXMLPropString(nodes[i], "memAccess"); - if (memAccessStr) { - rc = virMemAccessTypeFromString(memAccessStr); + if (ncpus <= 0) + goto cleanup; + def->cells_cpus += ncpus; - if (rc <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid 'memAccess' attribute " - "value '%s'"), - memAccessStr); - VIR_FREE(memAccessStr); - goto error; - } + def->cells[cur_cell].cpustr = tmp; - def->cells[cur_cell].memAccess = rc; + ctxt->node = nodes[i]; + if (virDomainParseMemory("./@memory", "./@unit", ctxt, + &def->cells[cur_cell].mem, true, false) < 0) + goto cleanup; - VIR_FREE(memAccessStr); + if ((tmp = virXMLPropString(nodes[i], "memAccess"))) { + if ((rc = virMemAccessTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid 'memAccess' attribute value '%s'"), + tmp); + goto cleanup; } + + def->cells[cur_cell].memAccess = rc; + VIR_FREE(tmp); } } ret = 0; - error: cleanup: ctxt->node = oldNode; VIR_FREE(nodes); + VIR_FREE(tmp); return ret; } diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 7b26e50..64f709a 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -243,7 +243,6 @@ virCapsPtr testQemuCapsInit(void) ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ host_cpu_features, /* features */ 0, /* ncells */ - 0, /* ncells_max */ NULL, /* cells */ 0, /* cells_cpus */ }; -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Rewrite the function to save a few local variables and reorder the code to make more sense.
Additionally the ncells_max member of the virCPUDef structure is used only for tracking alocation when parsing the numa definition, which can
a/alocation/allocation
be avoided by switching to VIR_ALLOC_N as the array is not resized after initial allocation.
Looks like initial implementation added RESIZE_N commit id '5f7b71b4' and 'ncells_max' for some reason...
--- src/conf/cpu_conf.c | 1 - src/conf/cpu_conf.h | 1 - src/conf/numa_conf.c | 129 +++++++++++++++++++++++--------------------------- tests/testutilsqemu.c | 1 - 4 files changed, 58 insertions(+), 74 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index f14b37a..98b309b 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -154,7 +154,6 @@ virCPUDefCopy(const virCPUDef *cpu) if (cpu->ncells) { if (VIR_ALLOC_N(copy->cells, cpu->ncells) < 0) goto error; - copy->ncells_max = copy->ncells = cpu->ncells;
for (i = 0; i < cpu->ncells; i++) { copy->cells[i].mem = cpu->cells[i].mem; diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 46fce25..e201656 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -127,7 +127,6 @@ struct _virCPUDef { size_t nfeatures_max; virCPUFeatureDefPtr features; size_t ncells; - size_t ncells_max; virCellDefPtr cells; unsigned int cells_cpus; }; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index e36a4be..c50369d 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -688,45 +688,36 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, { xmlNodePtr *nodes = NULL; xmlNodePtr oldNode = ctxt->node; + char *tmp = NULL; int n; size_t i; int ret = -1;
- if (virXPathNode("/domain/cpu/numa[1]", ctxt)) { - VIR_FREE(nodes); + /* check if NUMA definition is present */ + if (!virXPathNode("/domain/cpu/numa[1]", ctxt)) + return 0;
- n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes); - if (n <= 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("NUMA topology defined without NUMA cells")); - goto error; - } + if ((n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NUMA topology defined without NUMA cells")); + goto cleanup; + } + + if (VIR_ALLOC_N(def->cells, n) < 0) + goto cleanup; + def->ncells = n;
- if (VIR_RESIZE_N(def->cells, def->ncells_max, - def->ncells, n) < 0) - goto error; - - def->ncells = n; - - for (i = 0; i < n; i++) { - char *cpus, *memAccessStr; - int rc, ncpus = 0; - unsigned int cur_cell; - char *tmp = NULL; - - tmp = virXMLPropString(nodes[i], "id"); - if (!tmp) { - cur_cell = i; - } else { - rc = virStrToLong_ui(tmp, NULL, 10, &cur_cell); - if (rc == -1) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid 'id' attribute in NUMA cell: %s"), - tmp); - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); + for (i = 0; i < n; i++) { + int rc, ncpus = 0; + unsigned int cur_cell = i; + + /* cells are in order of parsing or explicitly numbered */ + if ((tmp = virXMLPropString(nodes[i], "id"))) { + if (virStrToLong_uip(tmp, NULL, 10, &cur_cell) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid 'id' attribute in NUMA cell: '%s'"), + tmp); + goto cleanup; }
if (cur_cell >= n) { @@ -734,60 +725,56 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, _("Exactly one 'cell' element per guest " "NUMA cell allowed, non-contiguous ranges or " "ranges not starting from 0 are not allowed")); - goto error; - } - - if (def->cells[cur_cell].cpustr) { - virReportError(VIR_ERR_XML_ERROR, - _("Duplicate NUMA cell info for cell id '%u'"), - cur_cell); - goto error; + goto cleanup; } + } + VIR_FREE(tmp);
- cpus = virXMLPropString(nodes[i], "cpus"); - if (!cpus) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'cpus' attribute in NUMA cell")); - goto error; - } - def->cells[cur_cell].cpustr = cpus; + if (def->cells[cur_cell].cpumask) {
using cpumask instead of cpustr - I guess it doesn't matter...
+ virReportError(VIR_ERR_XML_ERROR, + _("Duplicate NUMA cell info for cell id '%u'"), + cur_cell); + goto cleanup; + }
- ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask, - VIR_DOMAIN_CPUMASK_LEN); - if (ncpus <= 0) - goto error; - def->cells_cpus += ncpus; + if (!(tmp = virXMLPropString(nodes[i], "cpus"))) { ^ extraneous space
+ virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'cpus' attribute in NUMA cell")); + goto cleanup; + }
- ctxt->node = nodes[i]; - if (virDomainParseMemory("./@memory", "./@unit", ctxt, - &def->cells[cur_cell].mem, true, false) < 0) - goto cleanup; + ncpus = virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask, + VIR_DOMAIN_CPUMASK_LEN);
- memAccessStr = virXMLPropString(nodes[i], "memAccess"); - if (memAccessStr) { - rc = virMemAccessTypeFromString(memAccessStr); + if (ncpus <= 0) + goto cleanup; + def->cells_cpus += ncpus;
- if (rc <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid 'memAccess' attribute " - "value '%s'"), - memAccessStr); - VIR_FREE(memAccessStr); - goto error; - } + def->cells[cur_cell].cpustr = tmp;
tmp = NULL; Just in case we jump to cleanup as I suspect cleanup of *.cpustr later on will be VIR_FREE'd
- def->cells[cur_cell].memAccess = rc; + ctxt->node = nodes[i]; + if (virDomainParseMemory("./@memory", "./@unit", ctxt, + &def->cells[cur_cell].mem, true, false) < 0) + goto cleanup;
- VIR_FREE(memAccessStr); + if ((tmp = virXMLPropString(nodes[i], "memAccess"))) { + if ((rc = virMemAccessTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid 'memAccess' attribute value '%s'"), + tmp); + goto cleanup; } + + def->cells[cur_cell].memAccess = rc; + VIR_FREE(tmp); } }
ret = 0;
- error:
Yep - sometimes I look at multiple patches before sending and sometimes I don't... oh well nevermind my 2/24 comment ;-) ACK, John
cleanup: ctxt->node = oldNode; VIR_FREE(nodes); + VIR_FREE(tmp); return ret; } diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 7b26e50..64f709a 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -243,7 +243,6 @@ virCapsPtr testQemuCapsInit(void) ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ host_cpu_features, /* features */ 0, /* ncells */ - 0, /* ncells_max */ NULL, /* cells */ 0, /* cells_cpus */ };
The mask was stored both as a bitmap and as a string. The string is used for XML output only. Remove the string, as it can be reconstructed from the bitmap. The test change is necessary as the bitmap formatter doesn't "optimize" using the '^' operator. --- src/conf/cpu_conf.c | 14 +++++++------- src/conf/cpu_conf.h | 1 - src/conf/numa_conf.c | 3 +-- tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 98b309b..11ad5f4 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -86,10 +86,8 @@ virCPUDefFree(virCPUDefPtr def) virCPUDefFreeModel(def); - for (i = 0; i < def->ncells; i++) { + for (i = 0; i < def->ncells; i++) virBitmapFree(def->cells[i].cpumask); - VIR_FREE(def->cells[i].cpustr); - } VIR_FREE(def->cells); VIR_FREE(def->vendor_id); @@ -162,9 +160,6 @@ virCPUDefCopy(const virCPUDef *cpu) if (!copy->cells[i].cpumask) goto error; - - if (VIR_STRDUP(copy->cells[i].cpustr, cpu->cells[i].cpustr) < 0) - goto error; } copy->cells_cpus = cpu->cells_cpus; } @@ -601,16 +596,21 @@ virCPUDefFormatBuf(virBufferPtr buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < def->ncells; i++) { virMemAccess memAccess = def->cells[i].memAccess; + char *cpustr = NULL; + + if (!(cpustr = virBitmapFormat(def->cells[i].cpumask))) + return -1; virBufferAddLit(buf, "<cell"); virBufferAsprintf(buf, " id='%zu'", i); - virBufferAsprintf(buf, " cpus='%s'", def->cells[i].cpustr); + virBufferAsprintf(buf, " cpus='%s'", cpustr); virBufferAsprintf(buf, " memory='%llu'", def->cells[i].mem); virBufferAddLit(buf, " unit='KiB'"); if (memAccess) virBufferAsprintf(buf, " memAccess='%s'", virMemAccessTypeToString(memAccess)); virBufferAddLit(buf, "/>\n"); + VIR_FREE(cpustr); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</numa>\n"); diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index e201656..d6efba7 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -104,7 +104,6 @@ typedef struct _virCellDef virCellDef; typedef virCellDef *virCellDefPtr; struct _virCellDef { virBitmapPtr cpumask; /* CPUs that are part of this node */ - char *cpustr; /* CPUs stored in string form for dumpxml */ unsigned long long mem; /* Node memory in kB */ virMemAccess memAccess; }; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index c50369d..965d67b 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -749,8 +749,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, if (ncpus <= 0) goto cleanup; def->cells_cpus += ncpus; - - def->cells[cur_cell].cpustr = tmp; + VIR_FREE(tmp); ctxt->node = nodes[i]; if (virDomainParseMemory("./@memory", "./@unit", ctxt, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml index 8912017..73dfdf5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml @@ -17,7 +17,7 @@ <numa> <cell id='0' cpus='0' memory='20002' unit='KiB'/> <cell id='1' cpus='1-27,29' memory='660066' unit='KiB'/> - <cell id='2' cpus='28-31,^29' memory='24002400' unit='KiB'/> + <cell id='2' cpus='28,30-31' memory='24002400' unit='KiB'/> </numa> </cpu> <clock offset='utc'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml index ffc57cf..7542125 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml @@ -17,7 +17,7 @@ <numa> <cell id='0' cpus='0' memory='20002' unit='KiB'/> <cell id='1' cpus='1-27,29' memory='660066' unit='KiB'/> - <cell id='2' cpus='28-31,^29' memory='24002400' unit='KiB'/> + <cell id='2' cpus='28,30-31' memory='24002400' unit='KiB'/> </numa> </cpu> <clock offset='utc'/> -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
The mask was stored both as a bitmap and as a string. The string is used for XML output only. Remove the string, as it can be reconstructed from the bitmap.
The test change is necessary as the bitmap formatter doesn't "optimize" using the '^' operator. --- src/conf/cpu_conf.c | 14 +++++++------- src/conf/cpu_conf.h | 1 - src/conf/numa_conf.c | 3 +-- tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-numatune-memnode.xml | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-)
ACK - and I see the VIR_FREE(tmp) from 3/24 appears in the right place now ;-) John
Move the code that formats the /domain/cpu/numa element to numa_conf as it belongs there. --- src/conf/cpu_conf.c | 29 ++++------------------------- src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 1 + 3 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 11ad5f4..28fbead 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -495,8 +495,11 @@ virCPUDefFormatBufFull(virBufferPtr buf, virArchToString(def->arch)); if (virCPUDefFormatBuf(buf, def, updateCPU) < 0) return -1; - virBufferAdjustIndent(buf, -2); + if (virDomainNumaDefCPUFormat(buf, def) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</cpu>\n"); return 0; @@ -591,30 +594,6 @@ virCPUDefFormatBuf(virBufferPtr buf, } } - if (def->ncells) { - virBufferAddLit(buf, "<numa>\n"); - virBufferAdjustIndent(buf, 2); - for (i = 0; i < def->ncells; i++) { - virMemAccess memAccess = def->cells[i].memAccess; - char *cpustr = NULL; - - if (!(cpustr = virBitmapFormat(def->cells[i].cpumask))) - return -1; - - virBufferAddLit(buf, "<cell"); - virBufferAsprintf(buf, " id='%zu'", i); - virBufferAsprintf(buf, " cpus='%s'", cpustr); - virBufferAsprintf(buf, " memory='%llu'", def->cells[i].mem); - virBufferAddLit(buf, " unit='KiB'"); - if (memAccess) - virBufferAsprintf(buf, " memAccess='%s'", - virMemAccessTypeToString(memAccess)); - virBufferAddLit(buf, "/>\n"); - VIR_FREE(cpustr); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</numa>\n"); - } return 0; } diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 965d67b..d8f1739 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -777,3 +777,40 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, VIR_FREE(tmp); return ret; } + + +int +virDomainNumaDefCPUFormat(virBufferPtr buf, + virCPUDefPtr def) +{ + virMemAccess memAccess; + char *cpustr; + size_t i; + + if (def->ncells == 0) + return 0; + + virBufferAddLit(buf, "<numa>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < def->ncells; i++) { + memAccess = def->cells[i].memAccess; + + if (!(cpustr = virBitmapFormat(def->cells[i].cpumask))) + return -1; + + virBufferAddLit(buf, "<cell"); + virBufferAsprintf(buf, " id='%zu'", i); + virBufferAsprintf(buf, " cpus='%s'", cpustr); + virBufferAsprintf(buf, " memory='%llu'", def->cells[i].mem); + virBufferAddLit(buf, " unit='KiB'"); + if (memAccess) + virBufferAsprintf(buf, " memAccess='%s'", + virMemAccessTypeToString(memAccess)); + virBufferAddLit(buf, "/>\n"); + VIR_FREE(cpustr); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</numa>\n"); + + return 0; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 276d25a..9202355 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -114,5 +114,6 @@ bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, virBitmapPtr auto_nodeset); int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt); +int virDomainNumaDefCPUFormat(virBufferPtr buf, virCPUDefPtr def); #endif /* __NUMA_CONF_H__ */ -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Move the code that formats the /domain/cpu/numa element to numa_conf as it belongs there. --- src/conf/cpu_conf.c | 29 ++++------------------------- src/conf/numa_conf.c | 37 +++++++++++++++++++++++++++++++++++++ src/conf/numa_conf.h | 1 + 3 files changed, 42 insertions(+), 25 deletions(-)
ACK John
The structure will gradually become the only place for NUMA related config, thus rename it appropriately. --- src/conf/domain_conf.c | 10 +++--- src/conf/domain_conf.h | 2 +- src/conf/numa_conf.c | 78 +++++++++++++++++++++---------------------- src/conf/numa_conf.h | 34 +++++++++---------- src/libvirt_private.syms | 4 +-- src/lxc/lxc_cgroup.c | 4 +-- src/lxc/lxc_controller.c | 6 ++-- src/lxc/lxc_native.c | 2 +- src/parallels/parallels_sdk.c | 2 +- src/qemu/qemu_cgroup.c | 12 +++---- src/qemu/qemu_command.c | 8 ++--- src/qemu/qemu_driver.c | 20 +++++------ src/qemu/qemu_process.c | 4 +-- 13 files changed, 93 insertions(+), 93 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 06ed0fd..c9a65e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2249,7 +2249,7 @@ void virDomainDefFree(virDomainDefPtr def) virBitmapFree(def->cputune.iothreadsched[i].ids); VIR_FREE(def->cputune.iothreadsched); - virDomainNumatuneFree(def->numatune); + virDomainNumaFree(def->numa); virSysinfoDefFree(def->sysinfo); @@ -13508,14 +13508,14 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if (virDomainNumatuneParseXML(&def->numatune, + if (virDomainNumatuneParseXML(&def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, def->cpu ? def->cpu->ncells : 0, ctxt) < 0) goto error; - if (virDomainNumatuneHasPlacementAuto(def->numatune) && + if (virDomainNumatuneHasPlacementAuto(def->numa) && !def->cpumask && !def->cputune.vcpupin && !def->cputune.emulatorpin && !def->cputune.iothreadspin) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; @@ -19894,7 +19894,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (cputune) virBufferAddLit(buf, "</cputune>\n"); - if (virDomainNumatuneFormatXML(buf, def->numatune) < 0) + if (virDomainNumatuneFormatXML(buf, def->numa) < 0) goto error; if (def->resource) @@ -22322,7 +22322,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def) if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) return true; - if (virDomainNumatuneHasPlacementAuto(def->numatune)) + if (virDomainNumatuneHasPlacementAuto(def->numa)) return true; return false; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c45e303..1e04886 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2091,7 +2091,7 @@ struct _virDomainDef { virDomainCputune cputune; - virDomainNumatunePtr numatune; + virDomainNumaPtr numa; virDomainResourceDefPtr resource; virDomainIdMapDef idmap; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index d8f1739..bcb8023 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -43,10 +43,10 @@ VIR_ENUM_IMPL(virDomainNumatunePlacement, "static", "auto"); -typedef struct _virDomainNumatuneNode virDomainNumatuneNode; -typedef virDomainNumatuneNode *virDomainNumatuneNodePtr; +typedef struct _virDomainNumaNode virDomainNumaNode; +typedef virDomainNumaNode *virDomainNumaNodePtr; -struct _virDomainNumatune { +struct _virDomainNuma { struct { bool specified; virBitmapPtr nodeset; @@ -54,7 +54,7 @@ struct _virDomainNumatune { virDomainNumatunePlacement placement; } memory; /* pinning for all the memory */ - struct _virDomainNumatuneNode { + struct _virDomainNumaNode { virBitmapPtr nodeset; virDomainNumatuneMemMode mode; } *mem_nodes; /* fine tuning per guest node */ @@ -65,7 +65,7 @@ struct _virDomainNumatune { static inline bool -virDomainNumatuneNodeSpecified(virDomainNumatunePtr numatune, +virDomainNumatuneNodeSpecified(virDomainNumaPtr numatune, int cellid) { if (numatune && @@ -77,7 +77,7 @@ virDomainNumatuneNodeSpecified(virDomainNumatunePtr numatune, } static int -virDomainNumatuneNodeParseXML(virDomainNumatunePtr *numatunePtr, +virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, size_t ncells, xmlXPathContextPtr ctxt) { @@ -85,7 +85,7 @@ virDomainNumatuneNodeParseXML(virDomainNumatunePtr *numatunePtr, int n = 0; int ret = -1; size_t i = 0; - virDomainNumatunePtr numatune = *numatunePtr; + virDomainNumaPtr numatune = *numatunePtr; xmlNodePtr *nodes = NULL; if ((n = virXPathNodeSet("./numatune/memnode", ctxt, &nodes)) < 0) { @@ -126,7 +126,7 @@ virDomainNumatuneNodeParseXML(virDomainNumatunePtr *numatunePtr, for (i = 0; i < n; i++) { int mode = 0; unsigned int cellid = 0; - virDomainNumatuneNodePtr mem_node = NULL; + virDomainNumaNodePtr mem_node = NULL; xmlNodePtr cur_node = nodes[i]; tmp = virXMLPropString(cur_node, "cellid"); @@ -194,7 +194,7 @@ virDomainNumatuneNodeParseXML(virDomainNumatunePtr *numatunePtr, } int -virDomainNumatuneParseXML(virDomainNumatunePtr *numatunePtr, +virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, bool placement_static, size_t ncells, xmlXPathContextPtr ctxt) @@ -220,7 +220,7 @@ virDomainNumatuneParseXML(virDomainNumatunePtr *numatunePtr, node = virXPathNode("./numatune/memory[1]", ctxt); if (*numatunePtr) { - virDomainNumatuneFree(*numatunePtr); + virDomainNumaFree(*numatunePtr); *numatunePtr = NULL; } @@ -288,7 +288,7 @@ virDomainNumatuneParseXML(virDomainNumatunePtr *numatunePtr, int virDomainNumatuneFormatXML(virBufferPtr buf, - virDomainNumatunePtr numatune) + virDomainNumaPtr numatune) { const char *tmp = NULL; char *nodeset = NULL; @@ -316,7 +316,7 @@ virDomainNumatuneFormatXML(virBufferPtr buf, } for (i = 0; i < numatune->nmem_nodes; i++) { - virDomainNumatuneNodePtr mem_node = &numatune->mem_nodes[i]; + virDomainNumaNodePtr mem_node = &numatune->mem_nodes[i]; if (!mem_node->nodeset) continue; @@ -338,23 +338,23 @@ virDomainNumatuneFormatXML(virBufferPtr buf, } void -virDomainNumatuneFree(virDomainNumatunePtr numatune) +virDomainNumaFree(virDomainNumaPtr numa) { size_t i = 0; - if (!numatune) + if (!numa) return; - virBitmapFree(numatune->memory.nodeset); - for (i = 0; i < numatune->nmem_nodes; i++) - virBitmapFree(numatune->mem_nodes[i].nodeset); - VIR_FREE(numatune->mem_nodes); + virBitmapFree(numa->memory.nodeset); + for (i = 0; i < numa->nmem_nodes; i++) + virBitmapFree(numa->mem_nodes[i].nodeset); + VIR_FREE(numa->mem_nodes); - VIR_FREE(numatune); + VIR_FREE(numa); } virDomainNumatuneMemMode -virDomainNumatuneGetMode(virDomainNumatunePtr numatune, +virDomainNumatuneGetMode(virDomainNumaPtr numatune, int cellid) { if (!numatune) @@ -370,7 +370,7 @@ virDomainNumatuneGetMode(virDomainNumatunePtr numatune, } virBitmapPtr -virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune, +virDomainNumatuneGetNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, int cellid) { @@ -391,7 +391,7 @@ virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune, } char * -virDomainNumatuneFormatNodeset(virDomainNumatunePtr numatune, +virDomainNumatuneFormatNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, int cellid) { @@ -402,7 +402,7 @@ virDomainNumatuneFormatNodeset(virDomainNumatunePtr numatune, int -virDomainNumatuneMaybeGetNodeset(virDomainNumatunePtr numatune, +virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, virBitmapPtr *retNodeset, int cellid) @@ -432,7 +432,7 @@ virDomainNumatuneMaybeGetNodeset(virDomainNumatunePtr numatune, int -virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, +virDomainNumatuneMaybeFormatNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, char **mask, int cellid) @@ -451,7 +451,7 @@ virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, } int -virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr, +virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, bool placement_static, int placement, int mode, @@ -459,7 +459,7 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr, { bool created = false; int ret = -1; - virDomainNumatunePtr numatune; + virDomainNumaPtr numatune; /* No need to do anything in this case */ if (mode == -1 && placement == -1 && !nodeset) @@ -538,7 +538,7 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr, cleanup: if (ret < 0 && created) { - virDomainNumatuneFree(*numatunePtr); + virDomainNumaFree(*numatunePtr); *numatunePtr = NULL; } @@ -546,8 +546,8 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr, } static bool -virDomainNumatuneNodesEqual(virDomainNumatunePtr n1, - virDomainNumatunePtr n2) +virDomainNumaNodesEqual(virDomainNumaPtr n1, + virDomainNumaPtr n2) { size_t i = 0; @@ -555,8 +555,8 @@ virDomainNumatuneNodesEqual(virDomainNumatunePtr n1, return false; for (i = 0; i < n1->nmem_nodes; i++) { - virDomainNumatuneNodePtr nd1 = &n1->mem_nodes[i]; - virDomainNumatuneNodePtr nd2 = &n2->mem_nodes[i]; + virDomainNumaNodePtr nd1 = &n1->mem_nodes[i]; + virDomainNumaNodePtr nd2 = &n2->mem_nodes[i]; if (!nd1->nodeset && !nd2->nodeset) continue; @@ -572,8 +572,8 @@ virDomainNumatuneNodesEqual(virDomainNumatunePtr n1, } bool -virDomainNumatuneEquals(virDomainNumatunePtr n1, - virDomainNumatunePtr n2) +virDomainNumaEquals(virDomainNumaPtr n1, + virDomainNumaPtr n2) { if (!n1 && !n2) return true; @@ -582,7 +582,7 @@ virDomainNumatuneEquals(virDomainNumatunePtr n1, return false; if (!n1->memory.specified && !n2->memory.specified) - return virDomainNumatuneNodesEqual(n1, n2); + return virDomainNumaNodesEqual(n1, n2); if (!n1->memory.specified || !n2->memory.specified) return false; @@ -596,11 +596,11 @@ virDomainNumatuneEquals(virDomainNumatunePtr n1, if (!virBitmapEqual(n1->memory.nodeset, n2->memory.nodeset)) return false; - return virDomainNumatuneNodesEqual(n1, n2); + return virDomainNumaNodesEqual(n1, n2); } bool -virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune) +virDomainNumatuneHasPlacementAuto(virDomainNumaPtr numatune) { if (!numatune) return false; @@ -615,7 +615,7 @@ virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune) } bool -virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune) +virDomainNumatuneHasPerNodeBinding(virDomainNumaPtr numatune) { size_t i = 0; @@ -631,7 +631,7 @@ virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune) } int -virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) +virDomainNumatuneSpecifiedMaxNode(virDomainNumaPtr numatune) { int ret = -1; virBitmapPtr nodemask = NULL; @@ -659,7 +659,7 @@ virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) } bool -virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, +virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset) { size_t i = 0; diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 9202355..0adeaa4 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -32,8 +32,8 @@ # include "cpu_conf.h" -typedef struct _virDomainNumatune virDomainNumatune; -typedef virDomainNumatune *virDomainNumatunePtr; +typedef struct _virDomainNuma virDomainNuma; +typedef virDomainNuma *virDomainNumaPtr; typedef enum { VIR_DOMAIN_NUMATUNE_PLACEMENT_DEFAULT = 0, @@ -47,31 +47,31 @@ VIR_ENUM_DECL(virDomainNumatunePlacement) VIR_ENUM_DECL(virDomainNumatuneMemMode) -void virDomainNumatuneFree(virDomainNumatunePtr numatune); +void virDomainNumaFree(virDomainNumaPtr numa); /* * XML Parse/Format functions */ -int virDomainNumatuneParseXML(virDomainNumatunePtr *numatunePtr, +int virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, bool placement_static, size_t ncells, xmlXPathContextPtr ctxt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); -int virDomainNumatuneFormatXML(virBufferPtr buf, virDomainNumatunePtr numatune) +int virDomainNumatuneFormatXML(virBufferPtr buf, virDomainNumaPtr numatune) ATTRIBUTE_NONNULL(1); /* * Getters */ -virDomainNumatuneMemMode virDomainNumatuneGetMode(virDomainNumatunePtr numatune, +virDomainNumatuneMemMode virDomainNumatuneGetMode(virDomainNumaPtr numatune, int cellid); -virBitmapPtr virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune, +virBitmapPtr virDomainNumatuneGetNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, int cellid); -int virDomainNumatuneMaybeGetNodeset(virDomainNumatunePtr numatune, +int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, virBitmapPtr *retNodeset, int cellid); @@ -79,11 +79,11 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumatunePtr numatune, /* * Formatters */ -char *virDomainNumatuneFormatNodeset(virDomainNumatunePtr numatune, +char *virDomainNumatuneFormatNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, int cellid); -int virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, +int virDomainNumatuneMaybeFormatNodeset(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset, char **mask, int cellid); @@ -91,7 +91,7 @@ int virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune, /* * Setters */ -int virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr, +int virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, bool placement_static, int placement, int mode, @@ -101,16 +101,16 @@ int virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr, /* * Other accessors */ -bool virDomainNumatuneEquals(virDomainNumatunePtr n1, - virDomainNumatunePtr n2); +bool virDomainNumaEquals(virDomainNumaPtr n1, + virDomainNumaPtr n2); -bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune); +bool virDomainNumatuneHasPlacementAuto(virDomainNumaPtr numatune); -bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune); +bool virDomainNumatuneHasPerNodeBinding(virDomainNumaPtr numatune); -int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); +int virDomainNumatuneSpecifiedMaxNode(virDomainNumaPtr numatune); -bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune, +bool virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset); int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index edd54b8..278124c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -626,10 +626,10 @@ virNodeDeviceObjUnlock; # conf/numa_conf.h -virDomainNumatuneEquals; +virDomainNumaEquals; +virDomainNumaFree; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; -virDomainNumatuneFree; virDomainNumatuneGetMode; virDomainNumatuneGetNodeset; virDomainNumatuneHasPerNodeBinding; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 1dfa9a4..8e46a01 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -79,11 +79,11 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, goto cleanup; } - if (virDomainNumatuneGetMode(def->numatune, -1) != + if (virDomainNumatuneGetMode(def->numa, -1) != VIR_DOMAIN_NUMATUNE_MEM_STRICT) goto cleanup; - if (virDomainNumatuneMaybeFormatNodeset(def->numatune, nodemask, + if (virDomainNumatuneMaybeFormatNodeset(def->numa, nodemask, &mask, -1) < 0) goto cleanup; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 8a7c7e8..8545f29 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -747,8 +747,8 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0) goto cleanup; - nodeset = virDomainNumatuneGetNodeset(ctrl->def->numatune, auto_nodeset, -1); - mode = virDomainNumatuneGetMode(ctrl->def->numatune, -1); + nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1); + mode = virDomainNumatuneGetMode(ctrl->def->numa, -1); if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) goto cleanup; @@ -778,7 +778,7 @@ static int virLXCControllerSetupCgroupLimits(virLXCControllerPtr ctrl) if (virLXCControllerGetNumadAdvice(ctrl, &auto_nodeset) < 0) goto cleanup; - nodeset = virDomainNumatuneGetNodeset(ctrl->def->numatune, auto_nodeset, -1); + nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1); if (!(ctrl->cgroup = virLXCCgroupCreate(ctrl->def, ctrl->initpid, diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 961c24b..cd3b86b 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -847,7 +847,7 @@ lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties) value->str) { if (virBitmapParse(value->str, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) return -1; - if (virDomainNumatuneSet(&def->numatune, + if (virDomainNumatuneSet(&def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC, diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index d0d2ce2..8c05b32 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1825,7 +1825,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) return -1; } - if (def->numatune) { + if (def->numa) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("numa parameters are not supported " "by parallels driver")); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index fc46450..c33b121 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -619,11 +619,11 @@ qemuSetupCpusetMems(virDomainObjPtr vm) if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) return 0; - if (virDomainNumatuneGetMode(vm->def->numatune, -1) != + if (virDomainNumatuneGetMode(vm->def->numa, -1) != VIR_DOMAIN_NUMATUNE_MEM_STRICT) return 0; - if (virDomainNumatuneMaybeFormatNodeset(vm->def->numatune, + if (virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) goto cleanup; @@ -1030,9 +1030,9 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) return 0; } - if (virDomainNumatuneGetMode(vm->def->numatune, -1) == + if (virDomainNumatuneGetMode(vm->def->numa, -1) == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numatune, + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) goto cleanup; @@ -1201,9 +1201,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) return 0; } - if (virDomainNumatuneGetMode(vm->def->numatune, -1) == + if (virDomainNumatuneGetMode(vm->def->numa, -1) == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numatune, + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) goto cleanup; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 084530f..c4ae596 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4571,7 +4571,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (!(props = virJSONValueNewObject())) return -1; - mode = virDomainNumatuneGetMode(def->numatune, guestNode); + mode = virDomainNumatuneGetMode(def->numa, guestNode); if (pagesize == 0 || pagesize != system_page_size) { /* Find the huge page size we want to use */ @@ -4682,7 +4682,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, if (userNodeset) { nodemask = userNodeset; } else { - if (virDomainNumatuneMaybeGetNodeset(def->numatune, autoNodeset, + if (virDomainNumatuneMaybeGetNodeset(def->numa, autoNodeset, &nodemask, guestNode) < 0) goto cleanup; } @@ -7120,7 +7120,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, int ret = -1; const long system_page_size = virGetSystemPageSizeKB(); - if (virDomainNumatuneHasPerNodeBinding(def->numatune) && + if (virDomainNumatuneHasPerNodeBinding(def->numa) && !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -7138,7 +7138,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } - if (!virDomainNumatuneNodesetIsAvailable(def->numatune, auto_nodeset)) + if (!virDomainNumatuneNodesetIsAvailable(def->numa, auto_nodeset)) goto cleanup; for (i = 0; i < def->mem.nhugepages; i++) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 709f468..1c2ace9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4544,9 +4544,9 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, goto cleanup; } - if (virDomainNumatuneGetMode(vm->def->numatune, -1) == + if (virDomainNumatuneGetMode(vm->def->numa, -1) == VIR_DOMAIN_NUMATUNE_MEM_STRICT && - virDomainNumatuneMaybeFormatNodeset(vm->def->numatune, + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, priv->autoNodeset, &mem_mask, -1) < 0) goto cleanup; @@ -9404,7 +9404,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, size_t i = 0; int ret = -1; - if (virDomainNumatuneGetMode(vm->def->numatune, -1) != + if (virDomainNumatuneGetMode(vm->def->numa, -1) != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("change of nodeset for running domain " @@ -9533,7 +9533,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (mode != -1 && - virDomainNumatuneGetMode(vm->def->numatune, -1) != mode) { + virDomainNumatuneGetMode(vm->def->numa, -1) != mode) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't change numatune mode for running domain")); goto endjob; @@ -9543,7 +9543,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, qemuDomainSetNumaParamsLive(vm, nodeset) < 0) goto endjob; - if (virDomainNumatuneSet(&vm->def->numatune, + if (virDomainNumatuneSet(&vm->def->numa, vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, -1, mode, nodeset) < 0) @@ -9554,7 +9554,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainNumatuneSet(&persistentDef->numatune, + if (virDomainNumatuneSet(&persistentDef->numa, persistentDef->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, -1, mode, nodeset) < 0) @@ -9632,14 +9632,14 @@ qemuDomainGetNumaParameters(virDomainPtr dom, goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG) - param->value.i = virDomainNumatuneGetMode(persistentDef->numatune, -1); + param->value.i = virDomainNumatuneGetMode(persistentDef->numa, -1); else - param->value.i = virDomainNumatuneGetMode(vm->def->numatune, -1); + param->value.i = virDomainNumatuneGetMode(vm->def->numa, -1); break; case 1: /* fill numa nodeset here */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - nodeset = virDomainNumatuneFormatNodeset(persistentDef->numatune, + nodeset = virDomainNumatuneFormatNodeset(persistentDef->numa, NULL, -1); if (!nodeset) goto cleanup; @@ -9647,7 +9647,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom, if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY) || virCgroupGetCpusetMems(priv->cgroup, &nodeset) < 0) { - nodeset = virDomainNumatuneFormatNodeset(vm->def->numatune, + nodeset = virDomainNumatuneFormatNodeset(vm->def->numa, NULL, -1); if (!nodeset) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d4e957..61790f4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3257,8 +3257,8 @@ static int qemuProcessHook(void *data) if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0) goto cleanup; - mode = virDomainNumatuneGetMode(h->vm->def->numatune, -1); - nodeset = virDomainNumatuneGetNodeset(h->vm->def->numatune, + mode = virDomainNumatuneGetMode(h->vm->def->numa, -1); + nodeset = virDomainNumatuneGetNodeset(h->vm->def->numa, priv->autoNodeset, -1); if (virNumaSetupMemoryPolicy(mode, nodeset) < 0) -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
The structure will gradually become the only place for NUMA related config, thus rename it appropriately. --- src/conf/domain_conf.c | 10 +++--- src/conf/domain_conf.h | 2 +- src/conf/numa_conf.c | 78 +++++++++++++++++++++---------------------- src/conf/numa_conf.h | 34 +++++++++---------- src/libvirt_private.syms | 4 +-- src/lxc/lxc_cgroup.c | 4 +-- src/lxc/lxc_controller.c | 6 ++-- src/lxc/lxc_native.c | 2 +- src/parallels/parallels_sdk.c | 2 +- src/qemu/qemu_cgroup.c | 12 +++---- src/qemu/qemu_command.c | 8 ++--- src/qemu/qemu_driver.c | 20 +++++------ src/qemu/qemu_process.c | 4 +-- 13 files changed, 93 insertions(+), 93 deletions(-)
Interesting that only virDomainNumatuneFree and virDomainNumatuneEquals changes names, um err... I guess I should say my eyes thank you for not changing every function name too ;-) <...snip...>
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index d8f1739..bcb8023 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c
<...snip...>
@@ -572,8 +572,8 @@ virDomainNumatuneNodesEqual(virDomainNumatunePtr n1, }
bool -virDomainNumatuneEquals(virDomainNumatunePtr n1, - virDomainNumatunePtr n2) +virDomainNumaEquals(virDomainNumaPtr n1, + virDomainNumaPtr n2)
^^^^ Extraneous spacing
{ if (!n1 && !n2) return true;
ACK - John
On Thu, Feb 19, 2015 at 16:52:59 -0500, John Ferlan wrote:
On 02/16/2015 01:51 PM, Peter Krempa wrote:
The structure will gradually become the only place for NUMA related config, thus rename it appropriately. --- src/conf/domain_conf.c | 10 +++--- src/conf/domain_conf.h | 2 +- src/conf/numa_conf.c | 78 +++++++++++++++++++++---------------------- src/conf/numa_conf.h | 34 +++++++++---------- src/libvirt_private.syms | 4 +-- src/lxc/lxc_cgroup.c | 4 +-- src/lxc/lxc_controller.c | 6 ++-- src/lxc/lxc_native.c | 2 +- src/parallels/parallels_sdk.c | 2 +- src/qemu/qemu_cgroup.c | 12 +++---- src/qemu/qemu_command.c | 8 ++--- src/qemu/qemu_driver.c | 20 +++++------ src/qemu/qemu_process.c | 4 +-- 13 files changed, 93 insertions(+), 93 deletions(-)
Interesting that only virDomainNumatuneFree and virDomainNumatuneEquals changes names, um err... I guess I should say my eyes thank you for not changing every function name too ;-)
The rest of the functions I did not rename still deal only with data present in the <numatune> element so I feel they still can be called that way. The two I renamed are related to all the data. Peter
Name it virNumaMemAccess and add it to conf/numa_conf.[ch] Note that to avoid a circular dependency the type of the NUMA cell memAccess variable was changed to int. It will be turned back later after the circular dependency will not exist. --- src/conf/cpu_conf.c | 6 ------ src/conf/cpu_conf.h | 12 +----------- src/conf/numa_conf.c | 11 ++++++++--- src/conf/numa_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 10 +++++----- 5 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 28fbead..4923618 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -56,12 +56,6 @@ VIR_ENUM_IMPL(virCPUFeaturePolicy, VIR_CPU_FEATURE_LAST, "disable", "forbid") -VIR_ENUM_IMPL(virMemAccess, VIR_MEM_ACCESS_LAST, - "default", - "shared", - "private") - - void ATTRIBUTE_NONNULL(1) virCPUDefFreeModel(virCPUDefPtr def) { diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index d6efba7..0163384 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -83,16 +83,6 @@ typedef enum { VIR_ENUM_DECL(virCPUFeaturePolicy) -typedef enum { - VIR_MEM_ACCESS_DEFAULT, - VIR_MEM_ACCESS_SHARED, - VIR_MEM_ACCESS_PRIVATE, - - VIR_MEM_ACCESS_LAST, -} virMemAccess; - -VIR_ENUM_DECL(virMemAccess) - typedef struct _virCPUFeatureDef virCPUFeatureDef; typedef virCPUFeatureDef *virCPUFeatureDefPtr; struct _virCPUFeatureDef { @@ -105,7 +95,7 @@ typedef virCellDef *virCellDefPtr; struct _virCellDef { virBitmapPtr cpumask; /* CPUs that are part of this node */ unsigned long long mem; /* Node memory in kB */ - virMemAccess memAccess; + int memAccess; /* virNumaMemAccess */ }; typedef struct _virCPUDef virCPUDef; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index bcb8023..eea4172 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -43,6 +43,11 @@ VIR_ENUM_IMPL(virDomainNumatunePlacement, "static", "auto"); +VIR_ENUM_IMPL(virNumaMemAccess, VIR_NUMA_MEM_ACCESS_LAST, + "default", + "shared", + "private"); + typedef struct _virDomainNumaNode virDomainNumaNode; typedef virDomainNumaNode *virDomainNumaNodePtr; @@ -757,7 +762,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, goto cleanup; if ((tmp = virXMLPropString(nodes[i], "memAccess"))) { - if ((rc = virMemAccessTypeFromString(tmp)) <= 0) { + if ((rc = virNumaMemAccessTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid 'memAccess' attribute value '%s'"), tmp); @@ -783,7 +788,7 @@ int virDomainNumaDefCPUFormat(virBufferPtr buf, virCPUDefPtr def) { - virMemAccess memAccess; + virNumaMemAccess memAccess; char *cpustr; size_t i; @@ -805,7 +810,7 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, virBufferAddLit(buf, " unit='KiB'"); if (memAccess) virBufferAsprintf(buf, " memAccess='%s'", - virMemAccessTypeToString(memAccess)); + virNumaMemAccessTypeToString(memAccess)); virBufferAddLit(buf, "/>\n"); VIR_FREE(cpustr); } diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 0adeaa4..ca89e05 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -46,6 +46,15 @@ typedef enum { VIR_ENUM_DECL(virDomainNumatunePlacement) VIR_ENUM_DECL(virDomainNumatuneMemMode) +typedef enum { + VIR_NUMA_MEM_ACCESS_DEFAULT, + VIR_NUMA_MEM_ACCESS_SHARED, + VIR_NUMA_MEM_ACCESS_PRIVATE, + + VIR_NUMA_MEM_ACCESS_LAST +} virNumaMemAccess; + +VIR_ENUM_DECL(virNumaMemAccess) void virDomainNumaFree(virDomainNumaPtr numa); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c4ae596..b6fca9c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4558,7 +4558,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainHugePagePtr hugepage = NULL; virDomainNumatuneMemMode mode; const long system_page_size = virGetSystemPageSizeKB(); - virMemAccess memAccess = def->cpu->cells[guestNode].memAccess; + virNumaMemAccess memAccess = def->cpu->cells[guestNode].memAccess; size_t i; char *mem_path = NULL; virBitmapPtr nodemask = NULL; @@ -4651,18 +4651,18 @@ qemuBuildMemoryBackendStr(unsigned long long size, goto cleanup; switch (memAccess) { - case VIR_MEM_ACCESS_SHARED: + case VIR_NUMA_MEM_ACCESS_SHARED: if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0) goto cleanup; break; - case VIR_MEM_ACCESS_PRIVATE: + case VIR_NUMA_MEM_ACCESS_PRIVATE: if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0) goto cleanup; break; - case VIR_MEM_ACCESS_DEFAULT: - case VIR_MEM_ACCESS_LAST: + case VIR_NUMA_MEM_ACCESS_DEFAULT: + case VIR_NUMA_MEM_ACCESS_LAST: break; } } else { -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Name it virNumaMemAccess and add it to conf/numa_conf.[ch]
Note that to avoid a circular dependency the type of the NUMA cell memAccess variable was changed to int. It will be turned back later after the circular dependency will not exist. --- src/conf/cpu_conf.c | 6 ------ src/conf/cpu_conf.h | 12 +----------- src/conf/numa_conf.c | 11 ++++++++--- src/conf/numa_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 10 +++++----- 5 files changed, 23 insertions(+), 25 deletions(-)
ACK John
It's easier to recalculate the number in the one place it's used as having a separate varialbe to track it. It will also help with moving the NUMA code to the separate module. --- src/conf/cpu_conf.c | 1 - src/conf/cpu_conf.h | 1 - src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 23 +++++++++++++++++------ src/conf/numa_conf.h | 3 +++ tests/testutilsqemu.c | 1 - 6 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 4923618..0e3ce26 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -155,7 +155,6 @@ virCPUDefCopy(const virCPUDef *cpu) if (!copy->cells[i].cpumask) goto error; } - copy->cells_cpus = cpu->cells_cpus; } return copy; diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 0163384..d2563e2 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -117,7 +117,6 @@ struct _virCPUDef { virCPUFeatureDefPtr features; size_t ncells; virCellDefPtr cells; - unsigned int cells_cpus; }; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c9a65e1..870efc9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13501,7 +13501,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (def->cpu && - def->cpu->cells_cpus > def->maxvcpus) { + virDomainNumaGetCPUCountTotal(def->cpu) > def->maxvcpus) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Number of CPUs in <numa> exceeds the" " <vcpu> count")); diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index eea4172..41a7e01 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -713,7 +713,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, def->ncells = n; for (i = 0; i < n; i++) { - int rc, ncpus = 0; + int rc; unsigned int cur_cell = i; /* cells are in order of parsing or explicitly numbered */ @@ -748,12 +748,10 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, goto cleanup; } - ncpus = virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask, - VIR_DOMAIN_CPUMASK_LEN); - - if (ncpus <= 0) + if (virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask, + VIR_DOMAIN_CPUMASK_LEN) <= 0) goto cleanup; - def->cells_cpus += ncpus; + VIR_FREE(tmp); ctxt->node = nodes[i]; @@ -819,3 +817,16 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, return 0; } + + +unsigned int +virDomainNumaGetCPUCountTotal(virCPUDefPtr numa) +{ + size_t i; + unsigned int ret = 0; + + for (i = 0; i < numa->ncells; i++) + ret += virBitmapCountBits(numa->cells[i].cpumask); + + return ret; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index ca89e05..fa6b89b 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -125,4 +125,7 @@ bool virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune, int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt); int virDomainNumaDefCPUFormat(virBufferPtr buf, virCPUDefPtr def); +unsigned int virDomainNumaGetCPUCountTotal(virCPUDefPtr numa); + + #endif /* __NUMA_CONF_H__ */ diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 64f709a..4a3df8a 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -244,7 +244,6 @@ virCapsPtr testQemuCapsInit(void) host_cpu_features, /* features */ 0, /* ncells */ NULL, /* cells */ - 0, /* cells_cpus */ }; if ((caps = virCapabilitiesNew(host_cpu.arch, -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
It's easier to recalculate the number in the one place it's used as having a separate varialbe to track it. It will also help with moving
s/varialbe/variable
the NUMA code to the separate module. --- src/conf/cpu_conf.c | 1 - src/conf/cpu_conf.h | 1 - src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 23 +++++++++++++++++------ src/conf/numa_conf.h | 3 +++ tests/testutilsqemu.c | 1 - 6 files changed, 21 insertions(+), 10 deletions(-)
ACK John
Currently the code would exit without reporting an error as virBitmapParse reports one only if it fails to parse the bitmap, whereas the code was jumping to the error label even in case 0 cpus were correctly parsed in the map. --- src/conf/numa_conf.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 41a7e01..391ef15 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -749,9 +749,14 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, } if (virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask, - VIR_DOMAIN_CPUMASK_LEN) <= 0) + VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; + if (virBitmapIsAllClear(def->cells[cur_cell].cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("NUMA cell %d has no vCPUs assigned"), cur_cell); + goto cleanup; + } VIR_FREE(tmp); ctxt->node = nodes[i]; -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Currently the code would exit without reporting an error as virBitmapParse reports one only if it fails to parse the bitmap, whereas the code was jumping to the error label even in case 0 cpus were correctly parsed in the map. --- src/conf/numa_conf.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
ACK John
Make collapse few of the conditions so that the program flow is more clear. --- src/conf/numa_conf.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 391ef15..18c21d5 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -245,32 +245,24 @@ virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, goto cleanup; } - tmp = virXMLPropString(node, "mode"); - if (tmp) { - mode = virDomainNumatuneMemModeTypeFromString(tmp); - if (mode < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported NUMA memory tuning mode '%s'"), - tmp); - goto cleanup; - } + if ((tmp = virXMLPropString(node, "mode")) && + (mode = virDomainNumatuneMemModeTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported NUMA memory tuning mode '%s'"), tmp); + goto cleanup; } VIR_FREE(tmp); - tmp = virXMLPropString(node, "placement"); - if (tmp) { - placement = virDomainNumatunePlacementTypeFromString(tmp); - if (placement < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported NUMA memory placement mode '%s'"), - tmp); - goto cleanup; - } + if ((tmp = virXMLPropString(node, "placement")) && + (placement = virDomainNumatunePlacementTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported NUMA memory placement mode '%s'"), tmp); + goto cleanup; } VIR_FREE(tmp); - tmp = virXMLPropString(node, "nodeset"); - if (tmp && virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) + if ((tmp = virXMLPropString(node, "nodeset")) && + virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; VIR_FREE(tmp); -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Make collapse few of the conditions so that the program flow is more clear.
Reads awkwardly - I think remove the Make and you'll be OK
--- src/conf/numa_conf.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-)
ACK John
On Thu, Feb 19, 2015 at 17:07:51 -0500, John Ferlan wrote:
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Make collapse few of the conditions so that the program flow is more clear.
Reads awkwardly - I think remove the Make and you'll be OK
Huh, yeah. I probably got sidetracked while writing the commit message :). Peter
Shuffling around the logic will allow to simplify the code quite a bit. As an additional bonus the change in the logic now reports an error if automatic placement is selected and individual placement is configured. --- src/conf/numa_conf.c | 53 +++++++++++++++++++++------------------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 18c21d5..d5ba27f 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -229,42 +229,31 @@ virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, *numatunePtr = NULL; } - if (!node && placement_static) { - if (virDomainNumatuneNodeParseXML(numatunePtr, ncells, ctxt) < 0) - goto cleanup; - return 0; - } + if (!placement_static && !node) + placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO; - if (!node) { - /* We know that placement_mode is "auto" if we're here */ - ret = virDomainNumatuneSet(numatunePtr, - placement_static, - VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO, - -1, - NULL); - goto cleanup; - } + if (node) { + if ((tmp = virXMLPropString(node, "mode")) && + (mode = virDomainNumatuneMemModeTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported NUMA memory tuning mode '%s'"), tmp); + goto cleanup; + } + VIR_FREE(tmp); - if ((tmp = virXMLPropString(node, "mode")) && - (mode = virDomainNumatuneMemModeTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported NUMA memory tuning mode '%s'"), tmp); - goto cleanup; - } - VIR_FREE(tmp); + if ((tmp = virXMLPropString(node, "placement")) && + (placement = virDomainNumatunePlacementTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported NUMA memory placement mode '%s'"), tmp); + goto cleanup; + } + VIR_FREE(tmp); - if ((tmp = virXMLPropString(node, "placement")) && - (placement = virDomainNumatunePlacementTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported NUMA memory placement mode '%s'"), tmp); - goto cleanup; + if ((tmp = virXMLPropString(node, "nodeset")) && + virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + VIR_FREE(tmp); } - VIR_FREE(tmp); - - if ((tmp = virXMLPropString(node, "nodeset")) && - virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto cleanup; - VIR_FREE(tmp); if (virDomainNumatuneSet(numatunePtr, placement_static, -- 2.2.2
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Shuffling around the logic will allow to simplify the code quite a bit. As an additional bonus the change in the logic now reports an error if automatic placement is selected and individual placement is configured. --- src/conf/numa_conf.c | 53 +++++++++++++++++++++------------------------------- 1 file changed, 21 insertions(+), 32 deletions(-)
hmm... perhaps one extra thing that could be done... Now that the last/5th param to virDomainNumatuneSet isn't NULL for any caller (and the 4th isn't -1) - it looks like to me you can also modify numa_conf.h to add a ATTRIBUTE_NONNULL(5) for the function... ACK John
On Thu, Feb 19, 2015 at 17:25:13 -0500, John Ferlan wrote:
On 02/16/2015 01:51 PM, Peter Krempa wrote:
Shuffling around the logic will allow to simplify the code quite a bit. As an additional bonus the change in the logic now reports an error if automatic placement is selected and individual placement is configured. --- src/conf/numa_conf.c | 53 +++++++++++++++++++++------------------------------- 1 file changed, 21 insertions(+), 32 deletions(-)
hmm... perhaps one extra thing that could be done...
Now that the last/5th param to virDomainNumatuneSet isn't NULL for any caller (and the 4th isn't -1) - it looks like to me you can also modify numa_conf.h to add a ATTRIBUTE_NONNULL(5) for the function...
I'll do it separately as I don't fancy merge conflicts in a big series :)
ACK
Thanks. Peter
Do a content-aware check if formatting of the <numatune> element is necessary. Later on the def->numa structure will be always present so we cannot decide only on the basis whether it's allocated. --- src/conf/numa_conf.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index d5ba27f..359bdff 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -278,11 +278,22 @@ virDomainNumatuneFormatXML(virBufferPtr buf, { const char *tmp = NULL; char *nodeset = NULL; + bool nodesetSpecified = false; size_t i = 0; if (!numatune) return 0; + for (i = 0; i < numatune->nmem_nodes; i++) { + if (numatune->mem_nodes[i].nodeset) { + nodesetSpecified = true; + break; + } + } + + if (!nodesetSpecified && !numatune->memory.specified) + return 0; + virBufferAddLit(buf, "<numatune>\n"); virBufferAdjustIndent(buf, 2); -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Do a content-aware check if formatting of the <numatune> element is necessary. Later on the def->numa structure will be always present so we cannot decide only on the basis whether it's allocated. --- src/conf/numa_conf.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
ACK John
Move the existing virDomainDefNew to virDomainDefNewFull as it's setting a few things in the conf and re-introduce virDomainDefNew as a function without parameters for common use. --- src/conf/domain_conf.c | 20 ++++++++++++++++---- src/conf/domain_conf.h | 7 ++++--- src/libvirt_private.syms | 1 + src/xen/xen_hypervisor.c | 8 ++++---- src/xen/xend_internal.c | 4 ++-- src/xen/xm_internal.c | 4 ++-- 6 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 870efc9..420b713 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2314,13 +2314,25 @@ virDomainObjNew(virDomainXMLOptionPtr xmlopt) } -virDomainDefPtr virDomainDefNew(const char *name, - const unsigned char *uuid, - int id) +virDomainDefPtr +virDomainDefNew(void) +{ + virDomainDefPtr ret; + + ignore_value(VIR_ALLOC(ret)); + + return ret; +} + + +virDomainDefPtr +virDomainDefNewFull(const char *name, + const unsigned char *uuid, + int id) { virDomainDefPtr def; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) return NULL; if (VIR_STRDUP(def->name, name) < 0) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1e04886..86db2ab 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2414,9 +2414,10 @@ void virDomainDefFree(virDomainDefPtr vm); virDomainChrDefPtr virDomainChrDefNew(void); -virDomainDefPtr virDomainDefNew(const char *name, - const unsigned char *uuid, - int id); +virDomainDefPtr virDomainDefNew(void); +virDomainDefPtr virDomainDefNewFull(const char *name, + const unsigned char *uuid, + int id); enum { VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0), diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 278124c..4ba2142 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -204,6 +204,7 @@ virDomainDefMaybeAddController; virDomainDefMaybeAddInput; virDomainDefNeedsPlacementAdvice; virDomainDefNew; +virDomainDefNewFull; virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 31a2a1b..bc498ff 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -2634,9 +2634,9 @@ xenHypervisorLookupDomainByID(virConnectPtr conn, int id) if (!name) return NULL; - ret = virDomainDefNew(name, - XEN_GETDOMAININFO_UUID(dominfo), - id); + ret = virDomainDefNewFull(name, + XEN_GETDOMAININFO_UUID(dominfo), + id); VIR_FREE(name); return ret; } @@ -2699,7 +2699,7 @@ xenHypervisorLookupDomainByUUID(virConnectPtr conn, const unsigned char *uuid) if (!name) return NULL; - ret = virDomainDefNew(name, uuid, id); + ret = virDomainDefNewFull(name, uuid, id); if (ret) ret->id = id; VIR_FREE(name); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index c2b9098..ab03c1c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1152,7 +1152,7 @@ sexpr_to_domain(virConnectPtr conn, const struct sexpr *root) if (tmp) id = sexpr_int(root, "domain/domid"); - return virDomainDefNew(name, uuid, id); + return virDomainDefNewFull(name, uuid, id); error: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2117,7 +2117,7 @@ xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) if (name == NULL) return NULL; - ret = virDomainDefNew(name, uuid, id); + ret = virDomainDefNewFull(name, uuid, id); VIR_FREE(name); return ret; diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index dc6f88a..354ab3f 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -844,7 +844,7 @@ xenXMDomainLookupByName(virConnectPtr conn, const char *domname) if (!(entry = virHashLookup(priv->configCache, filename))) goto cleanup; - ret = virDomainDefNew(domname, entry->def->uuid, -1); + ret = virDomainDefNewFull(domname, entry->def->uuid, -1); cleanup: xenUnifiedUnlock(priv); @@ -887,7 +887,7 @@ xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) if (!(entry = virHashSearch(priv->configCache, xenXMDomainSearchForUUID, (const void *)uuid))) goto cleanup; - ret = virDomainDefNew(entry->def->name, uuid, -1); + ret = virDomainDefNewFull(entry->def->name, uuid, -1); cleanup: xenUnifiedUnlock(priv); -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Move the existing virDomainDefNew to virDomainDefNewFull as it's setting a few things in the conf and re-introduce virDomainDefNew as a function without parameters for common use. --- src/conf/domain_conf.c | 20 ++++++++++++++++---- src/conf/domain_conf.h | 7 ++++--- src/libvirt_private.syms | 1 + src/xen/xen_hypervisor.c | 8 ++++---- src/xen/xend_internal.c | 4 ++-- src/xen/xm_internal.c | 4 ++-- 6 files changed, 29 insertions(+), 15 deletions(-)
ACK John
Use the virDomainDefNew() helper to allocate the definition instead of doing it via VIR_ALLOC. --- src/conf/domain_conf.c | 2 +- src/lxc/lxc_native.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_sdk.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_command.c | 2 +- src/vbox/vbox_common.c | 8 ++++---- src/vmx/vmx.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- src/xenconfig/xen_xl.c | 2 +- src/xenconfig/xen_xm.c | 2 +- tests/openvzutilstest.c | 2 +- tests/securityselinuxtest.c | 2 +- 13 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 420b713..6dea109 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12910,7 +12910,7 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(schema); } - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) return NULL; if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index cd3b86b..99613af 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -1002,7 +1002,7 @@ lxcParseConfigString(const char *config) if (!(properties = virConfReadMem(config, 0, VIR_CONF_FLAG_LXC_FORMAT))) return NULL; - if (VIR_ALLOC(vmdef) < 0) + if (!(vmdef = virDomainDefNew())) goto error; if (virUUIDGenerate(vmdef->uuid) < 0) { diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index f955dda..848e230 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -543,7 +543,7 @@ int openvzLoadDomains(struct openvz_driver *driver) } *line++ = '\0'; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto cleanup; def->virtType = VIR_DOMAIN_VIRT_OPENVZ; diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 8c05b32..d5ea00a 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1186,7 +1186,7 @@ prlsdkLoadDomain(parallelsConnPtr privconn, virCheckNonNullArgGoto(privconn, error); virCheckNonNullArgGoto(sdkdom, error); - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto error; if (!olddom) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index d05f897..d69e29c 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1708,7 +1708,7 @@ phypDomainAttachDevice(virDomainPtr domain, const char *xml) virBuffer buf = VIR_BUFFER_INITIALIZER; char *domain_name = NULL; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto cleanup; domain_name = escape_specialcharacters(domain->name); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b6fca9c..8b660f0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11867,7 +11867,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, return NULL; } - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto error; /* allocate the cmdlinedef up-front; if it's unused, we'll free it later */ diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index deca490..55d3624 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3860,7 +3860,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (openSessionForMachine(data, dom->uuid, &iid, &machine, false) < 0) goto cleanup; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto cleanup; gVBoxAPI.UIMachine.GetAccessible(machine, &accessible); @@ -4114,7 +4114,7 @@ static int vboxDomainAttachDeviceImpl(virDomainPtr dom, return ret; VBOX_IID_INITIALIZE(&iid); - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) return ret; if (VIR_STRDUP(def->os.type, "hvm") < 0) @@ -4246,7 +4246,7 @@ static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml) return ret; VBOX_IID_INITIALIZE(&iid); - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) return ret; if (VIR_STRDUP(def->os.type, "hvm") < 0) @@ -6032,7 +6032,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) goto cleanup; - if (VIR_ALLOC(def) < 0 || VIR_ALLOC(def->dom) < 0) + if (VIR_ALLOC(def) < 0 || !(def->dom = virDomainDefNew())) goto cleanup; if (VIR_STRDUP(def->name, snapshot->name) < 0) goto cleanup; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 2a794c7..ac2542a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1298,7 +1298,7 @@ virVMXParseConfig(virVMXContext *ctx, } /* Allocate domain def */ - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto cleanup; def->virtType = VIR_DOMAIN_VIRT_VMWARE; diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 554d28f..3e18a7e 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -1093,7 +1093,7 @@ xenParseSxpr(const struct sexpr *root, virDomainDefPtr def; int hvm = 0, vmlocaltime; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto error; tmp = sexpr_node(root, "domain/domid"); diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 7913118..95ef5f4 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -295,7 +295,7 @@ xenParseXL(virConfPtr conf, virCapsPtr caps, int xendConfigVersion) { virDomainDefPtr def = NULL; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) return NULL; def->virtType = VIR_DOMAIN_VIRT_XEN; diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 1e57e24..2f57cd2 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -375,7 +375,7 @@ xenParseXM(virConfPtr conf, { virDomainDefPtr def = NULL; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) return NULL; def->virtType = VIR_DOMAIN_VIRT_XEN; diff --git a/tests/openvzutilstest.c b/tests/openvzutilstest.c index 5f87359..f9d1002 100644 --- a/tests/openvzutilstest.c +++ b/tests/openvzutilstest.c @@ -102,7 +102,7 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED) " </devices>\n" "</domain>\n"; - if (VIR_ALLOC(def) < 0 || + if (!(def = virDomainDefNew()) || VIR_STRDUP(def->os.type, "exe") < 0 || VIR_STRDUP(def->os.init, "/sbin/init") < 0) goto cleanup; diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index 3b5c3e5..38ab70e 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -70,7 +70,7 @@ testBuildDomainDef(bool dynamic, virDomainDefPtr def; virSecurityLabelDefPtr secdef; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainDefNew())) goto error; if (VIR_ALLOC_N(def->seclabels, 1) < 0) -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Use the virDomainDefNew() helper to allocate the definition instead of doing it via VIR_ALLOC. --- src/conf/domain_conf.c | 2 +- src/lxc/lxc_native.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_sdk.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_command.c | 2 +- src/vbox/vbox_common.c | 8 ++++---- src/vmx/vmx.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- src/xenconfig/xen_xl.c | 2 +- src/xenconfig/xen_xm.c | 2 +- tests/openvzutilstest.c | 2 +- tests/securityselinuxtest.c | 2 +- 13 files changed, 16 insertions(+), 16 deletions(-)
ACK (and hopefully no one snuck a new one it - too bad there wasn't an "easy" way to make a check rule on this) John
Since our formatter now handles well if the config is allocated and not filled and the parser always frees the definition before parsing we can safely always-allocate the NUMA config. This will help in later patches as the parser will be refactored to just fill the data. --- src/conf/domain_conf.c | 10 +++++++++- src/conf/numa_conf.c | 11 +++++++++++ src/conf/numa_conf.h | 1 + src/libvirt_private.syms | 1 + 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6dea109..83c5fd9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2319,9 +2319,17 @@ virDomainDefNew(void) { virDomainDefPtr ret; - ignore_value(VIR_ALLOC(ret)); + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (!(ret->numa = virDomainNumaNew())) + goto error; return ret; + + error: + virDomainDefFree(ret); + return NULL; } diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 359bdff..2a5fb3c 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -827,3 +827,14 @@ virDomainNumaGetCPUCountTotal(virCPUDefPtr numa) return ret; } + + +virDomainNumaPtr +virDomainNumaNew(void) +{ + virDomainNumaPtr ret = NULL; + + ignore_value(VIR_ALLOC(ret)); + + return ret; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index fa6b89b..e69489d 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -56,6 +56,7 @@ typedef enum { VIR_ENUM_DECL(virNumaMemAccess) +virDomainNumaPtr virDomainNumaNew(void); void virDomainNumaFree(virDomainNumaPtr numa); /* diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ba2142..6a746cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -629,6 +629,7 @@ virNodeDeviceObjUnlock; # conf/numa_conf.h virDomainNumaEquals; virDomainNumaFree; +virDomainNumaNew; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; virDomainNumatuneGetMode; -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Since our formatter now handles well if the config is allocated and not filled and the parser always frees the definition before parsing we can safely always-allocate the NUMA config.
This will help in later patches as the parser will be refactored to just fill the data. --- src/conf/domain_conf.c | 10 +++++++++- src/conf/numa_conf.c | 11 +++++++++++ src/conf/numa_conf.h | 1 + src/libvirt_private.syms | 1 + 4 files changed, 22 insertions(+), 1 deletion(-)
Hmm... well this one was interesting, but it seems from the following patch and the number of places that call into the numa* API's that one was pretty much allocated all the time before, right? It wasn't clear and I'm inclined to suggest this and the following patch be combined just to make it clearer... ACK - John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6dea109..83c5fd9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2319,9 +2319,17 @@ virDomainDefNew(void) { virDomainDefPtr ret;
- ignore_value(VIR_ALLOC(ret)); + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (!(ret->numa = virDomainNumaNew())) + goto error;
return ret; + + error: + virDomainDefFree(ret); + return NULL; }
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 359bdff..2a5fb3c 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -827,3 +827,14 @@ virDomainNumaGetCPUCountTotal(virCPUDefPtr numa)
return ret; } + + +virDomainNumaPtr +virDomainNumaNew(void) +{ + virDomainNumaPtr ret = NULL; + + ignore_value(VIR_ALLOC(ret)); + + return ret; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index fa6b89b..e69489d 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -56,6 +56,7 @@ typedef enum {
VIR_ENUM_DECL(virNumaMemAccess)
+virDomainNumaPtr virDomainNumaNew(void); void virDomainNumaFree(virDomainNumaPtr numa);
/* diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ba2142..6a746cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -629,6 +629,7 @@ virNodeDeviceObjUnlock; # conf/numa_conf.h virDomainNumaEquals; virDomainNumaFree; +virDomainNumaNew; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; virDomainNumatuneGetMode;
As the numa object is now always present we can avoid the ad-hoc allocation code. --- src/conf/numa_conf.c | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 2a5fb3c..70a38d6 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -117,11 +117,6 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, goto cleanup; } - if (!numatune && VIR_ALLOC(numatune) < 0) - goto cleanup; - - *numatunePtr = numatune; - VIR_FREE(numatune->mem_nodes); if (VIR_ALLOC_N(numatune->mem_nodes, ncells) < 0) goto cleanup; @@ -224,11 +219,6 @@ virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, node = virXPathNode("./numatune/memory[1]", ctxt); - if (*numatunePtr) { - virDomainNumaFree(*numatunePtr); - *numatunePtr = NULL; - } - if (!placement_static && !node) placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO; @@ -454,27 +444,20 @@ virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, int mode, virBitmapPtr nodeset) { - bool created = false; int ret = -1; - virDomainNumaPtr numatune; + virDomainNumaPtr numatune = *numatunePtr; /* No need to do anything in this case */ if (mode == -1 && placement == -1 && !nodeset) return 0; - if (!(*numatunePtr)) { - if (VIR_ALLOC(*numatunePtr) < 0) - goto cleanup; - - created = true; + if (!numatune->memory.specified) { if (mode == -1) mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; if (placement == -1) placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_DEFAULT; } - numatune = *numatunePtr; - /* Range checks */ if (mode != -1 && (mode < 0 || mode >= VIR_DOMAIN_NUMATUNE_MEM_LAST)) { @@ -534,11 +517,6 @@ virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, ret = 0; cleanup: - if (ret < 0 && created) { - virDomainNumaFree(*numatunePtr); - *numatunePtr = NULL; - } - return ret; } -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
As the numa object is now always present we can avoid the ad-hoc allocation code. --- src/conf/numa_conf.c | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-)
ACK - although I think this could/should be combined with 15/24... John
On Thu, Feb 19, 2015 at 18:17:08 -0500, John Ferlan wrote:
On 02/16/2015 01:52 PM, Peter Krempa wrote:
As the numa object is now always present we can avoid the ad-hoc allocation code. --- src/conf/numa_conf.c | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-)
ACK - although I think this could/should be combined with 15/24...
will do. It seems to make sense that way. Peter
As virDomainNumatuneSet now doesn't allocate the virDomainNuma object any longer it's not necessary to pass the pointer to a pointer to store the object as it will not change any longer. While touching the parameter definitions I've also changed the name of the parameter to "numa". --- src/conf/numa_conf.c | 28 +++++++++++++--------------- src/conf/numa_conf.h | 2 +- src/lxc/lxc_native.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 70a38d6..1b8232e 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -245,7 +245,7 @@ virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, VIR_FREE(tmp); } - if (virDomainNumatuneSet(numatunePtr, + if (virDomainNumatuneSet(*numatunePtr, placement_static, placement, mode, @@ -438,20 +438,19 @@ virDomainNumatuneMaybeFormatNodeset(virDomainNumaPtr numatune, } int -virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, +virDomainNumatuneSet(virDomainNumaPtr numa, bool placement_static, int placement, int mode, virBitmapPtr nodeset) { int ret = -1; - virDomainNumaPtr numatune = *numatunePtr; /* No need to do anything in this case */ if (mode == -1 && placement == -1 && !nodeset) return 0; - if (!numatune->memory.specified) { + if (!numa->memory.specified) { if (mode == -1) mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; if (placement == -1) @@ -476,26 +475,25 @@ virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, } if (mode != -1) - numatune->memory.mode = mode; + numa->memory.mode = mode; if (nodeset) { - virBitmapFree(numatune->memory.nodeset); - numatune->memory.nodeset = virBitmapNewCopy(nodeset); - if (!numatune->memory.nodeset) + virBitmapFree(numa->memory.nodeset); + if (!(numa->memory.nodeset = virBitmapNewCopy(nodeset))) goto cleanup; if (placement == -1) placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC; } if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_DEFAULT) { - if (numatune->memory.nodeset || placement_static) + if (numa->memory.nodeset || placement_static) placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC; else placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO; } if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC && - !numatune->memory.nodeset) { + !numa->memory.nodeset) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nodeset for NUMA memory tuning must be set " "if 'placement' is 'static'")); @@ -504,15 +502,15 @@ virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, /* setting nodeset when placement auto is invalid */ if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO && - numatune->memory.nodeset) { - virBitmapFree(numatune->memory.nodeset); - numatune->memory.nodeset = NULL; + numa->memory.nodeset) { + virBitmapFree(numa->memory.nodeset); + numa->memory.nodeset = NULL; } if (placement != -1) - numatune->memory.placement = placement; + numa->memory.placement = placement; - numatune->memory.specified = true; + numa->memory.specified = true; ret = 0; diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index e69489d..790f3bf 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -101,7 +101,7 @@ int virDomainNumatuneMaybeFormatNodeset(virDomainNumaPtr numatune, /* * Setters */ -int virDomainNumatuneSet(virDomainNumaPtr *numatunePtr, +int virDomainNumatuneSet(virDomainNumaPtr numa, bool placement_static, int placement, int mode, diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 99613af..abf07ce 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -847,7 +847,7 @@ lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties) value->str) { if (virBitmapParse(value->str, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) return -1; - if (virDomainNumatuneSet(&def->numa, + if (virDomainNumatuneSet(def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c2ace9..0706303 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9543,7 +9543,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, qemuDomainSetNumaParamsLive(vm, nodeset) < 0) goto endjob; - if (virDomainNumatuneSet(&vm->def->numa, + if (virDomainNumatuneSet(vm->def->numa, vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, -1, mode, nodeset) < 0) @@ -9554,7 +9554,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainNumatuneSet(&persistentDef->numa, + if (virDomainNumatuneSet(persistentDef->numa, persistentDef->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, -1, mode, nodeset) < 0) -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
As virDomainNumatuneSet now doesn't allocate the virDomainNuma object any longer it's not necessary to pass the pointer to a pointer to store the object as it will not change any longer.
While touching the parameter definitions I've also changed the name of the parameter to "numa". --- src/conf/numa_conf.c | 28 +++++++++++++--------------- src/conf/numa_conf.h | 2 +- src/lxc/lxc_native.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- 4 files changed, 17 insertions(+), 19 deletions(-)
ACK John
virDomainNumatuneParseXML now doesn't allocate the def->numa object any longer so we don't need to pass a double pointer. --- src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 23 +++++++++++------------ src/conf/numa_conf.h | 2 +- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 83c5fd9..ceaf092 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13528,7 +13528,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if (virDomainNumatuneParseXML(&def->numa, + if (virDomainNumatuneParseXML(def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, def->cpu ? def->cpu->ncells : 0, diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 1b8232e..8adec6f 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -82,7 +82,7 @@ virDomainNumatuneNodeSpecified(virDomainNumaPtr numatune, } static int -virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, +virDomainNumatuneNodeParseXML(virDomainNumaPtr numa, size_t ncells, xmlXPathContextPtr ctxt) { @@ -90,7 +90,6 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, int n = 0; int ret = -1; size_t i = 0; - virDomainNumaPtr numatune = *numatunePtr; xmlNodePtr *nodes = NULL; if ((n = virXPathNodeSet("./numatune/memnode", ctxt, &nodes)) < 0) { @@ -102,8 +101,8 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, if (!n) return 0; - if (numatune && numatune->memory.specified && - numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO) { + if (numa->memory.specified && + numa->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Per-node binding is not compatible with " "automatic NUMA placement.")); @@ -117,11 +116,11 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, goto cleanup; } - VIR_FREE(numatune->mem_nodes); - if (VIR_ALLOC_N(numatune->mem_nodes, ncells) < 0) + VIR_FREE(numa->mem_nodes); + if (VIR_ALLOC_N(numa->mem_nodes, ncells) < 0) goto cleanup; - numatune->nmem_nodes = ncells; + numa->nmem_nodes = ncells; for (i = 0; i < n; i++) { int mode = 0; @@ -144,14 +143,14 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, } VIR_FREE(tmp); - if (cellid >= numatune->nmem_nodes) { + if (cellid >= numa->nmem_nodes) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Argument 'cellid' in memnode element must " "correspond to existing guest's NUMA cell")); goto cleanup; } - mem_node = &numatune->mem_nodes[cellid]; + mem_node = &numa->mem_nodes[cellid]; if (mem_node->nodeset) { virReportError(VIR_ERR_XML_ERROR, @@ -194,7 +193,7 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr *numatunePtr, } int -virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, +virDomainNumatuneParseXML(virDomainNumaPtr numa, bool placement_static, size_t ncells, xmlXPathContextPtr ctxt) @@ -245,14 +244,14 @@ virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, VIR_FREE(tmp); } - if (virDomainNumatuneSet(*numatunePtr, + if (virDomainNumatuneSet(numa, placement_static, placement, mode, nodeset) < 0) goto cleanup; - if (virDomainNumatuneNodeParseXML(numatunePtr, ncells, ctxt) < 0) + if (virDomainNumatuneNodeParseXML(numa, ncells, ctxt) < 0) goto cleanup; ret = 0; diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 790f3bf..69eccf2 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -62,7 +62,7 @@ void virDomainNumaFree(virDomainNumaPtr numa); /* * XML Parse/Format functions */ -int virDomainNumatuneParseXML(virDomainNumaPtr *numatunePtr, +int virDomainNumatuneParseXML(virDomainNumaPtr numa, bool placement_static, size_t ncells, xmlXPathContextPtr ctxt) -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
virDomainNumatuneParseXML now doesn't allocate the def->numa object any longer so we don't need to pass a double pointer. --- src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 23 +++++++++++------------ src/conf/numa_conf.h | 2 +- 3 files changed, 13 insertions(+), 14 deletions(-)
You could probably also just pass "def" to virDomainNumatuneParseXML and let it handle the checking placement_mode and cpu rather than doing it as part of the caller. ACK to what's here though. John
The function uses the cell count in 6 places. Add a temp variable to hold the count as it will greatly simplify the refactor. --- src/qemu/qemu_command.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8b660f0..65d8874 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7118,6 +7118,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, bool needBackend = false; int rc; int ret = -1; + size_t ncells = def->cpu->ncells; const long system_page_size = virGetSystemPageSizeKB(); if (virDomainNumatuneHasPerNodeBinding(def->numa) && @@ -7150,10 +7151,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, continue; } - if (def->cpu->ncells) { + if (ncells) { /* Fortunately, we allow only guest NUMA nodes to be continuous * starting from zero. */ - pos = def->cpu->ncells - 1; + pos = ncells - 1; } next_bit = virBitmapNextSetBit(def->mem.hugepages[i].nodemask, pos); @@ -7165,12 +7166,12 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } } - if (VIR_ALLOC_N(nodeBackends, def->cpu->ncells) < 0) + if (VIR_ALLOC_N(nodeBackends, ncells) < 0) goto cleanup; /* using of -numa memdev= cannot be combined with -numa mem=, thus we * need to check which approach to use */ - for (i = 0; i < def->cpu->ncells; i++) { + for (i = 0; i < ncells; i++) { unsigned long long cellmem = VIR_DIV_UP(def->cpu->cells[i].mem, 1024); def->cpu->cells[i].mem = cellmem * 1024; @@ -7193,7 +7194,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } } - for (i = 0; i < def->cpu->ncells; i++) { + for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); if (!(cpumask = virBitmapFormat(def->cpu->cells[i].cpumask))) goto cleanup; @@ -7232,7 +7233,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, VIR_FREE(cpumask); if (nodeBackends) { - for (i = 0; i < def->cpu->ncells; i++) + for (i = 0; i < ncells; i++) VIR_FREE(nodeBackends[i]); VIR_FREE(nodeBackends); -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
The function uses the cell count in 6 places. Add a temp variable to hold the count as it will greatly simplify the refactor. --- src/qemu/qemu_command.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
ACK - had to peek forward a few patches to "see" why this becomes important though ;-)... I did ask myself - was there anything from the time it's defined/pulled from def->cpu->ncells to each time it was where the 'ncells' could change (I don't think so, but it does cross my mind when I see these). John
On Thu, Feb 19, 2015 at 18:38:38 -0500, John Ferlan wrote:
On 02/16/2015 01:52 PM, Peter Krempa wrote:
The function uses the cell count in 6 places. Add a temp variable to hold the count as it will greatly simplify the refactor. --- src/qemu/qemu_command.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
ACK - had to peek forward a few patches to "see" why this becomes important though ;-)... I did ask myself - was there anything from the time it's defined/pulled from def->cpu->ncells to each time it was where the 'ncells' could change (I don't think so, but it does cross my mind when I see these).
In that case the code would actually be a terrible spaghetti mess and would deserve an even bigger refactor :). In fact ncells is changed only when parsing the domain XML so this change is safe. Peter
Add an accessor so that a later refactor is simpler. --- src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 15 +++++++++++++-- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 6 +++--- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ceaf092..d34b9c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13531,7 +13531,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainNumatuneParseXML(def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, - def->cpu ? def->cpu->ncells : 0, + virDomainNumaGetNodeCount(def->cpu), ctxt) < 0) goto error; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 8adec6f..61dfea0 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -760,14 +760,15 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, { virNumaMemAccess memAccess; char *cpustr; + size_t ncells = virDomainNumaGetNodeCount(def); size_t i; - if (def->ncells == 0) + if (ncells == 0) return 0; virBufferAddLit(buf, "<numa>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < def->ncells; i++) { + for (i = 0; i < ncells; i++) { memAccess = def->cells[i].memAccess; if (!(cpustr = virBitmapFormat(def->cells[i].cpumask))) @@ -813,3 +814,13 @@ virDomainNumaNew(void) return ret; } + + +size_t +virDomainNumaGetNodeCount(virCPUDefPtr numa) +{ + if (!numa) + return 0; + + return numa->ncells; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 69eccf2..55a9fbe 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -86,6 +86,8 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, virBitmapPtr *retNodeset, int cellid); +size_t virDomainNumaGetNodeCount(virCPUDefPtr numa); + /* * Formatters */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6a746cf..4ba5fd0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -629,6 +629,7 @@ virNodeDeviceObjUnlock; # conf/numa_conf.h virDomainNumaEquals; virDomainNumaFree; +virDomainNumaGetNodeCount; virDomainNumaNew; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 65d8874..f009570 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7118,7 +7118,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, bool needBackend = false; int rc; int ret = -1; - size_t ncells = def->cpu->ncells; + size_t ncells = virDomainNumaGetNodeCount(def->cpu); const long system_page_size = virGetSystemPageSizeKB(); if (virDomainNumatuneHasPerNodeBinding(def->numa) && @@ -8315,7 +8315,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-m"); def->mem.max_balloon = VIR_DIV_UP(def->mem.max_balloon, 1024) * 1024; virCommandAddArgFormat(cmd, "%llu", def->mem.max_balloon / 1024); - if (def->mem.nhugepages && (!def->cpu || !def->cpu->ncells)) { + if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->cpu)) { const long system_page_size = virGetSystemPageSizeKB(); char *mem_path = NULL; @@ -8395,7 +8395,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (def->cpu && def->cpu->ncells) + if (virDomainNumaGetNodeCount(def->cpu)) if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) goto error; -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Add an accessor so that a later refactor is simpler. --- src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 15 +++++++++++++-- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 6 +++--- 5 files changed, 20 insertions(+), 6 deletions(-)
"Short term" there's a "naming" thing between "numa" and "cpu" in virDomainNumaGetNodeCount... (struct is CPUDef while name is numa) ACK - John
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Add an accessor so that a later refactor is simpler. --- src/conf/domain_conf.c | 2 +- src/conf/numa_conf.c | 15 +++++++++++++-- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 6 +++--- 5 files changed, 20 insertions(+), 6 deletions(-)
Doh... Light dawns... All the new accessor's should have a ATTRIBUTE_NONNULL(1) in the numa_conf.h Something you may want to check for any new function up through here and of course the next two patches at least... John
Add virDomainNumaGetNodeCpumask() and refactor a few places that would get the cpu mask without the helper. --- src/conf/numa_conf.c | 12 ++++++++++-- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 61dfea0..acda415 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -771,7 +771,7 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, for (i = 0; i < ncells; i++) { memAccess = def->cells[i].memAccess; - if (!(cpustr = virBitmapFormat(def->cells[i].cpumask))) + if (!(cpustr = virBitmapFormat(virDomainNumaGetNodeCpumask(def, i)))) return -1; virBufferAddLit(buf, "<cell"); @@ -799,7 +799,7 @@ virDomainNumaGetCPUCountTotal(virCPUDefPtr numa) unsigned int ret = 0; for (i = 0; i < numa->ncells; i++) - ret += virBitmapCountBits(numa->cells[i].cpumask); + ret += virBitmapCountBits(virDomainNumaGetNodeCpumask(numa, i)); return ret; } @@ -824,3 +824,11 @@ virDomainNumaGetNodeCount(virCPUDefPtr numa) return numa->ncells; } + + +virBitmapPtr +virDomainNumaGetNodeCpumask(virCPUDefPtr numa, + size_t node) +{ + return numa->cells[node].cpumask; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 55a9fbe..ab88ab7 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -87,6 +87,8 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, int cellid); size_t virDomainNumaGetNodeCount(virCPUDefPtr numa); +virBitmapPtr virDomainNumaGetNodeCpumask(virCPUDefPtr numa, + size_t node); /* * Formatters diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ba5fd0..770803b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -630,6 +630,7 @@ virNodeDeviceObjUnlock; virDomainNumaEquals; virDomainNumaFree; virDomainNumaGetNodeCount; +virDomainNumaGetNodeCpumask; virDomainNumaNew; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f009570..d276da3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7196,7 +7196,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); - if (!(cpumask = virBitmapFormat(def->cpu->cells[i].cpumask))) + if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->cpu, i)))) goto cleanup; if (strchr(cpumask, ',') && -- 2.2.2
$subj: s/accesor/accessor (even though my spell checker dislikes both!) On 02/16/2015 01:52 PM, Peter Krempa wrote:
Add virDomainNumaGetNodeCpumask() and refactor a few places that would get the cpu mask without the helper. --- src/conf/numa_conf.c | 12 ++++++++++-- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-)
ACK John
--- src/conf/numa_conf.c | 10 +++++++++- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 5 +++-- 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index acda415..4906687 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -769,7 +769,7 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, virBufferAddLit(buf, "<numa>\n"); virBufferAdjustIndent(buf, 2); for (i = 0; i < ncells; i++) { - memAccess = def->cells[i].memAccess; + memAccess = virDomainNumaGetNodeMemoryAccessMode(def, i); if (!(cpustr = virBitmapFormat(virDomainNumaGetNodeCpumask(def, i)))) return -1; @@ -832,3 +832,11 @@ virDomainNumaGetNodeCpumask(virCPUDefPtr numa, { return numa->cells[node].cpumask; } + + +virNumaMemAccess +virDomainNumaGetNodeMemoryAccessMode(virCPUDefPtr numa, + size_t node) +{ + return numa->cells[node].memAccess; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index ab88ab7..0ea2c93 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -89,6 +89,8 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, size_t virDomainNumaGetNodeCount(virCPUDefPtr numa); virBitmapPtr virDomainNumaGetNodeCpumask(virCPUDefPtr numa, size_t node); +virNumaMemAccess virDomainNumaGetNodeMemoryAccessMode(virCPUDefPtr numa, + size_t node); /* * Formatters diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 770803b..1f1ce14 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -631,6 +631,7 @@ virDomainNumaEquals; virDomainNumaFree; virDomainNumaGetNodeCount; virDomainNumaGetNodeCpumask; +virDomainNumaGetNodeMemoryAccessMode; virDomainNumaNew; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d276da3..05545ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4558,7 +4558,8 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainHugePagePtr hugepage = NULL; virDomainNumatuneMemMode mode; const long system_page_size = virGetSystemPageSizeKB(); - virNumaMemAccess memAccess = def->cpu->cells[guestNode].memAccess; + virNumaMemAccess memAccess = virDomainNumaGetNodeMemoryAccessMode(def->cpu, guestNode); + size_t i; char *mem_path = NULL; virBitmapPtr nodemask = NULL; @@ -7185,7 +7186,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, if (rc == 0) needBackend = true; } else { - if (def->cpu->cells[i].memAccess) { + if (virDomainNumaGetNodeMemoryAccessMode(def->cpu, i)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Shared memory mapping is not supported " "with this QEMU")); -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
--- src/conf/numa_conf.c | 10 +++++++++- src/conf/numa_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 5 +++-- 4 files changed, 15 insertions(+), 3 deletions(-)
ACK John
Add the helpers and refactor places where the value is accessed without them. --- src/conf/numa_conf.c | 20 +++++++++++++++++++- src/conf/numa_conf.h | 7 +++++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 10 ++++++---- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 4906687..ee7e65d 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -777,7 +777,8 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, virBufferAddLit(buf, "<cell"); virBufferAsprintf(buf, " id='%zu'", i); virBufferAsprintf(buf, " cpus='%s'", cpustr); - virBufferAsprintf(buf, " memory='%llu'", def->cells[i].mem); + virBufferAsprintf(buf, " memory='%llu'", + virDomainNumaGetNodeMemorySize(def, i)); virBufferAddLit(buf, " unit='KiB'"); if (memAccess) virBufferAsprintf(buf, " memAccess='%s'", @@ -840,3 +841,20 @@ virDomainNumaGetNodeMemoryAccessMode(virCPUDefPtr numa, { return numa->cells[node].memAccess; } + + +unsigned long long +virDomainNumaGetNodeMemorySize(virCPUDefPtr numa, + size_t node) +{ + return numa->cells[node].mem; +} + + +void +virDomainNumaSetNodeMemorySize(virCPUDefPtr numa, + size_t node, + unsigned long long size) +{ + numa->cells[node].mem = size; +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 0ea2c93..eadab43 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -91,6 +91,9 @@ virBitmapPtr virDomainNumaGetNodeCpumask(virCPUDefPtr numa, size_t node); virNumaMemAccess virDomainNumaGetNodeMemoryAccessMode(virCPUDefPtr numa, size_t node); +unsigned long long virDomainNumaGetNodeMemorySize(virCPUDefPtr numa, + size_t node); + /* * Formatters @@ -114,6 +117,10 @@ int virDomainNumatuneSet(virDomainNumaPtr numa, virBitmapPtr nodeset) ATTRIBUTE_NONNULL(1); +void virDomainNumaSetNodeMemorySize(virCPUDefPtr numa, + size_t node, + unsigned long long size); + /* * Other accessors */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f1ce14..8988b61 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -632,7 +632,9 @@ virDomainNumaFree; virDomainNumaGetNodeCount; virDomainNumaGetNodeCpumask; virDomainNumaGetNodeMemoryAccessMode; +virDomainNumaGetNodeMemorySize; virDomainNumaNew; +virDomainNumaSetNodeMemorySize; virDomainNumatuneFormatNodeset; virDomainNumatuneFormatXML; virDomainNumatuneGetMode; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 05545ee..a69d004 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4745,7 +4745,8 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, if (virAsprintf(&alias, "ram-node%zu", cell) < 0) goto cleanup; - if ((rc = qemuBuildMemoryBackendStr(def->cpu->cells[cell].mem, 0, cell, + if ((rc = qemuBuildMemoryBackendStr(virDomainNumaGetNodeMemorySize(def->cpu, cell), + 0, cell, NULL, auto_nodeset, def, qemuCaps, cfg, &backendType, &props, false)) < 0) @@ -7173,8 +7174,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, /* using of -numa memdev= cannot be combined with -numa mem=, thus we * need to check which approach to use */ for (i = 0; i < ncells; i++) { - unsigned long long cellmem = VIR_DIV_UP(def->cpu->cells[i].mem, 1024); - def->cpu->cells[i].mem = cellmem * 1024; + unsigned long long cellmem = virDomainNumaGetNodeMemorySize(def->cpu, i); + virDomainNumaSetNodeMemorySize(def->cpu, i, VIR_ROUND_UP(cellmem, 1024)); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { @@ -7224,7 +7225,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, if (needBackend) virBufferAsprintf(&buf, ",memdev=ram-node%zu", i); else - virBufferAsprintf(&buf, ",mem=%llu", def->cpu->cells[i].mem / 1024); + virBufferAsprintf(&buf, ",mem=%llu", + virDomainNumaGetNodeMemorySize(def->cpu, i) / 1024); virCommandAddArgBuffer(cmd, &buf); } -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
Add the helpers and refactor places where the value is accessed without them. --- src/conf/numa_conf.c | 20 +++++++++++++++++++- src/conf/numa_conf.h | 7 +++++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 10 ++++++---- 4 files changed, 34 insertions(+), 5 deletions(-)
Add the ATTRIBUTE_NONNULL(1) for Get/Set... ACK John
For historical reasons data regarding NUMA configuration were split between the CPU definition and numatune. We cannot do anything about the XML still being split, but we certainly can at least store the relevant data in one place. This patch moves the NUMA stuff to the right place. --- src/conf/cpu_conf.c | 26 +++---------------- src/conf/cpu_conf.h | 12 +++------ src/conf/domain_conf.c | 8 +++--- src/conf/numa_conf.c | 67 +++++++++++++++++++++++-------------------------- src/conf/numa_conf.h | 20 +++++++-------- src/cpu/cpu.c | 2 +- src/qemu/qemu_command.c | 20 +++++++-------- tests/cputest.c | 2 +- tests/testutilsqemu.c | 2 -- 9 files changed, 63 insertions(+), 96 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 0e3ce26..4e33934 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -73,16 +73,11 @@ virCPUDefFreeModel(virCPUDefPtr def) void virCPUDefFree(virCPUDefPtr def) { - size_t i; - if (!def) return; virCPUDefFreeModel(def); - for (i = 0; i < def->ncells; i++) - virBitmapFree(def->cells[i].cpumask); - VIR_FREE(def->cells); VIR_FREE(def->vendor_id); VIR_FREE(def); @@ -126,7 +121,6 @@ virCPUDefPtr virCPUDefCopy(const virCPUDef *cpu) { virCPUDefPtr copy; - size_t i; if (!cpu || VIR_ALLOC(copy) < 0) return NULL; @@ -143,20 +137,6 @@ virCPUDefCopy(const virCPUDef *cpu) if (virCPUDefCopyModel(copy, cpu, false) < 0) goto error; - if (cpu->ncells) { - if (VIR_ALLOC_N(copy->cells, cpu->ncells) < 0) - goto error; - - for (i = 0; i < cpu->ncells; i++) { - copy->cells[i].mem = cpu->cells[i].mem; - - copy->cells[i].cpumask = virBitmapNewCopy(cpu->cells[i].cpumask); - - if (!copy->cells[i].cpumask) - goto error; - } - } - return copy; error: @@ -429,11 +409,12 @@ virCPUDefParseXML(xmlNodePtr node, char * virCPUDefFormat(virCPUDefPtr def, + virDomainNumaPtr numa, bool updateCPU) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (virCPUDefFormatBufFull(&buf, def, updateCPU) < 0) + if (virCPUDefFormatBufFull(&buf, def, numa, updateCPU) < 0) goto cleanup; if (virBufferCheckError(&buf) < 0) @@ -450,6 +431,7 @@ virCPUDefFormat(virCPUDefPtr def, int virCPUDefFormatBufFull(virBufferPtr buf, virCPUDefPtr def, + virDomainNumaPtr numa, bool updateCPU) { if (!def) @@ -489,7 +471,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, if (virCPUDefFormatBuf(buf, def, updateCPU) < 0) return -1; - if (virDomainNumaDefCPUFormat(buf, def) < 0) + if (virDomainNumaDefCPUFormat(buf, numa) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index d2563e2..705ba6d 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -29,6 +29,7 @@ # include "virxml.h" # include "virbitmap.h" # include "virarch.h" +# include "numa_conf.h" # define VIR_CPU_VENDOR_ID_LENGTH 12 @@ -90,13 +91,6 @@ struct _virCPUFeatureDef { int policy; /* enum virCPUFeaturePolicy */ }; -typedef struct _virCellDef virCellDef; -typedef virCellDef *virCellDefPtr; -struct _virCellDef { - virBitmapPtr cpumask; /* CPUs that are part of this node */ - unsigned long long mem; /* Node memory in kB */ - int memAccess; /* virNumaMemAccess */ -}; typedef struct _virCPUDef virCPUDef; typedef virCPUDef *virCPUDefPtr; @@ -115,8 +109,6 @@ struct _virCPUDef { size_t nfeatures; size_t nfeatures_max; virCPUFeatureDefPtr features; - size_t ncells; - virCellDefPtr cells; }; @@ -145,6 +137,7 @@ virCPUDefIsEqual(virCPUDefPtr src, char * virCPUDefFormat(virCPUDefPtr def, + virDomainNumaPtr numa, bool updateCPU); int @@ -154,6 +147,7 @@ virCPUDefFormatBuf(virBufferPtr buf, int virCPUDefFormatBufFull(virBufferPtr buf, virCPUDefPtr def, + virDomainNumaPtr numa, bool updateCPU); int diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d34b9c4..fe73a5f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13517,11 +13517,10 @@ virDomainDefParseXML(xmlDocPtr xml, } - if (virDomainNumaDefCPUParseXML(def->cpu, ctxt) < 0) + if (virDomainNumaDefCPUParseXML(def->numa, ctxt) < 0) goto error; - if (def->cpu && - virDomainNumaGetCPUCountTotal(def->cpu) > def->maxvcpus) { + if (virDomainNumaGetCPUCountTotal(def->numa) > def->maxvcpus) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Number of CPUs in <numa> exceeds the" " <vcpu> count")); @@ -13531,7 +13530,6 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainNumatuneParseXML(def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, - virDomainNumaGetNodeCount(def->cpu), ctxt) < 0) goto error; @@ -20204,7 +20202,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</features>\n"); } - if (virCPUDefFormatBufFull(buf, def->cpu, + if (virCPUDefFormatBufFull(buf, def->cpu, def->numa, !!(flags & VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU)) < 0) goto error; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index ee7e65d..0a6f902 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -60,9 +60,12 @@ struct _virDomainNuma { } memory; /* pinning for all the memory */ struct _virDomainNumaNode { - virBitmapPtr nodeset; - virDomainNumatuneMemMode mode; - } *mem_nodes; /* fine tuning per guest node */ + unsigned long long mem; /* memory size in KiB */ + virBitmapPtr cpumask; /* bitmap of vCPUs corresponding to the node */ + virBitmapPtr nodeset; /* host memory nodes where this guest node resides */ + virDomainNumatuneMemMode mode; /* memory mode selection */ + virNumaMemAccess memAccess; /* shared memory access configuration */ + } *mem_nodes; /* guest node configuration */ size_t nmem_nodes; /* Future NUMA tuning related stuff should go here. */ @@ -83,7 +86,6 @@ virDomainNumatuneNodeSpecified(virDomainNumaPtr numatune, static int virDomainNumatuneNodeParseXML(virDomainNumaPtr numa, - size_t ncells, xmlXPathContextPtr ctxt) { char *tmp = NULL; @@ -109,19 +111,13 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr numa, goto cleanup; } - if (!ncells) { + if (!numa->nmem_nodes) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Element 'memnode' is invalid without " "any guest NUMA cells")); goto cleanup; } - VIR_FREE(numa->mem_nodes); - if (VIR_ALLOC_N(numa->mem_nodes, ncells) < 0) - goto cleanup; - - numa->nmem_nodes = ncells; - for (i = 0; i < n; i++) { int mode = 0; unsigned int cellid = 0; @@ -195,7 +191,6 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr numa, int virDomainNumatuneParseXML(virDomainNumaPtr numa, bool placement_static, - size_t ncells, xmlXPathContextPtr ctxt) { char *tmp = NULL; @@ -251,7 +246,7 @@ virDomainNumatuneParseXML(virDomainNumaPtr numa, nodeset) < 0) goto cleanup; - if (virDomainNumatuneNodeParseXML(numa, ncells, ctxt) < 0) + if (virDomainNumatuneNodeParseXML(numa, ctxt) < 0) goto cleanup; ret = 0; @@ -332,8 +327,10 @@ virDomainNumaFree(virDomainNumaPtr numa) return; virBitmapFree(numa->memory.nodeset); - for (i = 0; i < numa->nmem_nodes; i++) + for (i = 0; i < numa->nmem_nodes; i++) { + virBitmapFree(numa->mem_nodes[i].cpumask); virBitmapFree(numa->mem_nodes[i].nodeset); + } VIR_FREE(numa->mem_nodes); VIR_FREE(numa); @@ -655,7 +652,7 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune, int -virDomainNumaDefCPUParseXML(virCPUDefPtr def, +virDomainNumaDefCPUParseXML(virDomainNumaPtr def, xmlXPathContextPtr ctxt) { xmlNodePtr *nodes = NULL; @@ -675,9 +672,9 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, goto cleanup; } - if (VIR_ALLOC_N(def->cells, n) < 0) + if (VIR_ALLOC_N(def->mem_nodes, n) < 0) goto cleanup; - def->ncells = n; + def->nmem_nodes = n; for (i = 0; i < n; i++) { int rc; @@ -702,7 +699,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, } VIR_FREE(tmp); - if (def->cells[cur_cell].cpumask) { + if (def->mem_nodes[cur_cell].cpumask) { virReportError(VIR_ERR_XML_ERROR, _("Duplicate NUMA cell info for cell id '%u'"), cur_cell); @@ -715,11 +712,11 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, goto cleanup; } - if (virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask, + if (virBitmapParse(tmp, 0, &def->mem_nodes[cur_cell].cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; - if (virBitmapIsAllClear(def->cells[cur_cell].cpumask)) { + if (virBitmapIsAllClear(def->mem_nodes[cur_cell].cpumask)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("NUMA cell %d has no vCPUs assigned"), cur_cell); goto cleanup; @@ -728,7 +725,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, ctxt->node = nodes[i]; if (virDomainParseMemory("./@memory", "./@unit", ctxt, - &def->cells[cur_cell].mem, true, false) < 0) + &def->mem_nodes[cur_cell].mem, true, false) < 0) goto cleanup; if ((tmp = virXMLPropString(nodes[i], "memAccess"))) { @@ -739,7 +736,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, goto cleanup; } - def->cells[cur_cell].memAccess = rc; + def->mem_nodes[cur_cell].memAccess = rc; VIR_FREE(tmp); } } @@ -756,7 +753,7 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def, int virDomainNumaDefCPUFormat(virBufferPtr buf, - virCPUDefPtr def) + virDomainNumaPtr def) { virNumaMemAccess memAccess; char *cpustr; @@ -794,12 +791,12 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, unsigned int -virDomainNumaGetCPUCountTotal(virCPUDefPtr numa) +virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa) { size_t i; unsigned int ret = 0; - for (i = 0; i < numa->ncells; i++) + for (i = 0; i < numa->nmem_nodes; i++) ret += virBitmapCountBits(virDomainNumaGetNodeCpumask(numa, i)); return ret; @@ -818,43 +815,43 @@ virDomainNumaNew(void) size_t -virDomainNumaGetNodeCount(virCPUDefPtr numa) +virDomainNumaGetNodeCount(virDomainNumaPtr numa) { if (!numa) return 0; - return numa->ncells; + return numa->nmem_nodes; } virBitmapPtr -virDomainNumaGetNodeCpumask(virCPUDefPtr numa, +virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, size_t node) { - return numa->cells[node].cpumask; + return numa->mem_nodes[node].cpumask; } virNumaMemAccess -virDomainNumaGetNodeMemoryAccessMode(virCPUDefPtr numa, +virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa, size_t node) { - return numa->cells[node].memAccess; + return numa->mem_nodes[node].memAccess; } unsigned long long -virDomainNumaGetNodeMemorySize(virCPUDefPtr numa, +virDomainNumaGetNodeMemorySize(virDomainNumaPtr numa, size_t node) { - return numa->cells[node].mem; + return numa->mem_nodes[node].mem; } void -virDomainNumaSetNodeMemorySize(virCPUDefPtr numa, +virDomainNumaSetNodeMemorySize(virDomainNumaPtr numa, size_t node, unsigned long long size) { - numa->cells[node].mem = size; + numa->mem_nodes[node].mem = size; } diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index eadab43..8b3c437 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -29,7 +29,6 @@ # include "virutil.h" # include "virbitmap.h" # include "virbuffer.h" -# include "cpu_conf.h" typedef struct _virDomainNuma virDomainNuma; @@ -64,9 +63,8 @@ void virDomainNumaFree(virDomainNumaPtr numa); */ int virDomainNumatuneParseXML(virDomainNumaPtr numa, bool placement_static, - size_t ncells, xmlXPathContextPtr ctxt) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); int virDomainNumatuneFormatXML(virBufferPtr buf, virDomainNumaPtr numatune) ATTRIBUTE_NONNULL(1); @@ -86,12 +84,12 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, virBitmapPtr *retNodeset, int cellid); -size_t virDomainNumaGetNodeCount(virCPUDefPtr numa); -virBitmapPtr virDomainNumaGetNodeCpumask(virCPUDefPtr numa, +size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa); +virBitmapPtr virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, size_t node); -virNumaMemAccess virDomainNumaGetNodeMemoryAccessMode(virCPUDefPtr numa, +virNumaMemAccess virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa, size_t node); -unsigned long long virDomainNumaGetNodeMemorySize(virCPUDefPtr numa, +unsigned long long virDomainNumaGetNodeMemorySize(virDomainNumaPtr numa, size_t node); @@ -117,7 +115,7 @@ int virDomainNumatuneSet(virDomainNumaPtr numa, virBitmapPtr nodeset) ATTRIBUTE_NONNULL(1); -void virDomainNumaSetNodeMemorySize(virCPUDefPtr numa, +void virDomainNumaSetNodeMemorySize(virDomainNumaPtr numa, size_t node, unsigned long long size); @@ -136,10 +134,10 @@ int virDomainNumatuneSpecifiedMaxNode(virDomainNumaPtr numatune); bool virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune, virBitmapPtr auto_nodeset); -int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt); -int virDomainNumaDefCPUFormat(virBufferPtr buf, virCPUDefPtr def); +int virDomainNumaDefCPUParseXML(virDomainNumaPtr def, xmlXPathContextPtr ctxt); +int virDomainNumaDefCPUFormat(virBufferPtr buf, virDomainNumaPtr def); -unsigned int virDomainNumaGetCPUCountTotal(virCPUDefPtr numa); +unsigned int virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa); #endif /* __NUMA_CONF_H__ */ diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 6976b7b..9e67ddd 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -472,7 +472,7 @@ cpuBaselineXML(const char **xmlCPUs, if (!(cpu = cpuBaseline(cpus, ncpus, models, nmodels, flags))) goto error; - cpustr = virCPUDefFormat(cpu, false); + cpustr = virCPUDefFormat(cpu, NULL, false); cleanup: if (cpus) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a69d004..4b55558 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4558,7 +4558,7 @@ qemuBuildMemoryBackendStr(unsigned long long size, virDomainHugePagePtr hugepage = NULL; virDomainNumatuneMemMode mode; const long system_page_size = virGetSystemPageSizeKB(); - virNumaMemAccess memAccess = virDomainNumaGetNodeMemoryAccessMode(def->cpu, guestNode); + virNumaMemAccess memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode); size_t i; char *mem_path = NULL; @@ -4745,7 +4745,7 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, if (virAsprintf(&alias, "ram-node%zu", cell) < 0) goto cleanup; - if ((rc = qemuBuildMemoryBackendStr(virDomainNumaGetNodeMemorySize(def->cpu, cell), + if ((rc = qemuBuildMemoryBackendStr(virDomainNumaGetNodeMemorySize(def->numa, cell), 0, cell, NULL, auto_nodeset, def, qemuCaps, cfg, @@ -7120,7 +7120,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, bool needBackend = false; int rc; int ret = -1; - size_t ncells = virDomainNumaGetNodeCount(def->cpu); + size_t ncells = virDomainNumaGetNodeCount(def->numa); const long system_page_size = virGetSystemPageSizeKB(); if (virDomainNumatuneHasPerNodeBinding(def->numa) && @@ -7174,8 +7174,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, /* using of -numa memdev= cannot be combined with -numa mem=, thus we * need to check which approach to use */ for (i = 0; i < ncells; i++) { - unsigned long long cellmem = virDomainNumaGetNodeMemorySize(def->cpu, i); - virDomainNumaSetNodeMemorySize(def->cpu, i, VIR_ROUND_UP(cellmem, 1024)); + unsigned long long cellmem = virDomainNumaGetNodeMemorySize(def->numa, i); + virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(cellmem, 1024)); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { @@ -7187,7 +7187,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, if (rc == 0) needBackend = true; } else { - if (virDomainNumaGetNodeMemoryAccessMode(def->cpu, i)) { + if (virDomainNumaGetNodeMemoryAccessMode(def->numa, i)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Shared memory mapping is not supported " "with this QEMU")); @@ -7198,7 +7198,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); - if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->cpu, i)))) + if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, i)))) goto cleanup; if (strchr(cpumask, ',') && @@ -7226,7 +7226,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, virBufferAsprintf(&buf, ",memdev=ram-node%zu", i); else virBufferAsprintf(&buf, ",mem=%llu", - virDomainNumaGetNodeMemorySize(def->cpu, i) / 1024); + virDomainNumaGetNodeMemorySize(def->numa, i) / 1024); virCommandAddArgBuffer(cmd, &buf); } @@ -8318,7 +8318,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-m"); def->mem.max_balloon = VIR_DIV_UP(def->mem.max_balloon, 1024) * 1024; virCommandAddArgFormat(cmd, "%llu", def->mem.max_balloon / 1024); - if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->cpu)) { + if (def->mem.nhugepages && !virDomainNumaGetNodeCount(def->numa)) { const long system_page_size = virGetSystemPageSizeKB(); char *mem_path = NULL; @@ -8398,7 +8398,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (virDomainNumaGetNodeCount(def->cpu)) + if (virDomainNumaGetNodeCount(def->numa)) if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) goto error; diff --git a/tests/cputest.c b/tests/cputest.c index e49ae24..76cc9d4 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -167,7 +167,7 @@ cpuTestCompareXML(const char *arch, if (virtTestLoadFile(xml, &expected) < 0) goto cleanup; - if (!(actual = virCPUDefFormat(cpu, updateCPU))) + if (!(actual = virCPUDefFormat(cpu, NULL, updateCPU))) goto cleanup; if (STRNEQ(expected, actual)) { diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 4a3df8a..e3d4503 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -242,8 +242,6 @@ virCapsPtr testQemuCapsInit(void) ARRAY_CARDINALITY(host_cpu_features), /* nfeatures */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ host_cpu_features, /* features */ - 0, /* ncells */ - NULL, /* cells */ }; if ((caps = virCapabilitiesNew(host_cpu.arch, -- 2.2.2
On 02/16/2015 01:52 PM, Peter Krempa wrote:
For historical reasons data regarding NUMA configuration were split between the CPU definition and numatune. We cannot do anything about the XML still being split, but we certainly can at least store the relevant data in one place.
This patch moves the NUMA stuff to the right place. --- src/conf/cpu_conf.c | 26 +++---------------- src/conf/cpu_conf.h | 12 +++------ src/conf/domain_conf.c | 8 +++--- src/conf/numa_conf.c | 67 +++++++++++++++++++++++-------------------------- src/conf/numa_conf.h | 20 +++++++-------- src/cpu/cpu.c | 2 +- src/qemu/qemu_command.c | 20 +++++++-------- tests/cputest.c | 2 +- tests/testutilsqemu.c | 2 -- 9 files changed, 63 insertions(+), 96 deletions(-)
Ahh - the real magic comes in virDomainDefFormatInternal and virDomainNumaDefCPUParseXML.... Things see more logical now. ACK John - ohhh -- running out of steam - brain cell crash - If someone doesn't get to them before me - I'll start on the 3 and 7 patch series in the morning...
On Thu, Feb 19, 2015 at 19:28:56 -0500, John Ferlan wrote:
On 02/16/2015 01:52 PM, Peter Krempa wrote:
For historical reasons data regarding NUMA configuration were split between the CPU definition and numatune. We cannot do anything about the XML still being split, but we certainly can at least store the relevant data in one place.
This patch moves the NUMA stuff to the right place. --- src/conf/cpu_conf.c | 26 +++---------------- src/conf/cpu_conf.h | 12 +++------ src/conf/domain_conf.c | 8 +++--- src/conf/numa_conf.c | 67 +++++++++++++++++++++++-------------------------- src/conf/numa_conf.h | 20 +++++++-------- src/cpu/cpu.c | 2 +- src/qemu/qemu_command.c | 20 +++++++-------- tests/cputest.c | 2 +- tests/testutilsqemu.c | 2 -- 9 files changed, 63 insertions(+), 96 deletions(-)
Ahh - the real magic comes in virDomainDefFormatInternal and virDomainNumaDefCPUParseXML.... Things see more logical now.
ACK
I've fixed all the nits you've pointed out and pushed the series. Thanks.
John
Peter
- ohhh -- running out of steam - brain cell crash
- If someone doesn't get to them before me - I'll start on the 3 and 7 patch series in the morning...
You definitely deserve some rest, thanks again.
On Mon, Feb 16, 2015 at 19:51:48 +0100, Peter Krempa wrote:
Due to historical madne^Wreasons the NUMA configuration is split between /domain/cpu and /domain/numatune. Internally the data was also split into two places. We can't do anything about the external representation but we certainly can store all the definitions in one place internally.
This series does that.
This series no longer applies cleanly after commit commit 65c0fd9dfc712d23721e8052ce655100e230a3b3 Author: Michal Privoznik <mprivozn@redhat.com> Date: Thu Feb 12 17:37:46 2015 +0100 numatune_conf: Expose virDomainNumatuneNodeSpecified I can repost it if required. I will also provide a convenience branch on a public repo once I finish rebasing and a few other patches. Peter
On Wed, Feb 18, 2015 at 10:24:10 +0100, Peter Krempa wrote:
On Mon, Feb 16, 2015 at 19:51:48 +0100, Peter Krempa wrote:
Due to historical madne^Wreasons the NUMA configuration is split between /domain/cpu and /domain/numatune. Internally the data was also split into two places. We can't do anything about the external representation but we certainly can store all the definitions in one place internally.
This series does that.
This series no longer applies cleanly after commit
commit 65c0fd9dfc712d23721e8052ce655100e230a3b3 Author: Michal Privoznik <mprivozn@redhat.com> Date: Thu Feb 12 17:37:46 2015 +0100
numatune_conf: Expose virDomainNumatuneNodeSpecified
I can repost it if required. I will also provide a convenience branch on a public repo once I finish rebasing and a few other patches.
Peter
The rebased version including two other series that build on top of this one are now available at: git fetch git://pipo.sk/pipo/libvirt.git/ memory-refactors
participants (2)
-
John Ferlan -
Peter Krempa