| Message ID | 20210214155326.1783266-5-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Software fallback for bridging in DSA | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: linux-mediatek@lists.infradead.org matthias.bgg@gmail.com linux-arm-kernel@lists.infradead.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 151 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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> > > 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.
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 7b0dd2d5f3f8..4226ce1967d3 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -326,6 +326,20 @@ static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb) return skb; } +/* If the ingress port offloads the bridge, we mark the frame as autonomously + * forwarded by hardware, so the software bridge doesn't forward in twice, back + * to us, because we already did. However, if we're in fallback mode and we do + * software bridging, we are not offloading it, therefore the dp->bridge_dev + * pointer is not populated, and flooding needs to be done by software (we are + * effectively operating in standalone ports mode). + */ +static inline void dsa_default_offload_fwd_mark(struct sk_buff *skb) +{ + struct dsa_port *dp = dsa_slave_to_port(skb->dev); + + skb->offload_fwd_mark = !!(dp->bridge_dev); +} + /* switch.c */ int dsa_switch_register_notifier(struct dsa_switch *ds); void dsa_switch_unregister_notifier(struct dsa_switch *ds); diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c index e2577a7dcbca..a8880b3bb106 100644 --- a/net/dsa/tag_brcm.c +++ b/net/dsa/tag_brcm.c @@ -150,7 +150,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb, /* Remove Broadcom tag and update checksum */ skb_pull_rcsum(skb, BRCM_TAG_LEN); - skb->offload_fwd_mark = 1; + dsa_default_offload_fwd_mark(skb); return skb; } diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c index 7e7b7decdf39..91f405b59f75 100644 --- a/net/dsa/tag_dsa.c +++ b/net/dsa/tag_dsa.c @@ -162,8 +162,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, u8 extra) { + bool trap = false, trunk = false; int source_device, source_port; - bool trunk = false; enum dsa_code code; enum dsa_cmd cmd; u8 *dsa_header; @@ -174,8 +174,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, cmd = dsa_header[0] >> 6; switch (cmd) { case DSA_CMD_FORWARD: - skb->offload_fwd_mark = 1; - trunk = !!(dsa_header[1] & 7); break; @@ -194,7 +192,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, * device (like a bridge) that forwarding has * already been done by hardware. */ - skb->offload_fwd_mark = 1; break; case DSA_CODE_MGMT_TRAP: case DSA_CODE_IGMP_MLD_TRAP: @@ -202,6 +199,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, /* Traps have, by definition, not been * forwarded by hardware, so don't mark them. */ + trap = true; break; default: /* Reserved code, this could be anything. Drop @@ -235,6 +233,9 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, if (!skb->dev) return NULL; + if (!trap) + dsa_default_offload_fwd_mark(skb); + /* If the 'tagged' bit is set; convert the DSA tag to a 802.1Q * tag, and delete the ethertype (extra) if applicable. If the * 'tagged' bit is cleared; delete the DSA tag, and ethertype diff --git a/net/dsa/tag_hellcreek.c b/net/dsa/tag_hellcreek.c index a09805c8e1ab..c1ee6eefafe4 100644 --- a/net/dsa/tag_hellcreek.c +++ b/net/dsa/tag_hellcreek.c @@ -44,7 +44,7 @@ static struct sk_buff *hellcreek_rcv(struct sk_buff *skb, pskb_trim_rcsum(skb, skb->len - HELLCREEK_TAG_LEN); - skb->offload_fwd_mark = true; + dsa_default_offload_fwd_mark(skb); return skb; } diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c index 4820dbcedfa2..8eee63a5b93b 100644 --- a/net/dsa/tag_ksz.c +++ b/net/dsa/tag_ksz.c @@ -24,7 +24,7 @@ static struct sk_buff *ksz_common_rcv(struct sk_buff *skb, pskb_trim_rcsum(skb, skb->len - len); - skb->offload_fwd_mark = true; + dsa_default_offload_fwd_mark(skb); return skb; } diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c index aa1318dccaf0..3fd85139a3a6 100644 --- a/net/dsa/tag_lan9303.c +++ b/net/dsa/tag_lan9303.c @@ -115,7 +115,9 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev, skb_pull_rcsum(skb, 2 + 2); memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN), 2 * ETH_ALEN); - skb->offload_fwd_mark = !(lan9303_tag1 & LAN9303_TAG_RX_TRAPPED_TO_CPU); + + if (!(lan9303_tag1 & LAN9303_TAG_RX_TRAPPED_TO_CPU)) + dsa_default_offload_fwd_mark(skb); return skb; } diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c index 38dcdded74c0..08387fa37d17 100644 --- a/net/dsa/tag_mtk.c +++ b/net/dsa/tag_mtk.c @@ -97,7 +97,7 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev, /* Only unicast or broadcast frames are offloaded */ if (likely(!is_multicast_skb)) - skb->offload_fwd_mark = 1; + dsa_default_offload_fwd_mark(skb); return skb; } diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c index 16a1afd5b8e1..7f0898569876 100644 --- a/net/dsa/tag_ocelot.c +++ b/net/dsa/tag_ocelot.c @@ -225,7 +225,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb, */ return NULL; - skb->offload_fwd_mark = 1; + dsa_default_offload_fwd_mark(skb); skb->priority = qos_class; /* Ocelot switches copy frames unmodified to the CPU. However, it is diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c index 8991ebf098a3..a9ad03626b2e 100644 --- a/net/dsa/tag_ocelot_8021q.c +++ b/net/dsa/tag_ocelot_8021q.c @@ -48,7 +48,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb, if (!skb->dev) return NULL; - skb->offload_fwd_mark = 1; + dsa_default_offload_fwd_mark(skb); skb->priority = qos_class; return skb; diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c index 2646abe5a69e..c942d8697ed8 100644 --- a/net/dsa/tag_rtl4_a.c +++ b/net/dsa/tag_rtl4_a.c @@ -101,7 +101,7 @@ static struct sk_buff *rtl4a_tag_rcv(struct sk_buff *skb, skb->data - ETH_HLEN - RTL4_A_HDR_LEN, 2 * ETH_ALEN); - skb->offload_fwd_mark = 1; + dsa_default_offload_fwd_mark(skb); return skb; } diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c index 50496013cdb7..45cdf64f0e88 100644 --- a/net/dsa/tag_sja1105.c +++ b/net/dsa/tag_sja1105.c @@ -295,8 +295,6 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb, is_link_local = sja1105_is_link_local(skb); is_meta = sja1105_is_meta_frame(skb); - skb->offload_fwd_mark = 1; - if (is_tagged) { /* Normal traffic path. */ skb_push_rcsum(skb, ETH_HLEN); @@ -339,6 +337,8 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb, return NULL; } + dsa_default_offload_fwd_mark(skb); + if (subvlan) sja1105_decode_subvlan(skb, subvlan); 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); return skb; }