| Message ID | 20201108131953.2462644-4-olteanv@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Offload learnt bridge addresses to DSA | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 0 this patch: 2 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 65 lines checked |
| jkicinski/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 2 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Sun, Nov 8, 2020 at 9:20 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > Some DSA switches (and not only) cannot learn source MAC addresses from > packets injected from the CPU. They only perform hardware address > learning from inbound traffic. > > This can be problematic when we have a bridge spanning some DSA switch > ports and some non-DSA ports (which we'll call "foreign interfaces" from > DSA's perspective). > > There are 2 classes of problems created by the lack of learning on > CPU-injected traffic: > - excessive flooding, due to the fact that DSA treats those addresses as > unknown > - the risk of stale routes, which can lead to temporary packet loss > > To illustrate the second class, consider the following situation, which > is common in production equipment (wireless access points, where there > is a WLAN interface and an Ethernet switch, and these form a single > bridging domain). > > AP 1: > +------------------------------------------------------------------------+ > | br0 | > +------------------------------------------------------------------------+ > +------------+ +------------+ +------------+ +------------+ +------------+ > | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | > +------------+ +------------+ +------------+ +------------+ +------------+ > | ^ ^ > | | | > | | | > | Client A Client B > | > | > | > +------------+ +------------+ +------------+ +------------+ +------------+ > | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | > +------------+ +------------+ +------------+ +------------+ +------------+ > +------------------------------------------------------------------------+ > | br0 | > +------------------------------------------------------------------------+ > AP 2 > > - br0 of AP 1 will know that Clients A and B are reachable via wlan0 > - the hardware fdb of a DSA switch driver today is not kept in sync with > the software entries on other bridge ports, so it will not know that > clients A and B are reachable via the CPU port UNLESS the hardware > switch itself performs SA learning from traffic injected from the CPU. > Nonetheless, a substantial number of switches don't. > - the hardware fdb of the DSA switch on AP 2 may autonomously learn that > Client A and B are reachable through swp0. Therefore, the software br0 > of AP 2 also may or may not learn this. In the example we're > illustrating, some Ethernet traffic has been going on, and br0 from AP > 2 has indeed learnt that it can reach Client B through swp0. > > One of the wireless clients, say Client B, disconnects from AP 1 and > roams to AP 2. The topology now looks like this: > > AP 1: > +------------------------------------------------------------------------+ > | br0 | > +------------------------------------------------------------------------+ > +------------+ +------------+ +------------+ +------------+ +------------+ > | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | > +------------+ +------------+ +------------+ +------------+ +------------+ > | ^ > | | > | Client A > | > | > | Client B > | | > | v > +------------+ +------------+ +------------+ +------------+ +------------+ > | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | > +------------+ +------------+ +------------+ +------------+ +------------+ > +------------------------------------------------------------------------+ > | br0 | > +------------------------------------------------------------------------+ > AP 2 > > - br0 of AP 1 still knows that Client A is reachable via wlan0 (no change) > - br0 of AP 1 will (possibly) know that Client B has left wlan0. There > are cases where it might never find out though. Either way, DSA today > does not process that notification in any way. > - the hardware FDB of the DSA switch on AP 1 may learn autonomously that > Client B can be reached via swp0, if it receives any packet with > Client 1's source MAC address over Ethernet. > - the hardware FDB of the DSA switch on AP 2 still thinks that Client B > can be reached via swp0. It does not know that it has roamed to wlan0, > because it doesn't perform SA learning from the CPU port. > > Now Client A contacts Client B. > AP 1 routes the packet fine towards swp0 and delivers it on the Ethernet > segment. > AP 2 sees a frame on swp0 and its fdb says that the destination is swp0. > Hairpinning is disabled => drop. > > This problem comes from the fact that these switches have a 'blind spot' > for addresses coming from software bridging. The generic solution is not > to assume that hardware learning can be enabled somehow, but to listen > to more bridge learning events. It turns out that the bridge driver does > learn in software from all inbound frames, in __br_handle_local_finish. > A proper SWITCHDEV_FDB_ADD_TO_DEVICE notification is emitted for the > addresses serviced by the bridge on 'foreign' interfaces. The problem is > that DSA currently only cares about SWITCHDEV_FDB_ADD_TO_DEVICE events > received on its own interfaces, such as static FDB entries. > > Luckily we can change that, and DSA can listen to all switchdev FDB > add/del events in the system and figure out if those events were emitted > by a bridge that spans at least one of DSA's own ports. In case that is > true, DSA will also offload that address towards its own CPU port, in > the eventuality that there might be bridge clients attached to the DSA > switch who want to talk to the station connected to the foreign > interface. > > Reported-by: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > --- > net/dsa/slave.c | 51 ++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 46 insertions(+), 5 deletions(-) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index b34da39722c7..5b3b07a39105 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -2120,6 +2120,28 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) > dev_put(dp->slave); > } > > +static int dsa_lower_dev_walk(struct net_device *lower_dev, > + struct netdev_nested_priv *priv) > +{ > + if (dsa_slave_dev_check(lower_dev)) { > + priv->data = netdev_priv(lower_dev); > + return 1; > + } > + > + return 0; > +} > + > +struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev) > +{ > + struct netdev_nested_priv priv = { > + .data = NULL, > + }; > + > + netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv); > + > + return priv.data; > +} > + > /* Called under rcu_read_lock() */ > static int dsa_slave_switchdev_event(struct notifier_block *unused, > unsigned long event, void *ptr) > @@ -2140,13 +2162,32 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused, > case SWITCHDEV_FDB_DEL_TO_DEVICE: > fdb_info = ptr; > > - if (!dsa_slave_dev_check(dev)) > - return NOTIFY_DONE; > + if (dsa_slave_dev_check(dev)) { > + if (!fdb_info->added_by_user) > + return NOTIFY_OK; > + > + dp = dsa_slave_to_port(dev); > + } else { > + /* Snoop addresses learnt on foreign interfaces > + * bridged with us, for switches that don't > + * automatically learn SA from CPU-injected traffic > + */ Can it be turned off for switches that support SA learning from CPU? > + struct net_device *br_dev; > + struct dsa_slave_priv *p; > > - if (!fdb_info->added_by_user) > - return NOTIFY_OK; > + br_dev = netdev_master_upper_dev_get_rcu(dev); > + if (!br_dev) > + return NOTIFY_DONE; > > - dp = dsa_slave_to_port(dev); > + if (!netif_is_bridge_master(br_dev)) > + return NOTIFY_DONE; > + > + p = dsa_slave_dev_lower_find(br_dev); > + if (!p) > + return NOTIFY_DONE; > + > + dp = p->dp->cpu_dp; > + } > > switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); > if (!switchdev_work) > -- > 2.25.1 >
On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
> Can it be turned off for switches that support SA learning from CPU?
Is there a good reason I would add another property per switch and not
just do it unconditionally?
On Sun, Nov 08, 2020 at 07:23:55PM +0200, Vladimir Oltean wrote: > On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: > > Can it be turned off for switches that support SA learning from CPU? > > Is there a good reason I would add another property per switch and not > just do it unconditionally? Just throwing out ideas, i've no idea if they are relevant. I wonder if we can get into issues with fast ageing with a topology change? We don't have too much control over the hardware. I think some devices just flush everything, or maybe just one port. So we have different life times for CPU port database entries and user port database entries? We might also run into bugs with flushing removing static database entries which should not be. But that would be a bug. Also, dumping the database might run into bugs since we have not had entries for the CPU port before. We also need to make sure the static entries get removed correctly when a host moves. The mv88e6xxx will not replace a static entry with a dynamically learned one. It will probably rise an ATU violation interrupt that frames have come in the wrong port. What about switches which do not implement port_fdb_add? Do these patches at least do something sensible? Andrew
On Mon, Nov 09, 2020 at 12:59:39AM +0100, Andrew Lunn wrote: > On Sun, Nov 08, 2020 at 07:23:55PM +0200, Vladimir Oltean wrote: > > On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: > > > Can it be turned off for switches that support SA learning from CPU? > > > > Is there a good reason I would add another property per switch and not > > just do it unconditionally? > > Just throwing out ideas, i've no idea if they are relevant. I wonder > if we can get into issues with fast ageing with a topology change? We > don't have too much control over the hardware. I think some devices > just flush everything, or maybe just one port. So we have different > life times for CPU port database entries and user port database > entries? A quick scan for "port_fast_age" did not find any implementers who do not act upon the "port" argument. > We might also run into bugs with flushing removing static database > entries which should not be. But that would be a bug. I can imagine that happening, when there are multiple bridges spanning a DSA switch, each bridge also contains a "foreign" interface, and the 2 bridging domains service 2 stations that have the same MAC address. In that case, since the fdb_add and fdb_del are not reference-counted on the shared DSA CPU port, we would indeed trigger this bug. I was on the fence on whether to include the reference counting patch I have for host MDBs, and to make these addresses refcounted as well. What do you think? > Also, dumping the database might run into bugs since we have not had > entries for the CPU port before. I don't see what conditions can make this happen. > We also need to make sure the static entries get removed correctly > when a host moves. The mv88e6xxx will not replace a static entry with > a dynamically learned one. It will probably rise an ATU violation > interrupt that frames have come in the wrong port. This is a good one. Currently every implementer of .port_fdb_add assumes a static entry is what we want, but that is not the case here. We want an entry that can expire or the switch can move it to a different port when there is evidence that it's wrong. Should we add more arguments to the API? > What about switches which do not implement port_fdb_add? Do these > patches at least do something sensible? dsa_slave_switchdev_event -> dsa_slave_switchdev_event_work -> dsa_port_fdb_add -> dsa_port_notify(DSA_NOTIFIER_FDB_ADD) -> dsa_switch_fdb_add -> if (!ds->ops->port_fdb_add) return -EOPNOTSUPP; -> an error is printed with dev_dbg, and dsa_fdb_offload_notify(switchdev_work) is not called. On dsa_port_fdb_del error, there is also an attempt to call dev_close() on error, but only on user ports, which the CPU port is not. So, we do something almost sensible, but mostly by mistake it seems. I think the simplest would be to simply avoid all this nonsense right away in dsa_slave_switchdev_event: diff --git a/net/dsa/slave.c b/net/dsa/slave.c --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2188,6 +2188,9 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused, dp = p->dp->cpu_dp; } + if (!dp->ds->ops->port_fdb_add || !dp->ds->ops->port_fdb_del) + return NOTIFY_DONE; + switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); if (!switchdev_work) return NOTIFY_BAD;
> > We also need to make sure the static entries get removed correctly > > when a host moves. The mv88e6xxx will not replace a static entry with > > a dynamically learned one. It will probably rise an ATU violation > > interrupt that frames have come in the wrong port. > > This is a good one. Currently every implementer of .port_fdb_add assumes > a static entry is what we want, but that is not the case here. We want > an entry that can expire or the switch can move it to a different port > when there is evidence that it's wrong. I doubt you will find any hardware that actually does this. I expect there are static entries, and dynamic entries, and nothing hybrid. After a move, we need to rely on a broadcast packet making its way to the software bridge, which causes it to learn about the move, and delete the static CPU entry from the hardware. We can probably test this with having our wireless device move back and forth a few times, so we can see the full cycle a few times. Unfortunately, i don't have two boards with both a switch and WiFi. Andrew
On Mon, Nov 09, 2020 at 02:30, Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Nov 09, 2020 at 12:59:39AM +0100, Andrew Lunn wrote: >> We also need to make sure the static entries get removed correctly >> when a host moves. The mv88e6xxx will not replace a static entry with >> a dynamically learned one. It will probably rise an ATU violation >> interrupt that frames have come in the wrong port. > > This is a good one. Currently every implementer of .port_fdb_add assumes > a static entry is what we want, but that is not the case here. We want > an entry that can expire or the switch can move it to a different port > when there is evidence that it's wrong. Should we add more arguments to > the API? I don't think that would help. You would essentially be trading one situation where station moves causes loss of traffic for another one. But now you have also increased the background load of an already choked resource, the MDIO bus. At least on mv88e6xxx, your only option to allow the hardware to move the station to another port autonomously is to add the entry as a dynamically learnt one. However, since the switch does not perform any SA learning on the CPU port in this world, the entry would have to be refreshed by software, otherwise it would just age out. Then you run in to this situation: A and B are communicating. br0 .----'|'----. | | | swp0 swp1 wlan0 | | A B The switch's FDB: A: swp0 B: cpu0 (due to this patchset) Now B roams to an AP somewhere behind swp1 and continues to communicate with A. br0 .----'|'----. | | | swp0 swp1 wlan0 | | A B The switch's FDB: A: swp0 B: swp1 But br0 sees none of this, so at whatever interval we choose we will refresh the FDB, moving the station back to the cpu: A: swp0 B: cpu0 So now you have traded the issue of having to wait for the hardware to age out its entry, to the issue of having to wait for br0 to age out its entry. Right?
On Mon, Nov 09, 2020 at 09:09:37AM +0100, Tobias Waldekranz wrote: > On Mon, Nov 09, 2020 at 02:30, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Mon, Nov 09, 2020 at 12:59:39AM +0100, Andrew Lunn wrote: > >> We also need to make sure the static entries get removed correctly > >> when a host moves. The mv88e6xxx will not replace a static entry with > >> a dynamically learned one. It will probably rise an ATU violation > >> interrupt that frames have come in the wrong port. > > > > This is a good one. Currently every implementer of .port_fdb_add assumes > > a static entry is what we want, but that is not the case here. We want > > an entry that can expire or the switch can move it to a different port > > when there is evidence that it's wrong. Should we add more arguments to > > the API? > > I don't think that would help. You would essentially be trading one > situation where station moves causes loss of traffic for another > one. But now you have also increased the background load of an already > choked resource, the MDIO bus. In practice, DSA switches are already very demanding of their management interface throughput, for PTP and things like that. I do expect that if you spent any significant amount of time with DSA, you already know the ins and outs of your MDIO/SPI/I2C controller and it would already be optimized for efficiency. But ok, we can add this to the list of cons. > At least on mv88e6xxx, your only option to allow the hardware to move > the station to another port autonomously is to add the entry as a > dynamically learnt one. However, since the switch does not perform any > SA learning on the CPU port in this world, the entry would have to be > refreshed by software, otherwise it would just age out. > > Then you run in to this situation: > > A and B are communicating. > > br0 > .----'|'----. > | | | > swp0 swp1 wlan0 > | | > A B > > The switch's FDB: > A: swp0 > B: cpu0 (due to this patchset) > > Now B roams to an AP somewhere behind swp1 and continues to communicate > with A. > > br0 > .----'|'----. > | | | > swp0 swp1 wlan0 > | | > A B > > The switch's FDB: > A: swp0 > B: swp1 > > But br0 sees none of this, so at whatever interval we choose we will > refresh the FDB, moving the station back to the cpu: > > A: swp0 > B: cpu0 No, br0 should see some traffic from station B. Not the unicast traffic towards station A, of course (because that has already been learnt to go towards swp0), but some broadcast ARP, or some multicast ND. This is the big assumption behind any solution: that the stations are not silent and make their presence known somehow. > So now you have traded the issue of having to wait for the hardware to > age out its entry, to the issue of having to wait for br0 to age out its > entry. Right? That's the thing. The software bridge will never expire its entry in br_fdb_update if traffic is continuously coming in. fdb->updated will just keep getting larger and larger after each incoming packet. But the hardware bridge is not aware of this traffic. So: - if the hardware bridge has a dynamic entry installed (one that's subject to ageing), that entry will eventually expire within 5 minutes when its software equivalent won't. Then no switchdev event will ever come back to update the hardware bridge, since from the software's perspective it was never supposed to expire. It's as if we _do_ want the entry to be static. But: - if the hardware bridge has a static entry installed, then that entry might become wrong and cause connectivity loss until the software bridge figures it out. It's what Andrew described as a 'hybrid' entry. We would want a 'static' entry (one that doesn't age out based on a timer) that is 'weak' (can be overridden when traffic comes in on a different port). I'm not sure either that such thing exists. So for now, static entries are the best we've got. Let's re-run the simulation knowing that we're working with static addresses towards the CPU, to see how bad things are. AP 1: +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ | ^ ^ | | | | | | | Client A Client B | | | +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ AP 2 - br0 of AP 1 will lean that Clients A and B are reachable via wlan0. The DSA switch will snoop these and add static entries towards the CPU port. - the hardware fdb of the DSA switch, as well as br0 on AP 2, will learn that Clients A and B are reachable through swp0, because of our assumption of non-silent stations. There are no static entries involved on AP 2 for now. Client B disconnects from AP 1 and roams to AP 2. AP 1: +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ | ^ | | | Client A | | | Client B | | | v +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ AP 2 - br0 of AP 1 still knows that Client A is reachable via wlan0 (no change) - In the general case, br0 of AP 1 will _NOT_ know that Client B has left wlan0. So there is still a static entry for Client B towards the CPU port. - Right now, any attempt from Client A to directly address Client B via unicast would result, if the FDB were to be consulted, in packet drops, because the switch on AP 1 would say 'wait a minute, I'm receiving a packet for Client B from the CPU port, but Client B is reachable via the CPU port!'. Luckily for us, the switches that we're working with are not looking up the FDB for CPU injected traffic, remember? So I don't think this is a problem. So unicast packets would be delivered to anywhere that the software bridge wanted to. Right now, even the software bridge has a wrong impression of where Client B is. - remember the assumption that Client B is not silent at startup. So some broadcast packets with Client B's source MAC address will reach the Ethernet segment. The hardware switch on AP 1 will have no problem accepting these packets, since they are broadcast/multicast. They will reach the software bridge. At this point, the software bridge finally learns the new destination for Client B, and it emits a new SWITCHDEV_FDB_ADD_TO_DEVICE event. Today we ignore that, because added_by_user will be false. That's what we do wrong/incomplete in this RFC patch set. We should keep track of static addresses installed on the CPU port, and if we ever receive a !added_by_user notification on a DSA switch port for one of those addresses, we should update the hardware FDB of the DSA switch on AP 1. So there you have it, it's not that bad. More work needs to be done, but IMO it's still workable. But now maybe it makes more sense to treat the switches that perform hardware SA learning on the CPU port separately, after I've digested this a bit.
On Mon, Nov 09, 2020 at 12:03, Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Nov 09, 2020 at 09:09:37AM +0100, Tobias Waldekranz wrote: >> one. But now you have also increased the background load of an already >> choked resource, the MDIO bus. > > In practice, DSA switches are already very demanding of their management > interface throughput, for PTP and things like that. I do expect that if > you spent any significant amount of time with DSA, you already know the > ins and outs of your MDIO/SPI/I2C controller and it would already be > optimized for efficiency. But ok, we can add this to the list of cons. You are arguing for my position though, no? Yes it is demanding; that is why we must allocate it carefully. > So there you have it, it's not that bad. More work needs to be done, but > IMO it's still workable. If you bypass learning on all frames sent from the CPU (as today), yes I agree that you should be able to solve it with static entries. But I think that you will have lots of weird problems with initial packet loss as the FDB updates are not synchronous with the packet flow. I.e. the bridge will tell DSA to update the entry, but the update in HW will occur some time later when the workqueue actually performs the operation. > But now maybe it makes more sense to treat the switches that perform > hardware SA learning on the CPU port separately, after I've digested > this a bit. Yes, please. Because it will be impossible to add tx forward offloading otherwise.
On Mon, Nov 09, 2020 at 12:05:19PM +0100, Tobias Waldekranz wrote: > On Mon, Nov 09, 2020 at 12:03, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Mon, Nov 09, 2020 at 09:09:37AM +0100, Tobias Waldekranz wrote: > >> one. But now you have also increased the background load of an already > >> choked resource, the MDIO bus. > > > > In practice, DSA switches are already very demanding of their management > > interface throughput, for PTP and things like that. I do expect that if > > you spent any significant amount of time with DSA, you already know the > > ins and outs of your MDIO/SPI/I2C controller and it would already be > > optimized for efficiency. But ok, we can add this to the list of cons. > > You are arguing for my position though, no? Yes it is demanding; that is > why we must allocate it carefully. Yes, if the change brings additional load to the MDIO/SPI/I2C link and doesn't bring any benefit, then it makes sense to skip it. > > So there you have it, it's not that bad. More work needs to be done, but > > IMO it's still workable. > > If you bypass learning on all frames sent from the CPU (as today), yes I > agree that you should be able to solve it with static entries. But I > think that you will have lots of weird problems with initial packet loss > as the FDB updates are not synchronous with the packet flow. I.e. the > bridge will tell DSA to update the entry, but the update in HW will > occur some time later when the workqueue actually performs the > operation. I don't know how bad this is in practice. It's surely better than waiting 5 minutes though. > > But now maybe it makes more sense to treat the switches that perform > > hardware SA learning on the CPU port separately, after I've digested > > this a bit. > > Yes, please. Because it will be impossible to add tx forward offloading > otherwise. Ok, so this change, when applied to mv88e6xxx, would preclude you from using FORWARD frames for your other application of that feature, unless you explicitly turn off SA learning for FORWARD frames coming the CPU port, case in which you would still be ok. I need to sit on this for a while. How many DSA drivers do we have that don't do SA learning in hardware for CPU-injected packets? ocelot/felix and mv88e6xxx? Who else? Because if there aren't that many (or any at all except for these two), then I could try to spend some time and see how Felix behaves when I send FORWARD frames to it. Then we could go on full blast with the other alternative, to force-enable address learning from the CPU port, and declare this one as too complicated and not worth the effort.
On Mon, Nov 09, 2020 at 02:31:11PM +0200, Vladimir Oltean wrote: > I need to sit on this for a while. How many DSA drivers do we have that > don't do SA learning in hardware for CPU-injected packets? ocelot/felix > and mv88e6xxx? Who else? Because if there aren't that many (or any at > all except for these two), then I could try to spend some time and see > how Felix behaves when I send FORWARD frames to it. Then we could go on > full blast with the other alternative, to force-enable address learning > from the CPU port, and declare this one as too complicated and not worth > the effort. In fact I'm not sure that I should be expecting an answer to this question. We can evaluate the other alternative in parallel. Would you be so kind to send some sort of RFC for your TX-side offload_fwd_mark so that I could test with the hardware I have, and get a better understanding of the limitations there?
On Mon Nov 9, 2020 at 3:38 PM CET, Vladimir Oltean wrote: > On Mon, Nov 09, 2020 at 02:31:11PM +0200, Vladimir Oltean wrote: > > I need to sit on this for a while. How many DSA drivers do we have that > > don't do SA learning in hardware for CPU-injected packets? ocelot/felix > > and mv88e6xxx? Who else? Because if there aren't that many (or any at > > all except for these two), then I could try to spend some time and see > > how Felix behaves when I send FORWARD frames to it. Then we could go on > > full blast with the other alternative, to force-enable address learning > > from the CPU port, and declare this one as too complicated and not worth > > the effort. > > In fact I'm not sure that I should be expecting an answer to this > question. We can evaluate the other alternative in parallel. Would you > be so kind to send some sort of RFC for your TX-side offload_fwd_mark so > that I could test with the hardware I have, and get a better > understanding > of the limitations there? That is the plan. I have some stuff I need to get done before though. The current implementation is on a 4.19 kernel, so it's going to take some time to rebase it.
On 08.11.20 18:23, Vladimir Oltean wrote: > On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: >> Can it be turned off for switches that support SA learning from CPU? > > Is there a good reason I would add another property per switch and not > just do it unconditionally? > I have a similar concern for a future patch, where I want to turn on or off, whether the device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface. (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device) What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message?
Hi Alexandra, On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote: > On 08.11.20 18:23, Vladimir Oltean wrote: > > On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: > >> Can it be turned off for switches that support SA learning from CPU? > > > > Is there a good reason I would add another property per switch and not > > just do it unconditionally? > > > I have a similar concern for a future patch, where I want to turn on or off, whether the > device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface. > (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device) > What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message? My understanding is that "learning_sync" is for pushing learnt addresses from device to bridge, not from bridge to device.
On 11.11.20 11:36, Vladimir Oltean wrote: > Hi Alexandra, > > On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote: >> On 08.11.20 18:23, Vladimir Oltean wrote: >>> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: >>>> Can it be turned off for switches that support SA learning from CPU? >>> >>> Is there a good reason I would add another property per switch and not >>> just do it unconditionally? >>> >> I have a similar concern for a future patch, where I want to turn on or off, whether the >> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface. >> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device) >> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message? > > My understanding is that "learning_sync" is for pushing learnt addresses > from device to bridge, not from bridge to device. > uh, sorry copy-paste error. I meant: 'bridge link set dev $netdev learning on self'
On Wed, Nov 11, 2020 at 03:14:26PM +0100, Alexandra Winter wrote: > On 11.11.20 11:36, Vladimir Oltean wrote: > > Hi Alexandra, > > > > On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote: > >> On 08.11.20 18:23, Vladimir Oltean wrote: > >>> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: > >>>> Can it be turned off for switches that support SA learning from CPU? > >>> > >>> Is there a good reason I would add another property per switch and not > >>> just do it unconditionally? > >>> > >> I have a similar concern for a future patch, where I want to turn on or off, whether the > >> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface. > >> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device) > >> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message? > > > > My understanding is that "learning_sync" is for pushing learnt addresses > > from device to bridge, not from bridge to device. > > > uh, sorry copy-paste error. I meant: > 'bridge link set dev $netdev learning on self' Even with "learning" instead of "learning_sync", I don't understand what the "self" modifier would mean and how it would help, sorry.
On 11/9/2020 4:38 AM, Vladimir Oltean wrote: > On Mon, Nov 09, 2020 at 02:31:11PM +0200, Vladimir Oltean wrote: >> I need to sit on this for a while. How many DSA drivers do we have that >> don't do SA learning in hardware for CPU-injected packets? ocelot/felix >> and mv88e6xxx? Who else? Because if there aren't that many (or any at >> all except for these two), then I could try to spend some time and see >> how Felix behaves when I send FORWARD frames to it. Then we could go on >> full blast with the other alternative, to force-enable address learning >> from the CPU port, and declare this one as too complicated and not worth >> the effort. > > In fact I'm not sure that I should be expecting an answer to this > question. We can evaluate the other alternative in parallel. Would you > be so kind to send some sort of RFC for your TX-side offload_fwd_mark so > that I could test with the hardware I have, and get a better understanding > of the limitations there? For Broadcom switches, ARL (Address Resolution Logic, where learning happens) is bypassed when packets ingress the CPU port with opcode 1 which is what net/dsa/tag_brcm.c uses. When opcode 0 is used, address learning occurs. The reason why opcode 1 is used is because of the Advanced Congestion Buffering (ACB) which requires us to steer packets towards a particular switch port and egress queue number within that port. With opcode 0 we would not be able to do that. We could make the opcode dependent on the switch/DSA master since not all combinations support ACB, but given we have 3 or 4 Ethernet switches kind within DSA that do not do learning from the CPU port, I guess we need a solution to that problem somehow.
On 12.11.20 14:49, Vladimir Oltean wrote: > On Wed, Nov 11, 2020 at 03:14:26PM +0100, Alexandra Winter wrote: >> On 11.11.20 11:36, Vladimir Oltean wrote: >>> Hi Alexandra, >>> >>> On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote: >>>> On 08.11.20 18:23, Vladimir Oltean wrote: >>>>> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote: >>>>>> Can it be turned off for switches that support SA learning from CPU? >>>>> >>>>> Is there a good reason I would add another property per switch and not >>>>> just do it unconditionally? >>>>> >>>> I have a similar concern for a future patch, where I want to turn on or off, whether the >>>> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface. >>>> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device) >>>> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message? >>> >>> My understanding is that "learning_sync" is for pushing learnt addresses >>> from device to bridge, not from bridge to device. >>> >> uh, sorry copy-paste error. I meant: >> 'bridge link set dev $netdev learning on self' > > Even with "learning" instead of "learning_sync", I don't understand what > the "self" modifier would mean and how it would help, sorry. > With the self modifier, the command is not executed by the linux bridge but instead sent to the device driver of the respective bridgeport. So AFAIU 'learning on self' would mean, that instead of only the bridge doing the learning on this bridgeport, the device itself should do the learning. Which sounds to me like a good fit for SA learning from CPU. It seems that the self modifier is not used on hardware switches today, but rather on virtualized network cards, where it is e.g. used ot turn VEPA mode on or off per virtual interface. In drivers/s390/net/qeth we use 'learning_sync on self', to control whether a virtualized interface should synchronize the card's fdb with an attached linux bridge.
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index b34da39722c7..5b3b07a39105 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2120,6 +2120,28 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) dev_put(dp->slave); } +static int dsa_lower_dev_walk(struct net_device *lower_dev, + struct netdev_nested_priv *priv) +{ + if (dsa_slave_dev_check(lower_dev)) { + priv->data = netdev_priv(lower_dev); + return 1; + } + + return 0; +} + +struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev) +{ + struct netdev_nested_priv priv = { + .data = NULL, + }; + + netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv); + + return priv.data; +} + /* Called under rcu_read_lock() */ static int dsa_slave_switchdev_event(struct notifier_block *unused, unsigned long event, void *ptr) @@ -2140,13 +2162,32 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused, case SWITCHDEV_FDB_DEL_TO_DEVICE: fdb_info = ptr; - if (!dsa_slave_dev_check(dev)) - return NOTIFY_DONE; + if (dsa_slave_dev_check(dev)) { + if (!fdb_info->added_by_user) + return NOTIFY_OK; + + dp = dsa_slave_to_port(dev); + } else { + /* Snoop addresses learnt on foreign interfaces + * bridged with us, for switches that don't + * automatically learn SA from CPU-injected traffic + */ + struct net_device *br_dev; + struct dsa_slave_priv *p; - if (!fdb_info->added_by_user) - return NOTIFY_OK; + br_dev = netdev_master_upper_dev_get_rcu(dev); + if (!br_dev) + return NOTIFY_DONE; - dp = dsa_slave_to_port(dev); + if (!netif_is_bridge_master(br_dev)) + return NOTIFY_DONE; + + p = dsa_slave_dev_lower_find(br_dev); + if (!p) + return NOTIFY_DONE; + + dp = p->dp->cpu_dp; + } switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); if (!switchdev_work)
Some DSA switches (and not only) cannot learn source MAC addresses from packets injected from the CPU. They only perform hardware address learning from inbound traffic. This can be problematic when we have a bridge spanning some DSA switch ports and some non-DSA ports (which we'll call "foreign interfaces" from DSA's perspective). There are 2 classes of problems created by the lack of learning on CPU-injected traffic: - excessive flooding, due to the fact that DSA treats those addresses as unknown - the risk of stale routes, which can lead to temporary packet loss To illustrate the second class, consider the following situation, which is common in production equipment (wireless access points, where there is a WLAN interface and an Ethernet switch, and these form a single bridging domain). AP 1: +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ | ^ ^ | | | | | | | Client A Client B | | | +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ AP 2 - br0 of AP 1 will know that Clients A and B are reachable via wlan0 - the hardware fdb of a DSA switch driver today is not kept in sync with the software entries on other bridge ports, so it will not know that clients A and B are reachable via the CPU port UNLESS the hardware switch itself performs SA learning from traffic injected from the CPU. Nonetheless, a substantial number of switches don't. - the hardware fdb of the DSA switch on AP 2 may autonomously learn that Client A and B are reachable through swp0. Therefore, the software br0 of AP 2 also may or may not learn this. In the example we're illustrating, some Ethernet traffic has been going on, and br0 from AP 2 has indeed learnt that it can reach Client B through swp0. One of the wireless clients, say Client B, disconnects from AP 1 and roams to AP 2. The topology now looks like this: AP 1: +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ | ^ | | | Client A | | | Client B | | | v +------------+ +------------+ +------------+ +------------+ +------------+ | swp0 | | swp1 | | swp2 | | swp3 | | wlan0 | +------------+ +------------+ +------------+ +------------+ +------------+ +------------------------------------------------------------------------+ | br0 | +------------------------------------------------------------------------+ AP 2 - br0 of AP 1 still knows that Client A is reachable via wlan0 (no change) - br0 of AP 1 will (possibly) know that Client B has left wlan0. There are cases where it might never find out though. Either way, DSA today does not process that notification in any way. - the hardware FDB of the DSA switch on AP 1 may learn autonomously that Client B can be reached via swp0, if it receives any packet with Client 1's source MAC address over Ethernet. - the hardware FDB of the DSA switch on AP 2 still thinks that Client B can be reached via swp0. It does not know that it has roamed to wlan0, because it doesn't perform SA learning from the CPU port. Now Client A contacts Client B. AP 1 routes the packet fine towards swp0 and delivers it on the Ethernet segment. AP 2 sees a frame on swp0 and its fdb says that the destination is swp0. Hairpinning is disabled => drop. This problem comes from the fact that these switches have a 'blind spot' for addresses coming from software bridging. The generic solution is not to assume that hardware learning can be enabled somehow, but to listen to more bridge learning events. It turns out that the bridge driver does learn in software from all inbound frames, in __br_handle_local_finish. A proper SWITCHDEV_FDB_ADD_TO_DEVICE notification is emitted for the addresses serviced by the bridge on 'foreign' interfaces. The problem is that DSA currently only cares about SWITCHDEV_FDB_ADD_TO_DEVICE events received on its own interfaces, such as static FDB entries. Luckily we can change that, and DSA can listen to all switchdev FDB add/del events in the system and figure out if those events were emitted by a bridge that spans at least one of DSA's own ports. In case that is true, DSA will also offload that address towards its own CPU port, in the eventuality that there might be bridge clients attached to the DSA switch who want to talk to the station connected to the foreign interface. Reported-by: DENG Qingfang <dqfext@gmail.com> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- net/dsa/slave.c | 51 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 5 deletions(-)