netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/2] bonding: add its own Kconfig
@ 2014-07-15 10:15 Veaceslav Falico
  2014-07-15 10:15 ` [PATCH net-next 1/2] bonding: create " Veaceslav Falico
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 10:15 UTC (permalink / raw)
  To: netdev; +Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Veaceslav Falico

Currently to change some built-in defaults it's necessary to change the
code, and to maintain these patches on each update.

Create a bonding's Kconfig to make it possible to configure them at build
time.

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: netdev@vger.kernel.org
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>

---
 drivers/net/Kconfig           | 18 +-----------------
 drivers/net/bonding/Kconfig   | 33 +++++++++++++++++++++++++++++++++
 drivers/net/bonding/bonding.h |  6 +++---
 3 files changed, 37 insertions(+), 20 deletions(-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH net-next 1/2] bonding: create its own Kconfig
  2014-07-15 10:15 [PATCH net-next 0/2] bonding: add its own Kconfig Veaceslav Falico
@ 2014-07-15 10:15 ` Veaceslav Falico
  2014-07-15 10:15 ` [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build Veaceslav Falico
  2014-07-16  1:10 ` [PATCH net-next 0/2] bonding: add its own Kconfig David Miller
  2 siblings, 0 replies; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 10:15 UTC (permalink / raw)
  To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek, David S. Miller

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>
---
 drivers/net/Kconfig         | 18 +-----------------
 drivers/net/bonding/Kconfig | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 17 deletions(-)
 create mode 100644 drivers/net/bonding/Kconfig

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index c6f6f69..16b6b9c 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -37,23 +37,7 @@ config NET_CORE
 
 if NET_CORE
 
-config BONDING
-	tristate "Bonding driver support"
-	depends on INET
-	depends on IPV6 || IPV6=n
-	---help---
-	  Say 'Y' or 'M' if you wish to be able to 'bond' multiple Ethernet
-	  Channels together. This is called 'Etherchannel' by Cisco,
-	  'Trunking' by Sun, 802.3ad by the IEEE, and 'Bonding' in Linux.
-
-	  The driver supports multiple bonding modes to allow for both high
-	  performance and high availability operation.
-
-	  Refer to <file:Documentation/networking/bonding.txt> for more
-	  information.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called bonding.
+source "drivers/net/bonding/Kconfig"
 
 config DUMMY
 	tristate "Dummy net driver support"
