Re: [libvirt] [RFC PATCH v2 1/4] numa: describe siblings distances within cells

On Wed, Jun 28, 2017 at 03:56:36PM +0200, Wim Ten Have wrote: > From: Wim ten Have <wim.ten.have(a)oracle.com&gt; > > Add libvirtd NUMA cell domain administration functionality to > describe underlying cell id sibling distances in full fashion > when configuring HVM guests. > > [below is an example of a 4 node setup] > > <cpu> > <numa> > <cell id='0' cpus='0' memory='2097152' unit='KiB'> > <distances> > <sibling id='0' value='10'/> > <sibling id='1' value='21'/> > <sibling id='2' value='31'/> > <sibling id='3' value='41'/> > </distances> > </cell> > <cell id='1' cpus='1' memory='2097152' unit='KiB'> > <distances> > <sibling id='0' value='21'/> > <sibling id='1' value='10'/> > <sibling id='2' value='31'/> > <sibling id='3' value='41'/> > </distances> > </cell> > <cell id='2' cpus='2' memory='2097152' unit='KiB'> > <distances> > <sibling id='0' value='31'/> > <sibling id='1' value='21'/> > <sibling id='2' value='10'/> > <sibling id='3' value='21'/> > </distances> > <cell id='3' cpus='3' memory='2097152' unit='KiB'> > <distances> > <sibling id='0' value='41'/> > <sibling id='1' value='31'/> > <sibling id='2' value='21'/> > <sibling id='3' value='10'/> > </distances> > </cell> > </numa> > </cpu> > > Changes under this commit concern all those that require adding > the valid data structures, virDomainNuma* functional routines and the > XML doc schema enhancements to enforce appropriate administration. > > These changes are accompanied with topic related documentation under > docs/formatdomain.html within the "CPU model and topology" extending the > "Guest NUMA topology" paragraph. > > For terminology we refer to sockets as "nodes" where access to each > others' distinct resources such as memory make them "siblings" with a > designated "distance" between them. A specific design is described under > the ACPI (Advanced Configuration and Power Interface Specification) > within the chapter explaining the system's SLIT (System Locality Distance > Information Table). > > These patches extend core libvirt's XML description of a virtual machine's > hardware to include NUMA distance information for sibling nodes, which > is then passed to Xen guests via libxl. Recently qemu landed support for > constructing the SLIT since commit 0f203430dd ("numa: Allow setting NUMA > distance for different NUMA nodes"), hence these core libvirt extensions > can also help other drivers in supporting this feature. > > These changes alter the docs/schemas/cputypes.rng enforcing > domain administration to follow the syntax below per numa cell id. > > These changes also alter docs/schemas/basictypes.rng to add > "numaDistanceValue" which is an "unsignedInt" with a minimum value > of 10 as 0-9 are reserved values and can not be used as System > Locality Distance Information Table data. > > Signed-off-by: Wim ten Have <wim.ten.have(a)oracle.com&gt; > --- > docs/formatdomain.html.in | 64 ++++++++++- > docs/schemas/basictypes.rng | 8 ++ > docs/schemas/cputypes.rng | 18 +++ > src/conf/cpu_conf.c | 2 +- > src/conf/numa_conf.c | 260 +++++++++++++++++++++++++++++++++++++++++++- > src/conf/numa_conf.h | 25 ++++- > src/libvirt_private.syms | 6 + > 7 files changed, 376 insertions(+), 7 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 36bea67..b9736e3 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1510,7 +1510,69 @@ > </p> > > <p> > - This guest NUMA specification is currently available only for QEMU/KVM. > + This guest NUMA specification is currently available only for > + QEMU/KVM and Xen. Whereas Xen driver also allows for a distinct > + description of NUMA arranged <code>sibling</code> <code>cell</code> > + <code>distances</code> <span class="since">Since 3.6.0</span>. > + </p> > + > + <p> > + Under NUMA h/w architecture, distinct resources such as memory > + create a designated distance between <code>cell</code> and > + <code>siblings</code> that now can be described with the help of > + <code>distances</code>. A detailed describtion can be found within > + the ACPI (Advanced Configuration and Power Interface Specification) > + within the chapter explaining the system's SLIT (System Locality > + Distance Information Table). > + </p> > + > +<pre> > +... > +&lt;cpu&gt; > + ... > + &lt;numa&gt; > + &lt;cell id='0' cpus='0,4-7' memory='512000' unit='KiB'&gt; > + &lt;distances&gt; > + &lt;sibling id='0' value='10'/&gt; > + &lt;sibling id='1' value='21'/&gt; > + &lt;sibling id='2' value='31'/&gt; > + &lt;sibling id='3' value='41'/&gt; > + &lt;/distances&gt; > + &lt;/cell&gt; > + &lt;cell id='1' cpus='1,8-10,12-15' memory='512000' unit='KiB' memAccess='shared'&gt; > + &lt;distances&gt; > + &lt;sibling id='0' value='21'/&gt; > + &lt;sibling id='1' value='10'/&gt; > + &lt;sibling id='2' value='21'/&gt; > + &lt;sibling id='3' value='31'/&gt; > + &lt;/distances&gt; > + &lt;/cell&gt; > + &lt;cell id='2' cpus='2,11' memory='512000' unit='KiB' memAccess='shared'&gt; > + &lt;distances&gt; > + &lt;sibling id='0' value='31'/&gt; > + &lt;sibling id='1' value='21'/&gt; > + &lt;sibling id='2' value='10'/&gt; > + &lt;sibling id='3' value='21'/&gt; > + &lt;/distances&gt; > + &lt;/cell&gt; > + &lt;cell id='3' cpus='3' memory='512000' unit='KiB'&gt; > + &lt;distances&gt; > + &lt;sibling id='0' value='41'/&gt; > + &lt;sibling id='1' value='31'/&gt; > + &lt;sibling id='2' value='21'/&gt; > + &lt;sibling id='3' value='10'/&gt; > + &lt;/distances&gt; > + &lt;/cell&gt; > + &lt;/numa&gt; > + ... > +&lt;/cpu&gt; > +...</pre> > + > + <p> > + Under Xen driver, if no <code>distances</code> are given to describe > + the SLIT data between different cells, it will default to a scheme > + using 10 for local and 21 for other cells. Which is the assumption > + of guest OS when no SLIT is specified. > </p> We need to specify what happens if partial data is given too. In particular if you give a distance for A -> B, and not B -> A, then we should assume symmetry.

