virtio-net: consolidate guest announcement with qemu_announce_self

Commit Message

Vlad Yasevich March 29, 2017, 8:19 p.m. UTC

virtio announcements seem to run on its own timer with it's own
repetition loop and are invoked separately from qemu_announce_self.
This patch consolidates announcements into a single location and
allows other nics to provide it's own announcement capabilities, if
necessary.

This is also usefull in support of exposing qemu_announce_self through
qmp/hmp.

Signed-off-by: Vladislav Yasevich <[e-mail]>
---
 hw/net/virtio-net.c            | 30 ++++++++----------------------
 include/hw/virtio/virtio-net.h |  2 --
 include/net/net.h              |  2 ++
 migration/savevm.c             |  6 ++++++
 4 files changed, 16 insertions(+), 24 deletions(-)

Comments

Michael S. Tsirkin March 29, 2017, 8:35 p.m. UTC | #1

On Wed, Mar 29, 2017 at 04:19:50PM -0400, Vladislav Yasevich wrote:
> virtio announcements seem to run on its own timer with it's own
> repetition loop and are invoked separately from qemu_announce_self.
> This patch consolidates announcements into a single location and
> allows other nics to provide it's own

their own

> announcement capabilities, if
> necessary.
> 
> This is also usefull

or useful?

> in support of exposing qemu_announce_self through
> qmp/hmp.

I'm guessing this is the real reason for the change.
How is it helpful for that? Pls clarify in the commit log.

> Signed-off-by: Vladislav Yasevich <[e-mail]>
> ---
>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>  include/hw/virtio/virtio-net.h |  2 --
>  include/net/net.h              |  2 ++
>  migration/savevm.c             |  6 ++++++
>  4 files changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index c321680..6e9ce4f 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>  }
>  
> -static void virtio_net_announce_timer(void *opaque)
> +static void virtio_net_announce(NetClientState *nc)
>  {
> -    VirtIONet *n = opaque;
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>  
> -    n->announce_counter--;
> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> -    virtio_notify_config(vdev);
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> +            virtio_notify_config(vdev);
> +    }
>  }
>  
>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> @@ -427,8 +429,6 @@ 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;
>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>  
>      /* Flush any MAC and VLAN filter table state */
> @@ -868,11 +868,6 @@ 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,
> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> -                      self_announce_delay(n->announce_counter));
> -        }
>          return VIRTIO_NET_OK;
>      } else {
>          return VIRTIO_NET_ERR;

you will notice that this runs at VM speeds with QEMU_CLOCK_VIRTUAL,
while the self announce you are tying this to runs with
QEMU_CLOCK_REALTIME.

So this switch seems wrong to me: by the time VM is started
on destination all timers might have expired.


> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>      }
>  
> -    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));
> -    }
> -
>      return 0;
>  }
>  
> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>      .link_status_changed = virtio_net_set_link_status,
>      .query_rx_filter = virtio_net_query_rxfilter,
> +    .announce = virtio_net_announce,
>  };
>  
>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> @@ -1934,8 +1924,6 @@ 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);
>  
>      if (n->netclient_type) {
>          /*
> @@ -1997,8 +1985,6 @@ 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);
>      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..0c597f4 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>      char *netclient_name;
>      char *netclient_type;
>      uint64_t curr_guest_offloads;
> -    QEMUTimer *announce_timer;
> -    int announce_counter;
>      bool needs_vnet_hdr_swap;
>  } VirtIONet;
>  
> diff --git a/include/net/net.h b/include/net/net.h
> index 99b28d5..598f523 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>  typedef int (SetVnetBE)(NetClientState *, bool);
>  typedef struct SocketReadState SocketReadState;
>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> +typedef void (NetAnnounce)(NetClientState *);
>  
>  typedef struct NetClientInfo {
>      NetClientDriver type;
> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>      SetVnetHdrLen *set_vnet_hdr_len;
>      SetVnetLE *set_vnet_le;
>      SetVnetBE *set_vnet_be;
> +    NetAnnounce *announce;
>  } NetClientInfo;
>  
>  struct NetClientState {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 3b19a4a..5c1d8dc 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>      int len;
>  
>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> +
>      len = announce_self_create(buf, nic->conf->macaddr.a);
>  
>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> +
> +    /* if the NIC provides it's own announcement support, use it as well */
> +    if (nic->ncs->info->announce) {
> +        nic->ncs->info->announce(nic->ncs);
> +    }
>  }


Can we switch existing qemu announcements to QEMU_CLOCK_VIRTUAL?

It's an interesting question, and I'm not sure.

I note that this seems to interact interestingly with recent dgilbert's
patch sweaking announcement timing. Some coordination might be
helpful here.


>  
> -- 
> 2.7.4

Dr. David Alan Gilbert March 30, 2017, 8:24 a.m. UTC | #2

* Vladislav Yasevich ([e-mail]) wrote:
> virtio announcements seem to run on its own timer with it's own
> repetition loop and are invoked separately from qemu_announce_self.
> This patch consolidates announcements into a single location and
> allows other nics to provide it's own announcement capabilities, if
> necessary.
> 
> This is also usefull in support of exposing qemu_announce_self through
> qmp/hmp.
> 
> Signed-off-by: Vladislav Yasevich <[e-mail]>
> ---
>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>  include/hw/virtio/virtio-net.h |  2 --
>  include/net/net.h              |  2 ++
>  migration/savevm.c             |  6 ++++++
>  4 files changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index c321680..6e9ce4f 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>  }
>  
> -static void virtio_net_announce_timer(void *opaque)
> +static void virtio_net_announce(NetClientState *nc)
>  {
> -    VirtIONet *n = opaque;
> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>  
> -    n->announce_counter--;
> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> -    virtio_notify_config(vdev);
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> +            virtio_notify_config(vdev);
> +    }
>  }
>  
>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> @@ -427,8 +429,6 @@ 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;
>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>  
>      /* Flush any MAC and VLAN filter table state */
> @@ -868,11 +868,6 @@ 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,
> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> -                      self_announce_delay(n->announce_counter));
> -        }
>          return VIRTIO_NET_OK;
>      } else {
>          return VIRTIO_NET_ERR;
> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>      }
>  
> -    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));
> -    }
> -
>      return 0;
>  }
>  
> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>      .receive = virtio_net_receive,
>      .link_status_changed = virtio_net_set_link_status,
>      .query_rx_filter = virtio_net_query_rxfilter,
> +    .announce = virtio_net_announce,
>  };
>  
>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> @@ -1934,8 +1924,6 @@ 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);
>  
>      if (n->netclient_type) {
>          /*
> @@ -1997,8 +1985,6 @@ 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);
>      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..0c597f4 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>      char *netclient_name;
>      char *netclient_type;
>      uint64_t curr_guest_offloads;
> -    QEMUTimer *announce_timer;
> -    int announce_counter;
>      bool needs_vnet_hdr_swap;
>  } VirtIONet;
>  
> diff --git a/include/net/net.h b/include/net/net.h
> index 99b28d5..598f523 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>  typedef int (SetVnetBE)(NetClientState *, bool);
>  typedef struct SocketReadState SocketReadState;
>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> +typedef void (NetAnnounce)(NetClientState *);
>  
>  typedef struct NetClientInfo {
>      NetClientDriver type;
> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>      SetVnetHdrLen *set_vnet_hdr_len;
>      SetVnetLE *set_vnet_le;
>      SetVnetBE *set_vnet_be;
> +    NetAnnounce *announce;
>  } NetClientInfo;
>  
>  struct NetClientState {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 3b19a4a..5c1d8dc 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>      int len;
>  
>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> +
>      len = announce_self_create(buf, nic->conf->macaddr.a);
>  
>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> +
> +    /* if the NIC provides it's own announcement support, use it as well */
> +    if (nic->ncs->info->announce) {
> +        nic->ncs->info->announce(nic->ncs);
> +    }
>  }

Combining them doesn't necessarily seem like a bad thing; but
it does change the timing quite a bit - at the moment the QEMU RARPs
are sent at the end of migration, while the Virtio ARPs are sent
when the guest starts running which is quite a bit later.

In many ways the 'when the guest starts' is better, given that libvirt
etc has had a chance to reconfigure the networking,  but I'm not that
sure if it's safe to move the existing one - I had considered *adding*
another shot of the current mechanism after the guest is started.

I certainly think it's wrong to do the virtio announce at the earlier
point.

See also Germano's thread of being able to retrigger the announce
at arbitrary points, and the series I posted a couple of days ago
to extend the length/timing of the announce.

Dave

> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Vlad Yasevich March 30, 2017, 1:06 p.m. UTC | #3

On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
> * Vladislav Yasevich ([e-mail]) wrote:
>> virtio announcements seem to run on its own timer with it's own
>> repetition loop and are invoked separately from qemu_announce_self.
>> This patch consolidates announcements into a single location and
>> allows other nics to provide it's own announcement capabilities, if
>> necessary.
>>
>> This is also usefull in support of exposing qemu_announce_self through
>> qmp/hmp.
>>
>> Signed-off-by: Vladislav Yasevich <[e-mail]>
>> ---
>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>>  include/hw/virtio/virtio-net.h |  2 --
>>  include/net/net.h              |  2 ++
>>  migration/savevm.c             |  6 ++++++
>>  4 files changed, 16 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index c321680..6e9ce4f 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>  }
>>  
>> -static void virtio_net_announce_timer(void *opaque)
>> +static void virtio_net_announce(NetClientState *nc)
>>  {
>> -    VirtIONet *n = opaque;
>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>  
>> -    n->announce_counter--;
>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
>> -    virtio_notify_config(vdev);
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
>> +            virtio_notify_config(vdev);
>> +    }
>>  }
>>  
>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>> @@ -427,8 +429,6 @@ 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;
>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>  
>>      /* Flush any MAC and VLAN filter table state */
>> @@ -868,11 +868,6 @@ 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,
>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>> -                      self_announce_delay(n->announce_counter));
>> -        }
>>          return VIRTIO_NET_OK;
>>      } else {
>>          return VIRTIO_NET_ERR;
>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>>      }
>>  
>> -    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));
>> -    }
>> -
>>      return 0;
>>  }
>>  
>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>>      .receive = virtio_net_receive,
>>      .link_status_changed = virtio_net_set_link_status,
>>      .query_rx_filter = virtio_net_query_rxfilter,
>> +    .announce = virtio_net_announce,
>>  };
>>  
>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
>> @@ -1934,8 +1924,6 @@ 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);
>>  
>>      if (n->netclient_type) {
>>          /*
>> @@ -1997,8 +1985,6 @@ 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);
>>      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..0c597f4 100644
>> --- a/include/hw/virtio/virtio-net.h
>> +++ b/include/hw/virtio/virtio-net.h
>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>>      char *netclient_name;
>>      char *netclient_type;
>>      uint64_t curr_guest_offloads;
>> -    QEMUTimer *announce_timer;
>> -    int announce_counter;
>>      bool needs_vnet_hdr_swap;
>>  } VirtIONet;
>>  
>> diff --git a/include/net/net.h b/include/net/net.h
>> index 99b28d5..598f523 100644
>> --- a/include/net/net.h
>> +++ b/include/net/net.h
>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>>  typedef int (SetVnetBE)(NetClientState *, bool);
>>  typedef struct SocketReadState SocketReadState;
>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
>> +typedef void (NetAnnounce)(NetClientState *);
>>  
>>  typedef struct NetClientInfo {
>>      NetClientDriver type;
>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>>      SetVnetHdrLen *set_vnet_hdr_len;
>>      SetVnetLE *set_vnet_le;
>>      SetVnetBE *set_vnet_be;
>> +    NetAnnounce *announce;
>>  } NetClientInfo;
>>  
>>  struct NetClientState {
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 3b19a4a..5c1d8dc 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>>      int len;
>>  
>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
>> +
>>      len = announce_self_create(buf, nic->conf->macaddr.a);
>>  
>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>> +
>> +    /* if the NIC provides it's own announcement support, use it as well */
>> +    if (nic->ncs->info->announce) {
>> +        nic->ncs->info->announce(nic->ncs);
>> +    }
>>  }
> 
> Combining them doesn't necessarily seem like a bad thing; but
> it does change the timing quite a bit - at the moment the QEMU RARPs
> are sent at the end of migration, while the Virtio ARPs are sent
> when the guest starts running which is quite a bit later.
> 
> In many ways the 'when the guest starts' is better, given that libvirt
> etc has had a chance to reconfigure the networking,  but I'm not that
> sure if it's safe to move the existing one - I had considered *adding*
> another shot of the current mechanism after the guest is started.
> 

Yes, the timing of things have been giving me some issues, but I wanted to post
this patch to get some comments just like this one..

I've wondered why they qemu one happens before the guest starts running.

> I certainly think it's wrong to do the virtio announce at the earlier
> point.
>

I see.

> See also Germano's thread of being able to retrigger the announce
> at arbitrary points, and the series I posted a couple of days ago
> to extend the length/timing of the announce.
> 

That's kind of what prompted me to do try this.  The issue with just
exposing qemu_announce_self() is that RARPS just aren't enough in
some cases (vlans, multicast).  This attempts to add the callback
like Jason mentioned, but then we get timer interactions between the
virtio-net timers and qemu one.

-vlad

> Dave
> 
>> -- 
>> 2.7.4
>>
>>
> --
> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>

Vlad Yasevich March 30, 2017, 1:22 p.m. UTC | #4

On 03/29/2017 04:35 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 29, 2017 at 04:19:50PM -0400, Vladislav Yasevich wrote:
>> virtio announcements seem to run on its own timer with it's own
>> repetition loop and are invoked separately from qemu_announce_self.
>> This patch consolidates announcements into a single location and
>> allows other nics to provide it's own
> 
> their own
> 
>> announcement capabilities, if
>> necessary.
>>
>> This is also usefull
> 
> or useful?
> 
>> in support of exposing qemu_announce_self through
>> qmp/hmp.
> 
> I'm guessing this is the real reason for the change.
> How is it helpful for that? Pls clarify in the commit log.

Ok

> 
>> Signed-off-by: Vladislav Yasevich <[e-mail]>
>> ---
>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>>  include/hw/virtio/virtio-net.h |  2 --
>>  include/net/net.h              |  2 ++
>>  migration/savevm.c             |  6 ++++++
>>  4 files changed, 16 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index c321680..6e9ce4f 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>  }
>>  
>> -static void virtio_net_announce_timer(void *opaque)
>> +static void virtio_net_announce(NetClientState *nc)
>>  {
>> -    VirtIONet *n = opaque;
>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>  
>> -    n->announce_counter--;
>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
>> -    virtio_notify_config(vdev);
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
>> +            virtio_notify_config(vdev);
>> +    }
>>  }
>>  
>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>> @@ -427,8 +429,6 @@ 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;
>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>  
>>      /* Flush any MAC and VLAN filter table state */
>> @@ -868,11 +868,6 @@ 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,
>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>> -                      self_announce_delay(n->announce_counter));
>> -        }
>>          return VIRTIO_NET_OK;
>>      } else {
>>          return VIRTIO_NET_ERR;
> 
> you will notice that this runs at VM speeds with QEMU_CLOCK_VIRTUAL,
> while the self announce you are tying this to runs with
> QEMU_CLOCK_REALTIME.
> 
> So this switch seems wrong to me: by the time VM is started
> on destination all timers might have expired.
> 

That explains what I am seeing now.  Hmm... may be combining them this way
isn't the right approach.  I think I'll leave RARPs alone and export an ability
to start the a nic provided announce feature.

Then Germano's qmp command would be able to call this one if available and the old
one if not.  That way, on user-triggered announcements, we wouldn't need to do the
RARPs, if a better announcement capability is present.

That way current behavior is preserved for migrations.

Thanks
-vlad

> 
>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>>      }
>>  
>> -    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));
>> -    }
>> -
>>      return 0;
>>  }
>>  
>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>>      .receive = virtio_net_receive,
>>      .link_status_changed = virtio_net_set_link_status,
>>      .query_rx_filter = virtio_net_query_rxfilter,
>> +    .announce = virtio_net_announce,
>>  };
>>  
>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
>> @@ -1934,8 +1924,6 @@ 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);
>>  
>>      if (n->netclient_type) {
>>          /*
>> @@ -1997,8 +1985,6 @@ 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);
>>      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..0c597f4 100644
>> --- a/include/hw/virtio/virtio-net.h
>> +++ b/include/hw/virtio/virtio-net.h
>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>>      char *netclient_name;
>>      char *netclient_type;
>>      uint64_t curr_guest_offloads;
>> -    QEMUTimer *announce_timer;
>> -    int announce_counter;
>>      bool needs_vnet_hdr_swap;
>>  } VirtIONet;
>>  
>> diff --git a/include/net/net.h b/include/net/net.h
>> index 99b28d5..598f523 100644
>> --- a/include/net/net.h
>> +++ b/include/net/net.h
>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>>  typedef int (SetVnetBE)(NetClientState *, bool);
>>  typedef struct SocketReadState SocketReadState;
>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
>> +typedef void (NetAnnounce)(NetClientState *);
>>  
>>  typedef struct NetClientInfo {
>>      NetClientDriver type;
>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>>      SetVnetHdrLen *set_vnet_hdr_len;
>>      SetVnetLE *set_vnet_le;
>>      SetVnetBE *set_vnet_be;
>> +    NetAnnounce *announce;
>>  } NetClientInfo;
>>  
>>  struct NetClientState {
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 3b19a4a..5c1d8dc 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>>      int len;
>>  
>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
>> +
>>      len = announce_self_create(buf, nic->conf->macaddr.a);
>>  
>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>> +
>> +    /* if the NIC provides it's own announcement support, use it as well */
>> +    if (nic->ncs->info->announce) {
>> +        nic->ncs->info->announce(nic->ncs);
>> +    }
>>  }
> 
> 
> Can we switch existing qemu announcements to QEMU_CLOCK_VIRTUAL?
> 
> It's an interesting question, and I'm not sure.
> 
> I note that this seems to interact interestingly with recent dgilbert's
> patch sweaking announcement timing. Some coordination might be
> helpful here.
> 
> 
>>  
>> -- 
>> 2.7.4

Dr. David Alan Gilbert March 30, 2017, 2:53 p.m. UTC | #5

* Vlad Yasevich ([e-mail]) wrote:
> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
> > * Vladislav Yasevich ([e-mail]) wrote:
> >> virtio announcements seem to run on its own timer with it's own
> >> repetition loop and are invoked separately from qemu_announce_self.
> >> This patch consolidates announcements into a single location and
> >> allows other nics to provide it's own announcement capabilities, if
> >> necessary.
> >>
> >> This is also usefull in support of exposing qemu_announce_self through
> >> qmp/hmp.
> >>
> >> Signed-off-by: Vladislav Yasevich <[e-mail]>
> >> ---
> >>  hw/net/virtio-net.c            | 30 ++++++++----------------------
> >>  include/hw/virtio/virtio-net.h |  2 --
> >>  include/net/net.h              |  2 ++
> >>  migration/savevm.c             |  6 ++++++
> >>  4 files changed, 16 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index c321680..6e9ce4f 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
> >>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> >>  }
> >>  
> >> -static void virtio_net_announce_timer(void *opaque)
> >> +static void virtio_net_announce(NetClientState *nc)
> >>  {
> >> -    VirtIONet *n = opaque;
> >> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> >>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>  
> >> -    n->announce_counter--;
> >> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> >> -    virtio_notify_config(vdev);
> >> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> >> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> >> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> >> +            virtio_notify_config(vdev);
> >> +    }
> >>  }
> >>  
> >>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >> @@ -427,8 +429,6 @@ 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;
> >>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> >>  
> >>      /* Flush any MAC and VLAN filter table state */
> >> @@ -868,11 +868,6 @@ 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,
> >> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> >> -                      self_announce_delay(n->announce_counter));
> >> -        }
> >>          return VIRTIO_NET_OK;
> >>      } else {
> >>          return VIRTIO_NET_ERR;
> >> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
> >>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
> >>      }
> >>  
> >> -    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));
> >> -    }
> >> -
> >>      return 0;
> >>  }
> >>  
> >> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
> >>      .receive = virtio_net_receive,
> >>      .link_status_changed = virtio_net_set_link_status,
> >>      .query_rx_filter = virtio_net_query_rxfilter,
> >> +    .announce = virtio_net_announce,
> >>  };
> >>  
> >>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> >> @@ -1934,8 +1924,6 @@ 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);
> >>  
> >>      if (n->netclient_type) {
> >>          /*
> >> @@ -1997,8 +1985,6 @@ 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);
> >>      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..0c597f4 100644
> >> --- a/include/hw/virtio/virtio-net.h
> >> +++ b/include/hw/virtio/virtio-net.h
> >> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
> >>      char *netclient_name;
> >>      char *netclient_type;
> >>      uint64_t curr_guest_offloads;
> >> -    QEMUTimer *announce_timer;
> >> -    int announce_counter;
> >>      bool needs_vnet_hdr_swap;
> >>  } VirtIONet;
> >>  
> >> diff --git a/include/net/net.h b/include/net/net.h
> >> index 99b28d5..598f523 100644
> >> --- a/include/net/net.h
> >> +++ b/include/net/net.h
> >> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
> >>  typedef int (SetVnetBE)(NetClientState *, bool);
> >>  typedef struct SocketReadState SocketReadState;
> >>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> >> +typedef void (NetAnnounce)(NetClientState *);
> >>  
> >>  typedef struct NetClientInfo {
> >>      NetClientDriver type;
> >> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
> >>      SetVnetHdrLen *set_vnet_hdr_len;
> >>      SetVnetLE *set_vnet_le;
> >>      SetVnetBE *set_vnet_be;
> >> +    NetAnnounce *announce;
> >>  } NetClientInfo;
> >>  
> >>  struct NetClientState {
> >> diff --git a/migration/savevm.c b/migration/savevm.c
> >> index 3b19a4a..5c1d8dc 100644
> >> --- a/migration/savevm.c
> >> +++ b/migration/savevm.c
> >> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >>      int len;
> >>  
> >>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> >> +
> >>      len = announce_self_create(buf, nic->conf->macaddr.a);
> >>  
> >>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> >> +
> >> +    /* if the NIC provides it's own announcement support, use it as well */
> >> +    if (nic->ncs->info->announce) {
> >> +        nic->ncs->info->announce(nic->ncs);
> >> +    }
> >>  }
> > 
> > Combining them doesn't necessarily seem like a bad thing; but
> > it does change the timing quite a bit - at the moment the QEMU RARPs
> > are sent at the end of migration, while the Virtio ARPs are sent
> > when the guest starts running which is quite a bit later.
> > 
> > In many ways the 'when the guest starts' is better, given that libvirt
> > etc has had a chance to reconfigure the networking,  but I'm not that
> > sure if it's safe to move the existing one - I had considered *adding*
> > another shot of the current mechanism after the guest is started.
> > 
> 
> Yes, the timing of things have been giving me some issues, but I wanted to post
> this patch to get some comments just like this one..
> 
> I've wondered why they qemu one happens before the guest starts running.
> 
> > I certainly think it's wrong to do the virtio announce at the earlier
> > point.
> >
> 
> I see.

