| Message ID | 20210826130533.149111-1-razor@blackwall.org |
|---|---|
| Headers | show |
| Series | bridge: vlan: add global multicast options | expand |
On 26/08/2021 18:08, Stephen Hemminger wrote: > On Thu, 26 Aug 2021 16:05:17 +0300 > Nikolay Aleksandrov <razor@blackwall.org> wrote: > >> + } else if (matches(*argv, "mcast_vlan_snooping") == 0) { >> + __u32 mcvl_bit = 1 << BR_BOOLOPT_MCAST_VLAN_SNOOPING; > > Using matches() is problematic. since it will change how 'mcast' is > handled. > > Overall, bridge command (and rest of iproute2) needs to move > away from matches > Sure, I can send a follow up if you don't mind to switch all matches calls. I used it to be in line with the current code.
On 8/26/21 8:11 AM, Nikolay Aleksandrov wrote: > On 26/08/2021 18:08, Stephen Hemminger wrote: >> On Thu, 26 Aug 2021 16:05:17 +0300 >> Nikolay Aleksandrov <razor@blackwall.org> wrote: >> >>> + } else if (matches(*argv, "mcast_vlan_snooping") == 0) { >>> + __u32 mcvl_bit = 1 << BR_BOOLOPT_MCAST_VLAN_SNOOPING; >> >> Using matches() is problematic. since it will change how 'mcast' is >> handled. >> >> Overall, bridge command (and rest of iproute2) needs to move >> away from matches >> > > Sure, I can send a follow up if you don't mind to switch all matches calls. I used it > to be in line with the current code. > existing options need to stay using matches(); new options need to use full strcmp().
Hi Nik, awesome to see this patchset! :-) I've begun setting things up here for testing. Just have a question about this: On Thu, Aug 26, 2021 at 16:05, Nikolay Aleksandrov <razor@blackwall.org> wrote: > Add control and dump support for the global mcast_igmp_version option > which controls the IGMP version on the vlan (default 2). Why is the default IGMPv2? Since we support IGMPv3, surely that should be the default, with fallback to IGMPv2 when we detect end devices that don't support v3? The snooping RFC refers back to the IGMPv3 RFC https://datatracker.ietf.org/doc/html/rfc3376#section-7 I noticed the default for MLD is also set to the older version v1, and I'm guessing there's a reasoning behind both that I haven't yet grasped. Best regards /Joachim
On 31/08/2021 12:02, Joachim Wiberg wrote: > > Hi Nik, > > awesome to see this patchset! :-) I've begun setting things up here > for testing. Just have a question about this: > > On Thu, Aug 26, 2021 at 16:05, Nikolay Aleksandrov <razor@blackwall.org> wrote: >> Add control and dump support for the global mcast_igmp_version option >> which controls the IGMP version on the vlan (default 2). > > Why is the default IGMPv2? Since we support IGMPv3, surely that should > be the default, with fallback to IGMPv2 when we detect end devices that > don't support v3? > > The snooping RFC refers back to the IGMPv3 RFC > > https://datatracker.ietf.org/doc/html/rfc3376#section-7 > > I noticed the default for MLD is also set to the older version v1, and > I'm guessing there's a reasoning behind both that I haven't yet grasped. > > Best regards > /Joachim > Hi, The reason is to be consistent with the bridge config. It already has IGMPv2 as default. I added IGMPv3 support much later and we couldn't change the default. I'd prefer not to surprise users and have different behaviour when they switch snooping from bridge-wide to per-vlan. It is a config option after all, distributions can choose to change their default when they create devices. :) Cheers, Nik
On 31/08/2021 12:04, Nikolay Aleksandrov wrote: > On 31/08/2021 12:02, Joachim Wiberg wrote: >> >> Hi Nik, >> >> awesome to see this patchset! :-) I've begun setting things up here >> for testing. Just have a question about this: >> >> On Thu, Aug 26, 2021 at 16:05, Nikolay Aleksandrov <razor@blackwall.org> wrote: >>> Add control and dump support for the global mcast_igmp_version option >>> which controls the IGMP version on the vlan (default 2). >> >> Why is the default IGMPv2? Since we support IGMPv3, surely that should >> be the default, with fallback to IGMPv2 when we detect end devices that >> don't support v3? >> >> The snooping RFC refers back to the IGMPv3 RFC >> >> https://datatracker.ietf.org/doc/html/rfc3376#section-7 >> >> I noticed the default for MLD is also set to the older version v1, and >> I'm guessing there's a reasoning behind both that I haven't yet grasped. >> >> Best regards >> /Joachim >> > > Hi, > The reason is to be consistent with the bridge config. It already has IGMPv2 as default. > I added IGMPv3 support much later and we couldn't change the default. > I'd prefer not to surprise users and have different behaviour when they switch snooping > from bridge-wide to per-vlan. It is a config option after all, distributions can choose > to change their default when they create devices. :) > > Cheers, > Nik > Forgot to mention - the RFC fallback and compatibility logic is not implemented yet, so if we set v3 we'll be stuck with it even if there are v2 participants. In fact it's a bit of a mess when mixing v2 and v3 right now, their interop needs more work.
From: Nikolay Aleksandrov <nikolay@nvidia.com> Hi all, This set adds support for vlan multicast options. The feature is globally controlled by a new bridge option called mcast_vlan_snooping which is added by patch 01. Then patches 2 and 3 add support for dumping global vlan options and filtering on vlan id. Patch 04 adds support for setting global vlan options and then patches 05-16 add all the new global vlan options, finally patch 17 adds support for dumping vlan multicast router ports. These options are identical in meaning, names and functionality as the bridge-wide ones. All the new vlan global commands are under the global keyword: $ bridge vlan global show [ vid VID dev DEVICE ] $ bridge vlan global set vid VID dev DEVICE ... I've added command examples in each commit message. The patch-set is a bit bigger but the global options follow the same pattern so I don't see a point in breaking them. All man page descriptions have been taken from the same current bridge-wide mcast options. The only additional iproute2 change which is left to do is the per-vlan mcast router control which I'll send separately. Note to properly use this set you'll need the updated kernel headers where mcast router was moved from a global option to per-vlan/per-device one (changed uapi enum which was in net-next). Example: # enable vlan mcast snooping globally $ ip link set dev bridge type bridge mcast_vlan_snooping 1 # enable mcast querier on vlan 100 $ bridge vlan global set dev bridge vid 100 mcast_querier 1 # show vlan 100's global options $ bridge -s vlan global show vid 100 port vlan-id bridge 100 mcast_snooping 1 mcast_querier 1 mcast_igmp_version 2 mcast_mld_version 1 mcast_last_member_count 2 mcast_last_member_interval 100 mcast_startup_query_count 2 mcast_startup_query_interval 3125 mcast_membership_interval 26000 mcast_querier_interval 25500 mcast_query_interval 12500 mcast_query_response_interval 1000 A following kernel patch-set will add selftests which use these commands. Thanks, Nik Nikolay Aleksandrov (17): ip: bridge: add support for mcast_vlan_snooping bridge: vlan: add support to show global vlan options bridge: vlan: add support for vlan filtering when dumping options bridge: vlan: add support to set global vlan options bridge: vlan: add global mcast_snooping option bridge: vlan: add global mcast_igmp_version option bridge: vlan: add global mcast_mld_version option bridge: vlan: add global mcast_last_member_count option bridge: vlan: add global mcast_startup_query_count option bridge: vlan: add global mcast_last_member_interval option bridge: vlan: add global mcast_membership_interval option bridge: vlan: add global mcast_querier_interval option bridge: vlan: add global mcast_query_interval option bridge: vlan: add global mcast_query_response_interval option bridge: vlan: add global mcast_startup_query_interval option bridge: vlan: add global mcast_querier option bridge: vlan: add support for dumping router ports bridge/br_common.h | 4 +- bridge/mdb.c | 6 +- bridge/monitor.c | 2 +- bridge/vlan.c | 546 +++++++++++++++++++++++++++++++++++++----- ip/iplink_bridge.c | 29 +++ man/man8/bridge.8 | 130 ++++++++++ man/man8/ip-link.8.in | 8 + 7 files changed, 659 insertions(+), 66 deletions(-)