[libvirt] [PATCH 3/3] qemu: Report which iothread IDs are actually returned with VIR_DOMAIN_STATS_IOTHREAD

Peter Krempa posted 3 patches 12 weeks ago

[libvirt] [PATCH 3/3] qemu: Report which iothread IDs are actually returned with VIR_DOMAIN_STATS_IOTHREAD

Posted by Peter Krempa 12 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu: Report which iothread IDs are actually returned with VIR_DOMAIN_STATS_IOTHREAD

Posted by Ján Tomko 12 weeks ago
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu: Report which iothread IDs are actually returned with VIR_DOMAIN_STATS_IOTHREAD

Posted by Peter Krempa 12 weeks ago
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list