On Mon, Jan 7, 2013 at 2:49 PM, Stefan Hajnoczi <stefa...@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, ri...@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)
-----------------------------------------+-------------------------------
