Re: [Qemu-devel] [PATCH v2 3/3] net: virtio-net discards TX data after link down

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

On Wed, Nov 09, 2016 at 05:22:02PM +0200, yuri.benditovich@daynix.com wrote:
> From: Yuri Benditovich <yuri.benditovich@daynix.com>
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1295637
> Upon set_link monitor command or upon netdev deletion
> virtio-net sends link down indication to the guest
> and stops vhost if one is used.
> Guest driver can still submit data for TX until it
> recognizes link loss. If these packets not returned by
> the host, the Windows guest will never be able to finish
> disable/removal/shutdown.
> Now each packet sent by guest after NIC indicated link
> down will be completed immediately.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  hw/net/virtio-net.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 06bfe4b..ab4e18a 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -218,6 +218,16 @@ static void virtio_net_vnet_endian_status(VirtIONet *n, uint8_t status)
>      }
>  }
>  
> +static void virtio_net_drop_tx_queue_data(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtQueueElement *elem;
> +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
> +        virtqueue_push(vq, elem, 0);
> +        virtio_notify(vdev, vq);
> +        g_free(elem);
> +    }
> +}
> +
>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);

I don't like this part. This does too much queue parsing,
I would like to just copy head from avail to used ring.

For example, people want to support rings >1K in size.
Let's add bool virtqueue_drop(vq) and be done with it.


> @@ -262,6 +272,14 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>              } else {
>                  qemu_bh_cancel(q->tx_bh);
>              }
> +            if ((n->status & VIRTIO_NET_S_LINK_UP) == 0 &&
> +                (queue_status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +                /* if tx is waiting we are likely have some packets in tx queue
> +                 * and disabled notification */
> +                q->tx_waiting = 0;
> +                virtio_queue_set_notification(q->tx_vq, 1);
> +                virtio_net_drop_tx_queue_data(vdev, q->tx_vq);
> +            }
>          }
>      }
>  }

OK but what if guest keeps sending packets? What will drop them?


> @@ -1319,6 +1337,11 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      VirtIONetQueue *q = &n->vqs[vq2q(virtio_get_queue_index(vq))];
>  
> +    if (unlikely((n->status & VIRTIO_NET_S_LINK_UP) == 0)) {
> +        virtio_net_drop_tx_queue_data(vdev, vq);
> +        return;
> +    }
> +
>      /* This happens when device was stopped but VCPU wasn't. */
>      if (!vdev->vm_running) {
>          q->tx_waiting = 1;
> @@ -1345,6 +1368,11 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      VirtIONetQueue *q = &n->vqs[vq2q(virtio_get_queue_index(vq))];
>  
> +    if (unlikely((n->status & VIRTIO_NET_S_LINK_UP) == 0)) {
> +        virtio_net_drop_tx_queue_data(vdev, vq);
> +        return;
> +    }
> +
>      if (unlikely(q->tx_waiting)) {
>          return;
>      }
> -- 
> 1.9.1