Re: [Qemu-devel] [PATCH] fix unbounded qemu NetQueue

2013-01-17 Thread Luigi Rizzo 
On Thu, Jan 17, 2013 at 2:21 AM, Stefan Hajnoczi  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

Re: [Qemu-devel] [PATCH] fix unbounded qemu NetQueue

2013-01-17 Thread Stefan Hajnoczi 
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

[Qemu-devel] [PATCH] fix unbounded qemu NetQueue

2013-01-16 Thread Luigi Rizzo 
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 
--- ../orig/net/queue.c 2012-12-03 11:37:05.0 -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 = 1; /* 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;
 }