On 08/26/2014 01:03 AM, Martin Kletzander wrote:
On Mon, Aug 25, 2014 at 08:38:06PM -0400, John Ferlan wrote:
Introduce XML to allowing adding iothreads to the domain. These can be used by virtio-blk-pci devices in order to assign a specific thread to handle the workload for the device. The iothreads are the official implementation of the virtio-blk Data Plane that's been in tech preview for QEMU.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 26 ++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 12 ++++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 4 files changed, 68 insertions(+)
[...]
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9a89dd8..b4ac483 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -632,6 +632,12 @@ </optional>
<optional> + <element name="iothreads"> + <ref name="countIOThreads"/>
What's the difference between this and using ref name="unsignedInt" directly?
I (more or less) modeled (eg, cut, copy, paste) after the vcpu element which as you'll note is "countCPU" for both current and maxvcpus. Unlike the vcpu, I went with an 'int' instead of a 'short' although technically I cannot imagine more than a short number of threads or disks assigned to any domain! Also unlike the vcpu argument a "0" is a perfectly valid value.
+ </element> + </optional> + + <optional> <ref name="blkiotune"/> </optional>
@@ -4747,6 +4753,12 @@ <param name="minInclusive">1</param> </data> </define> + <define name="countIOThreads"> + <data type="unsignedInt"> + <param name="pattern">[0-9]+</param> + <param name="minInclusive">0</param> + </data> + </define> <define name="vcpuid"> <data type="unsignedShort"> <param name="pattern">[0-9]+</param> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 22a7f7e..671c41c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11953,6 +11953,23 @@ virDomainDefParseXML(xmlDocPtr xml, } }
+ /* Optional - iothreads */ + n = virXPathULong("string(./iothreads[1])", ctxt, &count); + if (n == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iothreads count must be an integer")); + goto error; + } else if (n < 0) { + def->iothreads = 0; + } else { + if ((unsigned int) count != count) {
Instead of this machinery, it would be more straightforward to just do (example written by hand, not tested):
tmp = virXPathString("string(./iothreads[1])", ctxt); if (tmp && virStrToLong_uip(tmp, NULL, 0, &def->iothreads) < 0) virReportError(VIR_ERR_XML_ERROR, _("invalid iothreads count '%s'"), tmp);
See my above explanation and then look about 10-20 lines before iothreads and see vcpu/current and maxvcpus. The power of the visual suggestion of what code around area I was changing was doing was far greater than thinking hmm.. should this code use the virStrToLong calls. Unlike of course when I added the iothread to the disk property which does use the function. Maybe in a different patch it'd be worth working through the various other calls to virXPathU* and seeing if they too could use the virStrToLong* interfaces. I'll adjust both - no problem. John