[PATCH net-next 0/2] bonding: add its own Kconfig

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 <redacted>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: netdev@vger.kernel.org
Signed-off-by: Veaceslav Falico <redacted>

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

CC: Jay Vosburgh <redacted>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <redacted>
---
 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

CC: Jay Vosburgh <redacted>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <redacted>
---
 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

On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico [off-list ref] wrote:

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.

On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote:

On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico [off-list ref] wrote:

quoted

CC: Jay Vosburgh <redacted>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <redacted>
---
 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.

quoted

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

On Tue, Jul 15, 2014 at 9:18 AM, Veaceslav Falico [off-list ref] wrote:

On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote:

quoted

On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico [off-list ref]
wrote:

quoted

+
+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?

Hello.

On 07/15/2014 02:15 PM, Veaceslav Falico wrote:

CC: Jay Vosburgh <redacted>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <redacted>
---
  drivers/net/bonding/Kconfig   | 16 ++++++++++++++++
  drivers/net/bonding/bonding.h |  6 +++---
  2 files changed, 19 insertions(+), 3 deletions(-)

    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

On Tue, Jul 15, 2014 at 09:45:36AM -0700, Alexei Starovoitov wrote:

On Tue, Jul 15, 2014 at 9:18 AM, Veaceslav Falico [off-list ref] wrote:

quoted

On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote:

quoted

On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico [off-list ref]
wrote:

quoted

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

quoted

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.

On Tue, Jul 15, 2014 at 10:11 AM, Veaceslav Falico [off-list ref] wrote:

On Tue, Jul 15, 2014 at 09:45:36AM -0700, Alexei Starovoitov wrote:

quoted

On Tue, Jul 15, 2014 at 9:18 AM, Veaceslav Falico [off-list ref]
wrote:

quoted

On Tue, Jul 15, 2014 at 08:48:01AM -0700, Alexei Starovoitov wrote:

quoted

On Tue, Jul 15, 2014 at 3:15 AM, Veaceslav Falico [off-list ref]
wrote:

quoted

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

quoted

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.

quoted

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.

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.

quoted

quoted

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.

quoted

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.

quoted

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.

On Tue, Jul 15, 2014 at 10:53 AM, Veaceslav Falico [off-list ref] wrote:

On Tue, Jul 15, 2014 at 10:33:25AM -0700, Alexei Starovoitov wrote:
..snip...

quoted

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.

On Tue, Jul 15, 2014 at 11:55:29AM -0700, Alexei Starovoitov wrote:

On Tue, Jul 15, 2014 at 10:53 AM, Veaceslav Falico [off-list ref] wrote:

quoted

On Tue, Jul 15, 2014 at 10:33:25AM -0700, Alexei Starovoitov wrote:
..snip...

quoted

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.

quoted

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.

From: Veaceslav Falico <redacted>
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".

From: Veaceslav Falico <redacted>
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.

On Tue, Jul 15, 2014 at 06:10:56PM -0700, David Miller wrote:

From: Veaceslav Falico <redacted>
Date: Tue, 15 Jul 2014 12:15:39 +0200

quoted

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!