Re: [PATCH] hw/virtio: reset virtio balloon stats on machine reset

[David Hildenbrand <david@redhat.com>]

On 18.12.24 18:29, Daniel P. Berrangé wrote:
> When a machine is first booted, all virtio balloon stats are initialized
> to their default value -1 (18446744073709551615 when represented as
> unsigned).
> 
> They remain that way while the firmware is loading, and early phase of
> guest OS boot, until the virtio-balloon driver is activated. Thereafter
> the reported stats reflect the guest OS activity.
> 
> When a machine reset is performed, however, the virtio-balloon stats are
> left unchanged by QEMU, despite the guest OS no longer updating them,
> nor indeed even still existing.
> 
> IOW, the mgmt app keeps getting stale stats until the guest OS starts
> once more and loads the virtio-balloon driver (if ever). At that point
> the app will see a discontinuity in the reported values as they sudden
> jump from the stale value to the new value. This jump is indigituishable
> from a valid data update.
> 
> While there is an "last-updated" field to report on the freshness of
> the stats, that does not unambiguously tell the mgmt app whether the
> stats are still conceptually relevant to the current running workload.
> 
> It is more conceptually useful to reset the stats to their default
> values on machine reset, given that the previous guest workload the
> stats reflect no longer exists. The mgmt app can now clearly identify
> that there are is no stats information available from the current
> executing workload.
> 
> The 'last-updated' time is also reset back to 0.
> 
> IOW, on every machine reset, the virtio stats are in the same clean
> state they were when the macine first powered on.
> 
> A functional test is added to validate this behaviour with a real
> world guest OS.
 > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> 
> One side-thought I have, is whether it makes sense to add a
> 'reset-count' field in the virtio stats, alongside the
> 'last-updated' field. While apps can infer a reset from seeing
> the stats all go back to their defaults, an explicit flag is
> simpler...
> 
>   MAINTAINERS                             |   1 +
>   hw/virtio/virtio-balloon.c              |  30 ++++-
>   include/hw/virtio/virtio-balloon.h      |   4 +
>   tests/functional/test_virtio_balloon.py | 161 ++++++++++++++++++++++++
>   4 files changed, 195 insertions(+), 1 deletion(-)
>   create mode 100755 tests/functional/test_virtio_balloon.py
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 822f34344b..1380d53d03 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2234,6 +2234,7 @@ F: include/hw/virtio/virtio-balloon.h
>   F: system/balloon.c
>   F: include/sysemu/balloon.h
>   F: tests/qtest/virtio-balloon-test.c
> +F: tests/functional/test_virtio_balloon.py
>   
>   virtio-9p
>   M: Greg Kurz <groug@kaod.org>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index ab2ee30475..fe0854e198 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -31,7 +31,7 @@
>   #include "trace.h"
>   #include "qemu/error-report.h"
>   #include "migration/misc.h"
> -
> +#include "sysemu/reset.h"
>   #include "hw/virtio/virtio-bus.h"
>   #include "hw/virtio/virtio-access.h"
>   
> @@ -910,6 +910,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>       }
>   
>       reset_stats(s);
> +    s->stats_last_update = 0;
> +    qemu_register_resettable(OBJECT(dev));
>   }
>   
>   static void virtio_balloon_device_unrealize(DeviceState *dev)
> @@ -917,6 +919,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>       VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>       VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>   
> +    qemu_unregister_resettable(OBJECT(dev));
>       if (s->free_page_bh) {
>           qemu_bh_delete(s->free_page_bh);
>           object_unref(OBJECT(s->iothread));
> @@ -987,6 +990,27 @@ static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
>       }
>   }

Using qemu_register_resettable() can have unfortunate side effects that 
this code is triggered when the device is reset, not necessarily when 
the complete machine.

For virtio-mem at least that's an issue, and here is how I'll fix it:

https://lore.kernel.org/qemu-devel/20241218105303.1966303-2-david@redhat.com/

-- 
Cheers,

David / dhildenb