On Mon, Feb 6, 2012 at 6:18 AM, Paolo Bonzini <pbonz...@redhat.com> 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
