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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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 6/2/22 10:08, Peter Krempa wrote: Ooops yes. This is what you get when you write patches but don't finish them before entering freeze :-) 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(). Thanks, Michal

On Thu, Jun 02, 2022 at 09:18:00 +0200, Michal Privoznik wrote: 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 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) With the above: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Thu, Jun 02, 2022 at 09:18:01 +0200, Michal Privoznik wrote: So here you call the enum THREAD_POOL_MIN ... as noted we should do the same in the XML. This could use some more description. 8.5.0 > + */ > +# define VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN "thread_pool_min" > + > +/** > + * VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX: > + * > + * Set the upper bound. -||- Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

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(a)redhat.com&gt; --- 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: [...] One more thing These are 'long long', but ... 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.

On Thu, Jun 02, 2022 at 09:18:03 +0200, Michal Privoznik wrote: Yet another case for using the same name in the XML. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Thu, Jun 02, 2022 at 09:18:04 +0200, Michal Privoznik wrote: [...] 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 6/6/22 13:40, Daniel P. Berrangé wrote: 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:16AM +0200, Michal Prívozník wrote: 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 :|

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2059511 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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