[Qemu-devel] [PATCH 1/1] virtio-balloon: reset the statistic timer to load device

[Qemu-devel] [PATCH 1/1] virtio-balloon: reset the statistic timer to load device

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

If before loading snapshot we had set the timer of statistics, then after
applying snapshot the expiry time would be irrelevant for the restored
state of the virtual clocks. A simple fix is just to restart the timer
after loading snapshot.

For the user it may look like a long delay of statistics update after switch
to the snapshot.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 22ad25c..c74101e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -426,6 +426,10 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
 
     s->num_pages = qemu_get_be32(f);
     s->actual = qemu_get_be32(f);
+
+    if (balloon_stats_enabled(s)) {
+        balloon_stats_change_timer(s, s->stats_poll_interval);
+    }
     return 0;
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/1] virtio-balloon: reset the statistic timer to load device

[Michael S. Tsirkin]

On Tue, Mar 29, 2016 at 05:00:49PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> If before loading snapshot we had set the timer of statistics, then after
> applying snapshot the expiry time would be irrelevant for the restored
> state of the virtual clocks. A simple fix is just to restart the timer
> after loading snapshot.
> 
> For the user it may look like a long delay of statistics update after switch
> to the snapshot.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael S. Tsirkin <mst@redhat.com>

I'm inclined to think we really should migrate the timer,
otherwise user might wait twice as long as expected ...

> ---
>  hw/virtio/virtio-balloon.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 22ad25c..c74101e 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -426,6 +426,10 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>  
>      s->num_pages = qemu_get_be32(f);
>      s->actual = qemu_get_be32(f);
> +
> +    if (balloon_stats_enabled(s)) {
> +        balloon_stats_change_timer(s, s->stats_poll_interval);
> +    }
>      return 0;
>  }
>  
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/1] virtio-balloon: reset the statistic timer to load device

[Denis V. Lunev]

On 03/29/2016 05:07 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 29, 2016 at 05:00:49PM +0300, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> If before loading snapshot we had set the timer of statistics, then after
>> applying snapshot the expiry time would be irrelevant for the restored
>> state of the virtual clocks. A simple fix is just to restart the timer
>> after loading snapshot.
>>
>> For the user it may look like a long delay of statistics update after switch
>> to the snapshot.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Michael S. Tsirkin <mst@redhat.com>
> I'm inclined to think we really should migrate the timer,
> otherwise user might wait twice as long as expected ...
this is not a big deal. This timer is a part of the QEMU state,
which was configured for this exact running instance, not
for the guest. Moreover, we have switched to new guest state
which can be on the different CPU with different timings etc
and thus we should let the guest to run for some time.

In the perfect world you should not save the time to the migration
state but save the time at reset callback and restore it here at
postload, but this seems over engineering.

Statistics delivery is "best effort", nobody will die if one
shot will be missed.

Re: [Qemu-devel] [PATCH 1/1] virtio-balloon: reset the statistic timer to load device

[Denis V. Lunev]

On 03/29/2016 05:18 PM, Denis V. Lunev wrote:
> On 03/29/2016 05:07 PM, Michael S. Tsirkin wrote:
>> On Tue, Mar 29, 2016 at 05:00:49PM +0300, Denis V. Lunev wrote:
>>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>
>>> If before loading snapshot we had set the timer of statistics, then 
>>> after
>>> applying snapshot the expiry time would be irrelevant for the restored
>>> state of the virtual clocks. A simple fix is just to restart the timer
>>> after loading snapshot.
>>>
>>> For the user it may look like a long delay of statistics update 
>>> after switch
>>> to the snapshot.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Michael S. Tsirkin <mst@redhat.com>
>> I'm inclined to think we really should migrate the timer,
>> otherwise user might wait twice as long as expected ...
> this is not a big deal. This timer is a part of the QEMU state,
> which was configured for this exact running instance, not
> for the guest. Moreover, we have switched to new guest state
> which can be on the different CPU with different timings etc
> and thus we should let the guest to run for some time.
>
> In the perfect world you should not save the time to the migration
> state but save the time at reset callback and restore it here at
> postload, but this seems over engineering.
>
> Statistics delivery is "best effort", nobody will die if one
> shot will be missed.
could you pls select on of the options below to highlight way
to merge pls?

1) keep code as is. Restarting timer to new policy is fine
2) restart with the remaining timeout of previous timer

For me 1) is fine, though...

Den