The problem with moving it earlier is that it won't really happen.
The guest wont be running at all at the earlier point so it wont consume
your requests to the guest to do the announce.



> > See also Germano's thread of being able to retrigger the announce
> > at arbitrary points, and the series I posted a couple of days ago
> > to extend the length/timing of the announce.
> > 
> 
> That's kind of what prompted me to do try this.  The issue with just
> exposing qemu_announce_self() is that RARPS just aren't enough in
> some cases (vlans, multicast).  This attempts to add the callback
> like Jason mentioned, but then we get timer interactions between the
> virtio-net timers and qemu one.

Yes, and I think it would be a good idea to add the virtio stuff
to germano's announces; however I don't think we can move those
announces earlier in the normal migration case.

Dave

> -vlad
> 
> > Dave
> > 
> >> -- 
> >> 2.7.4
> >>
> >>
> > --
> > Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Vlad Yasevich March 30, 2017, 3:03 p.m. UTC | #6

On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
> * Vlad Yasevich ([e-mail]) wrote:
>> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
>>> * Vladislav Yasevich ([e-mail]) wrote:
>>>> virtio announcements seem to run on its own timer with it's own
>>>> repetition loop and are invoked separately from qemu_announce_self.
>>>> This patch consolidates announcements into a single location and
>>>> allows other nics to provide it's own announcement capabilities, if
>>>> necessary.
>>>>
>>>> This is also usefull in support of exposing qemu_announce_self through
>>>> qmp/hmp.
>>>>
>>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
>>>> ---
>>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>>>>  include/hw/virtio/virtio-net.h |  2 --
>>>>  include/net/net.h              |  2 ++
>>>>  migration/savevm.c             |  6 ++++++
>>>>  4 files changed, 16 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index c321680..6e9ce4f 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>>>  }
>>>>  
>>>> -static void virtio_net_announce_timer(void *opaque)
>>>> +static void virtio_net_announce(NetClientState *nc)
>>>>  {
>>>> -    VirtIONet *n = opaque;
>>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>>  
>>>> -    n->announce_counter--;
>>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
>>>> -    virtio_notify_config(vdev);
>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
>>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
>>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
>>>> +            virtio_notify_config(vdev);
>>>> +    }
>>>>  }
>>>>  
>>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>> @@ -427,8 +429,6 @@ 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;
>>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>>>  
>>>>      /* Flush any MAC and VLAN filter table state */
>>>> @@ -868,11 +868,6 @@ 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,
>>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>>>> -                      self_announce_delay(n->announce_counter));
>>>> -        }
>>>>          return VIRTIO_NET_OK;
>>>>      } else {
>>>>          return VIRTIO_NET_ERR;
>>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>>>>      }
>>>>  
>>>> -    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));
>>>> -    }
>>>> -
>>>>      return 0;
>>>>  }
>>>>  
>>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>>>>      .receive = virtio_net_receive,
>>>>      .link_status_changed = virtio_net_set_link_status,
>>>>      .query_rx_filter = virtio_net_query_rxfilter,
>>>> +    .announce = virtio_net_announce,
>>>>  };
>>>>  
>>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
>>>> @@ -1934,8 +1924,6 @@ 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);
>>>>  
>>>>      if (n->netclient_type) {
>>>>          /*
>>>> @@ -1997,8 +1985,6 @@ 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);
>>>>      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..0c597f4 100644
>>>> --- a/include/hw/virtio/virtio-net.h
>>>> +++ b/include/hw/virtio/virtio-net.h
>>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>>>>      char *netclient_name;
>>>>      char *netclient_type;
>>>>      uint64_t curr_guest_offloads;
>>>> -    QEMUTimer *announce_timer;
>>>> -    int announce_counter;
>>>>      bool needs_vnet_hdr_swap;
>>>>  } VirtIONet;
>>>>  
>>>> diff --git a/include/net/net.h b/include/net/net.h
>>>> index 99b28d5..598f523 100644
>>>> --- a/include/net/net.h
>>>> +++ b/include/net/net.h
>>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>>>>  typedef int (SetVnetBE)(NetClientState *, bool);
>>>>  typedef struct SocketReadState SocketReadState;
>>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
>>>> +typedef void (NetAnnounce)(NetClientState *);
>>>>  
>>>>  typedef struct NetClientInfo {
>>>>      NetClientDriver type;
>>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>>>>      SetVnetHdrLen *set_vnet_hdr_len;
>>>>      SetVnetLE *set_vnet_le;
>>>>      SetVnetBE *set_vnet_be;
>>>> +    NetAnnounce *announce;
>>>>  } NetClientInfo;
>>>>  
>>>>  struct NetClientState {
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index 3b19a4a..5c1d8dc 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>>>>      int len;
>>>>  
>>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
>>>> +
>>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
>>>>  
>>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>>>> +
>>>> +    /* if the NIC provides it's own announcement support, use it as well */
>>>> +    if (nic->ncs->info->announce) {
>>>> +        nic->ncs->info->announce(nic->ncs);
>>>> +    }
>>>>  }
>>>
>>> Combining them doesn't necessarily seem like a bad thing; but
>>> it does change the timing quite a bit - at the moment the QEMU RARPs
>>> are sent at the end of migration, while the Virtio ARPs are sent
>>> when the guest starts running which is quite a bit later.
>>>
>>> In many ways the 'when the guest starts' is better, given that libvirt
>>> etc has had a chance to reconfigure the networking,  but I'm not that
>>> sure if it's safe to move the existing one - I had considered *adding*
>>> another shot of the current mechanism after the guest is started.
>>>
>>
>> Yes, the timing of things have been giving me some issues, but I wanted to post
>> this patch to get some comments just like this one..
>>
>> I've wondered why they qemu one happens before the guest starts running.
>>
>>> I certainly think it's wrong to do the virtio announce at the earlier
>>> point.
>>>
>>
>> I see.
> 
> The problem with moving it earlier is that it won't really happen.
> The guest wont be running at all at the earlier point so it wont consume
> your requests to the guest to do the announce.
> 
> 
> 
>>> See also Germano's thread of being able to retrigger the announce
>>> at arbitrary points, and the series I posted a couple of days ago
>>> to extend the length/timing of the announce.
>>>
>>
>> That's kind of what prompted me to do try this.  The issue with just
>> exposing qemu_announce_self() is that RARPS just aren't enough in
>> some cases (vlans, multicast).  This attempts to add the callback
>> like Jason mentioned, but then we get timer interactions between the
>> virtio-net timers and qemu one.
> 
> Yes, and I think it would be a good idea to add the virtio stuff
> to germano's announces; however I don't think we can move those
> announces earlier in the normal migration case.
> 

So, I am taking a different approach.  I am going to expose device specific
announcement capabilities, and once that's done, the qmp API can directly
tell the device to self-announce if it supports it, or tell qemu to announce
itself in the old-fashioned method.

That seems to provide the most flexibility.  An additional thought is for the qmp
command to optionally specify the number of announcement retries (like your set
allows for migration).

-vlad

> Dave
> 
>> -vlad
>>
>>> Dave
>>>
>>>> -- 
>>>> 2.7.4
>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>

Michael S. Tsirkin March 30, 2017, 3:26 p.m. UTC | #7

