[PATCH 00/16] qemu: Allow setting EventLoopBaseProperties
QEMU introduced a way to set minimal and maximal number of worker threads for its worker thread pools. Currently, only IOThreads and main loop pools have this ability. Nevertheless, setting these boundaries (and basically making QEMU spawn enough threads upfront) is crucial for real-time workloads where having to spawn a thread may lead to missing the time limit. Michal Prívozník (16): virml: Introduce VIR_XML_PROP_NONNEGATIVE flag virxml: Introduce virXMLPropLongLong() virDomainDefParseIOThreads: Use g_autoptr() for @iothrid virDomainIOThreadIDDefArrayInit: Decrease scope of @iothrid conf: Move iothread formatter into a separate function conf: Introduce allocator for virDomainIOThreadIDDef conf: Introduce pool_min and pool_max attributes to IOThread qemu: Introduce QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX qemu_validate: Check if QEMU's capable of setting iothread pool size qemu: Generate command line for IOThread pool size include: Introduce typed params for virDomainSetIOThreadParams wrt pool size qemu: Wire up new virDomainSetIOThreadParams parameters virsh: Wire up new virDomainSetIOThreadParams parameters conf: Expose QEMU's main loop object qemu_validate: Check if QEMU's capable of setting main loop pool size qemu: Generate command line for main-loop pool size docs/formatdomain.rst | 11 +- docs/manpages/virsh.rst | 7 +- include/libvirt/libvirt-domain.h | 18 ++ src/conf/domain_conf.c | 154 +++++++++++++++--- src/conf/domain_conf.h | 11 ++ src/conf/schemas/domaincommon.rng | 25 +++ src/conf/virconftypes.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 48 +++++- src/qemu/qemu_driver.c | 63 ++++++- src/qemu/qemu_monitor.h | 4 + src/qemu/qemu_monitor_json.c | 2 + src/qemu/qemu_validate.c | 44 +++++ src/util/virxml.c | 69 ++++++++ src/util/virxml.h | 12 ++ .../caps_7.1.0.x86_64.xml | 1 + ...othreads-ids-pool-sizes.x86_64-latest.args | 45 +++++ .../iothreads-ids-pool-sizes.xml | 62 +++++++ tests/qemuxml2argvtest.c | 1 + ...iothreads-ids-pool-sizes.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + tools/virsh-domain.c | 24 ++- 24 files changed, 575 insertions(+), 34 deletions(-) create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml create mode 120000 tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml -- 2.35.1
For easier attribute parsing we have virXMLProp*() family of functions. These accept flags through which a caller can pose some conditions onto the attribute value, for instance: VIR_XML_PROP_NONZERO when the attribute may not be zero, etc. What we are missing is VIR_XML_PROP_NONNEGATIVE when the attribute value may be non-negative. Obviously, this flag makes sense only for some members of the virXMLProp*() family. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virxml.c | 7 +++++++ src/util/virxml.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 12b2ef635a..d6e2e5dd91 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -643,6 +643,13 @@ virXMLPropInt(xmlNodePtr node, return -1; } + if ((flags & VIR_XML_PROP_NONNEGATIVE) && (val < 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid value for attribute '%s' in element '%s': '%s'. Expected non-negative value"), + name, node->name, tmp); + return -1; + } + if ((flags & VIR_XML_PROP_NONZERO) && (val == 0)) { virReportError(VIR_ERR_XML_ERROR, _("Invalid value for attribute '%s' in element '%s': Zero is not permitted"), diff --git a/src/util/virxml.h b/src/util/virxml.h index 5d49056bc7..539228a9ba 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -38,6 +38,9 @@ typedef enum { VIR_XML_PROP_NONE = 0, VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */ VIR_XML_PROP_NONZERO = 1 << 1, /* Attribute may not be zero */ + VIR_XML_PROP_NONNEGATIVE = 1 << 2, /* Attribute may not be negative, makes + sense only for some virXMLProp*() + functions. */ } virXMLPropFlags; -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:51 +0200, Michal Privoznik wrote:
For easier attribute parsing we have virXMLProp*() family of functions. These accept flags through which a caller can pose some conditions onto the attribute value, for instance: VIR_XML_PROP_NONZERO when the attribute may not be zero, etc.
What we are missing is VIR_XML_PROP_NONNEGATIVE when the attribute value may be non-negative. Obviously, this flag makes sense only for some members of the virXMLProp*() family.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virxml.c | 7 +++++++ src/util/virxml.h | 3 +++ 2 files changed, 10 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, we have functions that parse an enum, int, tristate bool, and what not but we have none for long long. Heavily inspired by virXMLPropInt(), introduce virXMLPropLongLong() for parsing long long attributes. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 62 ++++++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 9 ++++++ 3 files changed, 72 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bfedd85326..1e247878e3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3652,6 +3652,7 @@ virXMLPickShellSafeComment; virXMLPropEnum; virXMLPropEnumDefault; virXMLPropInt; +virXMLPropLongLong; virXMLPropString; virXMLPropTristateBool; virXMLPropTristateBoolAllowDefault; diff --git a/src/util/virxml.c b/src/util/virxml.c index d6e2e5dd91..22caf58131 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -721,6 +721,68 @@ virXMLPropUInt(xmlNodePtr node, } +/** + * virXMLPropLongLong: + * @node: XML dom node pointer + * @name: Name of the property (attribute) to get + * @base: Number base, see strtol + * @flags: Bitwise or of virXMLPropFlags + * @result: The returned value + * @defaultResult: default value of @result in case the property is not found + * + * Convenience function to return value of an long long attribute. + * + * Returns 1 in case of success in which case @result is set, + * or 0 if the attribute is not present, + * or -1 and reports an error on failure. + */ +int +virXMLPropLongLong(xmlNodePtr node, + const char* name, + int base, + virXMLPropFlags flags, + long long *result, + long long defaultResult) +{ + g_autofree char *tmp = NULL; + long long val; + + *result = defaultResult; + + if (!(tmp = virXMLPropString(node, name))) { + if (!(flags & VIR_XML_PROP_REQUIRED)) + return 0; + + virReportError(VIR_ERR_XML_ERROR, + _("Missing required attribute '%s' in element '%s'"), + name, node->name); + return -1; + } + + if (virStrToLong_ll(tmp, NULL, base, &val) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid value for attribute '%s' in element '%s': '%s'. Expected integer value"), + name, node->name, tmp); + return -1; + } + + if ((flags & VIR_XML_PROP_NONNEGATIVE) && (val < 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid value for attribute '%s' in element '%s': '%s'. Expected non-negative value"), + name, node->name, tmp); + return -1; + } + + if ((flags & VIR_XML_PROP_NONZERO) && (val == 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid value for attribute '%s' in element '%s': Zero is not permitted"), + name, node->name); + return -1; + } + + *result = val; + return 1; +} /** * virXMLPropULongLong: * @node: XML dom node pointer diff --git a/src/util/virxml.h b/src/util/virxml.h index 539228a9ba..94f3b9760c 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -137,6 +137,15 @@ virXMLPropUInt(xmlNodePtr node, unsigned int *result) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); +int +virXMLPropLongLong(xmlNodePtr node, + const char* name, + int base, + virXMLPropFlags flags, + long long *result, + long long defaultResult) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); + int virXMLPropULongLong(xmlNodePtr node, const char* name, -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:52 +0200, Michal Privoznik wrote:
So far, we have functions that parse an enum, int, tristate bool, and what not but we have none for long long. Heavily inspired by virXMLPropInt(), introduce virXMLPropLongLong() for parsing long long attributes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 62 ++++++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 9 ++++++ 3 files changed, 72 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Using g_autoptr() for @iothrid variable inside virDomainDefParseIOThreads() allows us to drop explicit call to virDomainIOThreadIDDefFree() in one case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5d0d436a40..3b4274e037 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17046,7 +17046,8 @@ virDomainDefParseIOThreads(virDomainDef *def, def->iothreadids = g_new0(virDomainIOThreadIDDef *, n); for (i = 0; i < n; i++) { - virDomainIOThreadIDDef *iothrid = NULL; + g_autoptr(virDomainIOThreadIDDef) iothrid = NULL; + if (!(iothrid = virDomainIOThreadIDDefParseXML(nodes[i]))) return -1; @@ -17054,10 +17055,9 @@ virDomainDefParseIOThreads(virDomainDef *def, virReportError(VIR_ERR_XML_ERROR, _("duplicate iothread id '%u' found"), iothrid->iothread_id); - virDomainIOThreadIDDefFree(iothrid); return -1; } - def->iothreadids[def->niothreadids++] = iothrid; + def->iothreadids[def->niothreadids++] = g_steal_pointer(&iothrid); } return virDomainIOThreadIDDefArrayInit(def, iothreads); -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:53 +0200, Michal Privoznik wrote:
Using g_autoptr() for @iothrid variable inside virDomainDefParseIOThreads() allows us to drop explicit call to virDomainIOThreadIDDefFree() in one case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In virDomainIOThreadIDDefArrayInit() the variable @iothrid is used only inside a loop but is declared for whole function. Bring the variable into the loop so that it's obvious that the variable is not used elsewhere. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b4274e037..eec5941089 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3508,7 +3508,6 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def, { size_t i; ssize_t nxt = -1; - virDomainIOThreadIDDef *iothrid = NULL; g_autoptr(virBitmap) thrmap = NULL; /* Same value (either 0 or some number), then we have none to fill in or @@ -3533,6 +3532,8 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def, /* Populate iothreadids[] using the set bit number from thrmap */ while (def->niothreadids < iothreads) { + g_autoptr(virDomainIOThreadIDDef) iothrid = NULL; + if ((nxt = virBitmapNextSetBit(thrmap, nxt)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to populate iothreadids")); @@ -3541,7 +3542,7 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def, iothrid = g_new0(virDomainIOThreadIDDef, 1); iothrid->iothread_id = nxt; iothrid->autofill = true; - def->iothreadids[def->niothreadids++] = iothrid; + def->iothreadids[def->niothreadids++] = g_steal_pointer(&iothrid); } return 0; -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:54 +0200, Michal Privoznik wrote:
In virDomainIOThreadIDDefArrayInit() the variable @iothrid is used only inside a loop but is declared for whole function. Bring the variable into the loop so that it's obvious that the variable is not used elsewhere.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Formatting iothreads is currently open coded inside of virDomainDefFormatInternalSetRootName(). While this works, it makes the function needlessly long, especially if the formatting code will expand in near future. Therefore, move it into a separate function. At the same time, make virDomainDefIothreadShouldFormat() accept const domain definition so that the new function can also accept const domain definition. Formatters shouldn't need to change definition. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eec5941089..c60ab4e064 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27569,7 +27569,7 @@ virDomainCpuDefFormat(virBuffer *buf, static bool -virDomainDefIothreadShouldFormat(virDomainDef *def) +virDomainDefIothreadShouldFormat(const virDomainDef *def) { size_t i; @@ -27582,6 +27582,31 @@ virDomainDefIothreadShouldFormat(virDomainDef *def) } +static void +virDomainDefIOThreadsFormat(virBuffer *buf, + const virDomainDef *def) +{ + g_auto(virBuffer) childrenBuf = VIR_BUFFER_INIT_CHILD(buf); + size_t i; + + if (def->niothreadids == 0) + return; + + virBufferAsprintf(buf, "<iothreads>%zu</iothreads>\n", + def->niothreadids); + + if (!virDomainDefIothreadShouldFormat(def)) + return; + + for (i = 0; i < def->niothreadids; i++) { + virBufferAsprintf(&childrenBuf, "<iothread id='%u'/>\n", + def->iothreadids[i]->iothread_id); + } + + virXMLFormatElement(buf, "iothreadids", NULL, &childrenBuf); +} + + static void virDomainIOMMUDefFormat(virBuffer *buf, const virDomainIOMMUDef *iommu) @@ -28227,20 +28252,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, if (virDomainCpuDefFormat(buf, def) < 0) return -1; - if (def->niothreadids > 0) { - virBufferAsprintf(buf, "<iothreads>%zu</iothreads>\n", - def->niothreadids); - if (virDomainDefIothreadShouldFormat(def)) { - virBufferAddLit(buf, "<iothreadids>\n"); - virBufferAdjustIndent(buf, 2); - for (i = 0; i < def->niothreadids; i++) { - virBufferAsprintf(buf, "<iothread id='%u'/>\n", - def->iothreadids[i]->iothread_id); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</iothreadids>\n"); - } - } + virDomainDefIOThreadsFormat(buf, def); if (virDomainCputuneDefFormat(buf, def, flags) < 0) return -1; -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:55 +0200, Michal Privoznik wrote:
Formatting iothreads is currently open coded inside of virDomainDefFormatInternalSetRootName(). While this works, it makes the function needlessly long, especially if the formatting code will expand in near future. Therefore, move it into a separate function. At the same time, make virDomainDefIothreadShouldFormat() accept const domain definition so that the new function can also accept const domain definition. Formatters shouldn't need to change definition.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, iothread configuration structure (virDomainIOThreadIDDef) is allocated by plain g_new0(). This is perfectly okay because all members of the struct default to value 0 anyway. But soon this is going to change. Therefore, replace those g_new0() with a function so that the default value can be set consistently in one place. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c60ab4e064..8d4b27dbc0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3476,6 +3476,15 @@ virDomainIOThreadIDArrayHasPin(virDomainDef *def) } +static virDomainIOThreadIDDef * +virDomainIOThreadIDDefNew(void) +{ + virDomainIOThreadIDDef *def = g_new0(virDomainIOThreadIDDef, 1); + + return def; +} + + void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def) { @@ -3539,7 +3548,7 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def, _("failed to populate iothreadids")); return -1; } - iothrid = g_new0(virDomainIOThreadIDDef, 1); + iothrid = virDomainIOThreadIDDefNew(); iothrid->iothread_id = nxt; iothrid->autofill = true; def->iothreadids[def->niothreadids++] = g_steal_pointer(&iothrid); @@ -17008,7 +17017,7 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, static virDomainIOThreadIDDef * virDomainIOThreadIDDefParseXML(xmlNodePtr node) { - g_autoptr(virDomainIOThreadIDDef) iothrid = g_new0(virDomainIOThreadIDDef, 1); + g_autoptr(virDomainIOThreadIDDef) iothrid = virDomainIOThreadIDDefNew(); if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, @@ -22931,8 +22940,7 @@ virDomainIOThreadIDAdd(virDomainDef *def, { virDomainIOThreadIDDef *iothrid = NULL; - iothrid = g_new0(virDomainIOThreadIDDef, 1); - + iothrid = virDomainIOThreadIDDefNew(); iothrid->iothread_id = iothread_id; VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, iothrid); -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:56 +0200, Michal Privoznik wrote:
So far, iothread configuration structure (virDomainIOThreadIDDef) is allocated by plain g_new0(). This is perfectly okay because all members of the struct default to value 0 anyway. But soon this is going to change. Therefore, replace those g_new0() with a function so that the default value can be set consistently in one place.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
At least in case of QEMU an IOThread is actually a pool of threads (see iothread_set_aio_context_params() in QEMU's code base). As such, it can have minimal and maximal number of worker threads. Allow setting them in domain XML. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 6 +- src/conf/domain_conf.c | 32 +++++++++- src/conf/domain_conf.h | 3 + src/conf/schemas/domaincommon.rng | 10 +++ .../iothreads-ids-pool-sizes.xml | 61 +++++++++++++++++++ ...iothreads-ids-pool-sizes.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml create mode 120000 tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 312b605a8b..de085f616a 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -675,7 +675,7 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` <iothread id="2"/> <iothread id="4"/> <iothread id="6"/> - <iothread id="8"/> + <iothread id="8" pool_min="2" pool_max="32"/> </iothreadids> ... </domain> @@ -696,6 +696,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` any predefined ``id``. If there are more ``iothreadids`` defined than ``iothreads`` defined for the domain, then the ``iothreads`` value will be adjusted accordingly. :since:`Since 1.2.15` + The element has two optional attributes ``pool_min`` and ``pool_max`` which + allow setting lower and upper boundary for number of worker threads for + given IOThread. :since:`Since 8.4.0` + CPU Tuning diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8d4b27dbc0..7572e62db1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3481,6 +3481,9 @@ virDomainIOThreadIDDefNew(void) { virDomainIOThreadIDDef *def = g_new0(virDomainIOThreadIDDef, 1); + def->pool_min = -1; + def->pool_max = -1; + return def; } @@ -17008,7 +17011,7 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, * * <iothreads>4</iothreads> * <iothreadids> - * <iothread id='1'/> + * <iothread id='1' pool_min="0" pool_max="60"/> * <iothread id='3'/> * <iothread id='5'/> * <iothread id='7'/> @@ -17024,6 +17027,14 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node) &iothrid->iothread_id) < 0) return NULL; + if (virXMLPropLongLong(node, "pool_min", 10, + VIR_XML_PROP_NONNEGATIVE, &iothrid->pool_min, -1) < 0) + return NULL; + + if (virXMLPropLongLong(node, "pool_max", 10, + VIR_XML_PROP_NONNEGATIVE, &iothrid->pool_max, -1) < 0) + return NULL; + return g_steal_pointer(&iothrid); } @@ -27607,8 +27618,23 @@ virDomainDefIOThreadsFormat(virBuffer *buf, return; for (i = 0; i < def->niothreadids; i++) { - virBufferAsprintf(&childrenBuf, "<iothread id='%u'/>\n", - def->iothreadids[i]->iothread_id); + virDomainIOThreadIDDef *iothread = def->iothreadids[i]; + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&attrBuf, " id='%u'", + iothread->iothread_id); + + if (iothread->pool_min >= 0) { + virBufferAsprintf(&attrBuf, " pool_min='%lld'", + iothread->pool_min); + } + + if (iothread->pool_max >= 0) { + virBufferAsprintf(&attrBuf, " pool_max='%lld'", + iothread->pool_max); + } + + virXMLFormatElement(&childrenBuf, "iothread", &attrBuf, NULL); } virXMLFormatElement(buf, "iothreadids", NULL, &childrenBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e7e0f24443..c502100d45 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2646,6 +2646,9 @@ struct _virDomainIOThreadIDDef { virBitmap *cpumask; virDomainThreadSchedParam sched; + + long long pool_min; + long long pool_max; }; void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def); diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index cc598212a8..94035c38e7 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -829,6 +829,16 @@ <attribute name="id"> <ref name="unsignedInt"/> </attribute> + <optional> + <attribute name="pool_min"> + <ref name="unsignedLong"/> + </attribute> + </optional> + <optional> + <attribute name="pool_max"> + <ref name="unsignedLong"/> + </attribute> + </optional> </element> </zeroOrMore> </element> diff --git a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml new file mode 100644 index 0000000000..5ff3b4b18e --- /dev/null +++ b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml @@ -0,0 +1,61 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>6</vcpu> + <iothreads>5</iothreads> + <iothreadids> + <iothread id='2' pool_min='0' pool_max='60'/> + <iothread id='4' pool_min='1' pool_max='1'/> + <iothread id='1'/> + <iothread id='3'/> + <iothread id='5'/> + </iothreadids> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='3' port='0xa'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml b/tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml new file mode 120000 index 0000000000..6ed642fe5f --- /dev/null +++ b/tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/iothreads-ids-pool-sizes.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3bd57306cc..b85d5fb757 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -597,6 +597,7 @@ mymain(void) DO_TEST_NOCAPS("smp"); DO_TEST_NOCAPS("iothreads"); DO_TEST_NOCAPS("iothreads-ids"); + DO_TEST_CAPS_LATEST("iothreads-ids-pool-sizes"); DO_TEST_NOCAPS("iothreads-ids-partial"); DO_TEST_NOCAPS("cputune-iothreads"); DO_TEST_NOCAPS("iothreads-disk"); -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:57 +0200, Michal Privoznik wrote:
At least in case of QEMU an IOThread is actually a pool of threads (see iothread_set_aio_context_params() in QEMU's code base). As such, it can have minimal and maximal number of worker threads. Allow setting them in domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 6 +- src/conf/domain_conf.c | 32 +++++++++- src/conf/domain_conf.h | 3 + src/conf/schemas/domaincommon.rng | 10 +++ .../iothreads-ids-pool-sizes.xml | 61 +++++++++++++++++++ ...iothreads-ids-pool-sizes.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml create mode 120000 tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 312b605a8b..de085f616a 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst
[...]
@@ -696,6 +696,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` any predefined ``id``. If there are more ``iothreadids`` defined than ``iothreads`` defined for the domain, then the ``iothreads`` value will be adjusted accordingly. :since:`Since 1.2.15` + The element has two optional attributes ``pool_min`` and ``pool_max`` which + allow setting lower and upper boundary for number of worker threads for + given IOThread. :since:`Since 8.4.0`
8.5.0
+
Drop this extra line. Also I'd go probably for 'thread_pool_min/max'. In case of iothreads it doesn't matter too much, but later I'll object to 'mainloop' qemuism being introduced as a term rivaling our established 'emulator'. Using just 'pool' there would be a bit too ambiguous IMO.
CPU Tuning
[...]
void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def); diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index cc598212a8..94035c38e7 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -829,6 +829,16 @@ <attribute name="id"> <ref name="unsignedInt"/> </attribute> + <optional> + <attribute name="pool_min"> + <ref name="unsignedLong"/> + </attribute> + </optional> + <optional> + <attribute name="pool_max"> + <ref name="unsignedLong"/> + </attribute> + </optional>
I wondered how we could have distinguished between a long and int in the schema and the answer is ... we don't. Additionally using long long even feels a bit overkill. 2 billion threads ought to be enough for everybody. With the doc fixes ... and potential re-name: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 6/2/22 10:08, Peter Krempa wrote:
On Thu, Jun 02, 2022 at 09:17:57 +0200, Michal Privoznik wrote:
At least in case of QEMU an IOThread is actually a pool of threads (see iothread_set_aio_context_params() in QEMU's code base). As such, it can have minimal and maximal number of worker threads. Allow setting them in domain XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 6 +- src/conf/domain_conf.c | 32 +++++++++- src/conf/domain_conf.h | 3 + src/conf/schemas/domaincommon.rng | 10 +++ .../iothreads-ids-pool-sizes.xml | 61 +++++++++++++++++++ ...iothreads-ids-pool-sizes.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml create mode 120000 tests/qemuxml2xmloutdata/iothreads-ids-pool-sizes.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 312b605a8b..de085f616a 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst
[...]
@@ -696,6 +696,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` any predefined ``id``. If there are more ``iothreadids`` defined than ``iothreads`` defined for the domain, then the ``iothreads`` value will be adjusted accordingly. :since:`Since 1.2.15` + The element has two optional attributes ``pool_min`` and ``pool_max`` which + allow setting lower and upper boundary for number of worker threads for + given IOThread. :since:`Since 8.4.0`
8.5.0
Ooops yes. This is what you get when you write patches but don't finish them before entering freeze :-)
+
Drop this extra line.
Also I'd go probably for 'thread_pool_min/max'. In case of iothreads it doesn't matter too much, but later I'll object to 'mainloop' qemuism being introduced as a term rivaling our established 'emulator'.
Using just 'pool' there would be a bit too ambiguous IMO.
CPU Tuning
[...]
void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def); diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index cc598212a8..94035c38e7 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -829,6 +829,16 @@ <attribute name="id"> <ref name="unsignedInt"/> </attribute> + <optional> + <attribute name="pool_min"> + <ref name="unsignedLong"/> + </attribute> + </optional> + <optional> + <attribute name="pool_max"> + <ref name="unsignedLong"/> + </attribute> + </optional>
I wondered how we could have distinguished between a long and int in the schema and the answer is ... we don't.
Additionally using long long even feels a bit overkill. 2 billion threads ought to be enough for everybody.
I can switch to int. I don't care that much honestly. I just looked at what QEMU has and mimicked that. Do you want me to switch? That would make patch 2 obsolete as nobody we already have virXMLPropInt().
With the doc fixes ... and potential re-name:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Thanks, Michal
This capability reflects whether QEMU allows setting thread-pool-min and thread-pool-max attributes on iothread object. Since both attributes were introduced in the same commit (v7.0.0-878-g71ad4713cc) and can't exist independently of each other we can stick with one capability covering both of them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d0c8217825..e8c96ffc1f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -676,6 +676,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 430 */ "chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */ "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */ + "iothread.thread-pool-max", /* QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */ ); @@ -1627,6 +1628,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "calc-dirty-rate/arg-type/mode", QEMU_CAPS_DIRTYRATE_MODE }, { "chardev-add/arg-type/backend/+qemu-vdagent", QEMU_CAPS_CHARDEV_QEMU_VDAGENT }, { "query-display-options/ret-type/+dbus", QEMU_CAPS_DISPLAY_DBUS }, + { "object-add/arg-type/+iothread/thread-pool-max", QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX }, }; typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 26cd655db3..9bb81f87c2 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -651,6 +651,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 430 */ QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent */ QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */ + QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml index bdc613a54a..83904a7fec 100644 --- a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml @@ -243,6 +243,7 @@ <flag name='virtio-net.rss'/> <flag name='chardev.qemu-vdagent'/> <flag name='display-dbus'/> + <flag name='iothread.thread-pool-max'/> <version>7000050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100244</microcodeVersion> -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:58 +0200, Michal Privoznik wrote:
This capability reflects whether QEMU allows setting thread-pool-min and thread-pool-max attributes on iothread object. Since both attributes were introduced in the same commit (v7.0.0-878-g71ad4713cc) and can't exist independently of each other we can stick with one capability covering both of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that we have a capability that reflects whether QEMU is capable of setting iothread pool size, let's introduce a validator check to make sure users are not trying to use this feature with QEMU that doesn't support it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_validate.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 9b6245e6d7..21b3407b2f 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -384,6 +384,27 @@ qemuValidateDomainDefCpu(virQEMUDriver *driver, } +static int +qemuValidateDomainDefIOThreads(const virDomainDef *def, + virQEMUCaps *qemuCaps) +{ + size_t i; + + for (i = 0; i < def->niothreadids; i++) { + virDomainIOThreadIDDef *iothread = def->iothreadids[i]; + + if ((iothread->pool_min != -1 || iothread->pool_max != -1) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("pool_min and pool_max is not supported by this QEMU binary")); + return -1; + } + } + + return 0; +} + + static int qemuValidateDomainDefClockTimers(const virDomainDef *def, virQEMUCaps *qemuCaps) @@ -1168,6 +1189,9 @@ qemuValidateDomainDef(const virDomainDef *def, if (qemuDomainDefValidateMemoryHotplug(def, NULL) < 0) return -1; + if (qemuValidateDomainDefIOThreads(def, qemuCaps) < 0) + return -1; + if (qemuValidateDomainDefClockTimers(def, qemuCaps) < 0) return -1; -- 2.35.1
On Thu, Jun 02, 2022 at 09:17:59 +0200, Michal Privoznik wrote:
Now that we have a capability that reflects whether QEMU is capable of setting iothread pool size, let's introduce a validator check to make sure users are not trying to use this feature with QEMU that doesn't support it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_validate.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 15 ++++++- ...othreads-ids-pool-sizes.x86_64-latest.args | 44 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 52e4ef03cd..86b5193651 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7437,11 +7437,24 @@ qemuBuildIOThreadCommandLine(virCommand *cmd, for (i = 0; i < def->niothreadids; i++) { g_autoptr(virJSONValue) props = NULL; - g_autofree char *alias = g_strdup_printf("iothread%u", def->iothreadids[i]->iothread_id); + const virDomainIOThreadIDDef *iothread = def->iothreadids[i]; + g_autofree char *alias = NULL; + + alias = g_strdup_printf("iothread%u", iothread->iothread_id); if (qemuMonitorCreateObjectProps(&props, "iothread", alias, NULL) < 0) return -1; + if (iothread->pool_min >= 0 && + virJSONValueObjectAppendNumberLong(props, "thread-pool-min", + iothread->pool_min) < 0) + return -1; + + if (iothread->pool_max >= 0 && + virJSONValueObjectAppendNumberLong(props, "thread-pool-max", + iothread->pool_max) < 0) + return -1; + if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0) return -1; } diff --git a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args new file mode 100644 index 0000000000..32517e3d29 --- /dev/null +++ b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args @@ -0,0 +1,44 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine q35,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-accel tcg \ +-cpu qemu64 \ +-m 214 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \ +-overcommit mem-lock=off \ +-smp 6,sockets=6,cores=1,threads=1 \ +-object '{"qom-type":"iothread","id":"iothread2","thread-pool-min":0,"thread-pool-max":60}' \ +-object '{"qom-type":"iothread","id":"iothread4","thread-pool-min":1,"thread-pool-max":1}' \ +-object '{"qom-type":"iothread","id":"iothread1"}' \ +-object '{"qom-type":"iothread","id":"iothread3"}' \ +-object '{"qom-type":"iothread","id":"iothread5"}' \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device '{"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x1"}' \ +-device '{"driver":"pcie-root-port","port":9,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x1.0x1"}' \ +-device '{"driver":"pcie-root-port","port":10,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1.0x2"}' \ +-device '{"driver":"qemu-xhci","id":"usb","bus":"pci.1","addr":"0x0"}' \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device '{"driver":"virtio-blk-pci","bus":"pci.2","addr":"0x0","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bffe7aef8a..22e2661793 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2038,6 +2038,7 @@ mymain(void) DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD); DO_TEST("iothreads-ids", QEMU_CAPS_OBJECT_IOTHREAD); DO_TEST("iothreads-ids-partial", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST_CAPS_LATEST("iothreads-ids-pool-sizes"); DO_TEST_FAILURE_NOCAPS("iothreads-nocap"); DO_TEST("iothreads-disk", QEMU_CAPS_OBJECT_IOTHREAD); DO_TEST("iothreads-disk-virtio-ccw", QEMU_CAPS_OBJECT_IOTHREAD, -- 2.35.1
On Thu, Jun 02, 2022 at 09:18:00 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 15 ++++++- ...othreads-ids-pool-sizes.x86_64-latest.args | 44 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 52e4ef03cd..86b5193651 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7437,11 +7437,24 @@ qemuBuildIOThreadCommandLine(virCommand *cmd,
for (i = 0; i < def->niothreadids; i++) { g_autoptr(virJSONValue) props = NULL; - g_autofree char *alias = g_strdup_printf("iothread%u", def->iothreadids[i]->iothread_id); + const virDomainIOThreadIDDef *iothread = def->iothreadids[i]; + g_autofree char *alias = NULL; + + alias = g_strdup_printf("iothread%u", iothread->iothread_id);
if (qemuMonitorCreateObjectProps(&props, "iothread", alias, NULL) < 0) return -1;
I've sent a patch introducing a 'K' modifier for adding non-negative longs via the classic machinery: https://listman.redhat.com/archives/libvir-list/2022-June/232136.html
+ if (iothread->pool_min >= 0 && + virJSONValueObjectAppendNumberLong(props, "thread-pool-min", + iothread->pool_min) < 0) + return -1; + + if (iothread->pool_max >= 0 && + virJSONValueObjectAppendNumberLong(props, "thread-pool-max", + iothread->pool_max) < 0) + return -1;
Please rewrite this as if (qemuMonitorCreateObjectProps(&props, "iothread", alias, "K:thread-pool-min", iothread->pool_min, "K:thread-pool-max", iothread->pool_max, NULL) < 0)
+ if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0) return -1; }
With the above: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Our public API offers virDomainSetIOThreadParams() function which allows users to set various aspects of IOThreads. Introduce two new typed parameters: VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX which will allow users to modify the thread-pool-min and thread-pool-max attributes of an iothread. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 24846046aa..e1daa7a91d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2493,6 +2493,24 @@ int virDomainDelIOThread(virDomainPtr domain, */ # define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink" +/** + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN: + * + * Set the lower bound. + * + * Since: 8.4.0 + */ +# define VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN "thread_pool_min" + +/** + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX: + * + * Set the upper bound. + * + * Since: 8.4.0 + */ +# define VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX "thread_pool_max" + int virDomainSetIOThreadParams(virDomainPtr domain, unsigned int iothread_id, virTypedParameterPtr params, -- 2.35.1
On Thu, Jun 02, 2022 at 09:18:01 +0200, Michal Privoznik wrote:
Our public API offers virDomainSetIOThreadParams() function which allows users to set various aspects of IOThreads. Introduce two new typed parameters: VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX which will allow users to modify the thread-pool-min and thread-pool-max attributes of an iothread.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 24846046aa..e1daa7a91d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2493,6 +2493,24 @@ int virDomainDelIOThread(virDomainPtr domain, */ # define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink"
+/** + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN:
So here you call the enum THREAD_POOL_MIN ... as noted we should do the same in the XML.
+ * + * Set the lower bound.
This could use some more description.
+ * + * Since: 8.4.0
8.5.0
+ */ +# define VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN "thread_pool_min" + +/** + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX: + * + * Set the upper bound. + * + * Since: 8.4.0
-||-
+ */ +# define VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX "thread_pool_max" + int virDomainSetIOThreadParams(virDomainPtr domain, unsigned int iothread_id, virTypedParameterPtr params,
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Thu, Jun 02, 2022 at 10:15:41 +0200, Peter Krempa wrote:
On Thu, Jun 02, 2022 at 09:18:01 +0200, Michal Privoznik wrote:
Our public API offers virDomainSetIOThreadParams() function which allows users to set various aspects of IOThreads. Introduce two new typed parameters: VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX which will allow users to modify the thread-pool-min and thread-pool-max attributes of an iothread.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 24846046aa..e1daa7a91d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2493,6 +2493,24 @@ int virDomainDelIOThread(virDomainPtr domain, */ # define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink"
+/** + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN:
So here you call the enum THREAD_POOL_MIN ... as noted we should do the same in the XML.
+ * + * Set the lower bound.
This could use some more description.
Don't forget to mention what value type is expected (long vs int). I'm putting it up vague as I dispute the usability of long long in this instance.
Introduced in previous commit, QEMU driver needs to be taught how to set VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX parameters on given IOThread. Fortunately, this is fairly trivial to do and since these two parameters are exposed in domain XML too the update of inactive XML can be wired up too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 63 +++++++++++++++++++++++++++++++++--- src/qemu/qemu_monitor.h | 4 +++ src/qemu/qemu_monitor_json.c | 2 ++ 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fb63e6550f..d6f9fa99a8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5313,6 +5313,26 @@ qemuDomainHotplugModIOThread(virQEMUDriver *driver, } +static int +qemuDomainHotplugModIOThreadIDDef(virDomainIOThreadIDDef *def, + qemuMonitorIOThreadInfo mondef) +{ + if (mondef.set_thread_pool_min) + def->pool_min = mondef.thread_pool_min; + + if (mondef.set_thread_pool_max) + def->pool_max = mondef.thread_pool_max; + + /* These have no representation in domain XML */ + if (mondef.set_poll_grow || + mondef.set_poll_max_ns || + mondef.set_poll_shrink) + return -1; + + return 0; +} + + static int qemuDomainHotplugDelIOThread(virQEMUDriver *driver, virDomainObj *vm, @@ -5420,6 +5440,10 @@ qemuDomainIOThreadParseParams(virTypedParameterPtr params, VIR_TYPED_PARAM_UINT, VIR_DOMAIN_IOTHREAD_POLL_SHRINK, VIR_TYPED_PARAM_UINT, + VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN, + VIR_TYPED_PARAM_LLONG, + VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX, + VIR_TYPED_PARAM_LLONG, NULL) < 0) return -1; @@ -5444,6 +5468,20 @@ qemuDomainIOThreadParseParams(virTypedParameterPtr params, if (rc == 1) iothread->set_poll_shrink = true; + if ((rc = virTypedParamsGetLLong(params, nparams, + VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN, + &iothread->thread_pool_min)) < 0) + return -1; + if (rc == 1) + iothread->set_thread_pool_min = true; + + if ((rc = virTypedParamsGetLLong(params, nparams, + VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX, + &iothread->thread_pool_max)) < 0) + return -1; + if (rc == 1) + iothread->set_thread_pool_max = true; + if (iothread->set_poll_max_ns && iothread->poll_max_ns > INT_MAX) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("poll-max-ns (%llu) must be less than or equal to %d"), @@ -5486,6 +5524,7 @@ qemuDomainChgIOThread(virQEMUDriver *driver, qemuDomainObjPrivate *priv; virDomainDef *def; virDomainDef *persistentDef; + virDomainIOThreadIDDef *iothreaddef = NULL; int ret = -1; cfg = virQEMUDriverGetConfig(driver); @@ -5525,7 +5564,9 @@ qemuDomainChgIOThread(virQEMUDriver *driver, break; case VIR_DOMAIN_IOTHREAD_ACTION_MOD: - if (!(virDomainIOThreadIDFind(def, iothread.iothread_id))) { + iothreaddef = virDomainIOThreadIDFind(def, iothread.iothread_id); + + if (!iothreaddef) { virReportError(VIR_ERR_INVALID_ARG, _("cannot find IOThread '%u' in iothreadids"), iothread.iothread_id); @@ -5535,6 +5576,8 @@ qemuDomainChgIOThread(virQEMUDriver *driver, if (qemuDomainHotplugModIOThread(driver, vm, iothread) < 0) goto endjob; + + qemuDomainHotplugModIOThreadIDDef(iothreaddef, iothread); break; } @@ -5562,10 +5605,20 @@ qemuDomainChgIOThread(virQEMUDriver *driver, break; case VIR_DOMAIN_IOTHREAD_ACTION_MOD: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("configuring persistent polling values is " - "not supported")); - goto endjob; + iothreaddef = virDomainIOThreadIDFind(def, iothread.iothread_id); + + if (!iothreaddef) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find IOThread '%u' in iothreadids"), + iothread.iothread_id); + goto endjob; + } + + if (qemuDomainHotplugModIOThreadIDDef(iothreaddef, iothread) < 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("configuring persistent polling values is not supported")); + goto endjob; + } break; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index be341d5196..6ce38ffe86 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1308,9 +1308,13 @@ struct _qemuMonitorIOThreadInfo { unsigned long long poll_max_ns; unsigned int poll_grow; unsigned int poll_shrink; + long long thread_pool_min; + long long thread_pool_max; bool set_poll_max_ns; bool set_poll_grow; bool set_poll_shrink; + bool set_thread_pool_min; + bool set_thread_pool_max; }; int qemuMonitorGetIOThreads(qemuMonitor *mon, qemuMonitorIOThreadInfo ***iothreads, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index dc05dfd047..e8fe1eceae 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7431,6 +7431,8 @@ qemuMonitorJSONSetIOThread(qemuMonitor *mon, VIR_IOTHREAD_SET_PROP("poll-max-ns", poll_max_ns); VIR_IOTHREAD_SET_PROP("poll-grow", poll_grow); VIR_IOTHREAD_SET_PROP("poll-shrink", poll_shrink); + VIR_IOTHREAD_SET_PROP("thread-pool-min", thread_pool_min); + VIR_IOTHREAD_SET_PROP("thread-pool-max", thread_pool_max); #undef VIR_IOTHREAD_SET_PROP -- 2.35.1
On Thu, Jun 02, 2022 at 09:18:02 +0200, Michal Privoznik wrote:
Introduced in previous commit, QEMU driver needs to be taught how to set VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX parameters on given IOThread. Fortunately, this is fairly trivial to do and since these two parameters are exposed in domain XML too the update of inactive XML can be wired up too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 63 +++++++++++++++++++++++++++++++++--- src/qemu/qemu_monitor.h | 4 +++ src/qemu/qemu_monitor_json.c | 2 ++ 3 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fb63e6550f..d6f9fa99a8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5313,6 +5313,26 @@ qemuDomainHotplugModIOThread(virQEMUDriver *driver, }
+static int +qemuDomainHotplugModIOThreadIDDef(virDomainIOThreadIDDef *def, + qemuMonitorIOThreadInfo mondef) +{ + if (mondef.set_thread_pool_min) + def->pool_min = mondef.thread_pool_min; + + if (mondef.set_thread_pool_max) + def->pool_max = mondef.thread_pool_max; + + /* These have no representation in domain XML */ + if (mondef.set_poll_grow || + mondef.set_poll_max_ns || + mondef.set_poll_shrink) + return -1;
These checks should be first, so that you don't modify the definition and then report an error while the XML was already partially modified.
+ + return 0; +}
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Thu, Jun 02, 2022 at 09:18:02 +0200, Michal Privoznik wrote:
Introduced in previous commit, QEMU driver needs to be taught how to set VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN and VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX parameters on given IOThread. Fortunately, this is fairly trivial to do and since these two parameters are exposed in domain XML too the update of inactive XML can be wired up too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 63 +++++++++++++++++++++++++++++++++--- src/qemu/qemu_monitor.h | 4 +++ src/qemu/qemu_monitor_json.c | 2 ++ 3 files changed, 64 insertions(+), 5 deletions(-)
[...] One more thing
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index be341d5196..6ce38ffe86 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1308,9 +1308,13 @@ struct _qemuMonitorIOThreadInfo { unsigned long long poll_max_ns; unsigned int poll_grow; unsigned int poll_shrink; + long long thread_pool_min; + long long thread_pool_max;
These are 'long long', but ...
bool set_poll_max_ns; bool set_poll_grow; bool set_poll_shrink; + bool set_thread_pool_min; + bool set_thread_pool_max; }; int qemuMonitorGetIOThreads(qemuMonitor *mon, qemuMonitorIOThreadInfo ***iothreads, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index dc05dfd047..e8fe1eceae 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7431,6 +7431,8 @@ qemuMonitorJSONSetIOThread(qemuMonitor *mon, VIR_IOTHREAD_SET_PROP("poll-max-ns", poll_max_ns); VIR_IOTHREAD_SET_PROP("poll-grow", poll_grow); VIR_IOTHREAD_SET_PROP("poll-shrink", poll_shrink); + VIR_IOTHREAD_SET_PROP("thread-pool-min", thread_pool_min); + VIR_IOTHREAD_SET_PROP("thread-pool-max", thread_pool_max);
This macro is defined as: #define VIR_IOTHREAD_SET_PROP(propName, propVal) \ if (iothreadInfo->set_##propVal) { \ memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); \ prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; \ prop.val.iv = iothreadInfo->propVal; \ if (qemuMonitorJSONSetObjectProperty(mon, path, propName, &prop) < 0) \ return -1; \ } So with your use you have an unchecked overflow possibility when down-converting the number into an int. I think all of the above headache can be solved by simply using 'int' for the thread count.
Since virsh implements a wrapper over virDomainSetIOThreadParams() (command iothreadset) let's wire up new typed parameters there too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/manpages/virsh.rst | 7 ++++++- tools/virsh-domain.c | 24 +++++++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 965b89c99d..8c066c9176 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2993,7 +2993,8 @@ iothreadset :: iothreadset domain iothread_id [[--poll-max-ns ns] [--poll-grow factor] - [--poll-shrink divisor]] + [--poll-shrink divisor] [--thread-pool-min value] + [--thread-pool-max value]] [[--config] [--live] | [--current]] Modifies an existing iothread of the domain using the specified @@ -3010,6 +3011,10 @@ for a running guest. Saving, destroying, stopping, etc. the guest will result in the polling values returning to hypervisor defaults at the next start, restore, etc. +The *--thread-pool-min* and *--thread-pool-max* options then set lower and +upper bound, respectively of number of threads in worker pool of given +iothread. + If *--live* is specified, affect a running guest. If the guest is not running an error is returned. If *--current* is specified or *--live* is not specified, then handle diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bac4cc0fb6..f652695c2d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7782,7 +7782,15 @@ static const vshCmdOptDef opts_iothreadset[] = { }, {.name = "poll-shrink", .type = VSH_OT_INT, - .help = N_("set the value for reduction of the IOThread polling time ") + .help = N_("set the value for reduction of the IOThread polling time") + }, + {.name = "thread-pool-min", + .type = VSH_OT_INT, + .help = N_("lower boundary for worker thread pool") + }, + {.name = "thread-pool-max", + .type = VSH_OT_INT, + .help = N_("upper boundary for worker thread pool") }, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, @@ -7802,6 +7810,7 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd) int maxparams = 0; unsigned long long poll_max; unsigned int poll_val; + long long thread_val; int rc; if (live) @@ -7839,6 +7848,19 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd) #undef VSH_IOTHREAD_SET_UINT_PARAMS +#define VSH_IOTHREAD_SET_LLONG_PARAMS(opt, param) \ + thread_val = -1; \ + if ((rc = vshCommandOptLongLong(ctl, cmd, opt, &thread_val)) < 0) \ + goto cleanup; \ + if (rc > 0 && \ + virTypedParamsAddLLong(¶ms, &nparams, &maxparams, \ + param, thread_val) < 0) \ + goto save_error; + + VSH_IOTHREAD_SET_LLONG_PARAMS("thread-pool-min", VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN) + VSH_IOTHREAD_SET_LLONG_PARAMS("thread-pool-max", VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX) +#undef VSH_IOTHREAD_SET_LLONG_PARAMS + if (virDomainSetIOThreadParams(dom, id, params, nparams, flags) < 0) goto cleanup; -- 2.35.1
On Thu, Jun 02, 2022 at 09:18:03 +0200, Michal Privoznik wrote:
Since virsh implements a wrapper over virDomainSetIOThreadParams() (command iothreadset) let's wire up new typed parameters there too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/manpages/virsh.rst | 7 ++++++- tools/virsh-domain.c | 24 +++++++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 965b89c99d..8c066c9176 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2993,7 +2993,8 @@ iothreadset ::
iothreadset domain iothread_id [[--poll-max-ns ns] [--poll-grow factor] - [--poll-shrink divisor]] + [--poll-shrink divisor] [--thread-pool-min value] + [--thread-pool-max value]] [[--config] [--live] | [--current]]
Modifies an existing iothread of the domain using the specified @@ -3010,6 +3011,10 @@ for a running guest. Saving, destroying, stopping, etc. the guest will result in the polling values returning to hypervisor defaults at the next start, restore, etc.
+The *--thread-pool-min* and *--thread-pool-max* options then set lower and +upper bound, respectively of number of threads in worker pool of given +iothread.
Yet another case for using the same name in the XML. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its main event loop as an QMP object. In the very next commit (v7.0.0-878-g71ad4713cc) it was extended for thread-pool-min and thread-pool-max attributes. Expose them under new <mainloop/> element. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 5 ++ src/conf/domain_conf.c | 57 +++++++++++++++++++ src/conf/domain_conf.h | 8 +++ src/conf/schemas/domaincommon.rng | 15 +++++ src/conf/virconftypes.h | 2 + .../iothreads-ids-pool-sizes.xml | 1 + 6 files changed, 88 insertions(+) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index de085f616a..5e0019c574 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -677,6 +677,7 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` <iothread id="6"/> <iothread id="8" pool_min="2" pool_max="32"/> </iothreadids> + <mainloop pool_min="8" pool_max="16"/> ... </domain> @@ -699,6 +700,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` The element has two optional attributes ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. :since:`Since 8.4.0` +``mainloop`` + The optional ``mainloop`` element contains two optional attributes + ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary + for number of worker threads for the main event loop. :since:`Since 8.4.0` diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7572e62db1..866270160e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3824,6 +3824,8 @@ void virDomainDefFree(virDomainDef *def) virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids); + g_free(def->mainLoop); + virBitmapFree(def->cputune.emulatorpin); g_free(def->cputune.emulatorsched); @@ -17085,6 +17087,32 @@ virDomainDefParseIOThreads(virDomainDef *def, } +static int +virDomainDefParseMainLoop(virDomainDef *def, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr node = NULL; + g_autofree virDomainMainLoopDef *ml = NULL; + + node = virXPathNode("./mainloop", ctxt); + if (!node) + return 0; + + ml = g_new0(virDomainMainLoopDef, 1); + + if (virXMLPropLongLong(node, "pool_min", 10, + VIR_XML_PROP_NONNEGATIVE, &ml->pool_min, -1) < 0) + return -1; + + if (virXMLPropLongLong(node, "pool_max", 10, + VIR_XML_PROP_NONNEGATIVE, &ml->pool_max, -1) < 0) + return -1; + + def->mainLoop = g_steal_pointer(&ml); + return 0; +} + + /* Parse the XML definition for a vcpupin * * vcpupin has the form of @@ -19259,6 +19287,9 @@ virDomainDefTunablesParse(virDomainDef *def, if (virDomainDefParseIOThreads(def, ctxt) < 0) return -1; + if (virDomainDefParseMainLoop(def, ctxt) < 0) + return -1; + /* Extract cpu tunables. */ if ((n = virXPathULongLong("string(./cputune/shares[1])", ctxt, &def->cputune.shares)) < -1) { @@ -27641,6 +27672,30 @@ virDomainDefIOThreadsFormat(virBuffer *buf, } +static void +virDomainDefMainLoopFormat(virBuffer *buf, + const virDomainDef *def) +{ + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + + if (!def->mainLoop) + return; + + if (def->mainLoop->pool_min >= 0) { + virBufferAsprintf(&attrBuf, " pool_min='%lld'", + def->mainLoop->pool_min); + } + + if (def->mainLoop->pool_max >= 0) { + virBufferAsprintf(&attrBuf, " pool_max='%lld'", + def->mainLoop->pool_max); + } + + + virXMLFormatElement(buf, "mainloop", &attrBuf, NULL); +} + + static void virDomainIOMMUDefFormat(virBuffer *buf, const virDomainIOMMUDef *iommu) @@ -28288,6 +28343,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, virDomainDefIOThreadsFormat(buf, def); + virDomainDefMainLoopFormat(buf, def); + if (virDomainCputuneDefFormat(buf, def, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c502100d45..3184d36d54 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2655,6 +2655,12 @@ void virDomainIOThreadIDDefFree(virDomainIOThreadIDDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainIOThreadIDDef, virDomainIOThreadIDDefFree); +struct _virDomainMainLoopDef { + long long pool_min; + long long pool_max; +}; + + struct _virDomainCputune { unsigned long long shares; bool sharesSpecified; @@ -2863,6 +2869,8 @@ struct _virDomainDef { size_t niothreadids; virDomainIOThreadIDDef **iothreadids; + virDomainMainLoopDef *mainLoop; + virDomainCputune cputune; virDomainResctrlDef **resctrls; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 94035c38e7..57c3f390f5 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -844,6 +844,21 @@ </element> </optional> + <optional> + <element name="mainloop"> + <optional> + <attribute name="pool_min"> + <ref name="unsignedLong"/> + </attribute> + </optional> + <optional> + <attribute name="pool_max"> + <ref name="unsignedLong"/> + </attribute> + </optional> + </element> + </optional> + <optional> <ref name="blkiotune"/> </optional> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index 21420ba8ea..f462642cbe 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -142,6 +142,8 @@ typedef struct _virDomainIOMMUDef virDomainIOMMUDef; typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; +typedef struct _virDomainMainLoopDef virDomainMainLoopDef; + typedef struct _virDomainIdMapDef virDomainIdMapDef; typedef struct _virDomainIdMapEntry virDomainIdMapEntry; diff --git a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml index 5ff3b4b18e..087564171b 100644 --- a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml +++ b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml @@ -12,6 +12,7 @@ <iothread id='3'/> <iothread id='5'/> </iothreadids> + <mainloop pool_min='10' pool_max='120'/> <os> <type arch='x86_64' machine='q35'>hvm</type> <boot dev='hd'/> -- 2.35.1
On Thu, Jun 02, 2022 at 09:18:04 +0200, Michal Privoznik wrote:
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its main event loop as an QMP object. In the very next commit (v7.0.0-878-g71ad4713cc) it was extended for thread-pool-min and thread-pool-max attributes. Expose them under new <mainloop/> element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 5 ++ src/conf/domain_conf.c | 57 +++++++++++++++++++ src/conf/domain_conf.h | 8 +++ src/conf/schemas/domaincommon.rng | 15 +++++ src/conf/virconftypes.h | 2 + .../iothreads-ids-pool-sizes.xml | 1 + 6 files changed, 88 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index de085f616a..5e0019c574 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst
[...]
@@ -699,6 +700,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` The element has two optional attributes ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. :since:`Since 8.4.0` +``mainloop`` + The optional ``mainloop`` element contains two optional attributes + ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary + for number of worker threads for the main event loop. :since:`Since 8.4.0`
I don't think we should use the qemu term "main loop" here. In general we refer to the qemu process as emulator, including stuff like emulatorpin and similar. Now the slight drawback from using 'emulator' is that we already have 'emulator' as part of <devices> so adding another one here is not a good idea. Having 'tread_pool_max'/min attribute on the emulator tag is a bit ugly but feasible and would bring it to be a bit more relevant. Also the documentation paragraph does a really bad job at explaining how to use these knobs and what they control so please update it a bit. Also note the other things outlined in previous replies namely: - version being 8.5.0 - 'long long' being too much and too complicated - using the 'tread_' prefix for the fields
On Thu, Jun 02, 2022 at 01:54:39PM +0200, Peter Krempa wrote:
On Thu, Jun 02, 2022 at 09:18:04 +0200, Michal Privoznik wrote:
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its main event loop as an QMP object. In the very next commit (v7.0.0-878-g71ad4713cc) it was extended for thread-pool-min and thread-pool-max attributes. Expose them under new <mainloop/> element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 5 ++ src/conf/domain_conf.c | 57 +++++++++++++++++++ src/conf/domain_conf.h | 8 +++ src/conf/schemas/domaincommon.rng | 15 +++++ src/conf/virconftypes.h | 2 + .../iothreads-ids-pool-sizes.xml | 1 + 6 files changed, 88 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index de085f616a..5e0019c574 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst
[...]
@@ -699,6 +700,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` The element has two optional attributes ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. :since:`Since 8.4.0` +``mainloop`` + The optional ``mainloop`` element contains two optional attributes + ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary + for number of worker threads for the main event loop. :since:`Since 8.4.0`
I don't think we should use the qemu term "main loop" here. In general we refer to the qemu process as emulator, including stuff like emulatorpin and similar.
It isn't even especially part of the QEMU main loop IIUC. Rather it is setting up a pool of threads that are used for serving I/O, when no specific I/O thread is configurd for the guest. Perhaps it can be '<defaultiothread/>' or something along those lines, to make it clear it is an I/O related tunable. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/6/22 13:40, Daniel P. Berrangé wrote:
On Thu, Jun 02, 2022 at 01:54:39PM +0200, Peter Krempa wrote:
On Thu, Jun 02, 2022 at 09:18:04 +0200, Michal Privoznik wrote:
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its main event loop as an QMP object. In the very next commit (v7.0.0-878-g71ad4713cc) it was extended for thread-pool-min and thread-pool-max attributes. Expose them under new <mainloop/> element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 5 ++ src/conf/domain_conf.c | 57 +++++++++++++++++++ src/conf/domain_conf.h | 8 +++ src/conf/schemas/domaincommon.rng | 15 +++++ src/conf/virconftypes.h | 2 + .../iothreads-ids-pool-sizes.xml | 1 + 6 files changed, 88 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index de085f616a..5e0019c574 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst
[...]
@@ -699,6 +700,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` The element has two optional attributes ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. :since:`Since 8.4.0` +``mainloop`` + The optional ``mainloop`` element contains two optional attributes + ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary + for number of worker threads for the main event loop. :since:`Since 8.4.0`
I don't think we should use the qemu term "main loop" here. In general we refer to the qemu process as emulator, including stuff like emulatorpin and similar.
It isn't even especially part of the QEMU main loop IIUC.
Rather it is setting up a pool of threads that are used for serving I/O, when no specific I/O thread is configurd for the guest.
Perhaps it can be '<defaultiothread/>' or something along those lines, to make it clear it is an I/O related tunable.
Right, I'd rather avoid putting it as an attribute to <emulator/> since we have a whole section dedicated to performance tuning. Not to mention <emulator/> lives under <devices/> and I don't think we put performance related knobs there. I didn't object to Peter's suggestion because I don't have better idea. So are you suggesting that <defaultiothread/> would be at the same level as <iothreads/> and <iothreadids/> or it would be nested somewhere? <domain> <name/> <iothreads>4</iothreads> <iothreadids> <iothread id="1" thread_pool_min="2" thread_pool_max="8"/> <iothread id="2"/> <iothreadids> <defaultiothread thread_pool_min="8" thread_pool_max="8"/> <devices/> </domain> This looks relatively nice. Michal
On Tue, Jun 07, 2022 at 10:06:16 +0200, Michal Prívozník wrote:
On 6/6/22 13:40, Daniel P. Berrangé wrote:
On Thu, Jun 02, 2022 at 01:54:39PM +0200, Peter Krempa wrote:
On Thu, Jun 02, 2022 at 09:18:04 +0200, Michal Privoznik wrote:
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its main event loop as an QMP object. In the very next commit (v7.0.0-878-g71ad4713cc) it was extended for thread-pool-min and thread-pool-max attributes. Expose them under new <mainloop/> element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
[...]
It isn't even especially part of the QEMU main loop IIUC.
Rather it is setting up a pool of threads that are used for serving I/O, when no specific I/O thread is configurd for the guest.
Perhaps it can be '<defaultiothread/>' or something along those lines, to make it clear it is an I/O related tunable.
Right, I'd rather avoid putting it as an attribute to <emulator/> since we have a whole section dedicated to performance tuning. Not to mention <emulator/> lives under <devices/> and I don't think we put performance related knobs there. I didn't object to Peter's suggestion because I don't have better idea.
So are you suggesting that <defaultiothread/> would be at the same level as <iothreads/> and <iothreadids/> or it would be nested somewhere?
<domain> <name/> <iothreads>4</iothreads> <iothreadids> <iothread id="1" thread_pool_min="2" thread_pool_max="8"/> <iothread id="2"/> <iothreadids> <defaultiothread thread_pool_min="8" thread_pool_max="8"/> <devices/> </domain>
This looks relatively nice.
Yup, that works also for me. It conveys the proper semantics for what's being set.
On Tue, Jun 07, 2022 at 10:06:16AM +0200, Michal Prívozník wrote:
On 6/6/22 13:40, Daniel P. Berrangé wrote:
On Thu, Jun 02, 2022 at 01:54:39PM +0200, Peter Krempa wrote:
On Thu, Jun 02, 2022 at 09:18:04 +0200, Michal Privoznik wrote:
As of v7.0.0-877-g70ac26b9e5 QEMU exposes its main event loop as an QMP object. In the very next commit (v7.0.0-878-g71ad4713cc) it was extended for thread-pool-min and thread-pool-max attributes. Expose them under new <mainloop/> element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.rst | 5 ++ src/conf/domain_conf.c | 57 +++++++++++++++++++ src/conf/domain_conf.h | 8 +++ src/conf/schemas/domaincommon.rng | 15 +++++ src/conf/virconftypes.h | 2 + .../iothreads-ids-pool-sizes.xml | 1 + 6 files changed, 88 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index de085f616a..5e0019c574 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst
[...]
@@ -699,6 +700,10 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` The element has two optional attributes ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. :since:`Since 8.4.0` +``mainloop`` + The optional ``mainloop`` element contains two optional attributes + ``pool_min`` and ``pool_max`` which allow setting lower and upper boundary + for number of worker threads for the main event loop. :since:`Since 8.4.0`
I don't think we should use the qemu term "main loop" here. In general we refer to the qemu process as emulator, including stuff like emulatorpin and similar.
It isn't even especially part of the QEMU main loop IIUC.
Rather it is setting up a pool of threads that are used for serving I/O, when no specific I/O thread is configurd for the guest.
Perhaps it can be '<defaultiothread/>' or something along those lines, to make it clear it is an I/O related tunable.
Right, I'd rather avoid putting it as an attribute to <emulator/> since we have a whole section dedicated to performance tuning. Not to mention <emulator/> lives under <devices/> and I don't think we put performance related knobs there. I didn't object to Peter's suggestion because I don't have better idea.
So are you suggesting that <defaultiothread/> would be at the same level as <iothreads/> and <iothreadids/> or it would be nested somewhere?
<domain> <name/> <iothreads>4</iothreads> <iothreadids> <iothread id="1" thread_pool_min="2" thread_pool_max="8"/> <iothread id="2"/> <iothreadids> <defaultiothread thread_pool_min="8" thread_pool_max="8"/> <devices/> </domain>
Yeah, that's pretty much what I was implying. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Since the main-loop and iothread classes are derived from the same class (EventLoopBaseClass) we don't need new capability and can use QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX directly to check whether QEMU's capable of setting worker pool size. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_validate.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 21b3407b2f..36153c69c7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -405,6 +405,23 @@ qemuValidateDomainDefIOThreads(const virDomainDef *def, } +static int +qemuValidateDomainDefMainLoop(const virDomainDef *def, + virQEMUCaps *qemuCaps) +{ + if (!def->mainLoop) + return 0; + + if ((def->mainLoop->pool_min >= 0 || def->mainLoop->pool_max >= 0) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("pool_min and pool_max is not supported by this QEMU binary")); + return -1; + } + + return 0; +} + static int qemuValidateDomainDefClockTimers(const virDomainDef *def, virQEMUCaps *qemuCaps) @@ -1192,6 +1209,9 @@ qemuValidateDomainDef(const virDomainDef *def, if (qemuValidateDomainDefIOThreads(def, qemuCaps) < 0) return -1; + if (qemuValidateDomainDefMainLoop(def, qemuCaps) < 0) + return -1; + if (qemuValidateDomainDefClockTimers(def, qemuCaps) < 0) return -1; -- 2.35.1
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2059511 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 33 +++++++++++++++++++ ...othreads-ids-pool-sizes.x86_64-latest.args | 1 + 2 files changed, 34 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 86b5193651..9a9e9699e5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7463,6 +7463,36 @@ qemuBuildIOThreadCommandLine(virCommand *cmd, } +static int +qemuBuildMainLoopCommandLine(virCommand *cmd, + const virDomainDef *def, + virQEMUCaps *qemuCaps) +{ + g_autoptr(virJSONValue) props = NULL; + + if (!def->mainLoop) + return 0; + + if (qemuMonitorCreateObjectProps(&props, "main-loop", "main-loop", NULL) < 0) + return -1; + + if (def->mainLoop->pool_min >= 0 && + virJSONValueObjectAppendNumberLong(props, "thread-pool-min", + def->mainLoop->pool_min) < 0) + return -1; + + if (def->mainLoop->pool_max >= 0 && + virJSONValueObjectAppendNumberLong(props, "thread-pool-max", + def->mainLoop->pool_max) < 0) + return -1; + + if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0) + return -1; + + return 0; +} + + static int qemuBuildNumaCellCache(virCommand *cmd, const virDomainDef *def, @@ -10556,6 +10586,9 @@ qemuBuildCommandLine(virDomainObj *vm, if (qemuBuildIOThreadCommandLine(cmd, def, qemuCaps) < 0) return NULL; + if (qemuBuildMainLoopCommandLine(cmd, def, qemuCaps) < 0) + return NULL; + if (virDomainNumaGetNodeCount(def->numa) && qemuBuildNumaCommandLine(cfg, def, cmd, priv) < 0) return NULL; diff --git a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args index 32517e3d29..5f88ec2ed9 100644 --- a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args +++ b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args @@ -22,6 +22,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -object '{"qom-type":"iothread","id":"iothread1"}' \ -object '{"qom-type":"iothread","id":"iothread3"}' \ -object '{"qom-type":"iothread","id":"iothread5"}' \ +-object '{"qom-type":"main-loop","id":"main-loop","thread-pool-min":10,"thread-pool-max":120}' \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -- 2.35.1
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa