[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Zhi Yong Wu
Subject: Re: [Qemu-devel] [QEMU] net: adapt dump to support the new syntax
Date: Thu, 17 May 2012 13:59:01 +0800

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


Zhi Yong Wu

reply via email to

[Prev in Thread] Current Thread [Next in Thread]