On Fri, Apr 10, 2015 at 17:36: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 iothreads element will have 'n' <iothread> children elements
> which will have attributes "id" and "name". The "id" will allow for
> definition of any "valid" (eg > 0) iothread_id value. The "name"
> attribute will allow for adding a name to the alias generated for
> the IOThread. The name cannot contain "iothread" since that's part
> of the default IOThread naming scheme already in use.
>
> 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.
> the input XML
>
> On output, only print out the <iothreadids> if they were read in.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 28 +++++
> docs/schemas/domaincommon.rng | 17 +++
> src/conf/domain_conf.c | 245 +++++++++++++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 23 ++++
> src/libvirt_private.syms | 6 ++
> 5 files changed, 317 insertions(+), 2 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 7ceb1fa..3224c20 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -521,6 +521,18 @@
> ...
> </domain>
> </pre>
> +<pre>
> +<domain>
> + ...
> + <iothreadids>
> + <iothread id="2" name="sysdisk"/>
> + <iothread id="4" name="userdisk"/>
> + <iothread id="6" name="datadisk"/>
> + <iothread id="8" name="datadisk"/>
> + </iothreadids>
> + ...
> +</domain>
> +</pre>
>
> <dl>
> <dt><code>iothreads</code></dt>
> @@ -530,7 +542,23 @@
> 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 and
> + the optional <code>name</code> attribute is a user defined name that
> + may be used to name the IOThread for the hypervisor. The id 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 id.
> + <span class="since">Since 1.2.15</span>
> + </dd>
> </dl>
>
> <h3><a name="elementsCPUTuning">CPU Tuning</a></h3>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1f5bf62..844caf6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -3304,6 +3332,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 (!virDomainIOThreadIDIsDuplicate(def, iothread_id)) {
> + if (virDomainIOThreadIDAdd(def, iothread_id, NULL) < 0)
> + return -1;
> + }
> + iothread_id++;
> + }
> + }
> +
> if (virDomainDefGetMemoryInitial(def) == 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("Memory size must be specified via <memory> or in the "
> @@ -13192,6 +13232,65 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt,
> return idmap;
> }
>
> +/* Parse the XML definition for an IOThread ID
> + *
> + * Format is :
> + *
> + * <iothreads>4</iothreads>
> + * <iothreadids>
> + * <iothread id='1' name='string'/>
> + * <iothread id='3' name='string'/>
> + * <iothread id='5' name='string'/>
> + * <iothread id='7' name='string'/>
> + * </iothreadids>
> + */
> +static virDomainIOThreadIDDefPtr
> +virDomainIOThreadIDDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt)
> +{
> + virDomainIOThreadIDDefPtr def;
> + xmlNodePtr oldnode = ctxt->node;
> + char *tmp = NULL;
> +
> + if (VIR_ALLOC(def) < 0)
> + return NULL;
> +
> + ctxt->node = node;
> +
> + if (!(tmp = virXPathString("string(./@id)", ctxt))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing <id> element in IOThread ID"));
'id' is an attribute, not an element.
> + goto error;
> + }
> + if (virStrToLong_uip(tmp, NULL, 10, &def->iothread_id) < 0 ||
> + def->iothread_id == 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid iothread 'id' value '%s'"), tmp);
> + goto error;
> + }
> +
> + def->name = virXMLPropString(node, "name");
> + if (def->name && strstr(def->name, "iothread")) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid iothread 'name' value '%s' - cannot "
> + "contain 'iothread'"),
> + def->name);
> + VIR_FREE(def->name);
This line can be dropped if ...
> + goto error;
> + }
> + def->defined = true;
> +
> + cleanup:
> + VIR_FREE(tmp);
> + ctxt->node = oldnode;
> + return def;
> +
> + error:
> + VIR_FREE(def);
... you use the freeing function you've added.
> + goto cleanup;
> +}
> +
> +
> /* Parse the XML definition for a vcpupin or emulatorpin.
> *
> * vcpupin has the form of
> @@ -13899,6 +13998,37 @@ 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) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("number of iothreadid iothread elements '%u' is "
> + "greater than the number of iothreads '%u'"),
> + n, def->iothreads);
Or you can perhaps silently fix def->iothreads so that the user doesn't
need to keep them in sync at least when increasing number of threads.
> + goto error;
> + }
> +
> + if (n && VIR_ALLOC_N(def->iothreadids, n) < 0)
> + goto error;
Here you'll need to allocate the array to MAX(n, def->iothreads) to
unify the structures as I'll suggest below.
> +
> + for (i = 0; i < n; i++) {
> + virDomainIOThreadIDDefPtr iothreadid = NULL;
> + if (!(iothreadid = virDomainIOThreadIDDefParseXML(nodes[i], ctxt)))
> + goto error;
> +
> + if (virDomainIOThreadIDIsDuplicate(def, iothreadid->iothread_id)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("duplicate iothread id '%u' found"),
> + iothreadid->iothread_id);
> + virDomainIOThreadIDDefFree(iothreadid);
> + goto error;
> + }
> + def->iothreadids[def->niothreadids++] = iothreadid;
> + }
> + VIR_FREE(nodes);
> +
> /* Extract cpu tunables. */
> if ((n = virXPathULong("string(./cputune/shares[1])", ctxt,
> &def->cputune.shares)) < -1) {
> @@ -17275,6 +17405,94 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
> return 0;
> }
>
> +bool
> +virDomainIOThreadIDIsDuplicate(virDomainDefPtr def,
> + unsigned int iothread_id)
> +{
> + size_t i;
> +
> + if (!def->iothreadids || !def->niothreadids)
> + return false;
> +
> + for (i = 0; i < def->niothreadids; i++) {
> + if (iothread_id == def->iothreadids[i]->iothread_id)
> + return true;
> + }
> +
> + return false;
How about "return !!virDomainIOThreadIDFind(def, iothread_id) rather
than open coding it?
> +}
> +
> +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,
> + const char *name)
> +{
> + virDomainIOThreadIDDefPtr iddef = NULL;
> +
> + if (virDomainIOThreadIDIsDuplicate(def, iothread_id)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot duplicate iothread_id '%u' in iothreadids"),
> + iothread_id);
> + return -1;
> + }
> +
> + if (VIR_ALLOC(iddef) < 0)
> + goto error;
> +
> + iddef->iothread_id = iothread_id;
> + if (name && VIR_STRDUP(iddef->name, name) < 0)
VIR_STRDUP handles NULL just fine, no need to check @name.
> + goto error;
> +
> + if (!def->iothreadids) {
> + if (VIR_ALLOC_N(def->iothreadids, 1) < 0)
> + goto error;
> + def->niothreadids = 1;
> + def->iothreadids[0] = iddef;
The clause above is not necessary, code below handles NULL arrays just
fine.
> + } else {
> + if (VIR_APPEND_ELEMENT(def->iothreadids, def->niothreadids, iddef) < 0)
> + goto error;
> + }
> +
> + return 0;
> +
> + error:
> + virDomainIOThreadIDDefFree(iddef);
> + 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]->name);
> + VIR_FREE(def->iothreadids[n]);
You've introduced a helper that frees the entries. You should use it
here.
> + VIR_DELETE_ELEMENT(def->iothreadids, n, def->niothreadids);
> + return;
> + }
> + }
> +}
> +
> /* Check if vcpupin with same id already exists.
> * Return 1 if exists, 0 if not. */
> bool
> @@ -20618,8 +20836,31 @@ 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 */
> + for (i = 0; i < def->niothreadids; i++) {
> + if (def->iothreadids[i]->defined)
> + break;
> + }
Dead code.
> + if (i < def->niothreadids) {
> + virBufferAddLit(buf, "<iothreadids>\n");
> + virBufferAdjustIndent(buf, 2);
> + for (i = 0; i < def->niothreadids; i++) {
> + if (!def->iothreadids[i]->defined)
> + continue;
If all iothreadids are implicit from the post parse callback this will
output an empty '<iothreadis>' element.
> + virBufferAsprintf(buf, "<iothread id='%u'",
> + def->iothreadids[i]->iothread_id);
> + if (def->iothreadids[i]->name)
> + virBufferAsprintf(buf, " name='%s'",
> + def->iothreadids[i]->name);
> + virBufferAddLit(buf, "/>\n");
> + }
> + 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 95cbb9c..03a0ecd 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2041,6 +2041,19 @@ struct _virDomainHugePage {
> unsigned long long size; /* hugepage size in KiB */
> };
>
> +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
> +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr;
> +
> +struct _virDomainIOThreadIDDef {
> + bool defined;
> + unsigned int iothread_id;
> + char *name;
This structure along with the one you add in the next patch and the
pinning structures that already exist make now three places where we
store data regarding IO threads. I don't think we should do that.
Keeping track of which data introduces messy code.
I think it will be desirable that you move the data regarding pinning of
iothreads into this structure along with thread ids from the monitor so
that we can keep everything in one place.
> +};
> +
> +void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
> +void virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
> + int nids);
> +
> typedef struct _virDomainCputune virDomainCputune;
> typedef virDomainCputune *virDomainCputunePtr;
>
I think the direction this series is taking is okay.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list