| Message ID | 20210214155326.1783266-1-olteanv@gmail.com |
|---|---|
| Headers | show |
| Series | Software fallback for bridging in DSA | expand |
On Sun, Feb 14, 2021 at 9:54 AM Vladimir Oltean <olteanv@gmail.com> wrote: [snip] > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c > index 858cdf9d2913..215ecceea89e 100644 > --- a/net/dsa/tag_xrs700x.c > +++ b/net/dsa/tag_xrs700x.c > @@ -45,8 +45,7 @@ static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, > if (pskb_trim_rcsum(skb, skb->len - 1)) > return NULL; > > - /* Frame is forwarded by hardware, don't forward in software. */ > - skb->offload_fwd_mark = 1; > + dsa_default_offload_fwd_mark(skb); Does it make sense that the following would have worked prior to this change? Is this only an issue for bridging between DSA ports when offloading is supported? lan0 is a port an an xrs700x switch: ip link set eth0 up ip link del veth0 ip link add veth0 type veth peer name veth1 for eth in veth0 veth1 lan1; do ip link set ${eth} up done ip link add br0 type bridge ip link set veth1 master br0 ip link set lan1 master br0 ip link set br0 up ip addr add 192.168.2.1/24 dev veth0 # ping host connected to physical LAN that lan0 is on ping 192.168.2.249 (works!) I was trying to come up with a way to test this change and expected this would fail (and your patch) would fix it based on what you're described. -George > > return skb; > } > -- > 2.25.1 >
Hi George, On Mon, Feb 15, 2021 at 09:48:38AM -0600, George McCollister wrote: > On Sun, Feb 14, 2021 at 9:54 AM Vladimir Oltean <olteanv@gmail.com> wrote: > [snip] > > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c > > index 858cdf9d2913..215ecceea89e 100644 > > --- a/net/dsa/tag_xrs700x.c > > +++ b/net/dsa/tag_xrs700x.c > > @@ -45,8 +45,7 @@ static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, > > if (pskb_trim_rcsum(skb, skb->len - 1)) > > return NULL; > > > > - /* Frame is forwarded by hardware, don't forward in software. */ > > - skb->offload_fwd_mark = 1; > > + dsa_default_offload_fwd_mark(skb); > > Does it make sense that the following would have worked prior to this > change? Is this only an issue for bridging between DSA ports when > offloading is supported? lan0 is a port an an xrs700x switch: > > ip link set eth0 up > ip link del veth0 > ip link add veth0 type veth peer name veth1 > > for eth in veth0 veth1 lan1; do > ip link set ${eth} up > done > ip link add br0 type bridge > ip link set veth1 master br0 > ip link set lan1 master br0 > ip link set br0 up > > ip addr add 192.168.2.1/24 dev veth0 > > # ping host connected to physical LAN that lan0 is on > ping 192.168.2.249 (works!) > > I was trying to come up with a way to test this change and expected > this would fail (and your patch) would fix it based on what you're > described. No, the configuration you've shown should be supported and functional already (as you've noticed, in fact). I call it 'bridging with a foreign interface', where a foreign interface is a bridge port that has a different switchdev mark compared to the DSA switch. A switchdev mark is a number assigned to every bridge port by nbp_switchdev_mark_set, based on the "physical switch id"*. There is a simple rule with switchdev: on reception of an skb, the bridge checks if it was marked as 'already forwarded in hardware' (checks if skb->offload_fwd_mark == 1), and if it is, it puts a mark of its own on that skb, with the switchdev mark of the ingress port. Then during forwarding, it enforces that the egress port must have a different switchdev mark than the ingress one (this is done in nbp_switchdev_allowed_egress). The veth driver does not implement any sort of method for retrieving a physical switch id (neither devlink nor .ndo_get_port_parent_id), therefore the bridge assigns it a switchdev mark of 0, and packets coming from it will always have skb->offload_fwd_mark = 0. So there aren't any restrictions. Problems appear as soon as software bridging is attempted between two interfaces that have the same switchdev mark. If skb->offload_fwd_mark=1, the bridge will say that forwarding was already performed in hw, so it will deny it in sw. The issue is that a bond0 (or hsr0) upper of lan0 will be assigned the same switchdev mark as lan0 itself, because the function that assigns switchdev marks to bridge ports, nbp_switchdev_mark_set, recurses through that port's lower interfaces until it finds something that implements devlink. What I tested is actually pretty laughable and a far cry from a real-life scenario: I commented out the .port_bridge_join and .port_bridge_leave methods of a driver and made sure that forwarding between ports still works regardless of what uppers they have (even that used not to). But this bypasses the switchdev mark checks in nbp_switchdev_allowed_egress because the skb->offload_fwd_mark=0 now. This is an important prerequisite for seamless operation, true, but it isn't quite what we want. For one thing, we may have a topology like this: +-- br0 -+ / / | \ / / | \ / / | \ / / | \ / / | \ / | | bond0 / | | / \ swp0 swp1 swp2 swp3 swp4 where it is desirable that the presence of swp3 and swp4 under a non-offloaded LAG does not preclude us from doing hardware bridging beteen swp0, swp1 and swp2. But this creates an impossible paradox if we continue in the way that I started in this patch. When the CPU receives a packet from swp0 (say, due to flooding), the tagger must set skb->offload_fwd_mark to something. If we set it to 0, then the bridge will forward it towards swp1, swp2 and bond0. But the switch has already forwarded it towards swp1 and swp2 (not to bond0, remember, that isn't offloaded, so as far as the switch is concerned, ports swp3 and swp4 are not looking up the FDB, and the entire bond0 is a destination that is strictly behind the CPU). But we don't want duplicated traffic towards swp1 and swp2, so it's not ok to set skb->offload_fwd_mark = 0. If we set it to 1, then the bridge will not forward the skb towards the ports with the same switchdev mark, i.e. not to swp1, swp2 and bond0. Towards swp1 and swp2 that's ok, but towards bond0? It should have forwarded the skb there. An actual solution to this problem, which has nothing to do with my series, is to give the bridge more hints as to what switchdev mark it should use for each port. Currently, the bridging offload is very 'silent': a driver registers a netdevice notifier, which is put on the netns's notifier chain, and which sniffs around for NETDEV_CHANGEUPPER events where the upper is a bridge, and the lower is an interface it knows about (one registered by this driver, normally). Then, from within that notifier, it does a bunch of stuff behind the bridge's back, without the bridge necessarily knowing that there's somebody offloading that port. It looks like this: ip link set lan0 master br0 | v bridge calls netdev_master_upper_dev_link | v call_netdevice_notifiers | v dsa_slave_netdevice_event | v oh, hey! it's for me! | v .port_bridge_join What we should probably do to solve the conundrum is to be less silent, and emit a notification back. Something like this: ip link set lan0 master br0 | v bridge calls netdev_master_upper_dev_link | v bridge: Aye! I'll use this call_netdevice_notifiers ^ switch_id as the | | switchdev mark for v | this port, and zero dsa_slave_netdevice_event | if I got nothing. | | v | oh, hey! it's for me! | | | v | .port_bridge_join | | | +------------------------+ call_switchdev_notifiers(lan0, SWITCHDEV_BRPORT_OFFLOADED, switch_id) Then stacked interfaces (like bond0 on top of swp3/swp4) would be treated differently in DSA, depending on whether we can or cannot offload them. The offload case: ip link set bond0 master br0 | v bridge calls netdev_master_upper_dev_link | v bridge: Aye! I'll use this call_netdevice_notifiers ^ switch_id as the | | switchdev mark for v | bond0. dsa_slave_netdevice_event | Coincidentally (or not), | | bond0 and swp0, swp1, swp2 v | all have the same switchdev hmm, it's not quite for me, | mark now, since the ASIC but my driver has already | is able to forward towards called .port_lag_join | all these ports in hw. for it, because I have | a port with dp->lag_dev == bond0. | | | v | .port_bridge_join | for swp3 and swp4 | | | +------------------------+ call_switchdev_notifiers(bond0, SWITCHDEV_BRPORT_OFFLOADED, switch_id) And the non-offload case: ip link set bond0 master br0 | v bridge calls netdev_master_upper_dev_link | v bridge waiting: call_netdevice_notifiers ^ huh, no SWITCHDEV_BRPORT_OFFLOADED | | event, okay, I'll use a switchdev v | mark of zero for this one. dsa_slave_netdevice_event : Then packets received on swp0 will | : not be forwarded towards swp1, but v : they will towards bond0. it's not for me, but bond0 is an upper of swp3 and swp4, but their dp->lag_dev is NULL because they couldn't offload it. This is what I should have really done. For some reason though, I was so trigger-happy that I got the data path working (without the surrounding control logic to manage the switchdev marks automatically) that I just got carried away and sent this small patch set. I need some time to take my mind off of this for a while, and then I'll come with a serious proposal eventually. Sorry again for the confusion. *This is retrieved, in DSA's case, through the "switch_id" attribute that we populate in dsa_port_devlink_setup. DSA says that the entire DSA switch tree dst has the same switch_id, because it assumes that any driver capable of cross-chip bridging (aka Marvell) is able to do hardware forwarding towards any other switch in the same "switching fabric". So it's not really a "switch_id", but a "port parent" somehow.
On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Starting with commit 058102a6e9eb ("net: dsa: Link aggregation support"), > DSA warns that certain configurations of upper interfaces are not offloaded > to hardware. When a DSA port does not offload a LAG interface, the > dp->lag_dev pointer is always NULL. However the same cannot be said about > offloading a bridge: dp->bridge_dev will get populated regardless of > whether the driver can put the port into the bridge's forwarding domain > or not. > > Instead of silently returning 0 if the driver doesn't implement > .port_bridge_join, return -EOPNOTSUPP instead, and print a message via > netlink extack that the configuration was not offloaded to hardware. > > Now we can use the check whether dp->bridge_dev is NULL in order to > avoid offloading at all switchdev attributes and objects for ports that > don't even offload the basic operation of switching. Those can still do > the required L2 forwarding using the bridge software datapath, but > enabling any hardware features specific to the bridge such as address > learning would just ask for problems. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The dsa_port_offloads_netdev check is inside dsa_slave_vlan_{add,del}, > but outside dsa_port_mdb_{add,del}. We can reduce the number of > occurrences of dsa_port_offloads_netdev by checking only once, at the > beginning of dsa_slave_port_obj_add and dsa_slave_port_obj_del. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > DSA has gained the recent ability to deal gracefully with upper > interfaces it cannot offload, such as the bridge, bonding or team > drivers. When such uppers exist, the ports are still in standalone mode > as far as the hardware is concerned. > > But when we deliver packets to the software bridge in order for that to > do the forwarding, there is an unpleasant surprise in that the bridge > will refuse to forward them. This is because we unconditionally set > skb->offload_fwd_mark = true, meaning that the bridge thinks the frames > were already forwarded in hardware by us. > > Since dp->bridge_dev is populated only when there is hardware offload > for it, but not in the software fallback case, let's introduce a new > helper that can be called from the tagger data path which sets the > skb->offload_fwd_mark accordingly to zero when there is no hardware > offload for bridging. This lets the bridge forward packets back to other > interfaces of our switch, if needed. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- For the generic and tag_dsa.c related changes: Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
On Thu, Feb 25, 2021 at 08:25:23PM +0100, Tobias Waldekranz wrote: > On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > DSA has gained the recent ability to deal gracefully with upper > > interfaces it cannot offload, such as the bridge, bonding or team > > drivers. When such uppers exist, the ports are still in standalone mode > > as far as the hardware is concerned. > > > > But when we deliver packets to the software bridge in order for that to > > do the forwarding, there is an unpleasant surprise in that the bridge > > will refuse to forward them. This is because we unconditionally set > > skb->offload_fwd_mark = true, meaning that the bridge thinks the frames > > were already forwarded in hardware by us. > > > > Since dp->bridge_dev is populated only when there is hardware offload > > for it, but not in the software fallback case, let's introduce a new > > helper that can be called from the tagger data path which sets the > > skb->offload_fwd_mark accordingly to zero when there is no hardware > > offload for bridging. This lets the bridge forward packets back to other > > interfaces of our switch, if needed. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > --- > > For the generic and tag_dsa.c related changes: > > Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com> Actually with my switchdev_bridge_port_offload_notify() proposal, I don't think this patch is going to be needed at all. I think the bridge happily ignores a packet with skb->offload_fwd_mark = 1 if it comes from a port which has an offload_fwd_mark of 0, although I haven't tested that.
From: Vladimir Oltean <vladimir.oltean@nxp.com> As was discussed here: https://patchwork.kernel.org/project/netdevbpf/patch/20201202091356.24075-3-tobias@waldekranz.com/ it is desirable to not reject a LAG interface (bonding, team) even if the switch isn't able to offload bridging towards that link aggregation group. At least the DSA setups I have are not that unbalanced between the horsepower of the CPU and the horsepower of the switch such that software forwarding to be completely impractical. This series makes all switch drivers theoretically able to do the right thing when they are configured in a way similar to this (credits to Tobias Waldekranz for the drawing): br0 / \ team0 \ / \ \ swp0 swp1 swp2 although in practice there is one more prerequisite: for software fallback mode, they need to disable address learning. It is preferable that they do this by implementing the .port_pre_bridge_join and .port_bridge_join methods. Vladimir Oltean (4): net: dsa: don't offload switchdev objects on ports that don't offload the bridge net: dsa: reject switchdev objects centrally from dsa_slave_port_obj_{add,del} net: dsa: return -EOPNOTSUPP if .port_lag_join is not implemented net: dsa: don't set skb->offload_fwd_mark when not offloading the bridge net/dsa/dsa_priv.h | 16 ++++++++++++++++ net/dsa/slave.c | 21 +++++++++++---------- net/dsa/switch.c | 13 ++++++++++--- net/dsa/tag_brcm.c | 2 +- net/dsa/tag_dsa.c | 9 +++++---- net/dsa/tag_hellcreek.c | 2 +- net/dsa/tag_ksz.c | 2 +- net/dsa/tag_lan9303.c | 4 +++- net/dsa/tag_mtk.c | 2 +- net/dsa/tag_ocelot.c | 2 +- net/dsa/tag_ocelot_8021q.c | 2 +- net/dsa/tag_rtl4_a.c | 2 +- net/dsa/tag_sja1105.c | 4 ++-- net/dsa/tag_xrs700x.c | 3 +-- 14 files changed, 55 insertions(+), 29 deletions(-)