[libvirt] [PATCH 0/3] qemu: fix regression and design deficiencies of VIR_DOMAIN_STATS_IOTHREAD
Fix regression of numbering of the output in the statistics for iothreads and improve few aspects so that the numbering is usable. Peter Krempa (3): lib: Fix documentation for the count field of VIR_DOMAIN_STATS_IOTHREAD qemu: Fix indexes in statistics of iothreads qemu: Report which iothread IDs are actually returned with VIR_DOMAIN_STATS_IOTHREAD src/libvirt-domain.c | 12 +++++++----- src/qemu/qemu_driver.c | 20 +++++++++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) -- 2.23.0
The original implementation used QEMU_ADD_COUNT_PARAM which added the 'count' suffix, but 'cnt' was documented. Fix the documentation to conform with the original implementation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 51fb79cddd..87110036ca 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11615,11 +11615,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * * The typed parameter keys are in this format: * - * "iothread.cnt" - maximum number of IOThreads in the subsequent list - * as unsigned int. Each IOThread in the list will - * will use it's iothread_id value as the <id>. There - * may be fewer <id> entries than the iothread.cnt - * value if the polling values are not supported. + * "iothread.count" - maximum number of IOThreads in the subsequent list + * as unsigned int. Each IOThread in the list will + * will use it's iothread_id value as the <id>. There + * may be fewer <id> entries than the iothread.count + * value if the polling values are not supported. * "iothread.<id>.poll-max-ns" - maximum polling time in ns as an unsigned * long long. A 0 (zero) means polling is * disabled. -- 2.23.0
On Fri, Nov 29, 2019 at 09:59:26AM +0100, Peter Krempa wrote:
The original implementation used QEMU_ADD_COUNT_PARAM which added the 'count' suffix, but 'cnt' was documented. Fix the documentation to conform with the original implementation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In commit 2ccb5335dc4 I've refactored how we fill the typed parameters for domain statistics. The commit introduced a regression in the formating of stats for IOthreads by using the array index to label the entries as it's common for all other types of statistics rather than the iothread IDs used for iothreads. Since only the design of iothread deviates from the common approach used in all other statistic types this was not caught. https://bugzilla.redhat.com/show_bug.cgi?id=1778014 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 18bd0101e7..b5300241a8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21208,13 +21208,16 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, for (i = 0; i < niothreads; i++) { if (iothreads[i]->poll_valid) { if (virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, - "iothread.%zu.poll-max-ns", i) < 0) + "iothread.%u.poll-max-ns", + iothreads[i]->iothread_id) < 0) goto cleanup; if (virTypedParamListAddUInt(params, iothreads[i]->poll_grow, - "iothread.%zu.poll-grow", i) < 0) + "iothread.%u.poll-grow", + iothreads[i]->iothread_id) < 0) goto cleanup; if (virTypedParamListAddUInt(params, iothreads[i]->poll_shrink, - "iothread.%zu.poll-shrink", i) < 0) + "iothread.%u.poll-shrink", + iothreads[i]->iothread_id) < 0) goto cleanup; } } -- 2.23.0
On Fri, Nov 29, 2019 at 09:59:27AM +0100, Peter Krempa wrote:
In commit 2ccb5335dc4 I've refactored how we fill the typed parameters for domain statistics. The commit introduced a regression in the formating of stats for IOthreads by using the array index to label the entries as it's common for all other types of statistics rather than the iothread IDs used for iothreads.
Since only the design of iothread deviates from the common approach used in all other statistic types this was not caught.
https://bugzilla.redhat.com/show_bug.cgi?id=1778014
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The design of the stats fields returned for VIR_DOMAIN_STATS_IOTHREAD domain statistics groups deviates from the established pattern. In this instance it's impossible to infer which values of <id> for iothread.<id>... fields will be reported back because they have no connection to the iothread.count field. Introduce iothread.ids which will report a comma-separated list of <id>s reported in the subsequent array in the order they will be reported. virsh domstats upstream --iothread Domain: 'upstream' iothread.count=2 iothread.ids=7,5 iothread.7.poll-max-ns=32768 iothread.7.poll-grow=0 iothread.7.poll-shrink=0 iothread.5.poll-max-ns=32768 iothread.5.poll-grow=0 iothread.5.poll-shrink=0 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 2 ++ src/qemu/qemu_driver.c | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 87110036ca..e6d5697445 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11620,6 +11620,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * will use it's iothread_id value as the <id>. There * may be fewer <id> entries than the iothread.count * value if the polling values are not supported. + * "iothread.ids" - a comma separated list of iotdread <id>s reported in the + * subsequent list reported as a string * "iothread.<id>.poll-max-ns" - maximum polling time in ns as an unsigned * long long. A 0 (zero) means polling is * disabled. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b5300241a8..4ccc9d3d4e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21189,6 +21189,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, qemuMonitorIOThreadInfoPtr *iothreads = NULL; int niothreads; int ret = -1; + g_auto(virBuffer) iothridbuf = VIR_BUFFER_INITIALIZER; + g_autofree char *iothridstr = NULL; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0; @@ -21205,6 +21207,15 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, if (virTypedParamListAddUInt(params, niothreads, "iothread.count") < 0) goto cleanup; + for (i = 0; i < niothreads; i++) + virBufferAsprintf(&iothridbuf, "%u,", iothreads[i]->iothread_id); + + virBufferTrim(&iothridbuf, ",", -1); + iothridstr = virBufferContentAndReset(&iothridbuf); + + if (virTypedParamListAddString(params, iothridstr, "iothread.ids") < 0) + goto cleanup; + for (i = 0; i < niothreads; i++) { if (iothreads[i]->poll_valid) { if (virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, -- 2.23.0
On Fri, Nov 29, 2019 at 09:59:28AM +0100, Peter Krempa wrote:
The design of the stats fields returned for VIR_DOMAIN_STATS_IOTHREAD domain statistics groups deviates from the established pattern. In this instance it's impossible to infer which values of <id> for iothread.<id>... fields will be reported back because they have no connection to the iothread.count field.
Knowing it upfront allows the users to construct the parameter name for the virTypedParamsGet APIs, but processing the returned parameters in this way seems inefficient - I'd expect any serious user to iterate over all the params and collect the iothread <id>s which are reported.
Introduce iothread.ids which will report a comma-separated list of <id>s reported in the subsequent array in the order they will be reported.
Why is the order important? Can it be returned as a stringified bitmap?
virsh domstats upstream --iothread Domain: 'upstream' iothread.count=2 iothread.ids=7,5 iothread.7.poll-max-ns=32768 iothread.7.poll-grow=0 iothread.7.poll-shrink=0 iothread.5.poll-max-ns=32768 iothread.5.poll-grow=0 iothread.5.poll-shrink=0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 2 ++ src/qemu/qemu_driver.c | 11 +++++++++++ 2 files changed, 13 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 87110036ca..e6d5697445 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11620,6 +11620,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * will use it's iothread_id value as the <id>. There * may be fewer <id> entries than the iothread.count * value if the polling values are not supported. + * "iothread.ids" - a comma separated list of iotdread <id>s reported in the
s/iotdread/iothread/ Jano
+ * subsequent list reported as a string * "iothread.<id>.poll-max-ns" - maximum polling time in ns as an unsigned * long long. A 0 (zero) means polling is * disabled. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b5300241a8..4ccc9d3d4e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21189,6 +21189,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, qemuMonitorIOThreadInfoPtr *iothreads = NULL; int niothreads; int ret = -1; + g_auto(virBuffer) iothridbuf = VIR_BUFFER_INITIALIZER; + g_autofree char *iothridstr = NULL;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0; @@ -21205,6 +21207,15 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, if (virTypedParamListAddUInt(params, niothreads, "iothread.count") < 0) goto cleanup;
+ for (i = 0; i < niothreads; i++) + virBufferAsprintf(&iothridbuf, "%u,", iothreads[i]->iothread_id); + + virBufferTrim(&iothridbuf, ",", -1); + iothridstr = virBufferContentAndReset(&iothridbuf); + + if (virTypedParamListAddString(params, iothridstr, "iothread.ids") < 0) + goto cleanup; + for (i = 0; i < niothreads; i++) { if (iothreads[i]->poll_valid) { if (virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, -- 2.23.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Nov 29, 2019 at 10:46:47 +0100, Ján Tomko wrote:
On Fri, Nov 29, 2019 at 09:59:28AM +0100, Peter Krempa wrote:
The design of the stats fields returned for VIR_DOMAIN_STATS_IOTHREAD domain statistics groups deviates from the established pattern. In this instance it's impossible to infer which values of <id> for iothread.<id>... fields will be reported back because they have no connection to the iothread.count field.
Knowing it upfront allows the users to construct the parameter name for the virTypedParamsGet APIs, but processing the returned parameters in this way seems inefficient - I'd expect any serious user to iterate over all the params and collect the iothread <id>s which are reported.
My grief is that there's no connection between the count field and the available ids later on. This is specifically irritating if the stats are not actually reported as then you have no way of figuring out which iothread ids were actually examined. Said this I don't think this patch is as important though so I can drop it.
Introduce iothread.ids which will report a comma-separated list of <id>s reported in the subsequent array in the order they will be reported.
Why is the order important? Can it be returned as a stringified bitmap?
The order is probably not important at all. This is a easy way to format the string but mainly the stringified bitmaps are very unpleasant to parse if you don't have our internal functions so I tried to avoid that.
virsh domstats upstream --iothread Domain: 'upstream' iothread.count=2 iothread.ids=7,5 iothread.7.poll-max-ns=32768 iothread.7.poll-grow=0 iothread.7.poll-shrink=0 iothread.5.poll-max-ns=32768 iothread.5.poll-grow=0 iothread.5.poll-shrink=0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 2 ++ src/qemu/qemu_driver.c | 11 +++++++++++ 2 files changed, 13 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 87110036ca..e6d5697445 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11620,6 +11620,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * will use it's iothread_id value as the <id>. There * may be fewer <id> entries than the iothread.count * value if the polling values are not supported. + * "iothread.ids" - a comma separated list of iotdread <id>s reported in the
s/iotdread/iothread/
Jano
+ * subsequent list reported as a string * "iothread.<id>.poll-max-ns" - maximum polling time in ns as an unsigned * long long. A 0 (zero) means polling is * disabled. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b5300241a8..4ccc9d3d4e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21189,6 +21189,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, qemuMonitorIOThreadInfoPtr *iothreads = NULL; int niothreads; int ret = -1; + g_auto(virBuffer) iothridbuf = VIR_BUFFER_INITIALIZER; + g_autofree char *iothridstr = NULL;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0; @@ -21205,6 +21207,15 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, if (virTypedParamListAddUInt(params, niothreads, "iothread.count") < 0) goto cleanup;
+ for (i = 0; i < niothreads; i++) + virBufferAsprintf(&iothridbuf, "%u,", iothreads[i]->iothread_id); + + virBufferTrim(&iothridbuf, ",", -1); + iothridstr = virBufferContentAndReset(&iothridbuf); + + if (virTypedParamListAddString(params, iothridstr, "iothread.ids") < 0) + goto cleanup; + for (i = 0; i < niothreads; i++) { if (iothreads[i]->poll_valid) { if (virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, -- 2.23.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Ján Tomko -
Peter Krempa