Re: [PATCH 1/1] perf: list all supported events when get domain stats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/09/2017 01:06 AM, Qiaowei Ren wrote:
> Currently when virDomainListGetStats is called, the stats for those
> disabled perf events won't be showed in result. This will produce
> some problems for applications based on libvirt sometime, e.g. the
> OpenStack bug https://bugs.launchpad.net/ceilometer/+bug/1670948
> 
> This patch just show '0' in result for disabled events for
> virDomainListGetStats API. I guess this should be also rational to
> show all stats even though the events are not enabled.
> 
> Signed-off-by: Qiaowei Ren <qiaowei.ren@xxxxxxxxx>
> ---
>  src/qemu/qemu_driver.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 

Without involving OpenStack (e.g., use virsh output) what is the
difference a user will see? I believe what you're indicating is that now
whatever the server can support will be returned with a value of 0
(zero) or whatever the current value that's queried whether or not the
collection of the event is enabled or not.

Here's my concern - wouldn't this go against the spirit of being able to
not see certain events by just disabling them? How would a client
distinguish between a "real" return value of 0 vs. a because we we're
not collecting this data so we return a value of 0?  How do applications
that have coded to that second model now view getting a 0 value back?

The client should be able to handle not getting a specific value,
shouldn't it?

I think there is history of only returning valid values that the target
supports and having the client make decisions from there.  I believe
it's possible to get a list of what perf has enabled. If you get that
list and it doesn't contain what you want, but that's needed for
something else, then the client can decide what to do rather than the
server (e.g. libvirt).

John
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2032fac..237bf57 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19355,8 +19355,10 @@ qemuDomainGetStatsPerfOneEvent(virPerfPtr perf,
>      char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>      uint64_t value = 0;
>  
> -    if (virPerfReadEvent(perf, type, &value) < 0)
> -        return -1;
> +    if (virPerfEventIsEnabled(perf, type)) {
> +        if (virPerfReadEvent(perf, type, &value) < 0)
> +            return -1;
> +    }
>  
>      snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "perf.%s",
>               virPerfEventTypeToString(type));
> @@ -19383,9 +19385,6 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>      int ret = -1;
>  
>      for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
> -        if (!virPerfEventIsEnabled(priv->perf, i))
> -             continue;
> -
>          if (qemuDomainGetStatsPerfOneEvent(priv->perf, i,
>                                             record, maxparams) < 0)
>              goto cleanup;
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux