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

[Luigi Rizzo]

On Mon, Jan 7, 2013 at 5:49 AM, 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.

...

 

> 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