Re: [PATCH net-next 2/2] bonding: make hard-coded defines configurable at build

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.