[Qemu-devel] [PATCH 0/5] Fix packet queueing to allow full tap queue drain

[Qemu-devel] [PATCH 0/5] Fix packet queueing to allow full tap queue drain

[Mark McLoughlin]

Hey,
        Scott points out that the current behaviour of draining the
entire queue in tap_send() doesn't work with non-virtio NICs because
we drop a packet when the NIC queue fills up.

        This series of patches first disables this behaviour, fixes
various aspects of the queueing logic and then re-enables the behaviour
in a way that works with all NICs.

        The first patch should also be applied to stable-0.11, I'll
follow up with a backport.

        Scott, Sven, if you could test the tap-drain-queue branch from
my tree, that would be great:

  http://repo.or.cz/w/qemu/markmc.git

Thanks,
Mark.

[Qemu-devel] [PATCH 1/5] tap: disable draining queue in one go

[Mark McLoughlin]

If qemu_send_packet_async() returns zero, it means the packet has been
queued and the sent callback will be invoked once it has been flushed.

This is only possible where the NIC's receive() handler returns zero
and promises to notify the networking core that room is available in its
queue again.

In the case where the receive handler does not have this capability
(and its queue fills up) it returns -1 and the networking core does not
queue up the packet. This condition is indicated by a -1 return from
qemu_send_packet_async().

Currently, tap handles this condition simply by dropping the packet. It
should do its best to avoid getting into this situation by checking such
NIC's have room for a packet before copying the packet from the tap
interface.

tap_send() used to achieve this by only reading a single packet before
returning to the mainloop. That way, tap_can_send() is called before
reading each packet.

tap_send() was changed to completely drain the tap interface queue
without taking into account the situation where the NIC returns an
error and the packet is not queued. Let's start fixing this by
reverting to the previous behaviour of reading one packet at a time.

Reported-by: Scott Tsai <scottt.tw@gmail.com>
Tested-by: Sven Rudolph <Sven_Rudolph@drewag.de>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net/tap.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 60354e4..f32226b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -187,26 +187,23 @@ static void tap_send_completed(VLANClientState *vc, ssize_t len)
 static void tap_send(void *opaque)
 {
     TAPState *s = opaque;
+    uint8_t *buf = s->buf;
     int size;
 
-    do {
-        uint8_t *buf = s->buf;
-
-        size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
-        if (size <= 0) {
-            break;
-        }
+    size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
+    if (size <= 0) {
+        return;
+    }
 
-        if (s->has_vnet_hdr && !s->using_vnet_hdr) {
-            buf  += sizeof(struct virtio_net_hdr);
-            size -= sizeof(struct virtio_net_hdr);
-        }
+    if (s->has_vnet_hdr && !s->using_vnet_hdr) {
+        buf  += sizeof(struct virtio_net_hdr);
+        size -= sizeof(struct virtio_net_hdr);
+    }
 
-        size = qemu_send_packet_async(s->vc, buf, size, tap_send_completed);
-        if (size == 0) {
-            tap_read_poll(s, 0);
-        }
-    } while (size > 0);
+    size = qemu_send_packet_async(s->vc, buf, size, tap_send_completed);
+    if (size == 0) {
+        tap_read_poll(s, 0);
+    }
 }
 
 int tap_has_ufo(VLANClientState *vc)
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] net: disable receiving if client returns zero

[Mark McLoughlin]

If a receiver returns zero, that means its queue is full and it will
notify us when room is available using qemu_flush_queued_packets().

Take note of that and disable that receiver until it flushes its queue.

This is a first step towards allowing can_receive() handlers to return
true even if no buffer space is available.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
 net.h |    1 +
 2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/net.c b/net.c
