Re: [Qemu-devel] [PATCH] virtio-net: announce self by guest

["Michael S. Tsirkin" <mst@redhat.com>]

Thanks, looks good.
Some minor comments below,

On Thu, May 15, 2014 at 03:16:47PM +0800, Jason Wang wrote:
> It's hard to track all mac addresses and their configurations (e.g
> vlan or ipv6) in qemu. Without those informations, it's impossible to

s/those informations/this information/

> build proper garp packet after migration. The only possible solution
> to this is let guest (who knew all configurations) to do this.

s/knew/knows/

> 
> So, this patch introduces a new readonly config status bit of virtio-net,
> VIRTIO_NET_S_ANNOUNCE which is used to notify guest to announce
> presence of its link through config update interrupt.When guest has
> done the announcement, it should ack the notification through
> VIRTIO_NET_CTRL_ANNOUNCE_ACK cmd. This feature is negotiated by a new
> feature bit VIRTIO_NET_F_ANNOUNCE (which has already been supported by
> Linux guest).
> 
> During load, a counter of announcing rounds were set so that the after

s/were/is/
s/the after/after/

> the vm is running it can trigger rounds of config interrupts to notify
> the guest to build and send the correct garps.
> 
> Tested with ping to guest with vlan during migration. Without the
> patch, lots of the packets were lost after migration. With the patch,
> could not notice packet loss after migration.

below changelog should go after ---, until the ack list.

> 
> Reference:
> RFC v2: https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg01750.html
> RFC v1: https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02648.html
> V7:     https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01127.html
> 
> Changes from RFC v2:
> - use QEMU_CLOCK_VIRTUAL instead of QEMU_CLOCK_REALTIME
> - compat self announce for 2.0 machine type
> 
> Changes from RFC v1:
> - clean VIRTIO_NET_S_ANNOUNCE bit during reset
> - free announce timer during clean
> - make announce work for non-vhost case
> 
> Changes from V7:
> - Instead of introducing a global method for each kind of nic, this
>   version limits the changes to virtio-net itself.
> 
> Cc: Liuyongan <liuyongan@huawei.com>
> Cc: Amos Kong <akong@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c            |   48 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h           |    5 ++++
>  include/hw/virtio/virtio-net.h |   16 +++++++++++++
>  3 files changed, 69 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 940a7cf..98d59e9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -25,6 +25,7 @@
>  #include "monitor/monitor.h"
>  
>  #define VIRTIO_NET_VM_VERSION    11
> +#define VIRTIO_NET_ANNOUNCE_ROUNDS    3
>  
>  #define MAC_TABLE_ENTRIES    64
>  #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */

I would make it  5 to be consistent with SELF_ANNOUNCE_ROUNDS
in savevm.c

in fact, let's move SELF_ANNOUNCE_ROUNDS to include/migration/vmstate.h
and reuse it.

> @@ -99,6 +100,25 @@ 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)
> +{
> +    VirtIONet *n = opaque;
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +
> +    if (n->announce &&

I would make it > 0 here, just in case it becomes negative as a result
of some bug.

> +        virtio_net_started(n, vdev->status) &&
> +        vdev->guest_features & (0x1 << VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> +        vdev->guest_features & (0x1 << VIRTIO_NET_F_CTRL_VQ)) {
> +
> +        n->announce--;
> +        n->status |= VIRTIO_NET_S_ANNOUNCE;
> +        virtio_notify_config(vdev);
> +    } else {
> +        timer_del(n->announce_timer);

why do this here?

> +        n->announce = 0;

why is this here?

> +    }
> +}
> +
>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> @@ -147,6 +167,8 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  
>      virtio_net_vhost_status(n, status);
>  
> +    virtio_net_announce_timer(n);
> +

why do this here? why not right after we set announce counter?

>      for (i = 0; i < n->max_queues; i++) {
>          q = &n->vqs[i];
>  
> @@ -322,6 +344,9 @@ 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 = 0;
> +    n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>  
>      /* Flush any MAC and VLAN filter table state */
>      n->mac_table.in_use = 0;
> @@ -731,6 +756,22 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>      return VIRTIO_NET_OK;
>  }
>  
> +static int virtio_net_handle_announce(VirtIONet *n, uint8_t cmd,
> +                                      struct iovec *iov, unsigned int iov_cnt)
> +{
> +    if (cmd == VIRTIO_NET_CTRL_ANNOUNCE_ACK) {
> +        n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> +        if (n->announce) {

I would make it > 0 here, just in case it becomes negative as a result
of some bug.

> +            timer_mod(n->announce_timer,
> +                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 50 +
> +                      (VIRTIO_NET_ANNOUNCE_ROUNDS - n->announce - 1) * 100);

savevm.c has this code, and a comment:
        /* delay 50ms, 150ms, 250ms, ... */
	50 + (SELF_ANNOUNCE_ROUNDS - count - 1) * 100

how about an API in include/migration/vmstate.h

static inline
int64_t self_announce_delay(int round)
{
	assert(round < SELF_ANNOUNCE_ROUNDS && round > 0);
        /* delay 50ms, 150ms, 250ms, ... */
	return 50 + (SELF_ANNOUNCE_ROUNDS - round - 1) * 100;
}

or something to this end.


> +        }
> +        return VIRTIO_NET_OK;
> +    } else {
> +        return VIRTIO_NET_ERR;
> +    }
> +}
> +
>  static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
>                                  struct iovec *iov, unsigned int iov_cnt)
>  {
> @@ -794,6 +835,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>              status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
>          } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
>              status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, iov_cnt);
> +        } else if (ctrl.class == VIRTIO_NET_CTRL_ANNOUNCE) {
> +            status = virtio_net_handle_announce(n, ctrl.cmd, iov, iov_cnt);
>          } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
>              status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt);
>          } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
> @@ -1451,6 +1494,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>      }
>  
> +    n->announce = VIRTIO_NET_ANNOUNCE_ROUNDS;

