Re: [PATCH v3 1/6] conf: Add new domain XML element 'iothreadids'

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

 



On Tue, Apr 14, 2015 at 21:18:21 -0400, John Ferlan wrote:
> Adding a new XML element 'iothreadids' in order to allow defining
> specific IOThread ID's rather than relying on the algorithm to assign
> IOThread ID's starting at 1 and incrementing to iothreads count.
> 
> This will allow future patches to be able to add new IOThreads by
> a specific iothread_id and of course delete any exisiting IOThread.
> 
> Each iothreadids element will have 'n' <iothread> children elements
> which will have attribute "id".  The "id" will allow for definition
> of any "valid" (eg > 0) iothread_id value.
> 
> On input, if any <iothreadids> <iothread>'s are provided, they will
> be marked so that we only print out what we read in.
> 
> On input, if no <iothreadids> are provided, the PostParse code will
> self generate a list of ID's starting at 1 and going to the number
> of iothreads defined for the domain (just like the current algorithm
> numbering scheme).  A future patch will rework the existing algorithm
> to make use of the iothreadids list.
> 
> On output, only print out the <iothreadids> if they were read in.
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  docs/formatdomain.html.in     |  30 +++++++
>  docs/schemas/domaincommon.rng |  12 +++
>  src/conf/domain_conf.c        | 190 +++++++++++++++++++++++++++++++++++++++++-
>  src/conf/domain_conf.h        |  15 ++++
>  src/libvirt_private.syms      |   3 +
>  5 files changed, 248 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index e921749..518f7c5 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -521,6 +521,18 @@
>    ...
>  &lt;/domain&gt;
>  </pre>
> +<pre>
> +&lt;domain&gt;
> +  ...
> +  &lt;iothreadids&gt;
> +    &lt;iothread id="2"/&gt;
> +    &lt;iothread id="4"/&gt;
> +    &lt;iothread id="6"/&gt;
> +    &lt;iothread id="8"/&gt;
> +  &lt;/iothreadids&gt;
> +  ...
> +&lt;/domain&gt;
> +</pre>
>  
>      <dl>
>        <dt><code>iothreads</code></dt>
> @@ -530,7 +542,25 @@
>          virtio-blk-pci and virtio-blk-ccw target storage devices. There
>          should be only 1 or 2 IOThreads per host CPU. There may be more
>          than one supported device assigned to each IOThread.
> +        <span class="since">Since 1.2.8</span>
>        </dd>
> +      <dt><code>iothreadids</code></dt>
> +      <dd>
> +        The optional <code>iothreadids</code> element provides the capability
> +        to specifically define the IOThread ID's for the domain.  By default,
> +        IOThread ID's are sequentially numbered starting from 1 through the
> +        number of <code>iothreads</code> defined for the domain. The
> +        <code>id</code> attribute is used to define the IOThread ID. The
> +        <code>id</code> attribute must be a positive integer greater than 0.
> +        If there are less <code>iothreadids</code> defined than
> +        <code>iothreads</code> defined for the domain, then libvirt will
> +        sequentially fill <code>iothreadids</code> starting at 1 avoiding
> +        any predefined <code>id</code>. If there are more
> +        <code>iothreadids</code> defined than <code>iothreads</code>
> +        defined for the domain, then the <code>iothreads</code> value
> +        will be adjusted accordingly.
> +        <span class="since">Since 1.2.15</span>
> +       </dd>
>      </dl>
>  
>      <h3><a name="elementsCPUTuning">CPU Tuning</a></h3>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 03fd541..4bd32bd 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -675,6 +675,18 @@
>        </optional>
>  
>        <optional>
> +        <element name="iothreadids">
> +          <zeroOrMore>
> +            <element name="iothread">
> +              <attribute name="id">
> +                <ref name="unsignedInt"/>
> +              </attribute>
> +            </element>
> +          </zeroOrMore>
> +        </element>
> +      </optional>
> +
> +      <optional>
>          <ref name="blkiotune"/>
>        </optional>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4d7e3c9..e86b7e2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2113,6 +2113,23 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
>      return NULL;
>  }
>  
> +
> +static void
> +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
> +                                int nids)
> +{
> +    size_t i;
> +
> +    if (!def)
> +        return;
> +
> +    for (i = 0; i < nids; i++)
> +        VIR_FREE(def[i]);
> +
> +    VIR_FREE(def);
> +}
> +
> +
>  void
>  virDomainPinDefFree(virDomainPinDefPtr def)
>  {
> @@ -2310,6 +2327,8 @@ void virDomainDefFree(virDomainDefPtr def)
>  
>      virCPUDefFree(def->cpu);
>  
> +    virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
> +
>      virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin);
>  
>      virDomainPinDefFree(def->cputune.emulatorpin);
> @@ -3304,6 +3323,18 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>          return -1;
>      }
>  
> +    /* Fully populate the IOThread ID list */
> +    if (def->iothreads && def->iothreads != def->niothreadids) {
> +        unsigned int iothread_id = 1;
> +        while (def->niothreadids != def->iothreads) {
> +            if (!virDomainIOThreadIDFind(def, iothread_id)) {
> +                if (virDomainIOThreadIDAdd(def, iothread_id) < 0)

This code is _adding_ fields for every iothread that was not specified
in <iothreadids> but is implied by <iothreads>, but before that you've
allocated all the structures in [1].


> +                    return -1;
> +            }
> +            iothread_id++;
> +        }
> +    }
> +
>      if (virDomainDefGetMemoryInitial(def) == 0) {
>          virReportError(VIR_ERR_XML_ERROR, "%s",
>                         _("Memory size must be specified via <memory> or in the "
> @@ -13173,6 +13204,56 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt,
>      return idmap;
>  }
>  
> +/* Parse the XML definition for an IOThread ID
> + *
> + * Format is :
> + *
> + *     <iothreads>4</iothreads>
> + *     <iothreadids>
> + *       <iothread id='1'/>
> + *       <iothread id='3'/>
> + *       <iothread id='5'/>
> + *       <iothread id='7'/>
> + *     </iothreadids>
> + */
> +static virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDDefParseXML(xmlNodePtr node,
> +                               xmlXPathContextPtr ctxt)
> +{
> +    virDomainIOThreadIDDefPtr iothrid;
> +    xmlNodePtr oldnode = ctxt->node;
> +    char *tmp = NULL;
> +
> +    if (VIR_ALLOC(iothrid) < 0)
> +        return NULL;
> +
> +    ctxt->node = node;
> +
> +    if (!(tmp = virXPathString("string(./@id)", ctxt))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Missing 'id' attribute in <iothread> element"));
> +        goto error;
> +    }
> +    if (virStrToLong_uip(tmp, NULL, 10, &iothrid->iothread_id) < 0 ||
> +        iothrid->iothread_id == 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                        _("invalid iothread 'id' value '%s'"), tmp);
> +        goto error;
> +    }
> +
> +    iothrid->defined = true;
> +
> + cleanup:
> +    VIR_FREE(tmp);
> +    ctxt->node = oldnode;
> +    return iothrid;
> +
> + error:
> +    VIR_FREE(iothrid);
> +    goto cleanup;
> +}
> +
> +
>  /* Parse the XML definition for a vcpupin
>   *
>   * vcpupin has the form of
> @@ -13948,6 +14029,32 @@ virDomainDefParseXML(xmlDocPtr xml,
>      }
>      VIR_FREE(tmp);
>  
> +    /* Extract any iothread id's defined */
> +    if ((n = virXPathNodeSet("./iothreadids/iothread", ctxt, &nodes)) < 0)
> +        goto error;
> +
> +    if (n > def->iothreads)
> +        def->iothreads = n;
> +
> +    if (n && VIR_ALLOC_N(def->iothreadids, MAX(n, def->iothreads)) < 0)
> +        goto error;

[1]

> +
> +    for (i = 0; i < n; i++) {
> +        virDomainIOThreadIDDefPtr iothrid = NULL;
> +        if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt)))
> +            goto error;
> +
> +        if (virDomainIOThreadIDFind(def, iothrid->iothread_id)) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("duplicate iothread id '%u' found"),
> +                           iothrid->iothread_id);
> +            VIR_FREE(iothrid);
> +            goto error;
> +        }
> +        def->iothreadids[def->niothreadids++] = iothrid;
> +    }
> +    VIR_FREE(nodes);
> +
>      /* Extract cpu tunables. */
>      if ((n = virXPathULong("string(./cputune/shares[1])", ctxt,
>                             &def->cputune.shares)) < -1) {
> @@ -17324,6 +17431,66 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
>      return 0;
>  }
>  
> +virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDFind(virDomainDefPtr def,
> +                        unsigned int iothread_id)
> +{
> +    size_t i;
> +
> +    if (!def->iothreadids || !def->niothreadids)
> +        return NULL;
> +
> +    for (i = 0; i < def->niothreadids; i++) {
> +        if (iothread_id == def->iothreadids[i]->iothread_id)
> +            return def->iothreadids[i];
> +    }
> +
> +    return NULL;
> +}
> +
> +int
> +virDomainIOThreadIDAdd(virDomainDefPtr def,
> +                       unsigned int iothread_id)

