Re: [Qemu-devel] [PATCH 03/12] migration: Switch to using announcement timer

[Jason Wang <jasowang@redhat.com>]

On 2017年05月25日 02:05, Vladislav Yasevich wrote:
> Switch qemu_announce_self and virtio annoucements to use
> the announcement timer framework.  This makes sure that both
> timers use the same timeouts and number of annoucement attempts
>
> Based on work by Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>   hw/net/virtio-net.c            | 28 ++++++++++++++++------------
>   include/hw/virtio/virtio-net.h |  3 +--
>   include/migration/vmstate.h    | 17 +++++++++++------
>   include/sysemu/sysemu.h        |  2 +-
>   migration/migration.c          |  2 +-
>   migration/savevm.c             | 28 ++++++++++++++--------------
>   6 files changed, 44 insertions(+), 36 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7d091c9..1c65825 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -25,6 +25,7 @@
>   #include "qapi/qmp/qjson.h"
>   #include "qapi-event.h"
>   #include "hw/virtio/virtio-access.h"
> +#include "migration/migration.h"
>   
>   #define VIRTIO_NET_VM_VERSION    11
>   
> @@ -115,7 +116,7 @@ static void virtio_net_announce_timer(void *opaque)
>       VirtIONet *n = opaque;
>       VirtIODevice *vdev = VIRTIO_DEVICE(n);
>   
> -    n->announce_counter--;
> +    n->announce_timer->round--;
>       n->status |= VIRTIO_NET_S_ANNOUNCE;
>       virtio_notify_config(vdev);
>   }
> @@ -427,8 +428,8 @@ static void virtio_net_reset(VirtIODevice *vdev)
>       n->nobcast = 0;
>       /* multiqueue is disabled by default */
>       n->curr_queues = 1;
> -    timer_del(n->announce_timer);
> -    n->announce_counter = 0;
> +    timer_del(n->announce_timer->tm);
> +    n->announce_timer->round = 0;
>       n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>   
>       /* Flush any MAC and VLAN filter table state */
> @@ -872,10 +873,10 @@ static int virtio_net_handle_announce(VirtIONet *n, uint8_t cmd,
>       if (cmd == VIRTIO_NET_CTRL_ANNOUNCE_ACK &&
>           n->status & VIRTIO_NET_S_ANNOUNCE) {
>           n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> -        if (n->announce_counter) {
> -            timer_mod(n->announce_timer,
> +        if (n->announce_timer->round) {
> +            timer_mod(n->announce_timer->tm,
>                         qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> -                      self_announce_delay(n->announce_counter));
> +                      self_announce_delay(n->announce_timer));
>           }
>           return VIRTIO_NET_OK;
>       } else {
> @@ -1615,8 +1616,8 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>   
>       if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
>           virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> -        n->announce_counter = SELF_ANNOUNCE_ROUNDS;
> -        timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
> +        n->announce_timer->round = n->announce_timer->params.rounds;
> +        timer_mod(n->announce_timer->tm, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
>       }
>   
>       return 0;
> @@ -1938,8 +1939,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>       qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
>       memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
>       n->status = VIRTIO_NET_S_LINK_UP;
> -    n->announce_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> -                                     virtio_net_announce_timer, n);
> +    n->announce_timer = qemu_announce_timer_new(qemu_get_announce_params(),
> +                                                QEMU_CLOCK_VIRTUAL);
> +    n->announce_timer->tm = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> +                                          virtio_net_announce_timer, n);
>   
>       if (n->netclient_type) {
>           /*
> @@ -2001,8 +2004,9 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
>           virtio_net_del_queue(n, i);
>       }
>   
> -    timer_del(n->announce_timer);
> -    timer_free(n->announce_timer);
> +    timer_del(n->announce_timer->tm);
> +    timer_free(n->announce_timer->tm);
> +    g_free(n->announce_timer);
>       g_free(n->vqs);
>       qemu_del_nic(n->nic);
>       virtio_cleanup(vdev);
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 1eec9a2..51dd4c3 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -94,8 +94,7 @@ typedef struct VirtIONet {
>       char *netclient_name;
>       char *netclient_type;
>       uint64_t curr_guest_offloads;
> -    QEMUTimer *announce_timer;
> -    int announce_counter;
> +    AnnounceTimer *announce_timer;
>       bool needs_vnet_hdr_swap;
>   } VirtIONet;
>   
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index a6bf84d..f8aed9b 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1022,8 +1022,6 @@ extern const VMStateInfo vmstate_info_qtailq;
>   #define VMSTATE_END_OF_LIST()                                         \
>       {}
>   
> -#define SELF_ANNOUNCE_ROUNDS 5
> -
>   void loadvm_free_handlers(MigrationIncomingState *mis);
>   
>   int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> @@ -1071,11 +1069,18 @@ AnnounceTimer *qemu_announce_timer_create(AnnounceParameters *params,
>                                             QEMUTimerCB *cb);
>   
>   static inline
> -int64_t self_announce_delay(int round)
> +int64_t self_announce_delay(AnnounceTimer *timer)
>   {
> -    assert(round < SELF_ANNOUNCE_ROUNDS && round > 0);
> -    /* delay 50ms, 150ms, 250ms, ... */
> -    return 50 + (SELF_ANNOUNCE_ROUNDS - round - 1) * 100;
> +    int64_t ret;
> +
> +    ret =  timer->params.initial +
> +           (timer->params.rounds - timer->round - 1) *
> +           timer->params.step;
> +
> +    if (ret < 0 || ret > timer->params.max) {
> +        ret = timer->params.max;
> +    }

Can we move this check to qemu_validate_announce_parameters()?

> +    return ret;
>   }
>   
>   void dump_vmstate_json_to_file(FILE *out_fp);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 7fd49c4..2ef1687 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -85,7 +85,7 @@ bool qemu_validate_announce_parameters(AnnounceParameters *params,
>                                          Error **errp);
>   void qemu_set_announce_parameters(AnnounceParameters *announce_params,
>                                     AnnounceParameters *params);
> -void qemu_announce_self(void);
> +void qemu_announce_self(AnnounceParameters *params);
>   
>   /* Subcommands for QEMU_VM_COMMAND */
>   enum qemu_vm_cmd {
> diff --git a/migration/migration.c b/migration/migration.c
> index 0304c01..987c1cf 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -345,7 +345,7 @@ static void process_incoming_migration_bh(void *opaque)
>        * This must happen after all error conditions are dealt with and
>        * we're sure the VM is going to be running on this host.
>        */
> -    qemu_announce_self();
> +    qemu_announce_self(qemu_get_announce_params());
>   
>       /* If global state section was not received or we are in running
>          state, we need to obey autostart. Any other state is set with
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 607b090..555157a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -212,21 +212,19 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>       qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>   }
>   
> -
>   static void qemu_announce_self_once(void *opaque)
>   {
> -    static int count = SELF_ANNOUNCE_ROUNDS;
> -    QEMUTimer *timer = *(QEMUTimer **)opaque;
> +    AnnounceTimer *timer = (AnnounceTimer *)opaque;
>   
>       qemu_foreach_nic(qemu_announce_self_iter, NULL);
>   
> -    if (--count) {
> -        /* delay 50ms, 150ms, 250ms, ... */
> -        timer_mod(timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> -                  self_announce_delay(count));
> +    if (--timer->round) {
> +        timer_mod(timer->tm, qemu_clock_get_ms(timer->type) +
> +                  self_announce_delay(timer));
>       } else {
> -            timer_del(timer);
> -            timer_free(timer);
> +            timer_del(timer->tm);
> +            timer_free(timer->tm);
> +            g_free(timer);
>       }
>   }
>   
> @@ -252,11 +250,13 @@ AnnounceTimer *qemu_announce_timer_create(AnnounceParameters *params,
>       return timer;
>   }
>   
> -void qemu_announce_self(void)
> +void qemu_announce_self(AnnounceParameters *params)
>   {
> -    static QEMUTimer *timer;
> -    timer = timer_new_ms(QEMU_CLOCK_REALTIME, qemu_announce_self_once, &timer);
> -    qemu_announce_self_once(&timer);
> +    AnnounceTimer *timer;
> +
> +    timer = qemu_announce_timer_create(params, QEMU_CLOCK_REALTIME,
> +                                       qemu_announce_self_once);
> +    qemu_announce_self_once(timer);
>   }
>   
>   /***********************************************************/
> @@ -1730,7 +1730,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
>        */
>       cpu_synchronize_all_post_init();
>   
> -    qemu_announce_self();
> +    qemu_announce_self(qemu_get_announce_params());
>   
>       /* Make sure all file formats flush their mutable metadata.
>        * If we get an error here, just don't restart the VM yet. */

Reviewed-by: Jason Wang <jasowang@redhat.com>