Well if virtio_net_handle_announce runs now it will start timer
in the past, right?
This seems wrong.

>      return 0;
>  }
>  
> @@ -1562,6 +1606,8 @@ 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_ns(QEMU_CLOCK_VIRTUAL,
> +                                     virtio_net_announce_timer, n);
>  
>      if (n->netclient_type) {
>          /*
> @@ -1642,6 +1688,8 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> +    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/i386/pc.h b/include/hw/i386/pc.h
> index 32a7687..f93b427 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -271,6 +271,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>              .driver   = "apic",\
>              .property = "version",\
>              .value    = stringify(0x11),\
> +        },\
> +        {\
> +            .driver   = "virtio-net-pci",\
> +            .property = "guest_announce",\
> +            .value    = "off",\
>          }
>  
>  #define PC_COMPAT_1_7 \
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 4b32440..ca1a9c5 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -49,12 +49,14 @@
>  #define VIRTIO_NET_F_CTRL_RX    18      /* Control channel RX mode support */
>  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
>  #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
> +#define VIRTIO_NET_F_GUEST_ANNOUNCE 21  /* Guest can announce itself */
>  #define VIRTIO_NET_F_MQ         22      /* Device supports Receive Flow
>                                           * Steering */
>  
>  #define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
>  
>  #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
> +#define VIRTIO_NET_S_ANNOUNCE   2       /* Announcement is needed */
>  
>  #define TX_TIMER_INTERVAL 150000 /* 150 us */
>  
> @@ -193,6 +195,8 @@ typedef struct VirtIONet {
>      char *netclient_name;
>      char *netclient_type;
>      uint64_t curr_guest_offloads;
> +    QEMUTimer *announce_timer;
> +    int announce;


rename announce_counter pls.

>  } VirtIONet;
>  
>  #define VIRTIO_NET_CTRL_MAC    1
> @@ -213,6 +217,17 @@ typedef struct VirtIONet {
>   #define VIRTIO_NET_CTRL_VLAN_DEL             1
>  
>  /*
> + * Control link announce acknowledgement
> + *

bunch of typos and vagueness below

> + * The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate that
> + * driver has recevied the notification and device would clear the
> + * VIRTIO_NET_S_ANNOUNCE bit in the status filed after it received
> + * this command.

fixed version:

VIRTIO_NET_S_ANNOUNCE bit in the status field requests link
announcement from guest driver. The driver is notified by config space change
interrupt.  The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate
that the driver has received the notification. It makes the device clear the
bit VIRTIO_NET_S_ANNOUNCE in the status field.


> + */
> +#define VIRTIO_NET_CTRL_ANNOUNCE       3
> + #define VIRTIO_NET_CTRL_ANNOUNCE_ACK         0
> +
> +/*
>   * Control Multiqueue
>   *
>   * The command VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET
> @@ -251,6 +266,7 @@ struct virtio_net_ctrl_mq {
>          DEFINE_PROP_BIT("guest_tso6", _state, _field, VIRTIO_NET_F_GUEST_TSO6, true), \
>          DEFINE_PROP_BIT("guest_ecn", _state, _field, VIRTIO_NET_F_GUEST_ECN, true), \
>          DEFINE_PROP_BIT("guest_ufo", _state, _field, VIRTIO_NET_F_GUEST_UFO, true), \
> +        DEFINE_PROP_BIT("guest_announce", _state, _field, VIRTIO_NET_F_GUEST_ANNOUNCE, true), \
>          DEFINE_PROP_BIT("host_tso4", _state, _field, VIRTIO_NET_F_HOST_TSO4, true), \
>          DEFINE_PROP_BIT("host_tso6", _state, _field, VIRTIO_NET_F_HOST_TSO6, true), \
>          DEFINE_PROP_BIT("host_ecn", _state, _field, VIRTIO_NET_F_HOST_ECN, true), \
> -- 
> 1.7.1