If this function returned the pointer to the added structure you'd be
able to use it later to modify other aspects of it.

> +{
> +    virDomainIOThreadIDDefPtr iothrid = NULL;
> +
> +    if (virDomainIOThreadIDFind(def, iothread_id)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("cannot duplicate iothread_id '%u' in iothreadids"),
> +                       iothread_id);
> +        return -1;
> +    }
> +
> +    if (VIR_ALLOC(iothrid) < 0)
> +        goto error;
> +
> +    iothrid->iothread_id = iothread_id;
> +
> +    if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iothrid) < 0)
> +        goto error;
> +
> +    return 0;
> +
> + error:
> +    VIR_FREE(iothrid);
> +    return -1;
> +}
> +
> +void
> +virDomainIOThreadIDDel(virDomainDefPtr def,
> +                       unsigned int iothread_id)
> +{
> +    int n;
> +
> +    for (n = 0; n < def->niothreadids; n++) {
> +        if (def->iothreadids[n]->iothread_id == iothread_id) {
> +            VIR_FREE(def->iothreadids[n]);

This could use a common freeing function, so that once you add more data
you won't have to tweak it.

> +            VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids);
> +            return;
> +        }
> +    }
> +}
> +
>  /* Check if vcpupin with same id already exists. */
>  bool
>  virDomainPinIsDuplicate(virDomainPinDefPtr *def,
> @@ -20666,8 +20833,27 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>          virBufferAsprintf(buf, " current='%u'", def->vcpus);
>      virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus);
>  
> -    if (def->iothreads > 0)
> -        virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads);
> +    if (def->iothreads > 0) {
> +        virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n",
> +                          def->iothreads);
> +        /* If we parsed the iothreadids, then write out those that we parsed */