Rather than having the defaults be duplicated in drivers, we should have a helper API in domain_conf.c that can be used to query the distance between 2 nodes that can be called by drivers. This can then apply defaults in a consistent manner.

> @@ -66,6 +68,12 @@ struct _virDomainNuma { > virBitmapPtr nodeset; /* host memory nodes where this guest node resides */ > virDomainNumatuneMemMode mode; /* memory mode selection */ > virDomainMemoryAccess memAccess; /* shared memory access configuration */ > + > + struct _virDomainNumaDistance { > + unsigned int value; /* locality value for node i*j */ Strictly speaking distances have an orderng, ie this is 'node i -> j' in that specific direction. 'node j -> i' can be different. > + unsigned int cellid; Needs extra indent - 4 spaces per indent level > + } *distances; /* remote node distances */ > + size_t ndistances; > } *mem_nodes; /* guest node configuration */ > size_t nmem_nodes; > > @@ -686,6 +694,95 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune, > } > + for (i = 0; i < ndistances; i++) { > + unsigned int sibling_id = i, sibling_value; > + > + /* siblings are in order of parsing or explicitly numbered */ > + if ((tmp = virXMLPropString(nodes[i], "id"))) { > + if (virStrToLong_uip(tmp, NULL, 10, &sibling_id) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid 'id' attribute in NUMA distances for sibling: '%s'"), > + tmp); > + goto cleanup; > + } > + > + if (sibling_id >= ndistances) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Exactly one 'sibling' element per NUMA distance " > + "is allowed, non-contiguous ranges or ranges not " > + "starting from 0 are not allowed")); We should allow non-contiguous ranges, starting from any node id. ie users should be free to provide sparse information, relying on defaults for the rest. > + goto cleanup; > + } > + } > + VIR_FREE(tmp); > + > + /* We need a locality value */ > + if (!(tmp = virXMLPropString(nodes[i], "value"))) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Missing 'value' attribute in NUMA distances for sibling id: '%d'"), > + sibling_id); > + goto cleanup; > + } > + > + /* It needs to be applicable */ > + if (virStrToLong_uip(tmp, NULL, 10, &sibling_value) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid 'value' attribute in NUMA distances for sibling id: '%d'"), > + sibling_id); > + goto cleanup; > + } > + VIR_FREE(tmp); > + > + distances[sibling_id].cellid = sibling_id; > + distances[sibling_id].value = sibling_value; > + } > + > + def->mem_nodes[cur_cell].distances = distances; > + def->mem_nodes[cur_cell].ndistances = ndistances; > + > + ret = 0; > + > + cleanup: > + if (ret) > + VIR_FREE(distances); > + VIR_FREE(nodes); > + VIR_FREE(tmp); > + > + return ret; > +} > + > int > virDomainNumaDefCPUParseXML(virDomainNumaPtr def, > xmlXPathContextPtr ctxt) > @@ -788,6 +885,14 @@ virDomainNumaDefCPUParseXML(virDomainNumaPtr def, > def->mem_nodes[cur_cell].memAccess = rc; > VIR_FREE(tmp); > } > + > + /* Parse NUMA distances info */ > + if (virDomainNumaDefNodeDistanceParseXML(def, ctxt, cur_cell) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("NUMA cell %d has incorrect 'distances' configured"), > + cur_cell); > + goto cleanup; > + } This method already reported an detailed error message, which is being overwritten here with a useless generic message. > +size_t > +virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes) > +{ > + if (!numa || !nmem_nodes) > + return 0; This error condition doesn't report any error message... > + > + if (numa->mem_nodes) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot alter an existing nmem_nodes set")); > + return 0; > + } ...but this does... > + > + if (VIR_ALLOC_N(numa->mem_nodes, nmem_nodes) < 0) > + return 0; ...and so does this.

Callers need to have consistent error reporting for all scenarios

> + > + numa->nmem_nodes = nmem_nodes; > + > + return numa->nmem_nodes; > +} > + > + > +size_t > +virDomainNumaGetNodeDistance(virDomainNumaPtr numa, > + size_t node, > + size_t cellid) > +{ > + virDomainNumaDistancePtr distances; > + > + if (!numa) > + return 0; Just mark 'numa' parameters as ATTRIBUTE_NONNULL and make sure callers don't run this method if numa is not configured. > + > + distances = numa->mem_nodes[node].distances; You've not bounds-checked 'node' vs mem_nodes size. > + if (!numa->mem_nodes[node].ndistances || !distances) > + return 0; This is where we should plug in defaults for partially specified data. > + > + return distances[cellid].value; > +} What are calling semantics here ? I think we should > + > + > +size_t > +virDomainNumaSetNodeDistance(virDomainNumaPtr numa, > + size_t node, > + size_t cellid, > + unsigned int value) > +{ > + virDomainNumaDistancePtr distances; > + > + /* > + * Advanced Configuration and Power Interface > + * Specification version 6.1. Chapter 5.2.17 > + * System Locality Distance Information Table > + * ... Distance values of 0-9 are reserved. > + */ > + if (!numa || value < 10) > + return 0; ATTRIBUTE_NONNULL instead, and report an error for values < 10 > + > + distances = numa->mem_nodes[node].distances; > + > + if (numa->mem_nodes[node].ndistances > 0 && > + distances[cellid].value) > + return 0; I'm not sure what this check is doing - it seems to be refusing to let you change the value if one is already set. I don't see the point of that, but if it is needed then we should report errors > + > + distances[cellid].cellid = cellid; > + distances[cellid].value = value; > + > + return distances[cellid].value; > +} > + > + > +size_t > +virDomainNumaGetNodeDistanceCount(virDomainNumaPtr numa, > + size_t node) > +{ > + if (!numa) > + return 0; ATTRIBUTE_NONNULL instead > + > + return numa->mem_nodes[node].ndistances; > +} > + > + > +size_t > +virDomainNumaSetNodeDistanceCount(virDomainNumaPtr numa, > + size_t node, > + size_t ndistances) > +{ > + virDomainNumaDistancePtr distances; > + > + if (!numa || !ndistances) > + return 0; ATTRIBUTE_NONNULL for first. For the second we should presumably free any existing distance data > + > + distances = numa->mem_nodes[node].distances; > + if (distances) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Cannot alter an existing nmem_nodes distances set for node: %zu"), > + node); > + return 0; > + } > + > + if (VIR_ALLOC_N(distances, ndistances) < 0) > + return 0; > + > + numa->mem_nodes[node].distances = distances; > + numa->mem_nodes[node].ndistances = ndistances; > + > + return numa->mem_nodes[node].ndistances; > +} > + > + > virBitmapPtr > virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, > size_t node) > @@ -937,6 +1175,20 @@ virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, > } > > > +virBitmapPtr > +virDomainNumaSetNodeCpumask(virDomainNumaPtr numa, > + size_t node, > + virBitmapPtr cpumask) > +{ > + if (!numa || !cpumask) > + return NULL; > + > + numa->mem_nodes[node].cpumask = cpumask; > + > + return numa->mem_nodes[node].cpumask; > +} > + Regards, Daniel