Re: [Qemu-devel] [QEMU] net: adapt dump to support the new syntax

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/24/2012 11:42 AM, Stefan Hajnoczi wrote:
> I refactored the network subsystem to drop the "VLAN" concept a while
> back but never got around to submitting the patches:
>
> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/vlan-hub
>
> This branch completely removes the "VLAN" feature.  Instead it
> introduces hubs, which are net clients that have multiple ports and
> broadcast packets between them.  This allows you to achieve the same
> behavior as "VLANs" except we remove all the hardcoded special cases
> in the net subsystem and instead push that feature out into the hub
> net client.

That looks cool!

I never worked with the network subsystem, but it seems to me that the 
only difference between hubs and VLANs in your branch is related to 
queues and flow control.  VLAN flow control is actually a bit mysterious 
to me, because all devices on a VLAN share a queue and I don't quite 
understand the consequences...  With a N-port hub instead you have N+1 
queues.

Regarding your TODO here:

> +        /* TODO use qemu_send_packet() or need to call *_deliver_* directly? */

I think uses qemu_send_packet() is right but I have some other doubts. 
Initially I spotted this code, where hubs and VLANs had separate handling.

int qemu_can_send_packet(VLANClientState *sender)
{
     VLANState *vlan = sender->vlan;
     VLANClientState *vc;

     if (sender->peer) {
         ...
     }
     ...
     QTAILQ_FOREACH(vc, &vlan->clients, next) {
         if (vc == sender) {
             continue;
         }

         /* no can_receive() handler, they can always receive */
         if (vc->info->can_receive && !vc->info->can_receive(vc)) {
             return 0;
         }
     }
     return 1;
}

This means VLANs will wait for all receivers to be ready and then do N-1 
synchronous sends.  Your code will queue N-1 asynchronous sends.

However, then I noticed that qemu_can_send_packet is not called very 
much, and I do not understand why qemu_net_queue_send and 
qemu_net_queue_send_iov do not call qemu_can_send_packet before calling 
deliver/deliver_iov.

If they did, hubs could then do their own flow control via can_receive. 
  When qemu_send_packet returns zero you increment a count of in-flight 
packets, and a sent-packet callback would decrement the same count. 
When the count is non-zero, can_receive returns false (and vice versa). 
  The sent_cb also needs to call qemu_flush_queued_packets when the 
count drop to zero.

With this in place, I think the other TODO about the return value is 
easily solved; receive/receive_iov callbacks can simply return immediate 
success, and later block further sends.

Due to the separate per-port send queues, when many sends are blocking 
many ports might linearize the same packet multiple times in 
qemu_net_queue_append_iov.  The limit however is packet size * number of 
ports, which is not more than a few KB; it's more of a performance 
problem and VLANs/hubs should only be used when performance doesn't 
matter (as with the dump client).

BTW, an additional cleanup that you can do on top is to remove the 
deliver/deliver_iov function pointers in net/queue.c and 
qemu_new_net_queue, because they will always be qemu_deliver_packet and 
qemu_deliver_packet_iov.

Paolo