On Thu, Mar 30, 2017 at 11:03:40AM -0400, Vlad Yasevich wrote:
> On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
> > * Vlad Yasevich ([e-mail]) wrote:
> >> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
> >>> * Vladislav Yasevich ([e-mail]) wrote:
> >>>> virtio announcements seem to run on its own timer with it's own
> >>>> repetition loop and are invoked separately from qemu_announce_self.
> >>>> This patch consolidates announcements into a single location and
> >>>> allows other nics to provide it's own announcement capabilities, if
> >>>> necessary.
> >>>>
> >>>> This is also usefull in support of exposing qemu_announce_self through
> >>>> qmp/hmp.
> >>>>
> >>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
> >>>> ---
> >>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
> >>>>  include/hw/virtio/virtio-net.h |  2 --
> >>>>  include/net/net.h              |  2 ++
> >>>>  migration/savevm.c             |  6 ++++++
> >>>>  4 files changed, 16 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index c321680..6e9ce4f 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
> >>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> >>>>  }
> >>>>  
> >>>> -static void virtio_net_announce_timer(void *opaque)
> >>>> +static void virtio_net_announce(NetClientState *nc)
> >>>>  {
> >>>> -    VirtIONet *n = opaque;
> >>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> >>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>>>  
> >>>> -    n->announce_counter--;
> >>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> >>>> -    virtio_notify_config(vdev);
> >>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> >>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> >>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> >>>> +            virtio_notify_config(vdev);
> >>>> +    }
> >>>>  }
> >>>>  
> >>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >>>> @@ -427,8 +429,6 @@ 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;
> >>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> >>>>  
> >>>>      /* Flush any MAC and VLAN filter table state */
> >>>> @@ -868,11 +868,6 @@ 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,
> >>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> >>>> -                      self_announce_delay(n->announce_counter));
> >>>> -        }
> >>>>          return VIRTIO_NET_OK;
> >>>>      } else {
> >>>>          return VIRTIO_NET_ERR;
> >>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
> >>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
> >>>>      }
> >>>>  
> >>>> -    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));
> >>>> -    }
> >>>> -
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
> >>>>      .receive = virtio_net_receive,
> >>>>      .link_status_changed = virtio_net_set_link_status,
> >>>>      .query_rx_filter = virtio_net_query_rxfilter,
> >>>> +    .announce = virtio_net_announce,
> >>>>  };
> >>>>  
> >>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> >>>> @@ -1934,8 +1924,6 @@ 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);
> >>>>  
> >>>>      if (n->netclient_type) {
> >>>>          /*
> >>>> @@ -1997,8 +1985,6 @@ 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);
> >>>>      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..0c597f4 100644
> >>>> --- a/include/hw/virtio/virtio-net.h
> >>>> +++ b/include/hw/virtio/virtio-net.h
> >>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
> >>>>      char *netclient_name;
> >>>>      char *netclient_type;
> >>>>      uint64_t curr_guest_offloads;
> >>>> -    QEMUTimer *announce_timer;
> >>>> -    int announce_counter;
> >>>>      bool needs_vnet_hdr_swap;
> >>>>  } VirtIONet;
> >>>>  
> >>>> diff --git a/include/net/net.h b/include/net/net.h
> >>>> index 99b28d5..598f523 100644
> >>>> --- a/include/net/net.h
> >>>> +++ b/include/net/net.h
> >>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
> >>>>  typedef int (SetVnetBE)(NetClientState *, bool);
> >>>>  typedef struct SocketReadState SocketReadState;
> >>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> >>>> +typedef void (NetAnnounce)(NetClientState *);
> >>>>  
> >>>>  typedef struct NetClientInfo {
> >>>>      NetClientDriver type;
> >>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
> >>>>      SetVnetHdrLen *set_vnet_hdr_len;
> >>>>      SetVnetLE *set_vnet_le;
> >>>>      SetVnetBE *set_vnet_be;
> >>>> +    NetAnnounce *announce;
> >>>>  } NetClientInfo;
> >>>>  
> >>>>  struct NetClientState {
> >>>> diff --git a/migration/savevm.c b/migration/savevm.c
> >>>> index 3b19a4a..5c1d8dc 100644
> >>>> --- a/migration/savevm.c
> >>>> +++ b/migration/savevm.c
> >>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >>>>      int len;
> >>>>  
> >>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> >>>> +
> >>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
> >>>>  
> >>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> >>>> +
> >>>> +    /* if the NIC provides it's own announcement support, use it as well */
> >>>> +    if (nic->ncs->info->announce) {
> >>>> +        nic->ncs->info->announce(nic->ncs);
> >>>> +    }
> >>>>  }
> >>>
> >>> Combining them doesn't necessarily seem like a bad thing; but
> >>> it does change the timing quite a bit - at the moment the QEMU RARPs
> >>> are sent at the end of migration, while the Virtio ARPs are sent
> >>> when the guest starts running which is quite a bit later.
> >>>
> >>> In many ways the 'when the guest starts' is better, given that libvirt
> >>> etc has had a chance to reconfigure the networking,  but I'm not that
> >>> sure if it's safe to move the existing one - I had considered *adding*
> >>> another shot of the current mechanism after the guest is started.
> >>>
> >>
> >> Yes, the timing of things have been giving me some issues, but I wanted to post
> >> this patch to get some comments just like this one..
> >>
> >> I've wondered why they qemu one happens before the guest starts running.
> >>
> >>> I certainly think it's wrong to do the virtio announce at the earlier
> >>> point.
> >>>
> >>
> >> I see.
> > 
> > The problem with moving it earlier is that it won't really happen.
> > The guest wont be running at all at the earlier point so it wont consume
> > your requests to the guest to do the announce.
> > 
> > 
> > 
> >>> See also Germano's thread of being able to retrigger the announce
> >>> at arbitrary points, and the series I posted a couple of days ago
> >>> to extend the length/timing of the announce.
> >>>
> >>
> >> That's kind of what prompted me to do try this.  The issue with just
> >> exposing qemu_announce_self() is that RARPS just aren't enough in
> >> some cases (vlans, multicast).  This attempts to add the callback
> >> like Jason mentioned, but then we get timer interactions between the
> >> virtio-net timers and qemu one.
> > 
> > Yes, and I think it would be a good idea to add the virtio stuff
> > to germano's announces; however I don't think we can move those
> > announces earlier in the normal migration case.
> > 
> 
> So, I am taking a different approach.  I am going to expose device specific
> announcement capabilities, and once that's done, the qmp API can directly
> tell the device to self-announce if it supports it, or tell qemu to announce
> itself in the old-fashioned method.
> 
> That seems to provide the most flexibility.  An additional thought is for the qmp
> command to optionally specify the number of announcement retries (like your set
> allows for migration).
> 
> -vlad

It's worth thinking about whether qemu announcements should run
when VM is not running. It certainly exposes a bunch of theoretical
issues e.g. if you try to run the VM on another host at the same time.

Are there advantages in doing it like this?

> > Dave
> > 
> >> -vlad
> >>
> >>> Dave
> >>>
> >>>> -- 
> >>>> 2.7.4
> >>>>
> >>>>
> >>> --
> >>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> >

Dr. David Alan Gilbert March 30, 2017, 3:26 p.m. UTC | #8

* Vlad Yasevich ([e-mail]) wrote:
> On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
> > * Vlad Yasevich ([e-mail]) wrote:
> >> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
> >>> * Vladislav Yasevich ([e-mail]) wrote:
> >>>> virtio announcements seem to run on its own timer with it's own
> >>>> repetition loop and are invoked separately from qemu_announce_self.
> >>>> This patch consolidates announcements into a single location and
> >>>> allows other nics to provide it's own announcement capabilities, if
> >>>> necessary.
> >>>>
> >>>> This is also usefull in support of exposing qemu_announce_self through
> >>>> qmp/hmp.
> >>>>
> >>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
> >>>> ---
> >>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
> >>>>  include/hw/virtio/virtio-net.h |  2 --
> >>>>  include/net/net.h              |  2 ++
> >>>>  migration/savevm.c             |  6 ++++++
> >>>>  4 files changed, 16 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index c321680..6e9ce4f 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
> >>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> >>>>  }
> >>>>  
> >>>> -static void virtio_net_announce_timer(void *opaque)
> >>>> +static void virtio_net_announce(NetClientState *nc)
> >>>>  {
> >>>> -    VirtIONet *n = opaque;
> >>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> >>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>>>  
> >>>> -    n->announce_counter--;
> >>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> >>>> -    virtio_notify_config(vdev);
> >>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> >>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> >>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> >>>> +            virtio_notify_config(vdev);
> >>>> +    }
> >>>>  }
> >>>>  
> >>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >>>> @@ -427,8 +429,6 @@ 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;
> >>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> >>>>  
> >>>>      /* Flush any MAC and VLAN filter table state */
> >>>> @@ -868,11 +868,6 @@ 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,
> >>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> >>>> -                      self_announce_delay(n->announce_counter));
> >>>> -        }
> >>>>          return VIRTIO_NET_OK;
> >>>>      } else {
> >>>>          return VIRTIO_NET_ERR;
> >>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
> >>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
> >>>>      }
> >>>>  
> >>>> -    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));
> >>>> -    }
> >>>> -
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
> >>>>      .receive = virtio_net_receive,
> >>>>      .link_status_changed = virtio_net_set_link_status,
> >>>>      .query_rx_filter = virtio_net_query_rxfilter,
> >>>> +    .announce = virtio_net_announce,
> >>>>  };
> >>>>  
> >>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> >>>> @@ -1934,8 +1924,6 @@ 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);
> >>>>  
> >>>>      if (n->netclient_type) {
> >>>>          /*
> >>>> @@ -1997,8 +1985,6 @@ 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);
> >>>>      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..0c597f4 100644
> >>>> --- a/include/hw/virtio/virtio-net.h
> >>>> +++ b/include/hw/virtio/virtio-net.h
> >>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
> >>>>      char *netclient_name;
> >>>>      char *netclient_type;
> >>>>      uint64_t curr_guest_offloads;
> >>>> -    QEMUTimer *announce_timer;
> >>>> -    int announce_counter;
> >>>>      bool needs_vnet_hdr_swap;
> >>>>  } VirtIONet;
> >>>>  
> >>>> diff --git a/include/net/net.h b/include/net/net.h
> >>>> index 99b28d5..598f523 100644
> >>>> --- a/include/net/net.h
> >>>> +++ b/include/net/net.h
> >>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
> >>>>  typedef int (SetVnetBE)(NetClientState *, bool);
> >>>>  typedef struct SocketReadState SocketReadState;
> >>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> >>>> +typedef void (NetAnnounce)(NetClientState *);
> >>>>  
> >>>>  typedef struct NetClientInfo {
> >>>>      NetClientDriver type;
> >>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
> >>>>      SetVnetHdrLen *set_vnet_hdr_len;
> >>>>      SetVnetLE *set_vnet_le;
> >>>>      SetVnetBE *set_vnet_be;
> >>>> +    NetAnnounce *announce;
> >>>>  } NetClientInfo;
> >>>>  
> >>>>  struct NetClientState {
> >>>> diff --git a/migration/savevm.c b/migration/savevm.c
> >>>> index 3b19a4a..5c1d8dc 100644
> >>>> --- a/migration/savevm.c
> >>>> +++ b/migration/savevm.c
> >>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >>>>      int len;
> >>>>  
> >>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> >>>> +
> >>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
> >>>>  
> >>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> >>>> +
> >>>> +    /* if the NIC provides it's own announcement support, use it as well */
> >>>> +    if (nic->ncs->info->announce) {
> >>>> +        nic->ncs->info->announce(nic->ncs);
> >>>> +    }
> >>>>  }
> >>>
> >>> Combining them doesn't necessarily seem like a bad thing; but
> >>> it does change the timing quite a bit - at the moment the QEMU RARPs
> >>> are sent at the end of migration, while the Virtio ARPs are sent
> >>> when the guest starts running which is quite a bit later.
> >>>
> >>> In many ways the 'when the guest starts' is better, given that libvirt
> >>> etc has had a chance to reconfigure the networking,  but I'm not that
> >>> sure if it's safe to move the existing one - I had considered *adding*
> >>> another shot of the current mechanism after the guest is started.
> >>>
> >>
> >> Yes, the timing of things have been giving me some issues, but I wanted to post
> >> this patch to get some comments just like this one..
> >>
> >> I've wondered why they qemu one happens before the guest starts running.
> >>
> >>> I certainly think it's wrong to do the virtio announce at the earlier
> >>> point.
> >>>
> >>
> >> I see.
> > 
> > The problem with moving it earlier is that it won't really happen.
> > The guest wont be running at all at the earlier point so it wont consume
> > your requests to the guest to do the announce.
> > 
> > 
> > 
> >>> See also Germano's thread of being able to retrigger the announce
> >>> at arbitrary points, and the series I posted a couple of days ago
> >>> to extend the length/timing of the announce.
> >>>
> >>
> >> That's kind of what prompted me to do try this.  The issue with just
> >> exposing qemu_announce_self() is that RARPS just aren't enough in
> >> some cases (vlans, multicast).  This attempts to add the callback
> >> like Jason mentioned, but then we get timer interactions between the
> >> virtio-net timers and qemu one.
> > 
> > Yes, and I think it would be a good idea to add the virtio stuff
> > to germano's announces; however I don't think we can move those
> > announces earlier in the normal migration case.
> > 
> 
> So, I am taking a different approach.  I am going to expose device specific
> announcement capabilities, and once that's done, the qmp API can directly
> tell the device to self-announce if it supports it, or tell qemu to announce
> itself in the old-fashioned method.
> 
> That seems to provide the most flexibility.  An additional thought is for the qmp
> command to optionally specify the number of announcement retries (like your set
> allows for migration).

