| Message ID | 1405419341-31510-3-git-send-email-vfalico@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico <vfalico@gmail.com> wrote: > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> > --- > drivers/net/bonding/Kconfig | 16 ++++++++++++++++ > drivers/net/bonding/bonding.h | 6 +++--- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/bonding/Kconfig b/drivers/net/bonding/Kconfig > index 7b1a0fa..28fb3af 100644 > --- a/drivers/net/bonding/Kconfig > +++ b/drivers/net/bonding/Kconfig > @@ -15,3 +15,19 @@ menuconfig BONDING > > To compile this driver as a module, choose M here: the module > will be called bonding. > + > +if BONDING > + > +config BOND_MAX_VLAN_ENCAP > + int "Maximum number of stacked vlans on top of bonding" > + default "2" > + I don't think we should allow changing these defaults so easily. Not a single HW supports 3 vlan tags. There is no standard for it either. Why you would ever change this? > +config BOND_MAX_ARP_TARGETS > + int "Maximum number of ip targets arp monitor supports" > + default "16" > + > +config BOND_DEFAULT_MIIMON > + int "Default miimon monitoring frequency in milliseconds" > + default "100" or this? imo existing built-ins are fine and adding extra knobs just scary. Users will have different numbers here and somebody would need to make sure that all different defaults still work. We should be minimizing the number of knobs instead. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote: >On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico <vfalico@gmail.com> wrote: >> CC: Jay Vosburgh <j.vosburgh@gmail.com> >> CC: Andy Gospodarek <andy@greyhouse.net> >> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >> --- >> drivers/net/bonding/Kconfig | 16 ++++++++++++++++ >> drivers/net/bonding/bonding.h | 6 +++--- >> 2 files changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/bonding/Kconfig b/drivers/net/bonding/Kconfig >> index 7b1a0fa..28fb3af 100644 >> --- a/drivers/net/bonding/Kconfig >> +++ b/drivers/net/bonding/Kconfig >> @@ -15,3 +15,19 @@ menuconfig BONDING >> >> To compile this driver as a module, choose M here: the module >> will be called bonding. >> + >> +if BONDING >> + >> +config BOND_MAX_VLAN_ENCAP >> + int "Maximum number of stacked vlans on top of bonding" >> + default "2" >> + > >I don't think we should allow changing these defaults so easily. >Not a single HW supports 3 vlan tags. There is no standard for it either. >Why you would ever change this? There have been discussions about vlan nestings for bonding, and the outcome was that more than 2 are possible. Also, iirc, no standard limits it to only 2. I'm not sure about the "changing these defaults so easily" - this default limits the depth at which we traverse the vlan tree when sending packets in arp monitor, so it has no actual impact on the work of vlans themselves. > >> +config BOND_MAX_ARP_TARGETS >> + int "Maximum number of ip targets arp monitor supports" >> + default "16" >> + >> +config BOND_DEFAULT_MIIMON >> + int "Default miimon monitoring frequency in milliseconds" >> + default "100" > >or this? This is the default value chosen when there's no configuration/illegal configuration, so it's a good point to configure it at build. >imo existing built-ins are fine and adding extra knobs just scary. >Users will have different numbers here and somebody would >need to make sure that all different defaults still work. >We should be minimizing the number of knobs instead. These defaults are scalable by their nature, and there are people maintaining their own patches to change them. So making them available to be configured at compile time is a good thing to do, I think. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 9:18 AM, Veaceslav Falico <vfalico@gmail.com> wrote: > On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote: >> >> On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico <vfalico@gmail.com> >> wrote: >>> >>> + >>> +config BOND_MAX_VLAN_ENCAP >>> + int "Maximum number of stacked vlans on top of bonding" >>> + default "2" >>> + >> >> I don't think we should allow changing these defaults so easily. >> Not a single HW supports 3 vlan tags. There is no standard for it either. >> Why you would ever change this? > > There have been discussions about vlan nestings for bonding, and the > outcome was that more than 2 are possible. Also, iirc, no standard limits > it to only 2. standard doesn't say that the maximum is 2, but it doesn't specify what should be done in such case, so all vlan-aware switches that I know will be just dropping packets with 3 vlans. Therefore for bond driver there is no reason to accept such packets in the first place from user space, since they won't go too far in the network. > These defaults are scalable by their nature, and there are people > maintaining their own patches to change them. So making them available to > be configured at compile time is a good thing to do, I think. people keep a patch to support 3 vlans? what's the use case? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 09:45:36AM -0700, Alexei Starovoitov wrote: >On Tue, Jul 15, 2014 at 9:18 AM, Veaceslav Falico <vfalico@gmail.com> wrote: >> On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote: >>> >>> On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico <vfalico@gmail.com> >>> wrote: >>>> >>>> + >>>> +config BOND_MAX_VLAN_ENCAP >>>> + int "Maximum number of stacked vlans on top of bonding" >>>> + default "2" >>>> + >>> >>> I don't think we should allow changing these defaults so easily. >>> Not a single HW supports 3 vlan tags. There is no standard for it either. >>> Why you would ever change this? >> >> There have been discussions about vlan nestings for bonding, and the >> outcome was that more than 2 are possible. Also, iirc, no standard limits >> it to only 2. > >standard doesn't say that the maximum is 2, but it doesn't specify what >should be done in such case, so all vlan-aware switches that I know will >be just dropping packets with 3 vlans. There might be switches that support it/don't really care, there are bridges (including soft ones) etc. that all can make use of it. Given that it's a build-time configuration option that affects a small part of arp monitoring, which is a small part of bonding module, which is ... - I don't understand why it can't be configured. >Therefore for bond driver there is no reason to accept such packets >in the first place from user space, since they won't go too far in the network. > >> These defaults are scalable by their nature, and there are people >> maintaining their own patches to change them. So making them available to >> be configured at compile time is a good thing to do, I think. > >people keep a patch to support 3 vlans? what's the use case? I guess it has something to do with virtualization, including nested one. But, again, does this matter? I don't see how it can give us something bad. It's a configuration option with a default value that suits most users, and that might be configured for some advanced/weird use-cases. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 07/15/2014 02:15 PM, Veaceslav Falico wrote: > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> > --- > drivers/net/bonding/Kconfig | 16 ++++++++++++++++ > drivers/net/bonding/bonding.h | 6 +++--- > 2 files changed, 19 insertions(+), 3 deletions(-) > diff --git a/drivers/net/bonding/Kconfig b/drivers/net/bonding/Kconfig > index 7b1a0fa..28fb3af 100644 > --- a/drivers/net/bonding/Kconfig > +++ b/drivers/net/bonding/Kconfig > @@ -15,3 +15,19 @@ menuconfig BONDING > > To compile this driver as a module, choose M here: the module > will be called bonding. > + > +if BONDING > + > +config BOND_MAX_VLAN_ENCAP > + int "Maximum number of stacked vlans on top of bonding" s/vlans/VLANs/? > + default "2" > + > +config BOND_MAX_ARP_TARGETS > + int "Maximum number of ip targets arp monitor supports" s/ip/IP/, s/arp/ARP/? WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 10:11 AM, Veaceslav Falico <vfalico@gmail.com> wrote: > On Tue, Jul 15, 2014 at 09:45:36AM -0700, Alexei Starovoitov wrote: >> >> On Tue, Jul 15, 2014 at 9:18 AM, Veaceslav Falico <vfalico@gmail.com> >> wrote: >>> >>> On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote: >>>> >>>> >>>> On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico <vfalico@gmail.com> >>>> wrote: >>>>> >>>>> >>>>> + >>>>> +config BOND_MAX_VLAN_ENCAP >>>>> + int "Maximum number of stacked vlans on top of bonding" >>>>> + default "2" >>>>> + >>>> >>>> >>>> I don't think we should allow changing these defaults so easily. >>>> Not a single HW supports 3 vlan tags. There is no standard for it >>>> either. >>>> Why you would ever change this? >>> >>> >>> There have been discussions about vlan nestings for bonding, and the >>> outcome was that more than 2 are possible. Also, iirc, no standard limits >>> it to only 2. >> >> >> standard doesn't say that the maximum is 2, but it doesn't specify what >> should be done in such case, so all vlan-aware switches that I know will >> be just dropping packets with 3 vlans. > > > There might be switches that support it/don't really care, there are > bridges (including soft ones) etc. that all can make use of it. > > Given that it's a build-time configuration option that affects a small part > of arp monitoring, which is a small part of bonding module, which is ... - > I don't understand why it can't be configured. For 3 vlan case to be useful, first somebody needs to define a meaning for it and real use case. I haven't seen one. Making bond driver support fictitious configuration is pointless. >> Therefore for bond driver there is no reason to accept such packets >> in the first place from user space, since they won't go too far in the >> network. >> >>> These defaults are scalable by their nature, and there are people >>> maintaining their own patches to change them. So making them available to >>> be configured at compile time is a good thing to do, I think. >> >> >> people keep a patch to support 3 vlans? what's the use case? > > > I guess it has something to do with virtualization, including nested one. sounds like you're inventing a use case instead of having it already. imo we shouldn't be adding features because it _feels_ useful. use cases gotta be real. > But, again, does this matter? I don't see how it can give us something bad. > It's a configuration option with a default value that suits most users, and > that might be configured for some advanced/weird use-cases. it's bad, since it increases test matrix. You said there are people out there that have some secret patches to tweak these defaults. Please share. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 10:33:25AM -0700, Alexei Starovoitov wrote: ..snip... >For 3 vlan case to be useful, first somebody needs to define a meaning >for it and real use case. I haven't seen one. You also haven't seen switches that support it, however juniper switches do support them, as a quick google shows. I can guess that cisco can also be made to support them. You'd be surprised which weird bonding configurations I've seen :). >Making bond driver support fictitious configuration is pointless. bond driver *already* supports it, by changing a hardcoded value. > >>> Therefore for bond driver there is no reason to accept such packets >>> in the first place from user space, since they won't go too far in the >>> network. >>> >>>> These defaults are scalable by their nature, and there are people >>>> maintaining their own patches to change them. So making them available to >>>> be configured at compile time is a good thing to do, I think. >>> >>> >>> people keep a patch to support 3 vlans? what's the use case? >> >> >> I guess it has something to do with virtualization, including nested one. > >sounds like you're inventing a use case instead of having it already. >imo we shouldn't be adding features because it _feels_ useful. >use cases gotta be real. I've got a report asking to make those hardcoded values configurable. I've never said it was specific to 3 vlans, it was your assumption. I've only tried to guess one possible way of using it. > >> But, again, does this matter? I don't see how it can give us something bad. >> It's a configuration option with a default value that suits most users, and >> that might be configured for some advanced/weird use-cases. > >it's bad, since it increases test matrix. Fair enough, and by this way we'll find bugs that otherwise wouldn't be found because of hardcoded values. That's good for bonding, actually, and for the networking stack itself. >You said there are people out there that have some secret patches >to tweak these defaults. Please share. I've got a request to make those hardcoded values configurable, as people tweak them. I don't really care how exactly they tweak them - by using more arp targets, or less, by tweaking mii default to not failover on first start if the NICs firmware wasn't fast enough, by using some weird QinQinQ configurations etc. - because these are values that any power-user can understand and use, and thus they should be made available to configure without getting into the code. I'll happily drop any/all of these configs if I'd see a reason NOT to add them. Till now I haven't seen anything except "I don't know why should I use it", and that's not a valid reason to me, sorry. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 10:53 AM, Veaceslav Falico <vfalico@gmail.com> wrote: > On Tue, Jul 15, 2014 at 10:33:25AM -0700, Alexei Starovoitov wrote: > ..snip... > >> For 3 vlan case to be useful, first somebody needs to define a meaning >> for it and real use case. I haven't seen one. > > > You also haven't seen switches that support it, however juniper switches do > support them, as a quick google shows. I can guess that cisco can also be > made to support them. I don't think juniper switches support them either. Please send the link to the spec. Protocol parsers in HW fail to parse beyond two, since behavior is undefined and it's considered invalid packet. I'm talking about broadcom/intel/cisco asics. Generally speaking I think it's a mistake of linux stack to support any nested level. Not too long ago we saw issues with broken hw offloads for basic qinq. I won't be surprised if creating triple stacked vlan devices actually breaks all sort of things. > I'll happily drop any/all of these configs if I'd see a reason NOT to add > them. Till now I haven't seen anything except "I don't know why should I > use it", and that's not a valid reason to me, sorry. correct. I don't see a good reason to change these defaults and you also seem to acknowledge the same. To me it's a red flag when somebody requests a feature without proper justification. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 15, 2014 at 11:55:29AM -0700, Alexei Starovoitov wrote: >On Tue, Jul 15, 2014 at 10:53 AM, Veaceslav Falico <vfalico@gmail.com> wrote: >> On Tue, Jul 15, 2014 at 10:33:25AM -0700, Alexei Starovoitov wrote: >> ..snip... >> >>> For 3 vlan case to be useful, first somebody needs to define a meaning >>> for it and real use case. I haven't seen one. >> >> >> You also haven't seen switches that support it, however juniper switches do >> support them, as a quick google shows. I can guess that cisco can also be >> made to support them. > >I don't think juniper switches support them either. >Please send the link to the spec. No spec, but a reply from a juniper employee, as it states: http://forums.juniper.net/t5/Ethernet-Switching/Is-it-possible-to-do-QinQinQ/td-p/29409 Also, it's a valid use case, described there. >Protocol parsers in HW fail to parse beyond two, since behavior >is undefined and it's considered invalid packet. >I'm talking about broadcom/intel/cisco asics. You're mentioning it for the 3rd or 4th time already. Can you please provide a proof? It'd be interesting to read, as I see over the net that people are managing to do triple vlan tagging on cisco. Also, could you explain what do you mean by "undefined behaviour"? I can't really understand what can be undefined in 3 tags. >Generally speaking I think it's a mistake of linux stack to support >any nested level. Not too long ago we saw issues with broken hw >offloads for basic qinq. I won't be surprised if creating triple stacked >vlan devices actually breaks all sort of things. That's not the problem with any nested vlan level, but with hw offload. I guess it's the source/consiquencies thing. > >> I'll happily drop any/all of these configs if I'd see a reason NOT to add >> them. Till now I haven't seen anything except "I don't know why should I >> use it", and that's not a valid reason to me, sorry. > >correct. I don't see a good reason to change these defaults and you also >seem to acknowledge the same. You're putting words in my mouth for the second time already. I didn't say that I don't see a good reason to change the defaults, I've said that I don't see a good reason to drop this enhancement only because someone doesn't know what to use it for. >To me it's a red flag when somebody >requests a feature without proper justification. The feature is there already. It's just a configuration to enable this feature without changing one define. And, also, the any-level nesting in linux stack *is* already present, and bonding just tries to stay up-to-date with that. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Veaceslav Falico <vfalico@gmail.com> Date: Tue, 15 Jul 2014 21:18:11 +0200 > No spec, but a reply from a juniper employee, as it states: > > http://forums.juniper.net/t5/Ethernet-Switching/Is-it-possible-to-do-QinQinQ/td-p/29409 I will note that this engineer uses the word "should". -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/Kconfig b/drivers/net/bonding/Kconfig index 7b1a0fa..28fb3af 100644 --- a/drivers/net/bonding/Kconfig +++ b/drivers/net/bonding/Kconfig @@ -15,3 +15,19 @@ menuconfig BONDING To compile this driver as a module, choose M here: the module will be called bonding. + +if BONDING + +config BOND_MAX_VLAN_ENCAP + int "Maximum number of stacked vlans on top of bonding" + default "2" + +config BOND_MAX_ARP_TARGETS + int "Maximum number of ip targets arp monitor supports" + default "16" + +config BOND_DEFAULT_MIIMON + int "Default miimon monitoring frequency in milliseconds" + default "100" + +endif diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 0b4d9cd..119ce68 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -36,10 +36,10 @@ #define bond_version DRV_DESCRIPTION ": v" DRV_VERSION " (" DRV_RELDATE ")\n" -#define BOND_MAX_VLAN_ENCAP 2 -#define BOND_MAX_ARP_TARGETS 16 +#define BOND_MAX_VLAN_ENCAP CONFIG_BOND_MAX_VLAN_ENCAP +#define BOND_MAX_ARP_TARGETS CONFIG_BOND_MAX_ARP_TARGETS -#define BOND_DEFAULT_MIIMON 100 +#define BOND_DEFAULT_MIIMON CONFIG_BOND_DEFAULT_MIIMON /* * Less bad way to call ioctl from within the kernel; this needs to be
CC: Jay Vosburgh <j.vosburgh@gmail.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> --- drivers/net/bonding/Kconfig | 16 ++++++++++++++++ drivers/net/bonding/bonding.h | 6 +++--- 2 files changed, 19 insertions(+), 3 deletions(-)