| Message ID | 20210826130533.149111-7-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | bridge: vlan: add global multicast options | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
diff --git a/bridge/vlan.c b/bridge/vlan.c index 372e5b43be0f..346026b6f955 100644 --- a/bridge/vlan.c +++ b/bridge/vlan.c @@ -39,6 +39,7 @@ static void usage(void) " bridge vlan { show } [ dev DEV ] [ vid VLAN_ID ]\n" " bridge vlan { tunnelshow } [ dev DEV ] [ vid VLAN_ID ]\n" " bridge vlan global { set } vid VLAN_ID dev DEV [ mcast_snooping MULTICAST_SNOOPING ]\n" + " [ mcast_igmp_version IGMP_VERSION ]\n" " bridge vlan global { show } [ dev DEV ] [ vid VLAN_ID ]\n"); exit(-1); } @@ -405,6 +406,12 @@ static int vlan_global_option_set(int argc, char **argv) invarg("invalid mcast_snooping", *argv); addattr8(&req.n, 1024, BRIDGE_VLANDB_GOPTS_MCAST_SNOOPING, val8); + } else if (strcmp(*argv, "mcast_igmp_version") == 0) { + NEXT_ARG(); + if (get_u8(&val8, *argv, 0)) + invarg("invalid mcast_igmp_version", *argv); + addattr8(&req.n, 1024, + BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION, val8); } else { if (matches(*argv, "help") == 0) NEXT_ARG(); @@ -743,6 +750,11 @@ static void print_vlan_global_opts(struct rtattr *a, int ifindex) print_uint(PRINT_ANY, "mcast_snooping", "mcast_snooping %u ", rta_getattr_u8(vattr)); } + if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION]) { + vattr = vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION]; + print_uint(PRINT_ANY, "mcast_igmp_version", + "mcast_igmp_version %u ", rta_getattr_u8(vattr)); + } print_nl(); close_json_object(); } diff --git a/man/man8/bridge.8 b/man/man8/bridge.8 index d894289b2dc2..224647b49843 100644 --- a/man/man8/bridge.8 +++ b/man/man8/bridge.8 @@ -159,7 +159,9 @@ bridge \- show / manipulate bridge addresses and devices .B vid .IR VID " [ " .B mcast_snooping -.IR MULTICAST_SNOOPING " ]" +.IR MULTICAST_SNOOPING " ] [ " +.B mcast_igmp_version +.IR IGMP_VERSION " ]" .ti -8 .BR "bridge vlan global" " [ " show " ] [ " @@ -931,6 +933,10 @@ turn multicast snooping for VLAN entry with VLAN ID on or off .RI ( MULTICAST_SNOOPING " == 0). Default is on. " +.TP +.BI mcast_igmp_version " IGMP_VERSION " +set the IGMP version. Default is 2. + .SS bridge vlan global show - list global vlan options. This command displays the global VLAN options for each VLAN entry.