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 @@
> ...
> </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,
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list