Hmm, we could do without yet another solution!

How about:
  a) Take Germano's patch
  b) My patch
  c) Something close to this patch you've already posted
  d) And we add a call to qemu_announce_self at the point we start the
     CPUs somewhere.

Germano's makes (d) safe even if it's overlapping QEMU's RARPs.
I don't think there's any harm in this patches code making it trigger
during the initial RARP stage as long as we also do (d).

Dave



> 
> -vlad
> 
> > Dave
> > 
> >> -vlad
> >>
> >>> Dave
> >>>
> >>>> -- 
> >>>> 2.7.4
> >>>>
> >>>>
> >>> --
> >>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Dr. David Alan Gilbert March 30, 2017, 3:29 p.m. UTC | #9

* Michael S. Tsirkin ([e-mail]) wrote:
> On Thu, Mar 30, 2017 at 11:03:40AM -0400, Vlad Yasevich wrote:
> > On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
> > > * Vlad Yasevich ([e-mail]) wrote:
> > >> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
> > >>> * Vladislav Yasevich ([e-mail]) wrote:
> > >>>> virtio announcements seem to run on its own timer with it's own
> > >>>> repetition loop and are invoked separately from qemu_announce_self.
> > >>>> This patch consolidates announcements into a single location and
> > >>>> allows other nics to provide it's own announcement capabilities, if
> > >>>> necessary.
> > >>>>
> > >>>> This is also usefull in support of exposing qemu_announce_self through
> > >>>> qmp/hmp.
> > >>>>
> > >>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
> > >>>> ---
> > >>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
> > >>>>  include/hw/virtio/virtio-net.h |  2 --
> > >>>>  include/net/net.h              |  2 ++
> > >>>>  migration/savevm.c             |  6 ++++++
> > >>>>  4 files changed, 16 insertions(+), 24 deletions(-)
> > >>>>
> > >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >>>> index c321680..6e9ce4f 100644
> > >>>> --- a/hw/net/virtio-net.c
> > >>>> +++ b/hw/net/virtio-net.c
> > >>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
> > >>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> > >>>>  }
> > >>>>  
> > >>>> -static void virtio_net_announce_timer(void *opaque)
> > >>>> +static void virtio_net_announce(NetClientState *nc)
> > >>>>  {
> > >>>> -    VirtIONet *n = opaque;
> > >>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> > >>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > >>>>  
> > >>>> -    n->announce_counter--;
> > >>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> > >>>> -    virtio_notify_config(vdev);
> > >>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> > >>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> > >>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> > >>>> +            virtio_notify_config(vdev);
> > >>>> +    }
> > >>>>  }
> > >>>>  
> > >>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> > >>>> @@ -427,8 +429,6 @@ 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;
> > >>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> > >>>>  
> > >>>>      /* Flush any MAC and VLAN filter table state */
> > >>>> @@ -868,11 +868,6 @@ 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,
> > >>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> > >>>> -                      self_announce_delay(n->announce_counter));
> > >>>> -        }
> > >>>>          return VIRTIO_NET_OK;
> > >>>>      } else {
> > >>>>          return VIRTIO_NET_ERR;
> > >>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
> > >>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
> > >>>>      }
> > >>>>  
> > >>>> -    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));
> > >>>> -    }
> > >>>> -
> > >>>>      return 0;
> > >>>>  }
> > >>>>  
> > >>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
> > >>>>      .receive = virtio_net_receive,
> > >>>>      .link_status_changed = virtio_net_set_link_status,
> > >>>>      .query_rx_filter = virtio_net_query_rxfilter,
> > >>>> +    .announce = virtio_net_announce,
> > >>>>  };
> > >>>>  
> > >>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> > >>>> @@ -1934,8 +1924,6 @@ 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);
> > >>>>  
> > >>>>      if (n->netclient_type) {
> > >>>>          /*
> > >>>> @@ -1997,8 +1985,6 @@ 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);
> > >>>>      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..0c597f4 100644
> > >>>> --- a/include/hw/virtio/virtio-net.h
> > >>>> +++ b/include/hw/virtio/virtio-net.h
> > >>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
> > >>>>      char *netclient_name;
> > >>>>      char *netclient_type;
> > >>>>      uint64_t curr_guest_offloads;
> > >>>> -    QEMUTimer *announce_timer;
> > >>>> -    int announce_counter;
> > >>>>      bool needs_vnet_hdr_swap;
> > >>>>  } VirtIONet;
> > >>>>  
> > >>>> diff --git a/include/net/net.h b/include/net/net.h
> > >>>> index 99b28d5..598f523 100644
> > >>>> --- a/include/net/net.h
> > >>>> +++ b/include/net/net.h
> > >>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
> > >>>>  typedef int (SetVnetBE)(NetClientState *, bool);
> > >>>>  typedef struct SocketReadState SocketReadState;
> > >>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> > >>>> +typedef void (NetAnnounce)(NetClientState *);
> > >>>>  
> > >>>>  typedef struct NetClientInfo {
> > >>>>      NetClientDriver type;
> > >>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
> > >>>>      SetVnetHdrLen *set_vnet_hdr_len;
> > >>>>      SetVnetLE *set_vnet_le;
> > >>>>      SetVnetBE *set_vnet_be;
> > >>>> +    NetAnnounce *announce;
> > >>>>  } NetClientInfo;
> > >>>>  
> > >>>>  struct NetClientState {
> > >>>> diff --git a/migration/savevm.c b/migration/savevm.c
> > >>>> index 3b19a4a..5c1d8dc 100644
> > >>>> --- a/migration/savevm.c
> > >>>> +++ b/migration/savevm.c
> > >>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
> > >>>>      int len;
> > >>>>  
> > >>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> > >>>> +
> > >>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
> > >>>>  
> > >>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> > >>>> +
> > >>>> +    /* if the NIC provides it's own announcement support, use it as well */
> > >>>> +    if (nic->ncs->info->announce) {
> > >>>> +        nic->ncs->info->announce(nic->ncs);
> > >>>> +    }
> > >>>>  }
> > >>>
> > >>> Combining them doesn't necessarily seem like a bad thing; but
> > >>> it does change the timing quite a bit - at the moment the QEMU RARPs
> > >>> are sent at the end of migration, while the Virtio ARPs are sent
> > >>> when the guest starts running which is quite a bit later.
> > >>>
> > >>> In many ways the 'when the guest starts' is better, given that libvirt
> > >>> etc has had a chance to reconfigure the networking,  but I'm not that
> > >>> sure if it's safe to move the existing one - I had considered *adding*
> > >>> another shot of the current mechanism after the guest is started.
> > >>>
> > >>
> > >> Yes, the timing of things have been giving me some issues, but I wanted to post
> > >> this patch to get some comments just like this one..
> > >>
> > >> I've wondered why they qemu one happens before the guest starts running.
> > >>
> > >>> I certainly think it's wrong to do the virtio announce at the earlier
> > >>> point.
> > >>>
> > >>
> > >> I see.
> > > 
> > > The problem with moving it earlier is that it won't really happen.
> > > The guest wont be running at all at the earlier point so it wont consume
> > > your requests to the guest to do the announce.
> > > 
> > > 
> > > 
> > >>> See also Germano's thread of being able to retrigger the announce
> > >>> at arbitrary points, and the series I posted a couple of days ago
> > >>> to extend the length/timing of the announce.
> > >>>
> > >>
> > >> That's kind of what prompted me to do try this.  The issue with just
> > >> exposing qemu_announce_self() is that RARPS just aren't enough in
> > >> some cases (vlans, multicast).  This attempts to add the callback
> > >> like Jason mentioned, but then we get timer interactions between the
> > >> virtio-net timers and qemu one.
> > > 
> > > Yes, and I think it would be a good idea to add the virtio stuff
> > > to germano's announces; however I don't think we can move those
> > > announces earlier in the normal migration case.
> > > 
> > 
> > So, I am taking a different approach.  I am going to expose device specific
> > announcement capabilities, and once that's done, the qmp API can directly
> > tell the device to self-announce if it supports it, or tell qemu to announce
> > itself in the old-fashioned method.
> > 
> > That seems to provide the most flexibility.  An additional thought is for the qmp
> > command to optionally specify the number of announcement retries (like your set
> > allows for migration).
> > 
> > -vlad
> 
> It's worth thinking about whether qemu announcements should run
> when VM is not running. It certainly exposes a bunch of theoretical
> issues e.g. if you try to run the VM on another host at the same time.
> 
> Are there advantages in doing it like this?

QEMUs interface with the rest of the world for this is pretty fuzzy (aka wrong)
I think the mechanism where they're sent just as the CPUs start is pretty good
but will that lose some packets if they arrive at the network at the same point,
I don't know.

You also get interaction with cancelling migration etc as well.

Dave

> > > Dave
> > > 
> > >> -vlad
> > >>
> > >>> Dave
> > >>>
> > >>>> -- 
> > >>>> 2.7.4
> > >>>>
> > >>>>
> > >>> --
> > >>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> > >>>
> > >>
> > > --
> > > Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> > > 
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Michael S. Tsirkin March 30, 2017, 3:38 p.m. UTC | #10

