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

[Stefan Hajnoczi <stefanha@gmail.com>]

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