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

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

[Stefan Hajnoczi]

On Tue, Jan 24, 2012 at 6:22 PM, Benjamin <mlspirat42@gmail.com> wrote:
> Hello Stefan,

Hi,
Please send QEMU development emails to the mailing list so others can
help or we can avoid duplicating work in case someone else is already
looking into this.  I have CCed the mailing list.

> I've seen your blog and I hope you can help me address the issue of
> -net dump not being compatible with the new -netdev syntax.

The reason is because packet capture is implemented as a net client -
it receives packets on the "VLAN" and writes them to the pcap file.
In the -netdev model each client has a peer (another net client that
is communicates with).  So it's not possible to plug the "dump" net
client together with an emulated NIC client and a host tap client, for
example.  Since "VLANs" broadcast packets to all attached net clients
it works there.

> I think it is possible to achieve but I've had a lot of trouble
> understanding the code, VLANs are everywhere and even though the new
> syntax doesn't use them it looks like the code still uses a VLAN logic.
>
> Before I really start to get serious about this task, do you have any
> advice or recommendation?

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.

If you rebase the vlan-hub branch onto a recent qemu.git, then you can
use -netdev syntax to create an emulated NIC, host device, dump
device, and a hub.  The three clients must be attached to the hub.

I think the code is already there, it should work.  I didn't submit
this because I wanted to implement automated tests to ensure that
these changes don't brake the old syntax for "VLANs".

Feel free to play around with the vlan-hub branch, test it, and push
it upstream along with changes that you make.

Stefan

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Tue, Jan 24, 2012 at 6:22 PM, Benjamin <mlspirat42@gmail.com> wrote:
>> Hello Stefan,
>
> Hi,
> Please send QEMU development emails to the mailing list so others can
> help or we can avoid duplicating work in case someone else is already
> looking into this.  I have CCed the mailing list.
>
>> I've seen your blog and I hope you can help me address the issue of
>> -net dump not being compatible with the new -netdev syntax.
>
> The reason is because packet capture is implemented as a net client -
> it receives packets on the "VLAN" and writes them to the pcap file.
> In the -netdev model each client has a peer (another net client that
> is communicates with).  So it's not possible to plug the "dump" net
> client together with an emulated NIC client and a host tap client, for
> example.  Since "VLANs" broadcast packets to all attached net clients
> it works there.
>
>> I think it is possible to achieve but I've had a lot of trouble
>> understanding the code, VLANs are everywhere and even though the new
>> syntax doesn't use them it looks like the code still uses a VLAN logic.
>>
>> Before I really start to get serious about this task, do you have any
>> advice or recommendation?
>
> 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.

Sounds more advanced than my experimental hackery to kill VLANs.

> If you rebase the vlan-hub branch onto a recent qemu.git, then you can
> use -netdev syntax to create an emulated NIC, host device, dump
> device, and a hub.  The three clients must be attached to the hub.
>
> I think the code is already there, it should work.  I didn't submit
> this because I wanted to implement automated tests to ensure that
> these changes don't brake the old syntax for "VLANs".
>
> Feel free to play around with the vlan-hub branch, test it, and push
> it upstream along with changes that you make.

Yes, please.

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

[Benjamin]

On 01/24/12 20:58, Markus Armbruster wrote:
> Stefan Hajnoczi<stefanha@gmail.com>  writes:
>
>> On Tue, Jan 24, 2012 at 6:22 PM, Benjamin<mlspirat42@gmail.com>  wrote:
>>> Hello Stefan,
>>
>> Hi,
>> Please send QEMU development emails to the mailing list so others can
>> help or we can avoid duplicating work in case someone else is already
>> looking into this.  I have CCed the mailing list.
>>
>>> I've seen your blog and I hope you can help me address the issue of
>>> -net dump not being compatible with the new -netdev syntax.
>>
>> The reason is because packet capture is implemented as a net client -
>> it receives packets on the "VLAN" and writes them to the pcap file.
>> In the -netdev model each client has a peer (another net client that
>> is communicates with).  So it's not possible to plug the "dump" net
>> client together with an emulated NIC client and a host tap client, for
>> example.  Since "VLANs" broadcast packets to all attached net clients
>> it works there.
>>
>>> I think it is possible to achieve but I've had a lot of trouble
>>> understanding the code, VLANs are everywhere and even though the new
>>> syntax doesn't use them it looks like the code still uses a VLAN logic.
>>>
>>> Before I really start to get serious about this task, do you have any
>>> advice or recommendation?
>>
>> 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.
>
> Sounds more advanced than my experimental hackery to kill VLANs.
>
>> If you rebase the vlan-hub branch onto a recent qemu.git, then you can
>> use -netdev syntax to create an emulated NIC, host device, dump
>> device, and a hub.  The three clients must be attached to the hub.
>>
>> I think the code is already there, it should work.  I didn't submit
>> this because I wanted to implement automated tests to ensure that
>> these changes don't brake the old syntax for "VLANs".
>>
>> Feel free to play around with the vlan-hub branch, test it, and push
>> it upstream along with changes that you make.
>
> Yes, please.


I will rebase it and test it then. Thanks.

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

[Paolo Bonzini]

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

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

[Zhi Yong Wu]

I would like to know if some one is playing around with the patchset.

If yes, can any one make one response? I am very interested in rebasing
it, and then playing with it.

On Tue, Jan 24, 2012 at 6:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jan 24, 2012 at 6:22 PM, Benjamin <mlspirat42@gmail.com> wrote:
>> Hello Stefan,
>
> Hi,
> Please send QEMU development emails to the mailing list so others can
> help or we can avoid duplicating work in case someone else is already
> looking into this.  I have CCed the mailing list.
>
>> I've seen your blog and I hope you can help me address the issue of
>> -net dump not being compatible with the new -netdev syntax.
>
> The reason is because packet capture is implemented as a net client -
> it receives packets on the "VLAN" and writes them to the pcap file.
> In the -netdev model each client has a peer (another net client that
> is communicates with).  So it's not possible to plug the "dump" net
> client together with an emulated NIC client and a host tap client, for
> example.  Since "VLANs" broadcast packets to all attached net clients
> it works there.
>
>> I think it is possible to achieve but I've had a lot of trouble
>> understanding the code, VLANs are everywhere and even though the new
>> syntax doesn't use them it looks like the code still uses a VLAN logic.
>>
>> Before I really start to get serious about this task, do you have any
>> advice or recommendation?
>
> 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.
>
> If you rebase the vlan-hub branch onto a recent qemu.git, then you can
> use -netdev syntax to create an emulated NIC, host device, dump
> device, and a hub.  The three clients must be attached to the hub.
>
> I think the code is already there, it should work.  I didn't submit
> this because I wanted to implement automated tests to ensure that
> these changes don't brake the old syntax for "VLANs".
>
> Feel free to play around with the vlan-hub branch, test it, and push
> it upstream along with changes that you make.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu

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

[Stefan Hajnoczi]

On Fri, Feb 17, 2012 at 6:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> I would like to know if some one is playing around with the patchset.
>
> If yes, can any one make one response? I am very interested in rebasing
> it, and then playing with it.

AFAIK no one is working on it - the main thing that is missing here is
tests so that we don't break the existing "VLAN" feature and
command-line interface.

If you have time to play with stuff though it would be good to fix the
block I/O limits issue with IDE.

Stefan

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

[Zhi Yong Wu]

On Fri, Feb 17, 2012 at 6:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Feb 17, 2012 at 6:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> I would like to know if some one is playing around with the patchset.
>>
>> If yes, can any one make one response? I am very interested in rebasing
>> it, and then playing with it.
>
> AFAIK no one is working on it - the main thing that is missing here is
> tests so that we don't break the existing "VLAN" feature and
> command-line interface.
Some new network features were merged after the code base of your
vlan-hub, such as bridge helper, etc. So some VLANClientState and
VLANState structure are included, and need to be dropped, but your
patchsets can not cover this.
>
> If you have time to play with stuff though it would be good to fix the
> block I/O limits issue with IDE.
I am working on this.:)
>
> Stefan



-- 
Regards,

Zhi Yong Wu

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

[Zhi Yong Wu]

On Fri, Feb 17, 2012 at 6:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Feb 17, 2012 at 6:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> I would like to know if some one is playing around with the patchset.
>>
>> If yes, can any one make one response? I am very interested in rebasing
>> it, and then playing with it.
>
> AFAIK no one is working on it - the main thing that is missing here is
> tests so that we don't break the existing "VLAN" feature and
> command-line interface.
If you are available, can you let us know what their difference among
-net, -netdev and hub? their drawback? their advantage? thanks