On Thu, Mar 30, 2017 at 04:26:40PM +0100, Dr. David Alan Gilbert wrote:
> How about:
>   a) Take Germano's patch
>   b) My patch
>   c) Something close to this patch you've already posted
>   d) And we add a call to qemu_announce_self at the point we start the
>      CPUs somewhere.

It would be nice to see it all in one place, at this
point I'm not sure I understand how it all interacts.

Vlad Yasevich March 30, 2017, 3:41 p.m. UTC | #11

On 03/30/2017 11:26 AM, Dr. David Alan Gilbert wrote:
> * Vlad Yasevich ([e-mail]) wrote:
>> On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
>>> * Vlad Yasevich ([e-mail]) wrote:
>>>> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
>>>>> * Vladislav Yasevich ([e-mail]) wrote:
>>>>>> virtio announcements seem to run on its own timer with it's own
>>>>>> repetition loop and are invoked separately from qemu_announce_self.
>>>>>> This patch consolidates announcements into a single location and
>>>>>> allows other nics to provide it's own announcement capabilities, if
>>>>>> necessary.
>>>>>>
>>>>>> This is also usefull in support of exposing qemu_announce_self through
>>>>>> qmp/hmp.
>>>>>>
>>>>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
>>>>>> ---
>>>>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>>>>>>  include/hw/virtio/virtio-net.h |  2 --
>>>>>>  include/net/net.h              |  2 ++
>>>>>>  migration/savevm.c             |  6 ++++++
>>>>>>  4 files changed, 16 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index c321680..6e9ce4f 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>>>>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>>>>>  }
>>>>>>  
>>>>>> -static void virtio_net_announce_timer(void *opaque)
>>>>>> +static void virtio_net_announce(NetClientState *nc)
>>>>>>  {
>>>>>> -    VirtIONet *n = opaque;
>>>>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>>>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>>>>  
>>>>>> -    n->announce_counter--;
>>>>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
>>>>>> -    virtio_notify_config(vdev);
>>>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
>>>>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
>>>>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
>>>>>> +            virtio_notify_config(vdev);
>>>>>> +    }
>>>>>>  }
>>>>>>  
>>>>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>>> @@ -427,8 +429,6 @@ 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;
>>>>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>>>>>  
>>>>>>      /* Flush any MAC and VLAN filter table state */
>>>>>> @@ -868,11 +868,6 @@ 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,
>>>>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>>>>>> -                      self_announce_delay(n->announce_counter));
>>>>>> -        }
>>>>>>          return VIRTIO_NET_OK;
>>>>>>      } else {
>>>>>>          return VIRTIO_NET_ERR;
>>>>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>>>>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>>>>>>      }
>>>>>>  
>>>>>> -    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));
>>>>>> -    }
>>>>>> -
>>>>>>      return 0;
>>>>>>  }
>>>>>>  
>>>>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>>>>>>      .receive = virtio_net_receive,
>>>>>>      .link_status_changed = virtio_net_set_link_status,
>>>>>>      .query_rx_filter = virtio_net_query_rxfilter,
>>>>>> +    .announce = virtio_net_announce,
>>>>>>  };
>>>>>>  
>>>>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
>>>>>> @@ -1934,8 +1924,6 @@ 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);
>>>>>>  
>>>>>>      if (n->netclient_type) {
>>>>>>          /*
>>>>>> @@ -1997,8 +1985,6 @@ 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);
>>>>>>      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..0c597f4 100644
>>>>>> --- a/include/hw/virtio/virtio-net.h
>>>>>> +++ b/include/hw/virtio/virtio-net.h
>>>>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>>>>>>      char *netclient_name;
>>>>>>      char *netclient_type;
>>>>>>      uint64_t curr_guest_offloads;
>>>>>> -    QEMUTimer *announce_timer;
>>>>>> -    int announce_counter;
>>>>>>      bool needs_vnet_hdr_swap;
>>>>>>  } VirtIONet;
>>>>>>  
>>>>>> diff --git a/include/net/net.h b/include/net/net.h
>>>>>> index 99b28d5..598f523 100644
>>>>>> --- a/include/net/net.h
>>>>>> +++ b/include/net/net.h
>>>>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>>>>>>  typedef int (SetVnetBE)(NetClientState *, bool);
>>>>>>  typedef struct SocketReadState SocketReadState;
>>>>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
>>>>>> +typedef void (NetAnnounce)(NetClientState *);
>>>>>>  
>>>>>>  typedef struct NetClientInfo {
>>>>>>      NetClientDriver type;
>>>>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>>>>>>      SetVnetHdrLen *set_vnet_hdr_len;
>>>>>>      SetVnetLE *set_vnet_le;
>>>>>>      SetVnetBE *set_vnet_be;
>>>>>> +    NetAnnounce *announce;
>>>>>>  } NetClientInfo;
>>>>>>  
>>>>>>  struct NetClientState {
>>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>>> index 3b19a4a..5c1d8dc 100644
>>>>>> --- a/migration/savevm.c
>>>>>> +++ b/migration/savevm.c
>>>>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>>>>>>      int len;
>>>>>>  
>>>>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
>>>>>> +
>>>>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
>>>>>>  
>>>>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>>>>>> +
>>>>>> +    /* if the NIC provides it's own announcement support, use it as well */
>>>>>> +    if (nic->ncs->info->announce) {
>>>>>> +        nic->ncs->info->announce(nic->ncs);
>>>>>> +    }
>>>>>>  }
>>>>>
>>>>> Combining them doesn't necessarily seem like a bad thing; but
>>>>> it does change the timing quite a bit - at the moment the QEMU RARPs
>>>>> are sent at the end of migration, while the Virtio ARPs are sent
>>>>> when the guest starts running which is quite a bit later.
>>>>>
>>>>> In many ways the 'when the guest starts' is better, given that libvirt
>>>>> etc has had a chance to reconfigure the networking,  but I'm not that
>>>>> sure if it's safe to move the existing one - I had considered *adding*
>>>>> another shot of the current mechanism after the guest is started.
>>>>>
>>>>
>>>> Yes, the timing of things have been giving me some issues, but I wanted to post
>>>> this patch to get some comments just like this one..
>>>>
>>>> I've wondered why they qemu one happens before the guest starts running.
>>>>
>>>>> I certainly think it's wrong to do the virtio announce at the earlier
>>>>> point.
>>>>>
>>>>
>>>> I see.
>>>
>>> The problem with moving it earlier is that it won't really happen.
>>> The guest wont be running at all at the earlier point so it wont consume
>>> your requests to the guest to do the announce.
>>>
>>>
>>>
>>>>> See also Germano's thread of being able to retrigger the announce
>>>>> at arbitrary points, and the series I posted a couple of days ago
>>>>> to extend the length/timing of the announce.
>>>>>
>>>>
>>>> That's kind of what prompted me to do try this.  The issue with just
>>>> exposing qemu_announce_self() is that RARPS just aren't enough in
>>>> some cases (vlans, multicast).  This attempts to add the callback
>>>> like Jason mentioned, but then we get timer interactions between the
>>>> virtio-net timers and qemu one.
>>>
>>> Yes, and I think it would be a good idea to add the virtio stuff
>>> to germano's announces; however I don't think we can move those
>>> announces earlier in the normal migration case.
>>>
>>
>> So, I am taking a different approach.  I am going to expose device specific
>> announcement capabilities, and once that's done, the qmp API can directly
>> tell the device to self-announce if it supports it, or tell qemu to announce
>> itself in the old-fashioned method.
>>
>> That seems to provide the most flexibility.  An additional thought is for the qmp
>> command to optionally specify the number of announcement retries (like your set
>> allows for migration).
> 
> Hmm, we could do without yet another solution!
> 
> How about:
>   a) Take Germano's patch
>   b) My patch

Swap the the 2 and add optional parameters to Germano's patch to specify announcement
options.  Irrespective of migration, those can be used by management layer to specify
at least the number of attempts.

>   c) Something close to this patch you've already posted
>   d) And we add a call to qemu_announce_self at the point we start the
>      CPUs somewhere.
> 
> Germano's makes (d) safe even if it's overlapping QEMU's RARPs.
> I don't think there's any harm in this patches code making it trigger
> during the initial RARP stage as long as we also do (d).

Having it all in one place is nice, but what about the different timer types?  Would
allowing for different timer types help?

-vlad

> 
> Dave
> 
> 
> 
>>
>> -vlad
>>
>>> Dave
>>>
>>>> -vlad
>>>>
>>>>> Dave
>>>>>
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>
>>>>>>
>>>>> --
>>>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>

Dr. David Alan Gilbert May 2, 2017, 10:41 a.m. UTC | #12

