| Message ID | 20100607150309.GA13369@redhat.com |
|---|---|
| State | New |
| Headers | show |
> With -netdev, there now seems to be little need to support vlans, > enabling them leads to user confusion and bad performance. > Disable support for vlans by default, add config option to enable. No. If you want to remove vlans, then actually do that. As I've said before if you want a point-point network model then you should implement that (and remove the vlan code, probably replacing with equivalent functionality). We should not have both point-point and broadcast interfaces. Paul
On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: > > With -netdev, there now seems to be little need to support vlans, > > enabling them leads to user confusion and bad performance. > > Disable support for vlans by default, add config option to enable. > > No. If you want to remove vlans, then actually do that. How is this not what this patch does? You mean kill the code completely, not just --contigure option? > As I've said before if you want a point-point network model then you should > implement that (and remove the vlan code, probably replacing with equivalent > functionality). We should not have both point-point and broadcast interfaces. > > Paul This is what netdev does: replaces vlan. What is left is remove vlan.
> On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: > > > With -netdev, there now seems to be little need to support vlans, > > > enabling them leads to user confusion and bad performance. > > > Disable support for vlans by default, add config option to enable. > > > > No. If you want to remove vlans, then actually do that. > > How is this not what this patch does? You mean kill the code > completely, not just --contigure option? Yes. Configure options are bad. If code isn't worth enabling by default then you've got to have a very good reason why it exists at all. Paul
On 06/07/2010 11:42 AM, Paul Brook wrote: >> On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: >> >>>> With -netdev, there now seems to be little need to support vlans, >>>> enabling them leads to user confusion and bad performance. >>>> Disable support for vlans by default, add config option to enable. >>>> >>> No. If you want to remove vlans, then actually do that. >>> >> How is this not what this patch does? You mean kill the code >> completely, not just --contigure option? >> > Yes. Configure options are bad. If code isn't worth enabling by default then > you've got to have a very good reason why it exists at all. > Configure options are bad except when they are good. Distributions don't want to support every possible bell and whistle that qemu supports. By having configuration options upstream, we ensure that everyone is consistently disabling thing in the same fashion and that the interfaces presented to the users are consistent. I certainly believe that we should not disable features by default. But I think it's important that we support disabling features from a downstream supportability perspective. Regards, Anthony Liguori > Paul > >
On Mon, Jun 07, 2010 at 05:42:55PM +0100, Paul Brook wrote: > > On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: > > > > With -netdev, there now seems to be little need to support vlans, > > > > enabling them leads to user confusion and bad performance. > > > > Disable support for vlans by default, add config option to enable. > > > > > > No. If you want to remove vlans, then actually do that. > > > > How is this not what this patch does? You mean kill the code > > completely, not just --contigure option? > > Yes. Configure options are bad. If code isn't worth enabling by default then > you've got to have a very good reason why it exists at all. > > Paul Everyone ok with disabling vlans with no config option?
On Mon, Jun 07, 2010 at 11:52:05AM -0500, Anthony Liguori wrote: > On 06/07/2010 11:42 AM, Paul Brook wrote: >>> On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: >>> >>>>> With -netdev, there now seems to be little need to support vlans, >>>>> enabling them leads to user confusion and bad performance. >>>>> Disable support for vlans by default, add config option to enable. >>>>> >>>> No. If you want to remove vlans, then actually do that. >>>> >>> How is this not what this patch does? You mean kill the code >>> completely, not just --contigure option? >>> >> Yes. Configure options are bad. If code isn't worth enabling by default then >> you've got to have a very good reason why it exists at all. >> > > Configure options are bad except when they are good. > > Distributions don't want to support every possible bell and whistle that > qemu supports. By having configuration options upstream, we ensure that > everyone is consistently disabling thing in the same fashion and that > the interfaces presented to the users are consistent. > > I certainly believe that we should not disable features by default. But > I think it's important that we support disabling features from a > downstream supportability perspective. > > Regards, > > Anthony Liguori So I see two ways to go forward: switch default value in my patch, or disable vlans unconditionally. Which will it be? >> Paul >> >>
On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: > > So I see two ways to go forward: switch default value in my patch, > or disable vlans unconditionally. > The problem with disabling vlans unconditionally is that you break -net socket and -net dump. If we can come up with an alternative way to do these things, I'm all for removing it. Regards, Anthony Liguori > Which will it be? > > >>> Paul >>> >>> >>>
On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: > On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >> >> So I see two ways to go forward: switch default value in my patch, >> or disable vlans unconditionally. >> > > The problem with disabling vlans unconditionally is that you break -net > socket and -net dump. > > If we can come up with an alternative way to do these things, I'm all > for removing it. Hmm, I'll try to look at supporting -net socket in netdev. Does -net dump do anything that can't be done with tap+tcpdump? > Regards, > > Anthony Liguori > >> Which will it be? >> >> >>>> Paul >>>> >>>> >>>>
On 06/07/2010 03:58 PM, Michael S. Tsirkin wrote: > On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: > >> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >> >>> So I see two ways to go forward: switch default value in my patch, >>> or disable vlans unconditionally. >>> >>> >> The problem with disabling vlans unconditionally is that you break -net >> socket and -net dump. >> >> If we can come up with an alternative way to do these things, I'm all >> for removing it. >> > Hmm, I'll try to look at supporting -net socket in netdev. > Does -net dump do anything that can't be done with tap+tcpdump? > tap+tcpdump requires root privileges (even if you have a tap helper). Plus tcpdump doesn't help with slirp and -net dump is very useful for debugging slirp. Regards, Anthony Liguroi >> Regards, >> >> Anthony Liguori >> >> >>> Which will it be? >>> >>> >>> >>>>> Paul >>>>> >>>>> >>>>> >>>>>
On 06/07/2010 04:37 PM, Anthony Liguori wrote: > On 06/07/2010 03:58 PM, Michael S. Tsirkin wrote: >> On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: >>> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >>>> So I see two ways to go forward: switch default value in my patch, >>>> or disable vlans unconditionally. >>>> >>> The problem with disabling vlans unconditionally is that you break -net >>> socket and -net dump. >>> >>> If we can come up with an alternative way to do these things, I'm all >>> for removing it. >> Hmm, I'll try to look at supporting -net socket in netdev. >> Does -net dump do anything that can't be done with tap+tcpdump? > > tap+tcpdump requires root privileges (even if you have a tap helper). > > Plus tcpdump doesn't help with slirp and -net dump is very useful for > debugging slirp. Of course, you could add this functionality to netdev. It's arguably better there too because then you can debug virtio-net+tap with full offload enabled (which you cannot do today). Regards, Anthony Liguori
On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: > On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >> >> So I see two ways to go forward: switch default value in my patch, >> or disable vlans unconditionally. >> > > The problem with disabling vlans unconditionally is that you break -net > socket and -net dump. -netdev socket seems to be supported. No? > If we can come up with an alternative way to do these things, I'm all > for removing it. > > Regards, > > Anthony Liguori > >> Which will it be? >> >> >>>> Paul >>>> >>>> >>>>
On Mon, Jun 07, 2010 at 04:57:09PM -0500, Anthony Liguori wrote: > On 06/07/2010 04:37 PM, Anthony Liguori wrote: >> On 06/07/2010 03:58 PM, Michael S. Tsirkin wrote: >>> On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: >>>> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >>>>> So I see two ways to go forward: switch default value in my patch, >>>>> or disable vlans unconditionally. >>>>> >>>> The problem with disabling vlans unconditionally is that you break -net >>>> socket and -net dump. >>>> >>>> If we can come up with an alternative way to do these things, I'm all >>>> for removing it. >>> Hmm, I'll try to look at supporting -net socket in netdev. >>> Does -net dump do anything that can't be done with tap+tcpdump? >> >> tap+tcpdump requires root privileges (even if you have a tap helper). >> >> Plus tcpdump doesn't help with slirp and -net dump is very useful for >> debugging slirp. Developer's need for root access for debugging seems a reasonable price to pay to prevent user confusion and complexity that we have now. > > Of course, you could add this functionality to netdev. It's arguably > better there too because then you can debug virtio-net+tap with full > offload enabled (which you cannot do today). > > Regards, > > Anthony Liguori Care taking on it? I never even heard about -net dump before today.
On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: > > With -netdev, there now seems to be little need to support vlans, > > enabling them leads to user confusion and bad performance. > > Disable support for vlans by default, add config option to enable. > > No. If you want to remove vlans, then actually do that. > As I've said before if you want a point-point network model then you should > implement that (and remove the vlan code, probably replacing with equivalent > functionality). We should not have both point-point and broadcast interfaces. > > Paul By the way I agree with this. It would have been better to simply prohibit more than 2 devices on a vlan instead of two competing ways to set up networking that we have now. And the conclusion I think should be to remove broadcast support ASAP.
On 06/08/2010 06:09 AM, Michael S. Tsirkin wrote: > On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: > >> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >> >>> So I see two ways to go forward: switch default value in my patch, >>> or disable vlans unconditionally. >>> >>> >> The problem with disabling vlans unconditionally is that you break -net >> socket and -net dump. >> > -netdev socket seems to be supported. No? > Sure, but it's of limited utility in the absence of vlans. A typical thing to do with -net socket would be to launch one instance of qemu with -net user, -net socket, and -net nic. Another qemu would be launched with -net socket and -net nic connected to the previous instance. Now you've got a working virtual network with external access. -netdev socket alone won't get you this. Regards, Anthony Liguori
On 06/08/2010 07:13 AM, Michael S. Tsirkin wrote: > On Mon, Jun 07, 2010 at 04:57:09PM -0500, Anthony Liguori wrote: > >> On 06/07/2010 04:37 PM, Anthony Liguori wrote: >> >>> On 06/07/2010 03:58 PM, Michael S. Tsirkin wrote: >>> >>>> On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: >>>> >>>>> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >>>>> >>>>>> So I see two ways to go forward: switch default value in my patch, >>>>>> or disable vlans unconditionally. >>>>>> >>>>>> >>>>> The problem with disabling vlans unconditionally is that you break -net >>>>> socket and -net dump. >>>>> >>>>> If we can come up with an alternative way to do these things, I'm all >>>>> for removing it. >>>>> >>>> Hmm, I'll try to look at supporting -net socket in netdev. >>>> Does -net dump do anything that can't be done with tap+tcpdump? >>>> >>> tap+tcpdump requires root privileges (even if you have a tap helper). >>> >>> Plus tcpdump doesn't help with slirp and -net dump is very useful for >>> debugging slirp. >>> > Developer's need for root access for debugging seems a reasonable price to > pay to prevent user confusion and complexity that we have now. > Removing vlans has the potential to break existing deployments. That's something we need to do very cautiously. What do we gain from removing vlan support today? Regards, Anthony Liguori >> Of course, you could add this functionality to netdev. It's arguably >> better there too because then you can debug virtio-net+tap with full >> offload enabled (which you cannot do today). >> >> Regards, >> >> Anthony Liguori >> > Care taking on it? I never even heard about -net dump before today. > >
On Tue, Jun 08, 2010 at 08:03:33AM -0500, Anthony Liguori wrote: > On 06/08/2010 07:13 AM, Michael S. Tsirkin wrote: >> On Mon, Jun 07, 2010 at 04:57:09PM -0500, Anthony Liguori wrote: >> >>> On 06/07/2010 04:37 PM, Anthony Liguori wrote: >>> >>>> On 06/07/2010 03:58 PM, Michael S. Tsirkin wrote: >>>> >>>>> On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: >>>>> >>>>>> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >>>>>> >>>>>>> So I see two ways to go forward: switch default value in my patch, >>>>>>> or disable vlans unconditionally. >>>>>>> >>>>>>> >>>>>> The problem with disabling vlans unconditionally is that you break -net >>>>>> socket and -net dump. >>>>>> >>>>>> If we can come up with an alternative way to do these things, I'm all >>>>>> for removing it. >>>>>> >>>>> Hmm, I'll try to look at supporting -net socket in netdev. >>>>> Does -net dump do anything that can't be done with tap+tcpdump? >>>>> >>>> tap+tcpdump requires root privileges (even if you have a tap helper). >>>> >>>> Plus tcpdump doesn't help with slirp and -net dump is very useful for >>>> debugging slirp. >>>> >> Developer's need for root access for debugging seems a reasonable price to >> pay to prevent user confusion and complexity that we have now. >> > > Removing vlans has the potential to break existing deployments. That's > something we need to do very cautiously. You mean you want to support both peer to peer and broadcast indefinitely? > What do we gain from removing vlan support today? > > Regards, > > Anthony Liguori This would address the following issues: - people configure vlans and get bad performance - people run --help and see info on vlans instead of peer - migration issues betwen vlan/non-vlan - hotplug is broken for vlan nics >>> Of course, you could add this functionality to netdev. It's arguably >>> better there too because then you can debug virtio-net+tap with full >>> offload enabled (which you cannot do today). >>> >>> Regards, >>> >>> Anthony Liguori >>> >> Care taking on it? I never even heard about -net dump before today. >> >>
On Tue, Jun 08, 2010 at 08:02:27AM -0500, Anthony Liguori wrote: > On 06/08/2010 06:09 AM, Michael S. Tsirkin wrote: >> On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: >> >>> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >>> >>>> So I see two ways to go forward: switch default value in my patch, >>>> or disable vlans unconditionally. >>>> >>>> >>> The problem with disabling vlans unconditionally is that you break -net >>> socket and -net dump. >>> >> -netdev socket seems to be supported. No? >> > > Sure, but it's of limited utility in the absence of vlans. A typical > thing to do with -net socket would be to launch one instance of qemu > with -net user, -net socket, and -net nic. Another qemu would be > launched with -net socket and -net nic connected to the previous > instance. Now you've got a working virtual network with external access. Only terribly slow, esp when tunneling tcp over tcp. > -netdev socket alone won't get you this. -socket should really support udp. Then you could get what you wanted with halfway decent speed using multicast. > Regards, > > Anthony Liguori
On 06/08/10 15:02, Anthony Liguori wrote: > On 06/08/2010 06:09 AM, Michael S. Tsirkin wrote: >> On Mon, Jun 07, 2010 at 03:40:57PM -0500, Anthony Liguori wrote: >>> On 06/07/2010 02:21 PM, Michael S. Tsirkin wrote: >>>> So I see two ways to go forward: switch default value in my patch, >>>> or disable vlans unconditionally. >>>> >>> The problem with disabling vlans unconditionally is that you break -net >>> socket and -net dump. >> -netdev socket seems to be supported. No? > > Sure, but it's of limited utility in the absence of vlans. A typical > thing to do with -net socket would be to launch one instance of qemu > with -net user, -net socket, and -net nic. Another qemu would be > launched with -net socket and -net nic connected to the previous > instance. Now you've got a working virtual network with external access. > > -netdev socket alone won't get you this. I see three possible options to handle this. (1) Write a hub (or morph the current vlan code into this). Then you can do something like: qemu -netdev socket,id=p1 \ -netdev user,id=p2 \ -netdev dump,id=p3 \ -switch peer=p1,peer=p2,monitor=p3,port=p4 \ -device $nic,netdev=p4 (2) Implement the same as external daemon which can be combined with -netdev socket. (3) Just point people who need this to the various virtual switch projects (vde, ...) they can use and drop vlan. cheers, Gerd
> I see three possible options to handle this. > > (1) Write a hub (or morph the current vlan code into this). Then > you can do something like: > > qemu -netdev socket,id=p1 \ > -netdev user,id=p2 \ > -netdev dump,id=p3 \ > -switch peer=p1,peer=p2,monitor=p3,port=p4 \ > -device $nic,netdev=p4 > > (2) Implement the same as external daemon which can be combined with > -netdev socket. > > (3) Just point people who need this to the various virtual switch > projects (vde, ...) they can use and drop vlan. (2) is just a special case of (3), where we decide that the existing implementations suck and go write our own. Paul
On 06/08/2010 09:37 AM, Paul Brook wrote: >> I see three possible options to handle this. >> >> (1) Write a hub (or morph the current vlan code into this). Then >> you can do something like: >> >> qemu -netdev socket,id=p1 \ >> -netdev user,id=p2 \ >> -netdev dump,id=p3 \ >> -switch peer=p1,peer=p2,monitor=p3,port=p4 \ >> -device $nic,netdev=p4 >> >> (2) Implement the same as external daemon which can be combined with >> -netdev socket. >> >> (3) Just point people who need this to the various virtual switch >> projects (vde, ...) they can use and drop vlan. >> > (2) is just a special case of (3), where we decide that the existing > implementations suck and go write our own. > To the extent that (1) is valuable, I think it's the best approach. I'd vote for officially deprecating vlans for 0.13 and then seeing how much people complain. If no one complains too much, then let's not bother introducing -switch. Regards, Anthony Liguori > Paul >
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Mon, Jun 07, 2010 at 05:42:55PM +0100, Paul Brook wrote: >> > On Mon, Jun 07, 2010 at 05:16:30PM +0100, Paul Brook wrote: >> > > > With -netdev, there now seems to be little need to support vlans, >> > > > enabling them leads to user confusion and bad performance. >> > > > Disable support for vlans by default, add config option to enable. >> > > >> > > No. If you want to remove vlans, then actually do that. >> > >> > How is this not what this patch does? You mean kill the code >> > completely, not just --contigure option? >> >> Yes. Configure options are bad. If code isn't worth enabling by default then >> you've got to have a very good reason why it exists at all. >> >> Paul > > Everyone ok with disabling vlans with no config option? Wrong question. You got to ask "anyone *not* ok with disabling vlans with no config option?"
Markus Armbruster <armbru@redhat.com> writes: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > Everyone ok with disabling vlans with no config option? > > Wrong question. You got to ask "anyone *not* ok with disabling vlans > with no config option?" We do use socket devices in the form -net nic,model=e1000,vlan=X,mac=MMM -net socket,vlan=X,mcast=Y:Z but presumably this can just be rewritten as -netdev socket,id=netX,mcast=Y:Z -device e1000,netdev=netX,mac=MMM It's only the case of a nic connected to multiple backends by a VLAN that's being deprecated here, not any of the previously supported backends? Cheers, Chris.
On Thu, Jun 10, 2010 at 08:20:56AM +0100, Chris Webb wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > Everyone ok with disabling vlans with no config option? > > > > Wrong question. You got to ask "anyone *not* ok with disabling vlans > > with no config option?" > > We do use socket devices in the form > > -net nic,model=e1000,vlan=X,mac=MMM -net socket,vlan=X,mcast=Y:Z > > but presumably this can just be rewritten as > > -netdev socket,id=netX,mcast=Y:Z -device e1000,netdev=netX,mac=MMM > > It's only the case of a nic connected to multiple backends by a VLAN that's > being deprecated here, not any of the previously supported backends? > > Cheers, > > Chris. Exactly.
diff --git a/configure b/configure index 3cd2c5f..728d9a1 100755 --- a/configure +++ b/configure @@ -299,6 +299,7 @@ pkgversion="" check_utests="no" user_pie="no" zero_malloc="" +vlans="no" # OS specific if check_define __linux__ ; then @@ -660,6 +661,10 @@ for opt do ;; --enable-vhost-net) vhost_net="yes" ;; + --disable-vlans) vlans="no" + ;; + --enable-vlans) vlans="yes" + ;; *) echo "ERROR: unknown option $opt"; show_help="yes" ;; esac @@ -826,6 +831,8 @@ echo " --enable-docs enable documentation build" echo " --disable-docs disable documentation build" echo " --disable-vhost-net disable vhost-net acceleration support" echo " --enable-vhost-net enable vhost-net acceleration support" +echo " --disable-vlans disable legacy qemu vlan support" +echo " --enable-vlans enable legacy qemu vlan support" echo "" echo "NOTE: The object files are built at the place where configure is launched" exit 1 @@ -2041,6 +2048,7 @@ echo "preadv support $preadv" echo "fdatasync $fdatasync" echo "uuid support $uuid" echo "vhost-net support $vhost_net" +echo "vlans support $vlans" if test $sdl_too_old = "yes"; then echo "-> Your SDL version is too old - please upgrade to have SDL support" @@ -2284,6 +2292,10 @@ bsd) ;; esac +if test $vlans = "yes" ; then + echo "CONFIG_VLANS=y" >> $config_target_mak +fi + tools= if test `expr "$target_list" : ".*softmmu.*"` != 0 ; then tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) $tools" diff --git a/net.c b/net.c index 378edfc..a6af5f7 100644 --- a/net.c +++ b/net.c @@ -1070,6 +1070,13 @@ int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev) return -1; } +#ifndef CONFIG_VLANS + if (!is_netdev) { + qerror_report(QERR_INVALID_PARAMETER_VALUE, "netdev", "netdev id"); + return -1; + } +#endif + if (is_netdev) { if (strcmp(type, "tap") != 0 && #ifdef CONFIG_SLIRP
With -netdev, there now seems to be little need to support vlans, enabling them leads to user confusion and bad performance. Disable support for vlans by default, add config option to enable. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- configure | 12 ++++++++++++ net.c | 7 +++++++ 2 files changed, 19 insertions(+), 0 deletions(-)