>
> If you have time to play with stuff though it would be good to fix the
> block I/O limits issue with IDE.
>
> Stefan



-- 
Regards,

Zhi Yong Wu

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

[Stefan Hajnoczi]

On Sat, Feb 18, 2012 at 8:57 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> On Fri, Feb 17, 2012 at 6:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Fri, Feb 17, 2012 at 6:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>>> I would like to know if some one is playing around with the patchset.
>>>
>>> If yes, can any one make one response? I am very interested in rebasing
>>> it, and then playing with it.
>>
>> AFAIK no one is working on it - the main thing that is missing here is
>> tests so that we don't break the existing "VLAN" feature and
>> command-line interface.
> If you are available, can you let us know what their difference among
> -net, -netdev and hub? their drawback? their advantage? thanks

-net is the old command-line option for defining both guest network
devices (e.g. e1000, virtio-net) and also host network devices (e.g.
tap, socket, user).  It uses the VLAN concept and does not use "peer"
IIRC.

-netdev/-device is the new way to specify networking.  You use -netdev
to define host network devices (e.g. tap, socket, user).  You use
-device to specify guest network devices (e.g. e1000, virtio-net-pci).
 This approach does not use the "VLAN" feature and instead uses
->peer.  That means nic->per == netdev and netdev->peer == nic.

There is a performance improvement with -netdev/-device because we do
not use "VLAN" forwarding in the network subsystem and I think it
enables tap offloads (checksum offload, GRO, TSO) but I don't remember
the detalis there.

The "hub" is the same as QEMU "VLANs" except it removes all the
special case code in the network subsystem and instead implements the
functionality in a normal NetClientState.  Now the hub is no longer
part of the network subsystem, its just a net device in QEMU.

The critical thing when transitioning to the hub device is that the
old "VLAN" command-line options must continue to work!  Users should
see no difference from before.  But internally the net subsystem will
only deal with NetClientState instead of VLANClientState plus
VLANState.  Everything will use ->peer.

Stefan

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

[Zhi Yong Wu]

On Sat, Feb 18, 2012 at 5:48 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Feb 18, 2012 at 8:57 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Fri, Feb 17, 2012 at 6:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Fri, Feb 17, 2012 at 6:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>>>> I would like to know if some one is playing around with the patchset.
>>>>
>>>> If yes, can any one make one response? I am very interested in rebasing
>>>> it, and then playing with it.
>>>
>>> AFAIK no one is working on it - the main thing that is missing here is
>>> tests so that we don't break the existing "VLAN" feature and
>>> command-line interface.
>> If you are available, can you let us know what their difference among
>> -net, -netdev and hub? their drawback? their advantage? thanks
>
> -net is the old command-line option for defining both guest network
> devices (e.g. e1000, virtio-net) and also host network devices (e.g.
> tap, socket, user).  It uses the VLAN concept and does not use "peer"
> IIRC.
>
> -netdev/-device is the new way to specify networking.  You use -netdev
> to define host network devices (e.g. tap, socket, user).  You use
> -device to specify guest network devices (e.g. e1000, virtio-net-pci).
>  This approach does not use the "VLAN" feature and instead uses
> ->peer.  That means nic->per == netdev and netdev->peer == nic.
>
> There is a performance improvement with -netdev/-device because we do
> not use "VLAN" forwarding in the network subsystem and I think it
> enables tap offloads (checksum offload, GRO, TSO) but I don't remember
> the detalis there.
>
> The "hub" is the same as QEMU "VLANs" except it removes all the
> special case code in the network subsystem and instead implements the
> functionality in a normal NetClientState.  Now the hub is no longer
> part of the network subsystem, its just a net device in QEMU.
>
> The critical thing when transitioning to the hub device is that the
> old "VLAN" command-line options must continue to work!  Users should
> see no difference from before.  But internally the net subsystem will
> only deal with NetClientState instead of VLANClientState plus
> VLANState.  Everything will use ->peer.
Nice, thanks.

>
> Stefan



-- 
Regards,

Zhi Yong Wu

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

[Michael S. Tsirkin]

On Fri, Feb 17, 2012 at 10:55:40AM +0000, Stefan Hajnoczi wrote:
> On Fri, Feb 17, 2012 at 6:54 AM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> > I would like to know if some one is playing around with the patchset.
> >
> > If yes, can any one make one response? I am very interested in rebasing
> > it, and then playing with it.
> 
> AFAIK no one is working on it - the main thing that is missing here is
> tests so that we don't break the existing "VLAN" feature and
> command-line interface.

