diff mbox

fix unbounded qemu NetQueue

Message ID 20130117060711.GA59416@onelab2.iet.unipi.it
State New
Headers show

Commit Message

Luigi Rizzo Jan. 17, 2013, 6:07 a.m. UTC 
The comment at the beginning of net/queue.c says that packets that
cannot be sent by qemu_net_queue_send() should not be enqueued
unless a callback is set.

This patch implements this behaviour, that prevents a queue to grow
unbounded (e.g. when a network backend is not connected).

Also for good measure the patch implements bounded size queues
(though it should not be necessary now because each source can only have
one packet queued). When a packet is dropped because excessive
queue size the callback is not supposed to be called.

cheers
luigi

Signed-off-by: Luigi Rizzo <rizzo@iet.unipi.it>

Comments

Stefan Hajnoczi Jan. 17, 2013, 10:21 a.m. UTC | #1 
On Thu, Jan 17, 2013 at 07:07:11AM +0100, Luigi Rizzo wrote:
> The comment at the beginning of net/queue.c says that packets that
> cannot be sent by qemu_net_queue_send() should not be enqueued
> unless a callback is set.
> 
> This patch implements this behaviour, that prevents a queue to grow
> unbounded (e.g. when a network backend is not connected).
> 
> Also for good measure the patch implements bounded size queues
> (though it should not be necessary now because each source can only have
> one packet queued). When a packet is dropped because excessive
> queue size the callback is not supposed to be called.

Although I appreciate the semantics that the comment tries to establish,
the code doesn't behave like this today and we cannot drop packets in
cases where we relied on queuing them.

More changes will be required to make the hub, USB, pcap scenario I
described previously work.

Stefan
Luigi Rizzo Jan. 17, 2013, 3:56 p.m. UTC | #2 
On Thu, Jan 17, 2013 at 2:21 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Jan 17, 2013 at 07:07:11AM +0100, Luigi Rizzo wrote:
> > The comment at the beginning of net/queue.c says that packets that
> > cannot be sent by qemu_net_queue_send() should not be enqueued
> > unless a callback is set.
> >
> > This patch implements this behaviour, that prevents a queue to grow
> > unbounded (e.g. when a network backend is not connected).
> >
> > Also for good measure the patch implements bounded size queues
> > (though it should not be necessary now because each source can only have
> > one packet queued). When a packet is dropped because excessive
> > queue size the callback is not supposed to be called.
>
> Although I appreciate the semantics that the comment tries to establish,
> the code doesn't behave like this today and we cannot drop packets in
> cases where we relied on queuing them.
>
> More changes will be required to make the hub, USB, pcap scenario I
> described previously work.
>

i see. then the other option would be to drop packets only
if the queue is oversize AND the callback is not set:

+    if (queue->nq_count >= queue->nq_maxlen && !sent_cb)
+       return;

so we should be able to use the queue to store packets when the
USB guest is slow, and avoid dropping precious packet with the callback set
(there should be only one of them for each source) ?
The queue might grow slightly overlimit but still be kept under control.

I cannot think of another way to handle the callback. I think we cannot run
it on a drop as it would cause a premature restart of the sender.
(at a cursory inspection of the code, just tap.c and virtio-net.c use
callbacks)

cheers
luigi
diff mbox

Patch

--- ../orig/net/queue.c	2012-12-03 11:37:05.000000000 -0800
+++ ./net/queue.c	2013-01-16 21:37:20.109732443 -0800
@@ -50,6 +50,8 @@  struct NetPacket {
 
 struct NetQueue {
     void *opaque;
+    uint32_t nq_maxlen;
+    uint32_t nq_count;
 
     QTAILQ_HEAD(packets, NetPacket) packets;
 
@@ -63,6 +65,8 @@  NetQueue *qemu_new_net_queue(void *opaqu
     queue = g_malloc0(sizeof(NetQueue));
 
     queue->opaque = opaque;
+    queue->nq_maxlen = 10000; /* arbitrary limit */
+    queue->nq_count = 0;
 
     QTAILQ_INIT(&queue->packets);
 
@@ -92,6 +96,8 @@  static void qemu_net_queue_append(NetQue
 {
     NetPacket *packet;
 
+    if (!sent_cb || queue->nq_count >= queue->nq_maxlen)
+	return;
     packet = g_malloc(sizeof(NetPacket) + size);
     packet->sender = sender;
     packet->flags = flags;
@@ -99,6 +105,7 @@  static void qemu_net_queue_append(NetQue
     packet->sent_cb = sent_cb;
     memcpy(packet->data, buf, size);
 
+    queue->nq_count++;
     QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
 }
 
@@ -113,6 +120,8 @@  static void qemu_net_queue_append_iov(Ne
     size_t max_len = 0;
     int i;
 
+    if (!sent_cb || queue->nq_count >= queue->nq_maxlen)
+	return;
     for (i = 0; i < iovcnt; i++) {
         max_len += iov[i].iov_len;
     }
@@ -130,6 +139,7 @@  static void qemu_net_queue_append_iov(Ne
         packet->size += len;
     }
 
+    queue->nq_count++;
     QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
 }
 
@@ -220,6 +230,7 @@  void qemu_net_queue_purge(NetQueue *queu
     QTAILQ_FOREACH_SAFE(packet, &queue->packets, entry, next) {
         if (packet->sender == from) {
             QTAILQ_REMOVE(&queue->packets, packet, entry);
+            queue->nq_count--;
             g_free(packet);
         }
     }
@@ -233,6 +244,7 @@  bool qemu_net_queue_flush(NetQueue *queu
 
         packet = QTAILQ_FIRST(&queue->packets);
         QTAILQ_REMOVE(&queue->packets, packet, entry);
+        queue->nq_count--;
 
         ret = qemu_net_queue_deliver(queue,
                                      packet->sender,
@@ -240,6 +252,7 @@  bool qemu_net_queue_flush(NetQueue *queu
                                      packet->data,
                                      packet->size);
         if (ret == 0) {
+            queue->nq_count++;
             QTAILQ_INSERT_HEAD(&queue->packets, packet, entry);
             return false;
         }