On Thu, Nov 10, 2016 at 01:56:05AM +0200, Yuri Benditovich wrote:
>
>
> On Wed, Nov 9, 2016 at 10:28 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
>
> On Wed, Nov 09, 2016 at 05:22:02PM +0200, yuri.benditov...@daynix.com
> wrote:
> > From: Yuri Benditovich <yuri.benditov...@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.benditov...@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.
>
>
> Please note that this code works only when link is down.
> For me this was too complicated to write simpler procedure
> with the same result.
Yes - it's somewhat problematic and risky that we process
the ring in qemu, but I don't see an easy way around that.
But at least let's limit the processing and assumptions we
make.
>
>
> > @@ -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
... we likely have some ...
> tx queue
> > + * and disabled notification */
what does this refer to?
> > + 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?
>
>
> This code fixes following problem in original code (example):
> We are in vhost=off and receive kick ->virtio_net_handle_tx_timer
> -> tx_waiting=1, notification disabled, timer set
> Now we receive link loss, cancel the timer and stay with packets in the queue
> and with
> disabled notification. Nobody will return them. (easy to reproduce with timer
> set to 5ms)
>
> Added code drops packets we already have and ensure we will report them
> as completed to guest. If guest keeps sending packets, they will be dropped
> in virtio_net_handle_tx_timer and in virtio_net_handle_tx_bh (in procedures
> just below)
> as we already with link down.
Yes I get that. I'm just not 100% sure all paths have
us listen on the ioeventfd and handle kicks without races -
this was previously assumed not to matter.
>
>
> > @@ -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
>
>
