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

[Zhi Yong Wu]

On Mon, Feb 6, 2012 at 6:18 AM, Paolo Bonzini <address@hidden> wrote:
> 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.
This case has existed in current upstream code, not only vlan-hub
code. Currently can_send function has been called by backend send
function before deliver/deliver_iov, If we put can_send in queue send
function, your idea will have a big challenge for slirp packet queue.
We can implement your idea below later, not in this patchset. What do
you think?

>
> 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
>



-- 
Regards,

Zhi Yong Wu