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 <[email protected]> > --- > 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 @@ > ... > </domain> > </pre> > +<pre> > +<domain> > + ... > + <iothreadids> > + <iothread id="2"/> > + <iothread id="4"/> > + <iothread id="6"/> > + <iothread id="8"/> > + </iothreadids> > + ... > +</domain> > +</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,
signature.asc
Description: Digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