In fact dump is the only reason we keep VLANs around.
Otherwise IMO we could just remove them.

> If you have time to play with stuff though it would be good to fix the
> block I/O limits issue with IDE.
> 
> Stefan

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 <pbonzini@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.
No,  if N ports are used, it should have 2N queues.(each of emulated
NIC, 2 hub ports and net backend has 1 queue).
>
> 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.
Yes.
>
> 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.
Network backends such as tap, slirp have called qemu_can_send_packet
function before it call qemu_net_queue_send or
qemu_net_queue_send_iov.
eg. tap_send() when packets are sent to guest emulated NIC from network backend.
Moreover, only emulated NICs provide can_recieve function, but network
backends such as tap, slirp, etc. have not; So it means that
qemu_can_send_packet is currently used to control packets flow from
network backends to emulated NIC.

For those packets from emulated NIC to network backends, we can do
this type of flow contol but need network backends or hub port provide
their can_recieve function. otherwise it will not get expected result.

>
> 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.
great idea.
>
> 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
One guest should usually only have several ports, not many in regular system.
> ports might linearize the same packet multiple times in
> qemu_net_queue_append_iov.  The limit however is packet size * number of
I am not sure which case you are talking about? when the packets are
sent from emulated NIC to hub port, or from one hub port to another
hub ports?
For every scenario, i don't think that your case can take place.
> 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.
Done.
>
> Paolo
>



-- 
Regards,

Zhi Yong Wu

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

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

[Paolo Bonzini]

Il 17/05/2012 07:59, Zhi Yong Wu ha scritto:
>> > 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.

Exactly why?  For SLIRP's receive path, SLIRP doesn't implement
can_receive at all so it will never block.  For the send path, when flow
control kicks qemu_net_queue_append will copy the packet so it is not a
problem for SLIRP's stack-allocated packets.

> We can implement your idea below later, not in this patchset. What do
> you think?

Note that my idea above was only means to an end.  If you can remove the
TODOs in a convincing manner, that would be fine.

Paolo

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

[Zhi Yong Wu]

On Thu, May 17, 2012 at 5:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/05/2012 07:59, Zhi Yong Wu ha scritto:
>>> > 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.
>
> Exactly why?  For SLIRP's receive path, SLIRP doesn't implement
> can_receive at all so it will never block.  For the send path, when flow
> control kicks qemu_net_queue_append will copy the packet so it is not a
> problem for SLIRP's stack-allocated packets.
You know that qemu_send_packet is void type, and has return value, if
can_send fails, if_encap will not currently get expected return value,
so this will cause that we have to modify the definition of
qemu_send_packet to make it return one valid value. a lot of functions
have called it, so i would not like to modify its definition.

>
>> We can implement your idea below later, not in this patchset. What do
>> you think?
>
> Note that my idea above was only means to an end.  If you can remove the
> TODOs in a convincing manner, that would be fine.
You mean that we need adopt another handling way? or directly TODO comments?

>
> Paolo



-- 
Regards,

Zhi Yong Wu

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

[Paolo Bonzini]

Il 17/05/2012 12:05, Zhi Yong Wu ha scritto:
> On Thu, May 17, 2012 at 5:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> 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.
>>
>> Exactly why?  For SLIRP's receive path, SLIRP doesn't implement
>> can_receive at all so it will never block.  For the send path, when flow
>> control kicks qemu_net_queue_append will copy the packet so it is not a
>> problem for SLIRP's stack-allocated packets.
>
> You know that qemu_send_packet is void type, and has return value, if
> can_send fails, if_encap will not currently get expected return value,
> so this will cause that we have to modify the definition of
> qemu_send_packet to make it return one valid value. a lot of functions
> have called it, so i would not like to modify its definition.

If qemu_can_send_packet returns false of course I would not drop the
packet.  I would append it to the queue (qemu_net_queue_append) for
later processing.

>>> We can implement your idea below later, not in this patchset. What do
>>> you think?
>>
>> Note that my idea above was only means to an end.  If you can remove the
>> TODOs in a convincing manner, that would be fine.
> You mean that we need adopt another handling way? or directly TODO comments?

You need to address the TODOs.  I proposed a way.  It may actually not
be correct, and there may of course be others.  But the TODOs are there,
and have to be solved.

Paolo