[Qemu-devel] unbounded qemu NetQue's ?

[Qemu-devel] unbounded qemu NetQue's ?

[Luigi Rizzo]

Hi,
while testing the tx path in qemu without a network backend connected,
i noticed that qemu_net_queue_send() builds up an unbounded
queue, because QTAILQ* have no notion of a queue length.

As a result, memory usage of a qemu instance can grow to extremely
large values.

I wonder if it makes sense to implement a hard limit on size of
NetQue's. The patch below is only a partial implementation
but probably sufficient for these purposes.

	cheers
	luigi

diff -urp qemu-1.3.0-orig/net/queue.c qemu-1.3.0/net/queue.c
--- qemu-1.3.0-orig/net/queue.c	2012-12-03 20:37:05.000000000 +0100
+++ qemu-1.3.0/net/queue.c	2013-01-06 19:38:12.000000000 +0100
@@ -92,6 +92,9 @@ static void qemu_net_queue_append(NetQue
 {
     NetPacket *packet;
 
+    if (queue->packets.tqh_count > 10000)
+	return; // XXX drop
+
     packet = g_malloc(sizeof(NetPacket) + size);
     packet->sender = sender;
     packet->flags = flags;
diff -urp qemu-1.3.0-orig/qemu-queue.h qemu-1.3.0/qemu-queue.h
--- qemu-1.3.0-orig/qemu-queue.h	2012-12-03 20:37:05.000000000 +0100
+++ qemu-1.3.0/qemu-queue.h	2013-01-06 19:34:01.000000000 +0100
@@ -320,11 +320,12 @@ struct {                                
 struct name {                                                           \
         qual type *tqh_first;           /* first element */             \
         qual type *qual *tqh_last;      /* addr of last next element */ \
+	int32_t tqh_count;						\
 }
 #define QTAILQ_HEAD(name, type)  Q_TAILQ_HEAD(name, struct type,)
 
 #define QTAILQ_HEAD_INITIALIZER(head)                                   \
-        { NULL, &(head).tqh_first }
+        { NULL, &(head).tqh_first, 0 }
 
 #define Q_TAILQ_ENTRY(type, qual)                                       \
 struct {                                                                \
@@ -339,6 +340,7 @@ struct {                                
 #define QTAILQ_INIT(head) do {                                          \
         (head)->tqh_first = NULL;                                       \
         (head)->tqh_last = &(head)->tqh_first;                          \
+        (head)->tqh_count = 0;						\
 } while (/*CONSTCOND*/0)
 
 #define QTAILQ_INSERT_HEAD(head, elm, field) do {                       \
@@ -348,6 +350,7 @@ struct {                                
         else                                                            \
                 (head)->tqh_last = &(elm)->field.tqe_next;              \
         (head)->tqh_first = (elm);                                      \
+        (head)->tqh_count++;						\
         (elm)->field.tqe_prev = &(head)->tqh_first;                     \
 } while (/*CONSTCOND*/0)
 
@@ -356,6 +359,7 @@ struct {                                
         (elm)->field.tqe_prev = (head)->tqh_last;                       \
         *(head)->tqh_last = (elm);                                      \
         (head)->tqh_last = &(elm)->field.tqe_next;                      \
+        (head)->tqh_count++;						\
 } while (/*CONSTCOND*/0)
 
 #define QTAILQ_INSERT_AFTER(head, listelm, elm, field) do {             \
@@ -381,6 +385,7 @@ struct {                                
                     (elm)->field.tqe_prev;                              \
         else                                                            \
                 (head)->tqh_last = (elm)->field.tqe_prev;               \
+        (head)->tqh_count--;						\
         *(elm)->field.tqe_prev = (elm)->field.tqe_next;                 \
 } while (/*CONSTCOND*/0)
 

Re: [Qemu-devel] unbounded qemu NetQue's ?

[Stefan Hajnoczi]

On Sun, Jan 06, 2013 at 08:23:56PM +0100, Luigi Rizzo wrote:
> Hi,
> while testing the tx path in qemu without a network backend connected,
> i noticed that qemu_net_queue_send() builds up an unbounded
> queue, because QTAILQ* have no notion of a queue length.
> 
> As a result, memory usage of a qemu instance can grow to extremely
> large values.
> 
> I wonder if it makes sense to implement a hard limit on size of
> NetQue's. The patch below is only a partial implementation
> but probably sufficient for these purposes.
> 
> 	cheers
> 	luigi

Hi Luigi,
Good idea, we should bound the queues to prevent rare situations or bugs
from hogging memory.

Ideally we would do away with queues completely and make net clients
hand buffers to each other ahead of time to impose flow control.  I've
thought about this a few times and always reached the conclusion that
it's not quite possible.

The scenario that calls for queuing packets is as follows:

The USB NIC code has a single buffer and it relies on the guest to empty
it before accepting the next packet.  The pcap net client consumes each
packet immediately.  Attach these net clients to a hub.  Since pcap can
always consume but USB NIC is very slow at consuming we have a problem.
This is where we rely on queuing.  Eventually all packets get processed
even by the slow USB NIC and the end result is that pcap and USB NIC
both see the same packets.

I thought about making pcap support built in to net.c but getting rid of
the pcap net client doesn't solve the problem, it just makes it less
extreme.

Any ideas on how to solve this?

> diff -urp qemu-1.3.0-orig/net/queue.c qemu-1.3.0/net/queue.c
> --- qemu-1.3.0-orig/net/queue.c	2012-12-03 20:37:05.000000000 +0100
> +++ qemu-1.3.0/net/queue.c	2013-01-06 19:38:12.000000000 +0100
> @@ -92,6 +92,9 @@ static void qemu_net_queue_append(NetQue

Please also do it for qemu_net_queue_append_iov().

>  {
>      NetPacket *packet;
>  
> +    if (queue->packets.tqh_count > 10000)
> +	return; // XXX drop
> +

sent_cb() must be invoked.  It's best to do this in a QEMUBH in case the
caller is not prepared for reentrancy.

>      packet = g_malloc(sizeof(NetPacket) + size);
>      packet->sender = sender;
>      packet->flags = flags;
> diff -urp qemu-1.3.0-orig/qemu-queue.h qemu-1.3.0/qemu-queue.h
> --- qemu-1.3.0-orig/qemu-queue.h	2012-12-03 20:37:05.000000000 +0100
> +++ qemu-1.3.0/qemu-queue.h	2013-01-06 19:34:01.000000000 +0100

Please don't modify qemu-queue.h.  It's a generic queue data structure
used by all of QEMU.  Instead, keep a counter in the NetQueue.

Stefan

Re: [Qemu-devel] unbounded qemu NetQue's ?

[Luigi Rizzo]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

On Mon, Jan 7, 2013 at 2:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Sun, Jan 06, 2013 at 08:23:56PM +0100, Luigi Rizzo wrote:
> > Hi,
> > while testing the tx path in qemu without a network backend connected,
> > i noticed that qemu_net_queue_send() builds up an unbounded
> > queue, because QTAILQ* have no notion of a queue length.
> >
> > As a result, memory usage of a qemu instance can grow to extremely
> > large values.
> >
> > I wonder if it makes sense to implement a hard limit on size of
> > NetQue's. The patch below is only a partial implementation
> > but probably sufficient for these purposes.
> >
> >       cheers
> >       luigi
>
> Hi Luigi,
> Good idea, we should bound the queues to prevent rare situations or bugs
> from hogging memory.
>
> Ideally we would do away with queues completely and make net clients
> hand buffers to each other ahead of time to impose flow control.  I've
> thought about this a few times and always reached the conclusion that
> it's not quite possible.
>

given that implementing flow control on the inbound (host->guest) path
is impossible, i tend to agree that removing queues is probably
not worth the effort even if possible at all.
...

Your comments below also remind me of a more general issues with the code:

 > diff -urp qemu-1.3.0-orig/net/queue.c qemu-1.3.0/net/queue.c
> > --- qemu-1.3.0-orig/net/queue.c       2012-12-03 20:37:05.000000000+0100
> > +++ qemu-1.3.0/net/queue.c    2013-01-06 19:38:12.000000000 +0100
> > @@ -92,6 +92,9 @@ static void qemu_net_queue_append(NetQue
>
> Please also do it for qemu_net_queue_append_iov().
>
>
the qemu code has many duplicate functions of the form foo() and foo_iov() .
Not only here, but even in the backents (e.g. see net/tap.c)
I think it would be good to settle on a single version of the function
and remove or convert the non-iov instances.

 >  {
> >      NetPacket *packet;
> >
> > +    if (queue->packets.tqh_count > 10000)
> > +     return; // XXX drop
> > +
>
> sent_cb() must be invoked.  It's best to do this in a QEMUBH in case the
> caller is not prepared for reentrancy.
>
>
i forgot that, but the use of sent_cb() is interesting:
the only place that actually uses it seems to be net/tap.c,
and the way it uses it only makes sense if the queue has
room!

>      packet = g_malloc(sizeof(NetPacket) + size);
> >      packet->sender = sender;
> >      packet->flags = flags;
> > diff -urp qemu-1.3.0-orig/qemu-queue.h qemu-1.3.0/qemu-queue.h
> > --- qemu-1.3.0-orig/qemu-queue.h      2012-12-03 20:37:05.000000000+0100
> > +++ qemu-1.3.0/qemu-queue.h   2013-01-06 19:34:01.000000000 +0100
>
> Please don't modify qemu-queue.h.  It's a generic queue data structure
> used by all of QEMU.  Instead, keep a counter in the NetQueue.
>
>
good suggestion, that also makes the change smaller.

cheers
luigi


> Stefan
>



-- 
-----------------------------------------+-------------------------------
 Prof. Luigi RIZZO, rizzo@iet.unipi.it  . Dip. di Ing. dell'Informazione
 http://www.iet.unipi.it/~luigi/        . Universita` di Pisa
 TEL      +39-050-2211611               . via Diotisalvi 2
 Mobile   +39-338-6809875               . 56122 PISA (Italy)
-----------------------------------------+-------------------------------

[-- Attachment #2: Type: text/html, Size: 5368 bytes --]

Re: [Qemu-devel] unbounded qemu NetQue's ?

[Stefan Hajnoczi]

On Mon, Jan 07, 2013 at 03:27:30PM +0100, Luigi Rizzo wrote:
> On Mon, Jan 7, 2013 at 2:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Sun, Jan 06, 2013 at 08:23:56PM +0100, Luigi Rizzo wrote:
>  > diff -urp qemu-1.3.0-orig/net/queue.c qemu-1.3.0/net/queue.c
> > > --- qemu-1.3.0-orig/net/queue.c       2012-12-03 20:37:05.000000000+0100
> > > +++ qemu-1.3.0/net/queue.c    2013-01-06 19:38:12.000000000 +0100
> > > @@ -92,6 +92,9 @@ static void qemu_net_queue_append(NetQue
> >
> > Please also do it for qemu_net_queue_append_iov().
> >
> >
> the qemu code has many duplicate functions of the form foo() and foo_iov() .
> Not only here, but even in the backents (e.g. see net/tap.c)
> I think it would be good to settle on a single version of the function
> and remove or convert the non-iov instances.

Yes.  Patches welcome.

Stefan

Re: [Qemu-devel] unbounded qemu NetQue's ?

[Luigi Rizzo]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]

On Mon, Jan 7, 2013 at 5:49 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Sun, Jan 06, 2013 at 08:23:56PM +0100, Luigi Rizzo wrote:
> > Hi,
> > while testing the tx path in qemu without a network backend connected,
> > i noticed that qemu_net_queue_send() builds up an unbounded
> > queue, because QTAILQ* have no notion of a queue length.
> >
> > As a result, memory usage of a qemu instance can grow to extremely
> > large values.
> >
> > I wonder if it makes sense to implement a hard limit on size of
> > NetQue's. The patch below is only a partial implementation
> > but probably sufficient for these purposes.
> >
> >       cheers
> >       luigi
>
> Hi Luigi,
> Good idea, we should bound the queues to prevent rare situations or bugs
> from hogging memory.
>

...


>  > diff -urp qemu-1.3.0-orig/net/queue.c qemu-1.3.0/net/queue.c
> > --- qemu-1.3.0-orig/net/queue.c       2012-12-03 20:37:05.000000000+0100
> > +++ qemu-1.3.0/net/queue.c    2013-01-06 19:38:12.000000000 +0100
> > @@ -92,6 +92,9 @@ static void qemu_net_queue_append(NetQue
>
> Please also do it for qemu_net_queue_append_iov().
>
> >  {
> >      NetPacket *packet;
> >
> > +    if (queue->packets.tqh_count > 10000)
> > +     return; // XXX drop
> > +
>
> sent_cb() must be invoked.  It's best to do this in a QEMUBH in case the
> caller is not prepared for reentrancy.
>

Stefan, the semantic of callbacks makes it difficult to run them on drops:
they are supposed to run only when the queue has been drained,
and apparently only once per sender, according to the comment
in the header of net/queue.c:

/* The delivery handler may only return zero if it will call
 * qemu_net_queue_flush() when it determines that it is once again able
 * to deliver packets. It must also call qemu_net_queue_purge() in its
 * cleanup path.
 *
 * If a sent callback is provided to send(), the caller must handle a
 * zero return from the delivery handler by not sending any more packets
 * until we have invoked the callback. Only in that case will we queue
 * the packet.
 *
 * If a sent callback isn't provided, we just drop the packet to avoid
 * unbounded queueing.
 */

This seems to suggest that a packet to qemu_net_queue_send()
should never be queued unless it has a callback
(hence the existing code is buggy, because it never ever drops packets),
so the queue can only hold one packet per sender,
hence there is no real risk of overflow.

cheers
luigi

[-- Attachment #2: Type: text/html, Size: 3634 bytes --]

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

[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 <rizzo@iet.unipi.it>
--- ../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;
         }

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

[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

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

[Luigi Rizzo]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 3816 bytes --]