[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, 19 Jun 2017 12:26:20 -0600
Jim Fehlig <jfehlig suse com> wrote:

> On 06/12/2017 12:54 PM, Wim Ten Have wrote:
> > From: Wim ten Have <wim ten have oracle com>
> >
> > 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.

> >       <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 oracle com>
> > ---
> >  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;
> >  
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]