* Vlad Yasevich ([e-mail]) wrote:
> On 03/30/2017 11:26 AM, Dr. David Alan Gilbert wrote:
> > * Vlad Yasevich ([e-mail]) wrote:
> >> On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
> >>> * Vlad Yasevich ([e-mail]) wrote:
> >>>> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
> >>>>> * Vladislav Yasevich ([e-mail]) wrote:
> >>>>>> virtio announcements seem to run on its own timer with it's own
> >>>>>> repetition loop and are invoked separately from qemu_announce_self.
> >>>>>> This patch consolidates announcements into a single location and
> >>>>>> allows other nics to provide it's own announcement capabilities, if
> >>>>>> necessary.
> >>>>>>
> >>>>>> This is also usefull in support of exposing qemu_announce_self through
> >>>>>> qmp/hmp.
> >>>>>>
> >>>>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
> >>>>>> ---
> >>>>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
> >>>>>>  include/hw/virtio/virtio-net.h |  2 --
> >>>>>>  include/net/net.h              |  2 ++
> >>>>>>  migration/savevm.c             |  6 ++++++
> >>>>>>  4 files changed, 16 insertions(+), 24 deletions(-)
> >>>>>>
> >>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>> index c321680..6e9ce4f 100644
> >>>>>> --- a/hw/net/virtio-net.c
> >>>>>> +++ b/hw/net/virtio-net.c
> >>>>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
> >>>>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> >>>>>>  }
> >>>>>>  
> >>>>>> -static void virtio_net_announce_timer(void *opaque)
> >>>>>> +static void virtio_net_announce(NetClientState *nc)
> >>>>>>  {
> >>>>>> -    VirtIONet *n = opaque;
> >>>>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
> >>>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>>>>>  
> >>>>>> -    n->announce_counter--;
> >>>>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
> >>>>>> -    virtio_notify_config(vdev);
> >>>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> >>>>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> >>>>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
> >>>>>> +            virtio_notify_config(vdev);
> >>>>>> +    }
> >>>>>>  }
> >>>>>>  
> >>>>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >>>>>> @@ -427,8 +429,6 @@ 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;
> >>>>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> >>>>>>  
> >>>>>>      /* Flush any MAC and VLAN filter table state */
> >>>>>> @@ -868,11 +868,6 @@ 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,
> >>>>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> >>>>>> -                      self_announce_delay(n->announce_counter));
> >>>>>> -        }
> >>>>>>          return VIRTIO_NET_OK;
> >>>>>>      } else {
> >>>>>>          return VIRTIO_NET_ERR;
> >>>>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
> >>>>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
> >>>>>>      }
> >>>>>>  
> >>>>>> -    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));
> >>>>>> -    }
> >>>>>> -
> >>>>>>      return 0;
> >>>>>>  }
> >>>>>>  
> >>>>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
> >>>>>>      .receive = virtio_net_receive,
> >>>>>>      .link_status_changed = virtio_net_set_link_status,
> >>>>>>      .query_rx_filter = virtio_net_query_rxfilter,
> >>>>>> +    .announce = virtio_net_announce,
> >>>>>>  };
> >>>>>>  
> >>>>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
> >>>>>> @@ -1934,8 +1924,6 @@ 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);
> >>>>>>  
> >>>>>>      if (n->netclient_type) {
> >>>>>>          /*
> >>>>>> @@ -1997,8 +1985,6 @@ 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);
> >>>>>>      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..0c597f4 100644
> >>>>>> --- a/include/hw/virtio/virtio-net.h
> >>>>>> +++ b/include/hw/virtio/virtio-net.h
> >>>>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
> >>>>>>      char *netclient_name;
> >>>>>>      char *netclient_type;
> >>>>>>      uint64_t curr_guest_offloads;
> >>>>>> -    QEMUTimer *announce_timer;
> >>>>>> -    int announce_counter;
> >>>>>>      bool needs_vnet_hdr_swap;
> >>>>>>  } VirtIONet;
> >>>>>>  
> >>>>>> diff --git a/include/net/net.h b/include/net/net.h
> >>>>>> index 99b28d5..598f523 100644
> >>>>>> --- a/include/net/net.h
> >>>>>> +++ b/include/net/net.h
> >>>>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
> >>>>>>  typedef int (SetVnetBE)(NetClientState *, bool);
> >>>>>>  typedef struct SocketReadState SocketReadState;
> >>>>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> >>>>>> +typedef void (NetAnnounce)(NetClientState *);
> >>>>>>  
> >>>>>>  typedef struct NetClientInfo {
> >>>>>>      NetClientDriver type;
> >>>>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
> >>>>>>      SetVnetHdrLen *set_vnet_hdr_len;
> >>>>>>      SetVnetLE *set_vnet_le;
> >>>>>>      SetVnetBE *set_vnet_be;
> >>>>>> +    NetAnnounce *announce;
> >>>>>>  } NetClientInfo;
> >>>>>>  
> >>>>>>  struct NetClientState {
> >>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
> >>>>>> index 3b19a4a..5c1d8dc 100644
> >>>>>> --- a/migration/savevm.c
> >>>>>> +++ b/migration/savevm.c
> >>>>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
> >>>>>>      int len;
> >>>>>>  
> >>>>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> >>>>>> +
> >>>>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
> >>>>>>  
> >>>>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> >>>>>> +
> >>>>>> +    /* if the NIC provides it's own announcement support, use it as well */
> >>>>>> +    if (nic->ncs->info->announce) {
> >>>>>> +        nic->ncs->info->announce(nic->ncs);
> >>>>>> +    }
> >>>>>>  }
> >>>>>
> >>>>> Combining them doesn't necessarily seem like a bad thing; but
> >>>>> it does change the timing quite a bit - at the moment the QEMU RARPs
> >>>>> are sent at the end of migration, while the Virtio ARPs are sent
> >>>>> when the guest starts running which is quite a bit later.
> >>>>>
> >>>>> In many ways the 'when the guest starts' is better, given that libvirt
> >>>>> etc has had a chance to reconfigure the networking,  but I'm not that
> >>>>> sure if it's safe to move the existing one - I had considered *adding*
> >>>>> another shot of the current mechanism after the guest is started.
> >>>>>
> >>>>
> >>>> Yes, the timing of things have been giving me some issues, but I wanted to post
> >>>> this patch to get some comments just like this one..
> >>>>
> >>>> I've wondered why they qemu one happens before the guest starts running.
> >>>>
> >>>>> I certainly think it's wrong to do the virtio announce at the earlier
> >>>>> point.
> >>>>>
> >>>>
> >>>> I see.
> >>>
> >>> The problem with moving it earlier is that it won't really happen.
> >>> The guest wont be running at all at the earlier point so it wont consume
> >>> your requests to the guest to do the announce.
> >>>
> >>>
> >>>
> >>>>> See also Germano's thread of being able to retrigger the announce
> >>>>> at arbitrary points, and the series I posted a couple of days ago
> >>>>> to extend the length/timing of the announce.
> >>>>>
> >>>>
> >>>> That's kind of what prompted me to do try this.  The issue with just
> >>>> exposing qemu_announce_self() is that RARPS just aren't enough in
> >>>> some cases (vlans, multicast).  This attempts to add the callback
> >>>> like Jason mentioned, but then we get timer interactions between the
> >>>> virtio-net timers and qemu one.
> >>>
> >>> Yes, and I think it would be a good idea to add the virtio stuff
> >>> to germano's announces; however I don't think we can move those
> >>> announces earlier in the normal migration case.
> >>>
> >>
> >> So, I am taking a different approach.  I am going to expose device specific
> >> announcement capabilities, and once that's done, the qmp API can directly
> >> tell the device to self-announce if it supports it, or tell qemu to announce
> >> itself in the old-fashioned method.
> >>
> >> That seems to provide the most flexibility.  An additional thought is for the qmp
> >> command to optionally specify the number of announcement retries (like your set
> >> allows for migration).
> > 
> > Hmm, we could do without yet another solution!
> > 
> > How about:
> >   a) Take Germano's patch
> >   b) My patch
> 
> Swap the the 2 and add optional parameters to Germano's patch to specify announcement
> options.  Irrespective of migration, those can be used by management layer to specify
> at least the number of attempts.

That would need to have the migration settings as well then to change what migration
does?

> >   c) Something close to this patch you've already posted
> >   d) And we add a call to qemu_announce_self at the point we start the
> >      CPUs somewhere.
> > 
> > Germano's makes (d) safe even if it's overlapping QEMU's RARPs.
> > I don't think there's any harm in this patches code making it trigger
> > during the initial RARP stage as long as we also do (d).
> 
> Having it all in one place is nice, but what about the different timer types?  Would
> allowing for different timer types help?

I guess you could share it and make it switchable.

We should get this pile of different patches rolling again; I keep getting prodded
by cases which need to work around broken networking during migration because
Openstack hasn't reconfigured the networking by the time we send the announces.

Dave

> -vlad
> 
> > 
> > Dave
> > 
> > 
> > 
> >>
> >> -vlad
> >>
> >>> Dave
> >>>
> >>>> -vlad
> >>>>
> >>>>> Dave
> >>>>>
> >>>>>> -- 
> >>>>>> 2.7.4
> >>>>>>
> >>>>>>
> >>>>> --
> >>>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> >>>>>
> >>>>
> >>> --
> >>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / [e-mail] / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Vlad Yasevich May 5, 2017, 8:06 p.m. UTC | #13