diff --git a/drivers/net/bonding/Kconfig b/drivers/net/bonding/Kconfig
new file mode 100644
index 0000000..7b1a0fa
--- /dev/null
+++ b/drivers/net/bonding/Kconfig
@@ -0,0 +1,17 @@
+menuconfig BONDING
+	tristate "Bonding driver support"
+	depends on INET
+	depends on IPV6 || IPV6=n
+	---help---
+	  Say 'Y' or 'M' if you wish to be able to 'bond' multiple Ethernet
+	  Channels together. This is called 'Etherchannel' by Cisco,
+	  'Trunking' by Sun, 802.3ad by the IEEE, and 'Bonding' in Linux.
+
+	  The driver supports multiple bonding modes to allow for both high
+	  performance and high availability operation.
+
+	  Refer to <file:Documentation/networking/bonding.txt> for more
+	  information.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called bonding.
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 10:15 [PATCH net-next 0/2] bonding: add its own Kconfig Veaceslav Falico
  2014-07-15 10:15 ` [PATCH net-next 1/2] bonding: create " Veaceslav Falico
@ 2014-07-15 10:15 ` Veaceslav Falico
  2014-07-15 15:48   ` Alexei Starovoitov
  2014-07-15 17:11   ` Sergei Shtylyov
  2014-07-16  1:10 ` [PATCH net-next 0/2] bonding: add its own Kconfig David Miller
  2 siblings, 2 replies; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 10:15 UTC (permalink / raw)
  To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek

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"
+
+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
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 10:15 ` [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build Veaceslav Falico
@ 2014-07-15 15:48   ` Alexei Starovoitov
  2014-07-15 16:18     ` Veaceslav Falico
  2014-07-15 17:11   ` Sergei Shtylyov
  1 sibling, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2014-07-15 15:48 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 15:48   ` Alexei Starovoitov
@ 2014-07-15 16:18     ` Veaceslav Falico
  2014-07-15 16:45       ` Alexei Starovoitov
  0 siblings, 1 reply; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 16:18 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 16:18     ` Veaceslav Falico
@ 2014-07-15 16:45       ` Alexei Starovoitov
  2014-07-15 17:11         ` Veaceslav Falico
  0 siblings, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2014-07-15 16:45 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 16:45       ` Alexei Starovoitov
@ 2014-07-15 17:11         ` Veaceslav Falico
  2014-07-15 17:33           ` Alexei Starovoitov
  0 siblings, 1 reply; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 17:11 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 10:15 ` [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build Veaceslav Falico
  2014-07-15 15:48   ` Alexei Starovoitov
@ 2014-07-15 17:11   ` Sergei Shtylyov
  1 sibling, 0 replies; 15+ messages in thread
From: Sergei Shtylyov @ 2014-07-15 17:11 UTC (permalink / raw)
  To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 17:11         ` Veaceslav Falico
@ 2014-07-15 17:33           ` Alexei Starovoitov
  2014-07-15 17:53             ` Veaceslav Falico
  0 siblings, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2014-07-15 17:33 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 17:33           ` Alexei Starovoitov
@ 2014-07-15 17:53             ` Veaceslav Falico
  2014-07-15 18:55               ` Alexei Starovoitov
  0 siblings, 1 reply; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 17:53 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 17:53             ` Veaceslav Falico
@ 2014-07-15 18:55               ` Alexei Starovoitov
  2014-07-15 19:18                 ` Veaceslav Falico
  0 siblings, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2014-07-15 18:55 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 18:55               ` Alexei Starovoitov
@ 2014-07-15 19:18                 ` Veaceslav Falico
  2014-07-16  1:01                   ` David Miller
  0 siblings, 1 reply; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-15 19:18 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: netdev, Jay Vosburgh, Andy Gospodarek

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build
  2014-07-15 19:18                 ` Veaceslav Falico
@ 2014-07-16  1:01                   ` David Miller
  0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2014-07-16  1:01 UTC (permalink / raw)
  To: vfalico; +Cc: alexei.starovoitov, netdev, j.vosburgh, andy

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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 0/2] bonding: add its own Kconfig
  2014-07-15 10:15 [PATCH net-next 0/2] bonding: add its own Kconfig Veaceslav Falico
  2014-07-15 10:15 ` [PATCH net-next 1/2] bonding: create " Veaceslav Falico
  2014-07-15 10:15 ` [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build Veaceslav Falico
@ 2014-07-16  1:10 ` David Miller
  2014-07-16  7:20   ` Veaceslav Falico
  2 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2014-07-16  1:10 UTC (permalink / raw)
  To: vfalico; +Cc: netdev, j.vosburgh, andy

From: Veaceslav Falico <vfalico@gmail.com>
Date: Tue, 15 Jul 2014 12:15:39 +0200

> Currently to change some built-in defaults it's necessary to change the
> code, and to maintain these patches on each update.
> 
> Create a bonding's Kconfig to make it possible to configure them at build
> time.

There is really no reason these values can't be set at run time, and that
is my only, but firm, objection to this patch series.

For the per-slave things, make new netlink attributes that can be provided
when the slave is added, using the existing default if not specified.

This means the fixed sized arrays involved here need to become
dynamically allocated.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next 0/2] bonding: add its own Kconfig
  2014-07-16  1:10 ` [PATCH net-next 0/2] bonding: add its own Kconfig David Miller
@ 2014-07-16  7:20   ` Veaceslav Falico
  0 siblings, 0 replies; 15+ messages in thread
From: Veaceslav Falico @ 2014-07-16  7:20 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, j.vosburgh, andy

On Tue, Jul 15, 2014 at 06:10:56PM -0700, David Miller wrote:
>From: Veaceslav Falico <vfalico@gmail.com>
>Date: Tue, 15 Jul 2014 12:15:39 +0200
>
>> Currently to change some built-in defaults it's necessary to change the
>> code, and to maintain these patches on each update.
>>
>> Create a bonding's Kconfig to make it possible to configure them at build
>> time.
>
>There is really no reason these values can't be set at run time, and that
>is my only, but firm, objection to this patch series.

Yeah, MIIMON default value doesn't really need to exist, vlan depth can be
configured dynamically ...

>
>For the per-slave things, make new netlink attributes that can be provided
>when the slave is added, using the existing default if not specified.
>
>This means the fixed sized arrays involved here need to become
>dynamically allocated.

... and arp ip targets can be also made into lists.

I'll try to change it and come up with split patchsets.

Thank you!

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-07-16  7:23 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-15 10:15 [PATCH net-next 0/2] bonding: add its own Kconfig Veaceslav Falico
2014-07-15 10:15 ` [PATCH net-next 1/2] bonding: create " Veaceslav Falico
2014-07-15 10:15 ` [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build Veaceslav Falico
2014-07-15 15:48   ` Alexei Starovoitov
2014-07-15 16:18     ` Veaceslav Falico
2014-07-15 16:45       ` Alexei Starovoitov
2014-07-15 17:11         ` Veaceslav Falico
2014-07-15 17:33           ` Alexei Starovoitov
2014-07-15 17:53             ` Veaceslav Falico
2014-07-15 18:55               ` Alexei Starovoitov
2014-07-15 19:18                 ` Veaceslav Falico
2014-07-16  1:01                   ` David Miller
2014-07-15 17:11   ` Sergei Shtylyov
2014-07-16  1:10 ` [PATCH net-next 0/2] bonding: add its own Kconfig David Miller
2014-07-16  7:20   ` Veaceslav Falico

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).