Seems rather obvious.

> +        for (i = 0; i < def->niothreadids; i++) {
> +            if (def->iothreadids[i]->defined)
> +                break;
> +        }
> +        if (i < def->niothreadids) {
> +            virBufferAddLit(buf, "<iothreadids>\n");
> +            virBufferAdjustIndent(buf, 2);
> +            for (i = 0; i < def->niothreadids; i++) {
> +                if (!def->iothreadids[i]->defined)
> +                    continue;
> +                virBufferAsprintf(buf, "<iothread id='%u'/>\n",
> +                                  def->iothreadids[i]->iothread_id);
> +            }
> +            virBufferAdjustIndent(buf, -2);
> +            virBufferAddLit(buf, "</iothreadids>\n");
> +        }
> +    }
>  
>      if (def->cputune.sharesSpecified ||
>          (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) ||
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index e6fa3c9..9e7bdf9 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2041,6 +2041,14 @@ struct _virDomainHugePage {
>      unsigned long long size;    /* hugepage size in KiB */
>  };
>  
> +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
> +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr;
> +
> +struct _virDomainIOThreadIDDef {
> +    bool defined;

You may want to invert this field, since the case where you add the
thread structs due to the fact that <iohreads> was more than the count
of entries is less common than other places that add the struct.

> +    unsigned int iothread_id;
> +};
> +
>  typedef struct _virDomainCputune virDomainCputune;
>  typedef virDomainCputune *virDomainCputunePtr;
>  
> @@ -2132,6 +2140,8 @@ struct _virDomainDef {
>      virBitmapPtr cpumask;
>  
>      unsigned int iothreads;
> +    size_t niothreadids;
> +    virDomainIOThreadIDDefPtr *iothreadids;
>  
>      virDomainCputune cputune;
>  
> @@ -2590,6 +2600,11 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src,
>  
>  int virDomainDefAddImplicitControllers(virDomainDefPtr def);
>  
> +virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id);

Usually the return type is on the same line and the argument list is
broken if it doesn't fit.

> +int virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id);
> +void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
> +
>  unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
>  
>  char *virDomainDefFormat(virDomainDefPtr def,

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]