index 661bbc1..9dea615 100644
--- a/net.c
+++ b/net.c
@@ -423,11 +423,13 @@ int qemu_can_send_packet(VLANClientState *sender)
     VLANClientState *vc;
 
     if (sender->peer) {
-        if (!sender->peer->can_receive ||
-            sender->peer->can_receive(sender->peer)) {
-            return 1;
-        } else {
+        if (sender->peer->receive_disabled) {
             return 0;
+        } else if (sender->peer->can_receive &&
+                   !sender->peer->can_receive(sender->peer)) {
+            return 0;
+        } else {
+            return 1;
         }
     }
 
@@ -455,15 +457,27 @@ static ssize_t qemu_deliver_packet(VLANClientState *sender,
                                    void *opaque)
 {
     VLANClientState *vc = opaque;
+    ssize_t ret;
 
     if (vc->link_down) {
         return size;
     }
 
-    if (flags & QEMU_NET_PACKET_FLAG_RAW && vc->receive_raw)
-        return vc->receive_raw(vc, data, size);
-    else
-        return vc->receive(vc, data, size);
+    if (vc->receive_disabled) {
+        return 0;
+    }
+
+    if (flags & QEMU_NET_PACKET_FLAG_RAW && vc->receive_raw) {
+        ret = vc->receive_raw(vc, data, size);
+    } else {
+        ret = vc->receive(vc, data, size);
+    }
+
+    if (ret == 0) {
+        vc->receive_disabled = 1;
+    };
+
+    return ret;
 }
 
 static ssize_t qemu_vlan_deliver_packet(VLANClientState *sender,
@@ -474,7 +488,7 @@ static ssize_t qemu_vlan_deliver_packet(VLANClientState *sender,
 {
     VLANState *vlan = opaque;
     VLANClientState *vc;
-    int ret = -1;
+    ssize_t ret = -1;
 
     QTAILQ_FOREACH(vc, &vlan->clients, next) {
         ssize_t len;
@@ -488,12 +502,23 @@ static ssize_t qemu_vlan_deliver_packet(VLANClientState *sender,
             continue;
         }
 
-        if (flags & QEMU_NET_PACKET_FLAG_RAW && vc->receive_raw)
+        if (vc->receive_disabled) {
+            ret = 0;
+            continue;
+        }
+
+        if (flags & QEMU_NET_PACKET_FLAG_RAW && vc->receive_raw) {
             len = vc->receive_raw(vc, buf, size);
-        else
+        } else {
             len = vc->receive(vc, buf, size);
+        }
+
+        if (len == 0) {
+            vc->receive_disabled = 1;
+        }
 
         ret = (ret >= 0) ? ret : len;
+
     }
 
     return ret;
@@ -520,6 +545,8 @@ void qemu_flush_queued_packets(VLANClientState *vc)
 {
     NetQueue *queue;
 
+    vc->receive_disabled = 0;
+
     if (vc->vlan) {
         queue = vc->vlan->send_queue;
     } else {
diff --git a/net.h b/net.h
index 8074c66..c1fe515 100644
--- a/net.h
+++ b/net.h
@@ -44,6 +44,7 @@ struct VLANClientState {
     char *model;
     char *name;
     char info_str[256];
+    unsigned receive_disabled : 1;
 };
 
 struct VLANState {
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] net/queue: queue packets even if sender doesn't supply a callback

[Mark McLoughlin]

Now that we disable any receiver whose queue is full, we do not require
senders to handle a zero return by supplying a sent callback.

This is a second step towards allowing can_receive() handlers to return
true even if no buffer space is available.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net/queue.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/queue.c b/net/queue.c
index e91a9a5..2ea6cd0 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -186,7 +186,7 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
     }
 
     ret = qemu_net_queue_deliver(queue, sender, flags, data, size);
-    if (ret == 0 && sent_cb != NULL) {
+    if (ret == 0) {
         qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
         return 0;
     }
@@ -210,7 +210,7 @@ ssize_t qemu_net_queue_send_iov(NetQueue *queue,
     }
 
     ret = qemu_net_queue_deliver_iov(queue, sender, flags, iov, iovcnt);
-    if (ret == 0 && sent_cb != NULL) {
+    if (ret == 0) {
         qemu_net_queue_append_iov(queue, sender, flags, iov, iovcnt, sent_cb);
         return 0;
     }
@@ -246,7 +246,7 @@ void qemu_net_queue_flush(NetQueue *queue)
                                      packet->flags,
                                      packet->data,
                                      packet->size);
-        if (ret == 0 && packet->sent_cb != NULL) {
+        if (ret == 0) {
             QTAILQ_INSERT_HEAD(&queue->packets, packet, entry);
             break;
         }
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] virtio-net: split the has_buffers() logic from can_receive()

[Mark McLoughlin]

We should only return zero from receive() for a condition which we'll
get notification of when it changes. Currently, we're returning zero
if the guest driver is not ready, but we won't ever flush our queue
when that status changes.

Also, don't check buffer space in can_receive(), but instead just allow
receive() to return zero when this condition occurs and have the caller
handle queueing the packet.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/virtio-net.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 7ee393b..a30aff4 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -367,12 +367,19 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
     qemu_notify_event();
 }
 
-static int do_virtio_net_can_receive(VirtIONet *n, int bufsize)
+static int virtio_net_can_receive(VLANClientState *vc)
 {
+    VirtIONet *n = vc->opaque;
+
     if (!virtio_queue_ready(n->rx_vq) ||
         !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
         return 0;
 
+    return 1;
+}
+
+static int virtio_net_has_buffers(VirtIONet *n, int bufsize)
+{
     if (virtio_queue_empty(n->rx_vq) ||
         (n->mergeable_rx_bufs &&
          !virtqueue_avail_bytes(n->rx_vq, bufsize, 0))) {
@@ -384,13 +391,6 @@ static int do_virtio_net_can_receive(VirtIONet *n, int bufsize)
     return 1;
 }
 
-static int virtio_net_can_receive(VLANClientState *vc)
-{
-    VirtIONet *n = vc->opaque;
-
-    return do_virtio_net_can_receive(n, VIRTIO_NET_MAX_BUFSIZE);
-}
-
 /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
  * it never finds out that the packets don't have valid checksums.  This
  * causes dhclient to get upset.  Fedora's carried a patch for ages to
@@ -517,7 +517,10 @@ static ssize_t virtio_net_receive(VLANClientState *vc, const uint8_t *buf, size_
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
     size_t hdr_len, offset, i;
 
-    if (!do_virtio_net_can_receive(n, size))
+    if (!virtio_net_can_receive(n->vc))
+        return -1;
+
+    if (!virtio_net_has_buffers(n, size))
         return 0;
 
     if (!receive_filter(n, buf, size))
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] tap: drain queue in tap_send()

[Mark McLoughlin]

Okay, let's try re-enabling the drain-entire-queue behaviour, with a
difference - before each subsequent packet, use qemu_can_send_packet()
to check that we can send it. This is similar to how we check before
polling the tap fd and avoids having to drop a packet if the receiver
cannot handle it.

This patch should be a performance improvement since we no longer have
to go through the mainloop for each packet.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net/tap.c |   33 ++++++++++++++++++---------------
 1 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index f32226b..5272d7d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -187,23 +187,26 @@ static void tap_send_completed(VLANClientState *vc, ssize_t len)
 static void tap_send(void *opaque)
 {
     TAPState *s = opaque;
-    uint8_t *buf = s->buf;
     int size;
 
-    size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
-    if (size <= 0) {
-        return;
-    }
+    do {
+        uint8_t *buf = s->buf;
 
-    if (s->has_vnet_hdr && !s->using_vnet_hdr) {
-        buf  += sizeof(struct virtio_net_hdr);
-        size -= sizeof(struct virtio_net_hdr);
-    }
+        size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
+        if (size <= 0) {
+            break;
+        }
 
-    size = qemu_send_packet_async(s->vc, buf, size, tap_send_completed);
-    if (size == 0) {
-        tap_read_poll(s, 0);
-    }
+        if (s->has_vnet_hdr && !s->using_vnet_hdr) {
+            buf  += sizeof(struct virtio_net_hdr);
+            size -= sizeof(struct virtio_net_hdr);
+        }
+
+        size = qemu_send_packet_async(s->vc, buf, size, tap_send_completed);
+        if (size == 0) {
+            tap_read_poll(s, 0);
+        }
+    } while (size > 0 && qemu_can_send_packet(s->vc));
 }
 
 int tap_has_ufo(VLANClientState *vc)
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 0/5] Fix packet queueing to allow full tap queue drain

[Scott Tsai]

>         Scott, Sven, if you could test the tap-drain-queue branch from
> my tree, that would be great:
>   http://repo.or.cz/w/qemu/markmc.git
> 
> Thanks,
> Mark. 

Mark,
I've tested http://repo.or.cz/w/qemu/markmc.git and it indeed works for my test case
(NFS through emulated smc91c111 NIC on arm-integragorcp)

How about adding a comment in tap_send() about when qemu_send_packet_async() would return 0 and -1?

I'd also recommend checking for the case of qemu_send_packet_async() returning -1 and dropping a packet
and have a debug message guarded by #ifdef DEBUG_TAP in case the qemu_can_send_packet() check
is ever removed in future modifications of the code.

Also, would it be possible to get the stable-0.11 version of the patch into the soon to be released Fedora 12?
I've written some embedded Linux tutorials for Taiwanese consumer electronics developers that recommends using qemu
on Fedora. Being able to rely on NFS always working in Fedora's qmeu packet would really help :)

[Qemu-devel] Re: [PATCH 0/5] Fix packet queueing to allow full tap queue drain

[Mark McLoughlin]

On Wed, 2009-10-28 at 11:36 +0800, Scott Tsai wrote:
> >         Scott, Sven, if you could test the tap-drain-queue branch from
> > my tree, that would be great:
> >   http://repo.or.cz/w/qemu/markmc.git
> > 
> > Thanks,
> > Mark. 
> 
> Mark,
> I've tested http://repo.or.cz/w/qemu/markmc.git and it indeed works
> for my test case
> (NFS through emulated smc91c111 NIC on arm-integragorcp)

Many thanks for the report and confirming the fix

> How about adding a comment in tap_send() about when
> qemu_send_packet_async() would return 0 and -1?
> 
> I'd also recommend checking for the case of qemu_send_packet_async()
> returning -1 and dropping a packet and have a debug message guarded by
> #ifdef DEBUG_TAP in case the qemu_can_send_packet() check is ever
> removed in future modifications of the code.

Yeah, it's fairly gnarly code alright. I'd prefer to make it more
obvious with something like:

  ret = qemu_send_packet_async();
  switch (ret) {
  case NET_PACKET_AGAIN:
      tap_disable_write_poll();
      return;

  case NET_PACKET_DROP:
      /* drop! */
      return;
  }

Added to my TODO list

> Also, would it be possible to get the stable-0.11 version of the patch
> into the soon to be released Fedora 12? I've written some embedded
> Linux tutorials for Taiwanese consumer electronics developers that
> recommends using qemu on Fedora. Being able to rely on NFS always
> working in Fedora's qmeu packet would really help :)

Filed https://bugzilla.redhat.com/531419 ... I'll pull it in later
today.

qemu-system-arm on F12 works okay then? That's good to know, it's the
first I've heard of someone using it :-)

Thanks,
Mark.

Re: [Qemu-devel] [PATCH 5/5] tap: drain queue in tap_send()

[Anthony Liguori]

Mark McLoughlin wrote:
> Okay, let's try re-enabling the drain-entire-queue behaviour, with a
> difference - before each subsequent packet, use qemu_can_send_packet()
> to check that we can send it. This is similar to how we check before
> polling the tap fd and avoids having to drop a packet if the receiver
> cannot handle it.
>
> This patch should be a performance improvement since we no longer have
> to go through the mainloop for each packet.
>
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
>   

Could you rebase and rend this patch against master?  I've got 1-4 in 
staging.

The GSO changes make resolving this non trivial (for me at least :-)).

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 5/5] tap: drain queue in tap_send()

[Mark McLoughlin]

On Fri, 2009-10-30 at 11:21 -0500, Anthony Liguori wrote:
> Mark McLoughlin wrote:
> > Okay, let's try re-enabling the drain-entire-queue behaviour, with a
> > difference - before each subsequent packet, use qemu_can_send_packet()
> > to check that we can send it. This is similar to how we check before
> > polling the tap fd and avoids having to drop a packet if the receiver
> > cannot handle it.
> >
> > This patch should be a performance improvement since we no longer have
> > to go through the mainloop for each packet.
> >
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> >   
> 
> Could you rebase and rend this patch against master?  I've got 1-4 in 
> staging.
> 
> The GSO changes make resolving this non trivial (for me at least :-)).

Um, you've lost me :-)

You've got it in staging AFAICS, and anyway ... 5/5 is mostly a revert
of 1/5, so if 1/5 applied so should 5/5

What's in staging looks fine to me, except it doesn't build because of
problems with other patches

Thanks,
Mark.

Re: [Qemu-devel] [PATCH 5/5] tap: drain queue in tap_send()

[Anthony Liguori]

Mark McLoughlin wrote:
> On Fri, 2009-10-30 at 11:21 -0500, Anthony Liguori wrote:
>   
>> Mark McLoughlin wrote:
>>     
>>> Okay, let's try re-enabling the drain-entire-queue behaviour, with a
>>> difference - before each subsequent packet, use qemu_can_send_packet()
>>> to check that we can send it. This is similar to how we check before
>>> polling the tap fd and avoids having to drop a packet if the receiver
>>> cannot handle it.
>>>
>>> This patch should be a performance improvement since we no longer have
>>> to go through the mainloop for each packet.
>>>
>>> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
>>>   
>>>       
>> Could you rebase and rend this patch against master?  I've got 1-4 in 
>> staging.
>>
>> The GSO changes make resolving this non trivial (for me at least :-)).
>>     
>
> Um, you've lost me :-)
>
> You've got it in staging AFAICS, and anyway ... 5/5 is mostly a revert
> of 1/5, so if 1/5 applied so should 5/5
>
> What's in staging looks fine to me, except it doesn't build because of
> problems with other patches
>   

Ignore it, I accidentally tried backporting the patch targeted for the 
stable tree and then responded to the wrong patch.

Regards,

Anthony Liguori