On Wed, 21 Jun 2017 21:08:30 +0200
Wim ten Have <wim.ten.have@xxxxxxxxxx> wrote:
> On Mon, 19 Jun 2017 12:26:20 -0600
> Jim Fehlig <jfehlig@xxxxxxxx> wrote:
>
> > On 06/12/2017 12:54 PM, Wim Ten Have wrote:
> > > From: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> > >
> > > Add libvirtd NUMA cell domain administration functionality to
> > > describe underlying cell id sibling distances in full fashion
> > > when configuring HVM guests.
> >
> > Thanks for the detailed cover letter :-). Some of that information (e.g. ACPI
> > spec, SLIT table, qemu support) would be useful in this commit message.
>
> I'll follow up on that under v2.
>
> > > [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>
> >
> > Others on the list are better XML designers, but the following seems to achieve
> > the same and is a bit more compact
> >
> > <cell id='0' cpus='0' memory='2097152' unit='KiB'>
> > <sibling id='0' distance='10'/>
> > <sibling id='1' distance='21'/>
> > <sibling id='2' distance='31'/>
> > <sibling id='3' distance='41'/>
> > </cell>
> >
> > CC danpb, who often has good ideas on schema design.
>
> Will give Daniel and (others) time prior submitting v2. Otherwise
> follow your suggestion above for its compact and clear annotation.
One thing on specific, forgot to mention previous reply, is that the
selected syntax was derived from way libvirt describes host capabilities
introduced under;
commit 8ba0a58f8d9db9eeed003b889dfcd3451d10fbca
Author: Michal Privoznik <mprivozn@xxxxxxxxxx>
Date: Tue Jun 3 15:18:27 2014 +0200
"virCaps: Expose distance between host NUMA nodes"
Regards,
- Wim.
> > > <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.
> >
> > One item you forgot was docs/formatdomain.html.in. Changes in schema should
> > always be described by accompanying changes in documentation.
>
> Oops ... noted. I'll go slow on coming back with v2 giving you and
> other time to further comment. Thanks for reviewing!
>
> - Wim.
>
> > Regards,
> > Jim
> >
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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 +
> > > 6 files changed, 313 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> > > index 1ea667c..a335b5d 100644
> > > --- a/docs/schemas/basictypes.rng
> > > +++ b/docs/schemas/basictypes.rng
> > > @@ -77,6 +77,14 @@
> > > </choice>
> > > </define>
> > >
> > > + <define name="numaDistanceValue">
> > > + <choice>
> > > + <data type="unsignedInt">
> > > + <param name="minInclusive">10</param>
> > > + </data>
> > > + </choice>
> > > + </define>
> > > +
> > > <define name="pciaddress">
> > > <optional>
> > > <attribute name="domain">
> > > diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
> > > index 3eef16a..c45b6df 100644
> > > --- a/docs/schemas/cputypes.rng
> > > +++ b/docs/schemas/cputypes.rng
> > > @@ -129,6 +129,24 @@
> > > </choice>
> > > </attribute>
> > > </optional>
> > > + <optional>
> > > + <element name="distances">
> > > + <oneOrMore>
> > > + <ref name="numaDistance"/>
> > > + </oneOrMore>
> > > + </element>
> > > + </optional>
> > > + </element>
> > > + </define>
> > > +
> > > + <define name="numaDistance">
> > > + <element name="sibling">
> > > + <attribute name="id">
> > > + <ref name="unsignedInt"/>
> > > + </attribute>
> > > + <attribute name="value">
> > > + <ref name="numaDistanceValue"/>
> > > + </attribute>
> > > </element>
> > > </define>
> > >
> > > diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> > > index da40e9b..5d8f7be3 100644
> > > --- a/src/conf/cpu_conf.c
> > > +++ b/src/conf/cpu_conf.c
> > > @@ -643,7 +643,7 @@ virCPUDefFormatBufFull(virBufferPtr buf,
> > > if (virCPUDefFormatBuf(&childrenBuf, def, updateCPU) < 0)
> > > goto cleanup;
> > >
> > > - if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0)
> > > + if (virDomainNumaDefCPUFormatXML(&childrenBuf, numa) < 0)
> > > goto cleanup;
> > >
> > > /* Put it all together */
> > > diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> > > index bfd3703..1914810 100644
> > > --- a/src/conf/numa_conf.c
> > > +++ b/src/conf/numa_conf.c
> > > @@ -48,6 +48,8 @@ VIR_ENUM_IMPL(virDomainMemoryAccess, VIR_DOMAIN_MEMORY_ACCESS_LAST,
> > > "shared",
> > > "private")
> > >
> > > +typedef struct _virDomainNumaDistance virDomainNumaDistance;
> > > +typedef virDomainNumaDistance *virDomainNumaDistancePtr;
> > >
> > > typedef struct _virDomainNumaNode virDomainNumaNode;
> > > typedef virDomainNumaNode *virDomainNumaNodePtr;
> > > @@ -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 */
> > > + unsigned int cellid;
> > > + } *distances; /* remote node distances */
> > > + size_t ndistances;
> > > } *mem_nodes; /* guest node configuration */
> > > size_t nmem_nodes;
> > >
> > > @@ -686,6 +694,95 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr numatune,
> > > }
> > >
> > >
> > > +static int
> > > +virDomainNumaDefNodeDistanceParseXML(virDomainNumaPtr def,
> > > + xmlXPathContextPtr ctxt,
> > > + unsigned int cur_cell)
> > > +{
> > > + int ret = -1;
> > > + char *tmp = NULL;
> > > + size_t i;
> > > + xmlNodePtr *nodes = NULL;
> > > + int ndistances;
> > > + virDomainNumaDistancePtr distances = NULL;
> > > +
> > > +
> > > + if (!virXPathNode("./distances[1]/sibling", ctxt))
> > > + return 0;
> > > +
> > > + if ((ndistances = virXPathNodeSet("./distances[1]/sibling", ctxt, &nodes)) <= 0) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("NUMA distances defined without siblings"));
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (ndistances < def->nmem_nodes) {
> > > + virReportError(VIR_ERR_XML_ERROR,
> > > + _("NUMA distances defined with fewer siblings than nodes for cell id: '%d'"),
> > > + cur_cell);
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (VIR_ALLOC_N(distances, ndistances) < 0)
> > > + goto cleanup;
> > > +
> > > + 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"));
> > > + 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;
> > > + }
> > > }
> > >
> > > ret = 0;
> > > @@ -801,8 +906,8 @@ virDomainNumaDefCPUParseXML(virDomainNumaPtr def,
> > >
> > >
> > > int
> > > -virDomainNumaDefCPUFormat(virBufferPtr buf,
> > > - virDomainNumaPtr def)
> > > +virDomainNumaDefCPUFormatXML(virBufferPtr buf,
> > > + virDomainNumaPtr def)
> > > {
> > > virDomainMemoryAccess memAccess;
> > > char *cpustr;
> > > @@ -815,6 +920,8 @@ virDomainNumaDefCPUFormat(virBufferPtr buf,
> > > virBufferAddLit(buf, "<numa>\n");
> > > virBufferAdjustIndent(buf, 2);
> > > for (i = 0; i < ncells; i++) {
> > > + int ndistances;
> > > +
> > > memAccess = virDomainNumaGetNodeMemoryAccessMode(def, i);
> > >
> > > if (!(cpustr = virBitmapFormat(virDomainNumaGetNodeCpumask(def, i))))
> > > @@ -829,7 +936,30 @@ virDomainNumaDefCPUFormat(virBufferPtr buf,
> > > if (memAccess)
> > > virBufferAsprintf(buf, " memAccess='%s'",
> > > virDomainMemoryAccessTypeToString(memAccess));
> > > - virBufferAddLit(buf, "/>\n");
> > > +
> > > + ndistances = def->mem_nodes[i].ndistances;
> > > + if (!ndistances) {
> > > + virBufferAddLit(buf, "/>\n");
> > > + } else {
> > > + size_t j;
> > > + virDomainNumaDistancePtr distances = def->mem_nodes[i].distances;
> > > +
> > > + virBufferAddLit(buf, ">\n");
> > > + virBufferAdjustIndent(buf, 2);
> > > + virBufferAddLit(buf, "<distances>\n");
> > > + virBufferAdjustIndent(buf, 2);
> > > + for (j = 0; j < ndistances; j++) {
> > > + virBufferAddLit(buf, "<sibling");
> > > + virBufferAsprintf(buf, " id='%d'", distances[j].cellid);
> > > + virBufferAsprintf(buf, " value='%d'", distances[j].value);
> > > + virBufferAddLit(buf, "/>\n");
> > > + }
> > > + virBufferAdjustIndent(buf, -2);
> > > + virBufferAddLit(buf, "</distances>\n");
> > > + virBufferAdjustIndent(buf, -2);
> > > + virBufferAddLit(buf, "</cell>\n");
> > > + }
> > > +
> > > VIR_FREE(cpustr);
> > > }
> > > virBufferAdjustIndent(buf, -2);
> > > @@ -922,13 +1052,121 @@ virDomainNumaCheckABIStability(virDomainNumaPtr src,
> > > size_t
> > > virDomainNumaGetNodeCount(virDomainNumaPtr numa)
> > > {
> > > - if (!numa)
> > > + if (!numa || !numa->mem_nodes)
> > > return 0;
> > >
> > > return numa->nmem_nodes;
> > > }
> > >
> > >
> > > +size_t
> > > +virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
> > > +{
> > > + if (!numa || !nmem_nodes)
> > > + return 0;
> > > +
> > > + if (numa->mem_nodes) {
> > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > + _("Cannot alter an existing nmem_nodes set"));
> > > + return 0;
> > > + }
> > > +
> > > + if (VIR_ALLOC_N(numa->mem_nodes, nmem_nodes) < 0)
> > > + return 0;
> > > +
> > > + numa->nmem_nodes = nmem_nodes;
> > > +
> > > + return numa->nmem_nodes;
> > > +}
> > > +
> > > +
> > > +size_t
> > > +virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
> > > + size_t node,
> > > + size_t cellid)
> > > +{
> > > + virDomainNumaDistancePtr distances;
> > > +
> > > + if (!numa)
> > > + return 0;
> > > +
> > > + distances = numa->mem_nodes[node].distances;
> > > + if (!numa->mem_nodes[node].ndistances || !distances)
> > > + return 0;
> > > +
> > > + return distances[cellid].value;
> > > +}
> > > +
> > > +
> > > +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;
> > > +
> > > + distances = numa->mem_nodes[node].distances;
> > > +
> > > + if (numa->mem_nodes[node].ndistances > 0 &&
> > > + distances[cellid].value)
> > > + return 0;
> > > +
> > > + distances[cellid].cellid = cellid;
> > > + distances[cellid].value = value;
> > > +
> > > + return distances[cellid].value;
> > > +}
> > > +
> > > +
> > > +size_t
> > > +virDomainNumaGetNodeDistanceCount(virDomainNumaPtr numa,
> > > + size_t node)
> > > +{
> > > + if (!numa)
> > > + return 0;
> > > +
> > > + return numa->mem_nodes[node].ndistances;
> > > +}
> > > +
> > > +
> > > +size_t
> > > +virDomainNumaSetNodeDistanceCount(virDomainNumaPtr numa,
> > > + size_t node,
> > > + size_t ndistances)
> > > +{
> > > + virDomainNumaDistancePtr distances;
> > > +
> > > + if (!numa || !ndistances)
> > > + return 0;
> > > +
> > > + 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;
> > > +}
> > > +
> > > +
> > > virDomainMemoryAccess
> > > virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa,
> > > size_t node)
> > > diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
> > > index b6a5354..96dda90 100644
> > > --- a/src/conf/numa_conf.h
> > > +++ b/src/conf/numa_conf.h
> > > @@ -87,12 +87,35 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune,
> > >
> > > size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa);
> > >
> > > +size_t virDomainNumaSetNodeCount(virDomainNumaPtr numa,
> > > + size_t nmem_nodes);
> > > +
> > > +size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
> > > + size_t node,
> > > + size_t sibling)
> > > + ATTRIBUTE_NONNULL(1);
> > > +size_t virDomainNumaSetNodeDistance(virDomainNumaPtr numa,
> > > + size_t node,
> > > + size_t sibling,
> > > + unsigned int value)
> > > + ATTRIBUTE_NONNULL(1);
> > > +size_t virDomainNumaGetNodeDistanceCount(virDomainNumaPtr numa,
> > > + size_t node)
> > > + ATTRIBUTE_NONNULL(1);
> > > +size_t virDomainNumaSetNodeDistanceCount(virDomainNumaPtr numa,
> > > + size_t node,
> > > + size_t ndistances)
> > > + ATTRIBUTE_NONNULL(1);
> > > virBitmapPtr virDomainNumaGetNodeCpumask(virDomainNumaPtr numa,
> > > size_t node)
> > > ATTRIBUTE_NONNULL(1);
> > > virDomainMemoryAccess virDomainNumaGetNodeMemoryAccessMode(virDomainNumaPtr numa,
> > > size_t node)
> > > ATTRIBUTE_NONNULL(1);
> > > +virBitmapPtr virDomainNumaSetNodeCpumask(virDomainNumaPtr numa,
> > > + size_t node,
> > > + virBitmapPtr cpumask)
> > > + ATTRIBUTE_NONNULL(1);
> > > unsigned long long virDomainNumaGetNodeMemorySize(virDomainNumaPtr numa,
> > > size_t node)
> > > ATTRIBUTE_NONNULL(1);
> > > @@ -151,7 +174,7 @@ bool virDomainNumatuneNodeSpecified(virDomainNumaPtr numatune,
> > > int cellid);
> > >
> > > int virDomainNumaDefCPUParseXML(virDomainNumaPtr def, xmlXPathContextPtr ctxt);
> > > -int virDomainNumaDefCPUFormat(virBufferPtr buf, virDomainNumaPtr def);
> > > +int virDomainNumaDefCPUFormatXML(virBufferPtr buf, virDomainNumaPtr def);
> > >
> > > unsigned int virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa);
> > >
> > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > index 044510f..e7fb9c0 100644
> > > --- a/src/libvirt_private.syms
> > > +++ b/src/libvirt_private.syms
> > > @@ -703,9 +703,15 @@ virDomainNumaGetMaxCPUID;
> > > virDomainNumaGetMemorySize;
> > > virDomainNumaGetNodeCount;
> > > virDomainNumaGetNodeCpumask;
> > > +virDomainNumaGetNodeDistance;
> > > +virDomainNumaGetNodeDistanceCount;
> > > virDomainNumaGetNodeMemoryAccessMode;
> > > virDomainNumaGetNodeMemorySize;
> > > virDomainNumaNew;
> > > +virDomainNumaSetNodeCount;
> > > +virDomainNumaSetNodeCpumask;
> > > +virDomainNumaSetNodeDistance;
> > > +virDomainNumaSetNodeDistanceCount;
> > > virDomainNumaSetNodeMemorySize;
> > > virDomainNumatuneFormatNodeset;
> > > virDomainNumatuneFormatXML;
> > >
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