On 05/02/2017 06:41 AM, Dr. David Alan Gilbert wrote:
> * Vlad Yasevich ([e-mail]) wrote:
>> On 03/30/2017 11:26 AM, Dr. David Alan Gilbert wrote:
>>> * Vlad Yasevich ([e-mail]) wrote:
>>>> On 03/30/2017 10:53 AM, Dr. David Alan Gilbert wrote:
>>>>> * Vlad Yasevich ([e-mail]) wrote:
>>>>>> On 03/30/2017 04:24 AM, Dr. David Alan Gilbert wrote:
>>>>>>> * Vladislav Yasevich ([e-mail]) wrote:
>>>>>>>> virtio announcements seem to run on its own timer with it's own
>>>>>>>> repetition loop and are invoked separately from qemu_announce_self.
>>>>>>>> This patch consolidates announcements into a single location and
>>>>>>>> allows other nics to provide it's own announcement capabilities, if
>>>>>>>> necessary.
>>>>>>>>
>>>>>>>> This is also usefull in support of exposing qemu_announce_self through
>>>>>>>> qmp/hmp.
>>>>>>>>
>>>>>>>> Signed-off-by: Vladislav Yasevich <[e-mail]>
>>>>>>>> ---
>>>>>>>>  hw/net/virtio-net.c            | 30 ++++++++----------------------
>>>>>>>>  include/hw/virtio/virtio-net.h |  2 --
>>>>>>>>  include/net/net.h              |  2 ++
>>>>>>>>  migration/savevm.c             |  6 ++++++
>>>>>>>>  4 files changed, 16 insertions(+), 24 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>>> index c321680..6e9ce4f 100644
>>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>>> @@ -110,14 +110,16 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
>>>>>>>>          (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> -static void virtio_net_announce_timer(void *opaque)
>>>>>>>> +static void virtio_net_announce(NetClientState *nc)
>>>>>>>>  {
>>>>>>>> -    VirtIONet *n = opaque;
>>>>>>>> +    VirtIONet *n = qemu_get_nic_opaque(nc);
>>>>>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>>>>>>  
>>>>>>>> -    n->announce_counter--;
>>>>>>>> -    n->status |= VIRTIO_NET_S_ANNOUNCE;
>>>>>>>> -    virtio_notify_config(vdev);
>>>>>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
>>>>>>>> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
>>>>>>>> +            n->status |= VIRTIO_NET_S_ANNOUNCE;
>>>>>>>> +            virtio_notify_config(vdev);
>>>>>>>> +    }
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>>>>> @@ -427,8 +429,6 @@ 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;
>>>>>>>>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>>>>>>>  
>>>>>>>>      /* Flush any MAC and VLAN filter table state */
>>>>>>>> @@ -868,11 +868,6 @@ 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,
>>>>>>>> -                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>>>>>>>> -                      self_announce_delay(n->announce_counter));
>>>>>>>> -        }
>>>>>>>>          return VIRTIO_NET_OK;
>>>>>>>>      } else {
>>>>>>>>          return VIRTIO_NET_ERR;
>>>>>>>> @@ -1609,12 +1604,6 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>>>>>>>>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>>>>>>>>      }
>>>>>>>>  
>>>>>>>> -    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));
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>>      return 0;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> @@ -1829,6 +1818,7 @@ static NetClientInfo net_virtio_info = {
>>>>>>>>      .receive = virtio_net_receive,
>>>>>>>>      .link_status_changed = virtio_net_set_link_status,
>>>>>>>>      .query_rx_filter = virtio_net_query_rxfilter,
>>>>>>>> +    .announce = virtio_net_announce,
>>>>>>>>  };
>>>>>>>>  
>>>>>>>>  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
>>>>>>>> @@ -1934,8 +1924,6 @@ 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);
>>>>>>>>  
>>>>>>>>      if (n->netclient_type) {
>>>>>>>>          /*
>>>>>>>> @@ -1997,8 +1985,6 @@ 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);
>>>>>>>>      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..0c597f4 100644
>>>>>>>> --- a/include/hw/virtio/virtio-net.h
>>>>>>>> +++ b/include/hw/virtio/virtio-net.h
>>>>>>>> @@ -94,8 +94,6 @@ typedef struct VirtIONet {
>>>>>>>>      char *netclient_name;
>>>>>>>>      char *netclient_type;
>>>>>>>>      uint64_t curr_guest_offloads;
>>>>>>>> -    QEMUTimer *announce_timer;
>>>>>>>> -    int announce_counter;
>>>>>>>>      bool needs_vnet_hdr_swap;
>>>>>>>>  } VirtIONet;
>>>>>>>>  
>>>>>>>> diff --git a/include/net/net.h b/include/net/net.h
>>>>>>>> index 99b28d5..598f523 100644
>>>>>>>> --- a/include/net/net.h
>>>>>>>> +++ b/include/net/net.h
>>>>>>>> @@ -64,6 +64,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
>>>>>>>>  typedef int (SetVnetBE)(NetClientState *, bool);
>>>>>>>>  typedef struct SocketReadState SocketReadState;
>>>>>>>>  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
>>>>>>>> +typedef void (NetAnnounce)(NetClientState *);
>>>>>>>>  
>>>>>>>>  typedef struct NetClientInfo {
>>>>>>>>      NetClientDriver type;
>>>>>>>> @@ -84,6 +85,7 @@ typedef struct NetClientInfo {
>>>>>>>>      SetVnetHdrLen *set_vnet_hdr_len;
>>>>>>>>      SetVnetLE *set_vnet_le;
>>>>>>>>      SetVnetBE *set_vnet_be;
>>>>>>>> +    NetAnnounce *announce;
>>>>>>>>  } NetClientInfo;
>>>>>>>>  
>>>>>>>>  struct NetClientState {
>>>>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>>>>> index 3b19a4a..5c1d8dc 100644
>>>>>>>> --- a/migration/savevm.c
>>>>>>>> +++ b/migration/savevm.c
>>>>>>>> @@ -113,9 +113,15 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>>>>>>>>      int len;
>>>>>>>>  
>>>>>>>>      trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
>>>>>>>> +
>>>>>>>>      len = announce_self_create(buf, nic->conf->macaddr.a);
>>>>>>>>  
>>>>>>>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>>>>>>>> +
>>>>>>>> +    /* if the NIC provides it's own announcement support, use it as well */
>>>>>>>> +    if (nic->ncs->info->announce) {
>>>>>>>> +        nic->ncs->info->announce(nic->ncs);
>>>>>>>> +    }
>>>>>>>>  }
>>>>>>>
>>>>>>> Combining them doesn't necessarily seem like a bad thing; but
>>>>>>> it does change the timing quite a bit - at the moment the QEMU RARPs
>>>>>>> are sent at the end of migration, while the Virtio ARPs are sent
>>>>>>> when the guest starts running which is quite a bit later.
>>>>>>>
>>>>>>> In many ways the 'when the guest starts' is better, given that libvirt
>>>>>>> etc has had a chance to reconfigure the networking,  but I'm not that
>>>>>>> sure if it's safe to move the existing one - I had considered *adding*
>>>>>>> another shot of the current mechanism after the guest is started.
>>>>>>>
>>>>>>
>>>>>> Yes, the timing of things have been giving me some issues, but I wanted to post
>>>>>> this patch to get some comments just like this one..
>>>>>>
>>>>>> I've wondered why they qemu one happens before the guest starts running.
>>>>>>
>>>>>>> I certainly think it's wrong to do the virtio announce at the earlier
>>>>>>> point.
>>>>>>>
>>>>>>
>>>>>> I see.
>>>>>
>>>>> The problem with moving it earlier is that it won't really happen.
>>>>> The guest wont be running at all at the earlier point so it wont consume
>>>>> your requests to the guest to do the announce.
>>>>>
>>>>>
>>>>>
>>>>>>> See also Germano's thread of being able to retrigger the announce
>>>>>>> at arbitrary points, and the series I posted a couple of days ago
>>>>>>> to extend the length/timing of the announce.
>>>>>>>
>>>>>>
>>>>>> That's kind of what prompted me to do try this.  The issue with just
>>>>>> exposing qemu_announce_self() is that RARPS just aren't enough in
>>>>>> some cases (vlans, multicast).  This attempts to add the callback
>>>>>> like Jason mentioned, but then we get timer interactions between the
>>>>>> virtio-net timers and qemu one.
>>>>>
>>>>> Yes, and I think it would be a good idea to add the virtio stuff
>>>>> to germano's announces; however I don't think we can move those
>>>>> announces earlier in the normal migration case.
>>>>>
>>>>
>>>> So, I am taking a different approach.  I am going to expose device specific
>>>> announcement capabilities, and once that's done, the qmp API can directly
>>>> tell the device to self-announce if it supports it, or tell qemu to announce
>>>> itself in the old-fashioned method.
>>>>
>>>> That seems to provide the most flexibility.  An additional thought is for the qmp
>>>> command to optionally specify the number of announcement retries (like your set
>>>> allows for migration).
>>>
>>> Hmm, we could do without yet another solution!
>>>
>>> How about:
>>>   a) Take Germano's patch
>>>   b) My patch
>>
>> Swap the the 2 and add optional parameters to Germano's patch to specify announcement
>> options.  Irrespective of migration, those can be used by management layer to specify
>> at least the number of attempts.
> 
> That would need to have the migration settings as well then to change what migration
> does?

Hmm...

Since we want to expose announcements, it would make sense to have configurable timeouts
and separate them from migration specific things.

May be when we create announcement timers we can pass the parameters to them?

If we take Germano's patch first, I guess we could add this with your series

> 
>>>   c) Something close to this patch you've already posted
>>>   d) And we add a call to qemu_announce_self at the point we start the
>>>      CPUs somewhere.
>>>
>>> Germano's makes (d) safe even if it's overlapping QEMU's RARPs.
>>> I don't think there's any harm in this patches code making it trigger
>>> during the initial RARP stage as long as we also do (d).
>>
>> Having it all in one place is nice, but what about the different timer types?  Would
>> allowing for different timer types help?
> 
> I guess you could share it and make it switchable.
> 
> We should get this pile of different patches rolling again; I keep getting prodded
> by cases which need to work around broken networking during migration because
> Openstack hasn't reconfigured the networking by the time we send the announces.

Ok.  I am updating my set now.

-vlad

> 
> Dave
> 
>> -vlad
>>
>>>
>>> Dave
>>>
>>>
>>>
>>>>
>>>> -vlad
>>>>
>>>>> Dave
>>>>>
>>>>>> -vlad
>>>>>>
>>>>>>> Dave
>>>>>>>
>>>>>>>> -- 
>>>>>>>> 2.7.4
>>>>>>>>
>>>>>>>>
>>>>>>> --
>>>>>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>>>>>>>
>>>>>>
>>>>> --
>>>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / [e-mail] / Manchester, UK
>

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c321680..6e9ce4f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -110,14 +110,16 @@  static bool virtio_net_started(VirtIONet *n, uint8_t status)
         (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
 }
 
-static void virtio_net_announce_timer(void *opaque)
+static void virtio_net_announce(NetClientState *nc)
 {
-    VirtIONet *n = opaque;
+    VirtIONet *n = qemu_get_nic_opaque(nc);
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->announce_counter--;
-    n->status |= VIRTIO_NET_S_ANNOUNCE;
-    virtio_notify_config(vdev);
+    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
+        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
+            n->status |= VIRTIO_NET_S_ANNOUNCE;
+            virtio_notify_config(vdev);
+    }
 }
 
 static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
@@ -427,8 +429,6 @@  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;
     n->status &= ~VIRTIO_NET_S_ANNOUNCE;
 
     /* Flush any MAC and VLAN filter table state */
@@ -868,11 +868,6 @@  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,
-                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
-                      self_announce_delay(n->announce_counter));
-        }
         return VIRTIO_NET_OK;
     } else {
         return VIRTIO_NET_ERR;
@@ -1609,12 +1604,6 @@  static int virtio_net_post_load_device(void *opaque, int version_id)
         qemu_get_subqueue(n->nic, i)->link_down = link_down;
     }
 
-    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));
-    }
-
     return 0;
 }
 
@@ -1829,6 +1818,7 @@  static NetClientInfo net_virtio_info = {
     .receive = virtio_net_receive,
     .link_status_changed = virtio_net_set_link_status,
     .query_rx_filter = virtio_net_query_rxfilter,
+    .announce = virtio_net_announce,
 };
 
 static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
@@ -1934,8 +1924,6 @@  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);
 
     if (n->netclient_type) {
         /*
@@ -1997,8 +1985,6 @@  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);
     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..0c597f4 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -94,8 +94,6 @@  typedef struct VirtIONet {
     char *netclient_name;
     char *netclient_type;
     uint64_t curr_guest_offloads;
-    QEMUTimer *announce_timer;
-    int announce_counter;
     bool needs_vnet_hdr_swap;
 } VirtIONet;
 
diff --git a/include/net/net.h b/include/net/net.h
index 99b28d5..598f523 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -64,6 +64,7 @@  typedef int (SetVnetLE)(NetClientState *, bool);
 typedef int (SetVnetBE)(NetClientState *, bool);
 typedef struct SocketReadState SocketReadState;
 typedef void (SocketReadStateFinalize)(SocketReadState *rs);
+typedef void (NetAnnounce)(NetClientState *);
 
 typedef struct NetClientInfo {
     NetClientDriver type;
@@ -84,6 +85,7 @@  typedef struct NetClientInfo {
     SetVnetHdrLen *set_vnet_hdr_len;
     SetVnetLE *set_vnet_le;
     SetVnetBE *set_vnet_be;
+    NetAnnounce *announce;
 } NetClientInfo;
 
 struct NetClientState {
diff --git a/migration/savevm.c b/migration/savevm.c
index 3b19a4a..5c1d8dc 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -113,9 +113,15 @@  static void qemu_announce_self_iter(NICState *nic, void *opaque)
     int len;
 
     trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
+
     len = announce_self_create(buf, nic->conf->macaddr.a);
 
     qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
+
+    /* if the NIC provides it's own announcement support, use it as well */
+    if (nic->ncs->info->announce) {
+        nic->ncs->info->announce(nic->ncs);
+    }
 }