* [RFC net-next] net: dsa: add support for MC_DISABLED attribute @ 2019-06-20 23:56 Vivien Didelot 2019-06-21 2:24 ` Florian Fainelli 0 siblings, 1 reply; 17+ messages in thread From: Vivien Didelot @ 2019-06-20 23:56 UTC (permalink / raw) To: netdev; +Cc: linux, f.fainelli, idosch, andrew, davem, Vivien Didelot This patch adds support for enabling or disabling the flooding of unknown multicast traffic on the CPU ports, depending on the value of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute. This allows the user to prevent the CPU to be flooded with a lot of undesirable traffic that the network stack needs to filter in software. The bridge has multicast snooping enabled by default, hence CPU ports aren't bottlenecked with arbitrary network applications anymore. But this can be an issue in some scenarios such as pinging the bridge's IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to 0 would restore unknown multicast flooding and thus fix ICMPv6. As an alternative, enabling multicast_querier would program the bridge address into the switch. Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com> --- net/dsa/dsa_priv.h | 2 ++ net/dsa/port.c | 19 +++++++++++++++++++ net/dsa/slave.c | 4 ++++ 3 files changed, 25 insertions(+) diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index a4853c22c2ff..54d838956499 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -163,6 +163,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp, unsigned long flags, struct switchdev_trans *trans); int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags, struct switchdev_trans *trans); +int dsa_port_bridge_mc_disabled(const struct dsa_port *dp, bool mc_disabled, + struct switchdev_trans *trans); int dsa_port_vlan_add(struct dsa_port *dp, const struct switchdev_obj_port_vlan *vlan, struct switchdev_trans *trans); diff --git a/net/dsa/port.c b/net/dsa/port.c index d2b65e8dc60c..79d14c36ef9a 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -261,6 +261,25 @@ int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags, return err; } +int dsa_port_bridge_mc_disabled(const struct dsa_port *dp, bool mc_disabled, + struct switchdev_trans *trans) +{ + struct dsa_switch *ds = dp->ds; + int port = dp->index; + + if (switchdev_trans_ph_prepare(trans)) { + if (!ds->ops->port_egress_floods) + return -EOPNOTSUPP; + + return 0; + } + + /* When multicast snooping is disabled, + * every multicast packet should be flooded to the CPU port. + */ + return ds->ops->port_egress_floods(ds, port, true, mc_disabled); +} + int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, u16 vid) { diff --git a/net/dsa/slave.c b/net/dsa/slave.c index db58e748557d..9308ffa4f22c 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -302,6 +302,10 @@ static int dsa_slave_port_attr_set(struct net_device *dev, case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS: ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, trans); break; + case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED: + ret = dsa_port_bridge_mc_disabled(dp->cpu_dp, + attr->u.mc_disabled, trans); + break; default: ret = -EOPNOTSUPP; break; -- 2.22.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-20 23:56 [RFC net-next] net: dsa: add support for MC_DISABLED attribute Vivien Didelot @ 2019-06-21 2:24 ` Florian Fainelli 2019-06-21 21:29 ` Vivien Didelot 2019-06-23 6:48 ` Ido Schimmel 0 siblings, 2 replies; 17+ messages in thread From: Florian Fainelli @ 2019-06-21 2:24 UTC (permalink / raw) To: Vivien Didelot, netdev, idosch, Jiri Pirko; +Cc: linux, andrew, davem On 6/20/2019 4:56 PM, Vivien Didelot wrote: > This patch adds support for enabling or disabling the flooding of > unknown multicast traffic on the CPU ports, depending on the value > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute. > > This allows the user to prevent the CPU to be flooded with a lot of > undesirable traffic that the network stack needs to filter in software. > > The bridge has multicast snooping enabled by default, hence CPU ports > aren't bottlenecked with arbitrary network applications anymore. > But this can be an issue in some scenarios such as pinging the bridge's > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to > 0 would restore unknown multicast flooding and thus fix ICMPv6. As > an alternative, enabling multicast_querier would program the bridge > address into the switch. From what I can read from mlxsw, we should probably also implement the SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent. Since the attribute MC_DISABLED is on the bridge master, we should also iterate over the list of switch ports being a member of that bridge and change their flooding attribute, taking into account whether BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing what mlxsw does here again... Once you act on the user-facing ports, you might be able to leave the CPU port flooding unconditionally, since it would only "flood" the CPU port either because an user-facing port has BR_MCAST_FLOOD set, or because this is known MC traffic that got programmed via the bridge's MDB. Would that work? On a higher level, I really wish we did not have to re-implement a lot of identical or similar logic in each switch drivers and had a more central model of what is behaviorally expected. [snip] > +int dsa_port_bridge_mc_disabled(const struct dsa_port *dp, bool mc_disabled, > + struct switchdev_trans *trans) > +{ > + struct dsa_switch *ds = dp->ds; > + int port = dp->index; > + > + if (switchdev_trans_ph_prepare(trans)) { > + if (!ds->ops->port_egress_floods) > + return -EOPNOTSUPP; > + > + return 0; > + } > + > + /* When multicast snooping is disabled, > + * every multicast packet should be flooded to the CPU port. > + */ The comment alignment is a bit off. -- Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-21 2:24 ` Florian Fainelli @ 2019-06-21 21:29 ` Vivien Didelot 2019-06-21 22:09 ` Russell King - ARM Linux admin 2019-06-23 7:09 ` Ido Schimmel 2019-06-23 6:48 ` Ido Schimmel 1 sibling, 2 replies; 17+ messages in thread From: Vivien Didelot @ 2019-06-21 21:29 UTC (permalink / raw) To: Florian Fainelli; +Cc: netdev, idosch, Jiri Pirko, linux, andrew, davem On Thu, 20 Jun 2019 19:24:47 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote: > > This patch adds support for enabling or disabling the flooding of > > unknown multicast traffic on the CPU ports, depending on the value > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute. > > > > This allows the user to prevent the CPU to be flooded with a lot of > > undesirable traffic that the network stack needs to filter in software. > > > > The bridge has multicast snooping enabled by default, hence CPU ports > > aren't bottlenecked with arbitrary network applications anymore. > > But this can be an issue in some scenarios such as pinging the bridge's > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As > > an alternative, enabling multicast_querier would program the bridge > > address into the switch. > From what I can read from mlxsw, we should probably also implement the > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent. > > Since the attribute MC_DISABLED is on the bridge master, we should also > iterate over the list of switch ports being a member of that bridge and > change their flooding attribute, taking into account whether > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing > what mlxsw does here again... Ouch, doesn't sound like what a driver should be doing :-( Ido, I cannot find documentation for multicast_snooping or MC_DISABLED and the naming isn't clear. Can this be considered as an equivalent of mcast_flood but targeting the bridge device itself, describing whether the bridge is interested or not in unknown multicast traffic? > Once you act on the user-facing ports, you might be able to leave the > CPU port flooding unconditionally, since it would only "flood" the CPU > port either because an user-facing port has BR_MCAST_FLOOD set, or > because this is known MC traffic that got programmed via the bridge's > MDB. Would that work? You may want the machine or network connected behind a bridge port to be flooded with unknown multicast traffic, without having the CPU conduit clogged up with this traffic. So these are two distinct settings for me. The only scenario I can think of needing the CPU to be flooded is if there's a non-DSA port in the bridge maybe. But IMHO this should be handled by the bridge, offloading or not the appropriate attribute. > On a higher level, I really wish we did not have to re-implement a lot > of identical or similar logic in each switch drivers and had a more > central model of what is behaviorally expected. I couldn't agree more, ethernet switch drivers should only offload the notified bridge configuration, not noodling their own logic... Russell, Ido, Florian, so far I understand that a multicast-unaware bridge must flood unknown traffic everywhere (CPU included); and a multicast-aware bridge must only flood its ports if their mcast_flood is on, and known traffic targeting the bridge must be offloaded accordingly. Do you guys agree with this? Thanks, Vivien ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-21 21:29 ` Vivien Didelot @ 2019-06-21 22:09 ` Russell King - ARM Linux admin 2019-06-23 7:09 ` Ido Schimmel 1 sibling, 0 replies; 17+ messages in thread From: Russell King - ARM Linux admin @ 2019-06-21 22:09 UTC (permalink / raw) To: Vivien Didelot Cc: Florian Fainelli, netdev, idosch, Jiri Pirko, andrew, davem On Fri, Jun 21, 2019 at 05:29:52PM -0400, Vivien Didelot wrote: > Russell, Ido, Florian, so far I understand that a multicast-unaware > bridge must flood unknown traffic everywhere (CPU included); ^ multicast > and a multicast-aware bridge must only flood its ports if their ^ unknown multicast traffic to > mcast_flood is on, and known traffic targeting the bridge must be > offloaded accordingly. Do you guys agree with this? I don't see a problem with that with the clarification that we're talking about multicast packets here. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-21 21:29 ` Vivien Didelot 2019-06-21 22:09 ` Russell King - ARM Linux admin @ 2019-06-23 7:09 ` Ido Schimmel 2019-06-23 7:26 ` Russell King - ARM Linux admin 2019-07-05 16:01 ` Vivien Didelot 1 sibling, 2 replies; 17+ messages in thread From: Ido Schimmel @ 2019-06-23 7:09 UTC (permalink / raw) To: Vivien Didelot; +Cc: Florian Fainelli, netdev, Jiri Pirko, linux, andrew, davem On Fri, Jun 21, 2019 at 05:29:52PM -0400, Vivien Didelot wrote: > On Thu, 20 Jun 2019 19:24:47 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > This patch adds support for enabling or disabling the flooding of > > > unknown multicast traffic on the CPU ports, depending on the value > > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute. > > > > > > This allows the user to prevent the CPU to be flooded with a lot of > > > undesirable traffic that the network stack needs to filter in software. > > > > > > The bridge has multicast snooping enabled by default, hence CPU ports > > > aren't bottlenecked with arbitrary network applications anymore. > > > But this can be an issue in some scenarios such as pinging the bridge's > > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to > > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As > > > an alternative, enabling multicast_querier would program the bridge > > > address into the switch. > > From what I can read from mlxsw, we should probably also implement the > > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent. > > > > Since the attribute MC_DISABLED is on the bridge master, we should also > > iterate over the list of switch ports being a member of that bridge and > > change their flooding attribute, taking into account whether > > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing > > what mlxsw does here again... > > Ouch, doesn't sound like what a driver should be doing :-( > > Ido, I cannot find documentation for multicast_snooping or MC_DISABLED > and the naming isn't clear. Can this be considered as an equivalent > of mcast_flood but targeting the bridge device itself, describing > whether the bridge is interested or not in unknown multicast traffic? Not really, you're only looking at one aspect of this, but there is more. For example, when multicast snooping is disabled, the MDB is not considered. > > Once you act on the user-facing ports, you might be able to leave the > > CPU port flooding unconditionally, since it would only "flood" the CPU > > port either because an user-facing port has BR_MCAST_FLOOD set, or > > because this is known MC traffic that got programmed via the bridge's > > MDB. Would that work? > > You may want the machine or network connected behind a bridge port > to be flooded with unknown multicast traffic, without having the > CPU conduit clogged up with this traffic. So these are two distinct > settings for me. > > The only scenario I can think of needing the CPU to be flooded is if > there's a non-DSA port in the bridge maybe. But IMHO this should be > handled by the bridge, offloading or not the appropriate attribute. > > > On a higher level, I really wish we did not have to re-implement a lot > > of identical or similar logic in each switch drivers and had a more > > central model of what is behaviorally expected. > > I couldn't agree more, ethernet switch drivers should only offload > the notified bridge configuration, not noodling their own logic... Please see my comment to Florian. The driver is doing what needs to be done in order to comply with the behavior of the bridge driver. As I wrote to Florian, we can probably move some logic up the stack and simplify drivers. > Russell, Ido, Florian, so far I understand that a multicast-unaware > bridge must flood unknown traffic everywhere (CPU included); > and a multicast-aware bridge must only flood its ports if their > mcast_flood is on, and known traffic targeting the bridge must be > offloaded accordingly. Do you guys agree with this? When multicast snooping is enabled unregistered multicast traffic should only be flooded to mrouter ports. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-23 7:09 ` Ido Schimmel @ 2019-06-23 7:26 ` Russell King - ARM Linux admin 2019-06-23 7:44 ` Ido Schimmel 2019-07-05 16:01 ` Vivien Didelot 1 sibling, 1 reply; 17+ messages in thread From: Russell King - ARM Linux admin @ 2019-06-23 7:26 UTC (permalink / raw) To: Ido Schimmel Cc: Vivien Didelot, Florian Fainelli, netdev, Jiri Pirko, andrew, davem On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote: > When multicast snooping is enabled unregistered multicast traffic should > only be flooded to mrouter ports. Given that IPv6 relies upon multicast working, and multicast snooping is a kernel configuration option, and MLD messages will only be sent when whenever the configuration on the target changes, and there may not be a multicast querier in the system, who does that ensure that IPv6 can work on a bridge where the kernel configured and built with multicast snooping enabled? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-23 7:26 ` Russell King - ARM Linux admin @ 2019-06-23 7:44 ` Ido Schimmel 2019-06-29 16:29 ` Ido Schimmel 0 siblings, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2019-06-23 7:44 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Ido Schimmel, Vivien Didelot, Florian Fainelli, netdev, Jiri Pirko, andrew, davem On Sun, Jun 23, 2019 at 08:26:05AM +0100, Russell King - ARM Linux admin wrote: > On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote: > > When multicast snooping is enabled unregistered multicast traffic should > > only be flooded to mrouter ports. > > Given that IPv6 relies upon multicast working, and multicast snooping > is a kernel configuration option, and MLD messages will only be sent > when whenever the configuration on the target changes, and there may > not be a multicast querier in the system, who does that ensure that > IPv6 can work on a bridge where the kernel configured and built with > multicast snooping enabled? See commit b00589af3b04 ("bridge: disable snooping if there is no querier"). I think that's unfortunate behavior that we need because multicast snooping is enabled by default. If it weren't enabled by default, then anyone enabling it would also make sure there's a querier in the network. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-23 7:44 ` Ido Schimmel @ 2019-06-29 16:29 ` Ido Schimmel 2019-06-30 16:56 ` Linus Lüssing 0 siblings, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2019-06-29 16:29 UTC (permalink / raw) To: Russell King - ARM Linux admin, nikolay, linus.luessing Cc: Ido Schimmel, Vivien Didelot, Florian Fainelli, netdev, Jiri Pirko, andrew, davem On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote: > On Sun, Jun 23, 2019 at 08:26:05AM +0100, Russell King - ARM Linux admin wrote: > > On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote: > > > When multicast snooping is enabled unregistered multicast traffic should > > > only be flooded to mrouter ports. > > > > Given that IPv6 relies upon multicast working, and multicast snooping > > is a kernel configuration option, and MLD messages will only be sent > > when whenever the configuration on the target changes, and there may > > not be a multicast querier in the system, who does that ensure that > > IPv6 can work on a bridge where the kernel configured and built with > > multicast snooping enabled? > > See commit b00589af3b04 ("bridge: disable snooping if there is no > querier"). I think that's unfortunate behavior that we need because > multicast snooping is enabled by default. If it weren't enabled by > default, then anyone enabling it would also make sure there's a querier > in the network. Linus, Nik, I brought this problem in the past, but we didn't reach a solution, so I'll try again :) The problem: Even if multicast snooping is enabled, the bridge driver will flood multicast packets to all the ports if no querier was detected on the link. The querier states (IPv4 & IPv6) are not currently reflected to switchdev drivers which means that the hardware data path will only flood unregistered multicast packets to mrouter ports (which can be an empty list). In default configurations (where multicast snooping is enabled and the bridge querier is disabled), this can prevent IPv6 ping from passing, as there are no mrouter ports and there is no MDB entry corresponding to the solicited-node multicast address. Is there anything we can do about it? Enable the bridge querier if no other querier was detected? Commit c5c23260594c ("bridge: Add multicast_querier toggle and disable queries by default") disabled queries by default, but I'm only suggesting to turn them on if no other querier was detected on the link. Do you think it's still a problem? I would like to avoid having drivers take the querier state into account as it will only complicate things further. Thanks ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-29 16:29 ` Ido Schimmel @ 2019-06-30 16:56 ` Linus Lüssing 2019-07-02 14:27 ` Nikolay Aleksandrov 2019-07-02 17:11 ` Ido Schimmel 0 siblings, 2 replies; 17+ messages in thread From: Linus Lüssing @ 2019-06-30 16:56 UTC (permalink / raw) To: Ido Schimmel Cc: Russell King - ARM Linux admin, nikolay, Ido Schimmel, Vivien Didelot, Florian Fainelli, netdev, Jiri Pirko, andrew, davem, bridge, b.a.t.m.a.n On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote: > I would like to avoid having drivers take the querier state into account > as it will only complicate things further. I absolutely share your pain. Initially in the early prototypes of multicast awareness in batman-adv we did not consider the querier state. And doing so later did indeed complicate the code a good bit in batman-adv (together with the IGMP/MLD suppression issues). I would have loved to avoid that. > Is there anything we can do about it? Enable the bridge querier if no > other querier was detected? Commit c5c23260594c ("bridge: Add > multicast_querier toggle and disable queries by default") disabled > queries by default, but I'm only suggesting to turn them on if no other > querier was detected on the link. Do you think it's still a problem? As soon as you start becoming the querier, you will not be able to reliably detect anymore whether you are the only querier candidate. If any random Linux host using a bridge device were potentially becoming a querier, that would cause quite some trouble when this host is behind some bad, bottleneck connection. This host will receive all multicast traffic, not just IGMP/MLD reports. And with a congested connection and then unreliable IGMP/MLD, multicast would become unreliable overall in this domain. So it's important that your querier is not running in the "dark, remote, dusty closet" of your network (topologically speaking). > On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote: > > See commit b00589af3b04 ("bridge: disable snooping if there is no > > querier"). I think that's unfortunate behavior that we need because > > multicast snooping is enabled by default. If it weren't enabled by > > default, then anyone enabling it would also make sure there's a querier > > in the network. I do not quite understand that point. In a way, that's what we have right now, isn't it? By default it's disabled, because by default there is no querier on the link. So anyone wanting to use multicast snooping will need to make sure there's a querier in the network. Overall I think the querier (election) mechanism in the standards could need an update. While the lowest-address first might have worked well back then, in uniform, fully wired networks where the position of the querier did not matter, this is not a good solution anymore in networks involving wireless, dynamic connections. Especially in wireless mesh networks this is a bit of an issue for us. Ideally, the querier mechanism were dismissed in favour of simply unsolicited, periodic IGMP/MLD reports... But of course, updating IETF standards is no solution for now. While more complicated, it would not be impossible to consider the querier state, would it? I mean you probably already need to consider the case of a user disabling multicast snooping during runtime, right? So similarly, you could react to appearing or disappearing queriers? Cheers, Linus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-30 16:56 ` Linus Lüssing @ 2019-07-02 14:27 ` Nikolay Aleksandrov 2019-07-02 17:11 ` Ido Schimmel 1 sibling, 0 replies; 17+ messages in thread From: Nikolay Aleksandrov @ 2019-07-02 14:27 UTC (permalink / raw) To: Linus Lüssing, Ido Schimmel Cc: Russell King - ARM Linux admin, Ido Schimmel, Vivien Didelot, Florian Fainelli, netdev, Jiri Pirko, andrew, davem, bridge, b.a.t.m.a.n On 30/06/2019 19:56, Linus Lüssing wrote: > On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote: >> I would like to avoid having drivers take the querier state into account >> as it will only complicate things further. > > I absolutely share your pain. Initially in the early prototypes of > multicast awareness in batman-adv we did not consider the querier state. > And doing so later did indeed complicate the code a good bit in batman-adv > (together with the IGMP/MLD suppression issues). I would have loved to > avoid that. > > >> Is there anything we can do about it? Enable the bridge querier if no >> other querier was detected? Commit c5c23260594c ("bridge: Add >> multicast_querier toggle and disable queries by default") disabled >> queries by default, but I'm only suggesting to turn them on if no other >> querier was detected on the link. Do you think it's still a problem? > > As soon as you start becoming the querier, you will not be able to reliably > detect anymore whether you are the only querier candidate. > > If any random Linux host using a bridge device were potentially becoming > a querier, that would cause quite some trouble when this host is > behind some bad, bottleneck connection. This host will receive > all multicast traffic, not just IGMP/MLD reports. And with a > congested connection and then unreliable IGMP/MLD, multicast would > become unreliable overall in this domain. So it's important that > your querier is not running in the "dark, remote, dusty closet" of > your network (topologically speaking). > +1 We definitely don't want random hosts becoming queriers >> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote: >>> See commit b00589af3b04 ("bridge: disable snooping if there is no >>> querier"). I think that's unfortunate behavior that we need because >>> multicast snooping is enabled by default. If it weren't enabled by >>> default, then anyone enabling it would also make sure there's a querier >>> in the network. > > I do not quite understand that point. In a way, that's what we > have right now, isn't it? By default it's disabled, because by > default there is no querier on the link. So anyone wanting to use > multicast snooping will need to make sure there's a querier in the > network. > Indeed, also you could create the bridge with explicit mcast parameters if you need different behaviour on start. Unfortunately I think you'll have to handle the querier state. > > Overall I think the querier (election) mechanism in the standards could > need an update. While the lowest-address first might have > worked well back then, in uniform, fully wired networks where the > position of the querier did not matter, this is not a good > solution anymore in networks involving wireless, dynamic connections. > Especially in wireless mesh networks this is a bit of an issue for > us. Ideally, the querier mechanism were dismissed in favour of simply > unsolicited, periodic IGMP/MLD reports... > > But of course, updating IETF standards is no solution for now. > > While more complicated, it would not be impossible to consider the > querier state, would it? I mean you probably already need to > consider the case of a user disabling multicast snooping during > runtime, right? So similarly, you could react to appearing or > disappearing queriers? > > Cheers, Linus > Thanks, Nik ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-30 16:56 ` Linus Lüssing 2019-07-02 14:27 ` Nikolay Aleksandrov @ 2019-07-02 17:11 ` Ido Schimmel [not found] ` <20190702231308.GA2414@otheros> 1 sibling, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2019-07-02 17:11 UTC (permalink / raw) To: Linus Lüssing Cc: Russell King - ARM Linux admin, nikolay, Ido Schimmel, Vivien Didelot, Florian Fainelli, netdev, Jiri Pirko, andrew, davem, bridge, b.a.t.m.a.n On Sun, Jun 30, 2019 at 06:56:01PM +0200, Linus Lüssing wrote: > > On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote: > > > See commit b00589af3b04 ("bridge: disable snooping if there is no > > > querier"). I think that's unfortunate behavior that we need because > > > multicast snooping is enabled by default. If it weren't enabled by > > > default, then anyone enabling it would also make sure there's a querier > > > in the network. > > I do not quite understand that point. In a way, that's what we > have right now, isn't it? By default it's disabled, because by > default there is no querier on the link. So anyone wanting to use > multicast snooping will need to make sure there's a querier in the > network. Hi Linus, Querier state is not reflected to drivers ATM, so drivers believe the bridge is multicast aware and unregistered multicast packets are only flooded to mrouter ports. Hosts that are silent (because there is no querier) never get the traffic addressed to them (f.e., IPv6 neighbour solicitation). > Overall I think the querier (election) mechanism in the standards could > need an update. While the lowest-address first might have > worked well back then, in uniform, fully wired networks where the > position of the querier did not matter, this is not a good > solution anymore in networks involving wireless, dynamic connections. > Especially in wireless mesh networks this is a bit of an issue for > us. Ideally, the querier mechanism were dismissed in favour of simply > unsolicited, periodic IGMP/MLD reports... > > But of course, updating IETF standards is no solution for now. > > While more complicated, it would not be impossible to consider the > querier state, would it? I mean you probably already need to > consider the case of a user disabling multicast snooping during > runtime, right? Sure, this is implemented. > So similarly, you could react to appearing or disappearing queriers? Yes, but it's a bit more complicated since we need to differentiate between IPv4 and IPv6. If the bridge is multicast aware, but there is only IPv4 querier on the link, then: 1. All the IPv6 MDB entries need to be removed from the device. At least in mlxsw, we do not have a way to ignore only IPv6 entries. From the device's perspective, an MDB entry is just a multicast DMAC with a bitmap of ports packets should be replicated to. 2. We need to split the flood tables used for IPv4 and IPv6 unregistered multicast packets. For IPv4, packets should only be flooded to mrouter ports whereas for IPv6 packets should be flooded to all the member ports. Do you differentiate between IPv4 and IPv6 in batman-adv? > Cheers, Linus Thanks for the feedback! ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <20190702231308.GA2414@otheros>]
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute [not found] ` <20190702231308.GA2414@otheros> @ 2019-07-07 9:07 ` Ido Schimmel 0 siblings, 0 replies; 17+ messages in thread From: Ido Schimmel @ 2019-07-07 9:07 UTC (permalink / raw) To: Linus Lüssing Cc: Florian Fainelli, Jiri Pirko, nikolay, netdev, bridge, Russell King - ARM Linux admin, davem, Ido Schimmel, Vivien Didelot, b.a.t.m.a.n On Wed, Jul 03, 2019 at 01:13:08AM +0200, Linus Lüssing wrote: > Hi Ido, > > > Do you differentiate between IPv4 and IPv6 in batman-adv? > > For most things, yes: The querier state is kept separately for > IPv4 and IPv6. And we do have something like a "router node" > flag to signalize that a node needs all multicast traffic, which > is split into IPv4 and IPv6. > > The "MDB" equivalent in batman-adv (multicast entries in our "TT", > translation table) are on MAC address base right now, not on an IP > address base yet, so that sounds similar to what you do in mlxsw? Yes. > Regarding querier state, we periodically ask the > bridge via "br_multicast_has_querier_anywhere(dev, ETH_P_IP)" > and "br_multicast_has_querier_anywhere(dev, ETH_P_IPV6)". > > (Something more event based with handler functions would probably > be nicer, but this was the easier thing to start with.) Thanks for the reference. Will check the code. I believe that we will add switchdev notifications for querier state change, so it might be useful for you as well. > > 1. All the IPv6 MDB entries need to be removed from the device. At least > > in mlxsw, we do not have a way to ignore only IPv6 entries. From the > > device's perspective, an MDB entry is just a multicast DMAC with a > > bitmap of ports packets should be replicated to. > > Ah, I see, yes. We had a similar "issue". Initially we just always > added any multicast entry into our translation table offered by > the IP stack or bridge, no matter what a querier state or "router > node" state said. Which would have led to a node receiving two > copies of a multicast packet if it were both a querier or router > and were also having a listener announced via IGMP/MLD. > > So there we also just recently changed that, to filter out > IPv6 multicast TT entries if this node were also announcing itself as > an MLD querier or IPv6 router. And same, independently for > IPv4/IGMP. This is actually not a problem with mlxsw. The ports a packet should be replicated to are represented using a bitmap. It does not matter if we set the bit because it has an MDB entry or because it is an mrouter port. And obviously it does not matter if we set it twice :) > > 2. We need to split the flood tables used for IPv4 and IPv6 unregistered > > multicast packets. For IPv4, packets should only be flooded to mrouter > > ports whereas for IPv6 packets should be flooded to all the member > > ports. > > This one I do not fully understand yet. Why don't you apply the > "flood to all ports" (in the no IGMP querier present case) > for IPv4, too? > > Sure, for IPv4 nothing "essential" will break, as IPv4 unicast > does not rely on multicast (contrary to IPv6, due to NDP, as you > mentioned). But still there would be potential multicast packet loss > for a 239.x.x.x listener on the local link, for instance, wouldn't > there? > > > If I'm not mistaken, we do not apply differing behaviour for IPv4 > vs. IPv6 in the bridge either and would flood on all ports for IPv4 > with no querier present, too. I think I was not clear, so I will explain again. I was referring to a situation where IPv4 has a querier, but IPv6 does not. In this case, the bridge will flood IPv4 unregistered multicast packets to mrouter ports only. On the other hand, IPv6 unregistered multicast packets will be flooded to all the ports. Based on my reading of the code, this is controlled by 'mcast_hit' in br_handle_frame_finish(). In mlxsw, each packet type (e.g., unknown unicast, IPvX unregistered multicast) is associated with a flood table (basically a huge matrix, where row corresponds to VLAN and column corresponds to a port). If we are to handle the case where IPv4 unregistered multicast packets need to be flooded to mrouter ports only, whereas IPv6 unregistered multicast packets need to be flooded to all the ports, then we need to use a separate flood table for each. Alternatively, we can use the same flood table, but flood to all the ports if IPv4 or IPv6 querier is missing. I do not think anything will break, it is just very efficient. This seems to be allowed by RFC 4541 (Section 2.1.2): "If a switch receives an unregistered packet, it must forward that packet on all ports to which an IGMP router is attached. A switch may default to forwarding unregistered packets on all ports." > Regards, Linus Linus, thanks a lot for the great feedback. I really appreciate it! ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-23 7:09 ` Ido Schimmel 2019-06-23 7:26 ` Russell King - ARM Linux admin @ 2019-07-05 16:01 ` Vivien Didelot 2019-07-07 10:28 ` Ido Schimmel 1 sibling, 1 reply; 17+ messages in thread From: Vivien Didelot @ 2019-07-05 16:01 UTC (permalink / raw) To: Ido Schimmel; +Cc: Florian Fainelli, netdev, Jiri Pirko, linux, andrew, davem Hi Ido, On Sun, 23 Jun 2019 07:09:52 +0000, Ido Schimmel <idosch@mellanox.com> wrote: > > Russell, Ido, Florian, so far I understand that a multicast-unaware > > bridge must flood unknown traffic everywhere (CPU included); > > and a multicast-aware bridge must only flood its ports if their > > mcast_flood is on, and known traffic targeting the bridge must be > > offloaded accordingly. Do you guys agree with this? > > When multicast snooping is enabled unregistered multicast traffic should > only be flooded to mrouter ports. I've figured out that this is what I need to prevent the flooding of undesired multicast traffic to the CPU port of the switch. The bridge itself has a multicast_router attribute which can be disabled, that is when I should drop unknown multicast traffic. However with SWITCHDEV_ATTR_ID_BRIDGE_MROUTER implemented, this attribute is always called with .mrouter=0, regardless the value of /sys/class/net/br0/bridge/multicast_router. Do I miss something here? Thanks, Vivien ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-07-05 16:01 ` Vivien Didelot @ 2019-07-07 10:28 ` Ido Schimmel 0 siblings, 0 replies; 17+ messages in thread From: Ido Schimmel @ 2019-07-07 10:28 UTC (permalink / raw) To: Vivien Didelot Cc: Ido Schimmel, Florian Fainelli, netdev, Jiri Pirko, linux, andrew, davem On Fri, Jul 05, 2019 at 12:01:49PM -0400, Vivien Didelot wrote: > Hi Ido, > > On Sun, 23 Jun 2019 07:09:52 +0000, Ido Schimmel <idosch@mellanox.com> wrote: > > > Russell, Ido, Florian, so far I understand that a multicast-unaware > > > bridge must flood unknown traffic everywhere (CPU included); > > > and a multicast-aware bridge must only flood its ports if their > > > mcast_flood is on, and known traffic targeting the bridge must be > > > offloaded accordingly. Do you guys agree with this? > > > > When multicast snooping is enabled unregistered multicast traffic should > > only be flooded to mrouter ports. > > I've figured out that this is what I need to prevent the flooding of undesired > multicast traffic to the CPU port of the switch. The bridge itself has a > multicast_router attribute which can be disabled, that is when I should drop > unknown multicast traffic. > > However with SWITCHDEV_ATTR_ID_BRIDGE_MROUTER implemented, this > attribute is always called with .mrouter=0, regardless the value of > /sys/class/net/br0/bridge/multicast_router. Do I miss something here? Hi Vivien, I just checked this and it seems to work as expected: # echo 2 > /sys/class/net/br0/bridge/multicast_router We get a notification with mrouter=1 to mlxsw # echo 0 > /sys/class/net/br0/bridge/multicast_router We get a notification with mrouter=0 to mlxsw ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-21 2:24 ` Florian Fainelli 2019-06-21 21:29 ` Vivien Didelot @ 2019-06-23 6:48 ` Ido Schimmel 2019-06-29 15:31 ` Ido Schimmel 1 sibling, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2019-06-23 6:48 UTC (permalink / raw) To: Florian Fainelli; +Cc: Vivien Didelot, netdev, Jiri Pirko, linux, andrew, davem On Thu, Jun 20, 2019 at 07:24:47PM -0700, Florian Fainelli wrote: > On 6/20/2019 4:56 PM, Vivien Didelot wrote: > > This patch adds support for enabling or disabling the flooding of > > unknown multicast traffic on the CPU ports, depending on the value > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute. > > > > This allows the user to prevent the CPU to be flooded with a lot of > > undesirable traffic that the network stack needs to filter in software. > > > > The bridge has multicast snooping enabled by default, hence CPU ports > > aren't bottlenecked with arbitrary network applications anymore. > > But this can be an issue in some scenarios such as pinging the bridge's > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As > > an alternative, enabling multicast_querier would program the bridge > > address into the switch. > From what I can read from mlxsw, we should probably also implement the > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent. > > Since the attribute MC_DISABLED is on the bridge master, we should also > iterate over the list of switch ports being a member of that bridge and > change their flooding attribute, taking into account whether > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing > what mlxsw does here again... When multicast snooping is enabled, unregistered multicast traffic should be flooded to mrouter ports only. Otherwise, it should be flooded to all ports. > Once you act on the user-facing ports, you might be able to leave the > CPU port flooding unconditionally, since it would only "flood" the CPU > port either because an user-facing port has BR_MCAST_FLOOD set, or > because this is known MC traffic that got programmed via the bridge's > MDB. Would that work? > > On a higher level, I really wish we did not have to re-implement a lot > of identical or similar logic in each switch drivers and had a more > central model of what is behaviorally expected. Well, that model is the bridge driver... But I agree that we can probably simplify the interface towards drivers and move more code up the stack. For example, two things mlxsw is doing when multicast snooping is enabled: 1. Writing MDB entries to the device. When multicast snooping is disabled, MDB entries are ignored by the bridge driver. Can we agree to have the bridge driver generate SWITCHDEV_OBJ_ID_PORT_MDB add / delete for all MDB entries when multicast snooping is toggled? 2. Flooding unregistered multicast traffic only to mrouter ports. The bridge driver can iterate over the bridge members and toggle BR_MCAST_FLOOD accordingly. It will not actually change this value. Only emulate this change towards drivers. I will try to come up with a more detailed list later this week. I really want to start moving more logic out of drivers. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-23 6:48 ` Ido Schimmel @ 2019-06-29 15:31 ` Ido Schimmel 2019-06-29 23:06 ` Andrew Lunn 0 siblings, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2019-06-29 15:31 UTC (permalink / raw) To: f.fainelli, vivien.didelot; +Cc: netdev, Jiri Pirko, linux, andrew, davem On Sun, Jun 23, 2019 at 06:48:41AM +0000, Ido Schimmel wrote: > On Thu, Jun 20, 2019 at 07:24:47PM -0700, Florian Fainelli wrote: > > On 6/20/2019 4:56 PM, Vivien Didelot wrote: > > > This patch adds support for enabling or disabling the flooding of > > > unknown multicast traffic on the CPU ports, depending on the value > > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute. > > > > > > This allows the user to prevent the CPU to be flooded with a lot of > > > undesirable traffic that the network stack needs to filter in software. > > > > > > The bridge has multicast snooping enabled by default, hence CPU ports > > > aren't bottlenecked with arbitrary network applications anymore. > > > But this can be an issue in some scenarios such as pinging the bridge's > > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to > > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As > > > an alternative, enabling multicast_querier would program the bridge > > > address into the switch. > > From what I can read from mlxsw, we should probably also implement the > > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent. > > > > Since the attribute MC_DISABLED is on the bridge master, we should also > > iterate over the list of switch ports being a member of that bridge and > > change their flooding attribute, taking into account whether > > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing > > what mlxsw does here again... > > When multicast snooping is enabled, unregistered multicast traffic > should be flooded to mrouter ports only. Otherwise, it should be flooded > to all ports. > > > Once you act on the user-facing ports, you might be able to leave the > > CPU port flooding unconditionally, since it would only "flood" the CPU > > port either because an user-facing port has BR_MCAST_FLOOD set, or > > because this is known MC traffic that got programmed via the bridge's > > MDB. Would that work? > > > > On a higher level, I really wish we did not have to re-implement a lot > > of identical or similar logic in each switch drivers and had a more > > central model of what is behaviorally expected. > > Well, that model is the bridge driver... But I agree that we can > probably simplify the interface towards drivers and move more code up > the stack. > > For example, two things mlxsw is doing when multicast snooping is > enabled: > > 1. Writing MDB entries to the device. When multicast snooping is > disabled, MDB entries are ignored by the bridge driver. Can we agree to > have the bridge driver generate SWITCHDEV_OBJ_ID_PORT_MDB add / delete > for all MDB entries when multicast snooping is toggled? > > 2. Flooding unregistered multicast traffic only to mrouter ports. The > bridge driver can iterate over the bridge members and toggle > BR_MCAST_FLOOD accordingly. It will not actually change this value. Only > emulate this change towards drivers. > > I will try to come up with a more detailed list later this week. I reviewed the MC logic in mlxsw again and while I found some things that can be improved, I don't think major simplification can happen there. Regarding "central model of what is behaviorally expected". IMO, the best thing is to make sure that all the implementations pass tests that codify what is to be expected. Given that the model is the Linux bridge, the tests should of course pass with veth pairs. We have some tests under tools/testing/selftests/net/forwarding/, but not so much for MC. However, even if such tests were to be contributed, would you be able to run them on your hardware? I remember that in the past you complained about the number of required ports. Is there something you can do about it? How many ports are available on your platforms? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute 2019-06-29 15:31 ` Ido Schimmel @ 2019-06-29 23:06 ` Andrew Lunn 0 siblings, 0 replies; 17+ messages in thread From: Andrew Lunn @ 2019-06-29 23:06 UTC (permalink / raw) To: Ido Schimmel; +Cc: f.fainelli, vivien.didelot, netdev, Jiri Pirko, linux, davem > We have some tests under tools/testing/selftests/net/forwarding/, but > not so much for MC. However, even if such tests were to be contributed, > would you be able to run them on your hardware? I remember that in the > past you complained about the number of required ports. Hi Ido Most devices have 4 or 5 ports. Some have more, some less. Given that the current tests need a second port for every port under test, this limits you to 2 ports under test. For IGMP snooping and MC in general, i don't think that is enough. You need one port as the source of the MC traffic, one port with a host interested in the traffic, and one port without interest in the traffic. You can then test that traffic is correctly forwarded and blocked. The testing we do with DSA makes use of a test host with 4 interfaces, and connect them one to one to the switch. That gives us 4 ports under test, and so that gives us many more possibilities for running interesting tests. Andrew ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-07-07 10:28 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-20 23:56 [RFC net-next] net: dsa: add support for MC_DISABLED attribute Vivien Didelot
2019-06-21 2:24 ` Florian Fainelli
2019-06-21 21:29 ` Vivien Didelot
2019-06-21 22:09 ` Russell King - ARM Linux admin
2019-06-23 7:09 ` Ido Schimmel
2019-06-23 7:26 ` Russell King - ARM Linux admin
2019-06-23 7:44 ` Ido Schimmel
2019-06-29 16:29 ` Ido Schimmel
2019-06-30 16:56 ` Linus Lüssing
2019-07-02 14:27 ` Nikolay Aleksandrov
2019-07-02 17:11 ` Ido Schimmel
[not found] ` <20190702231308.GA2414@otheros>
2019-07-07 9:07 ` Ido Schimmel
2019-07-05 16:01 ` Vivien Didelot
2019-07-07 10:28 ` Ido Schimmel
2019-06-23 6:48 ` Ido Schimmel
2019-06-29 15:31 ` Ido Schimmel
2019-06-29 23:06 ` Andrew Lunn
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).