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

[Luigi Rizzo]

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