Message ID | 20210703115705.1034112-3-vladimir.oltean@nxp.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | Allow forwarding for the software bridge data path to be offloaded to capable devices | 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 | success | CCed 6 of 6 maintainers |
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: 3 this patch: 3 |
netdev/kdoc | success | Errors and warnings before: 3 this patch: 3 |
netdev/verify_fixes | success | Link |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 98 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
netdev/header_inline | success | Link |
Hi On 03/07/2021 14:56, Vladimir Oltean wrote: > From: Tobias Waldekranz <tobias@waldekranz.com> > > Before this change, four related - but distinct - concepts where named > offload_fwd_mark: > > - skb->offload_fwd_mark: Set by the switchdev driver if the underlying > hardware has already forwarded this frame to the other ports in the > same hardware domain. > > - nbp->offload_fwd_mark: An idetifier used to group ports that share > the same hardware forwarding domain. > > - br->offload_fwd_mark: Counter used to make sure that unique IDs are > used in cases where a bridge contains ports from multiple hardware > domains. > > - skb->cb->offload_fwd_mark: The hardware domain on which the frame > ingressed and was forwarded. > > Introduce the term "hardware forwarding domain" ("hwdom") in the > bridge to denote a set of ports with the following property: > > If an skb with skb->offload_fwd_mark set, is received on a port > belonging to hwdom N, that frame has already been forwarded to all > other ports in hwdom N. > > By decoupling the name from "offload_fwd_mark", we can extend the > term's definition in the future - e.g. to add constraints that > describe expected egress behavior - without overloading the meaning of > "offload_fwd_mark". > > - nbp->offload_fwd_mark thus becomes nbp->hwdom. > > - br->offload_fwd_mark becomes br->last_hwdom. > > - skb->cb->offload_fwd_mark becomes skb->cb->src_hwdom. The slight > change in naming here mandates a slight change in behavior of the > nbp_switchdev_frame_mark() function. Previously, it only set this > value in skb->cb for packets with skb->offload_fwd_mark true (ones > which were forwarded in hardware). Whereas now we always track the > incoming hwdom for all packets coming from a switchdev (even for the > packets which weren't forwarded in hardware, such as STP BPDUs, IGMP > reports etc). As all uses of skb->cb->offload_fwd_mark were already > gated behind checks of skb->offload_fwd_mark, this will not introduce > any functional change, but it paves the way for future changes where > the ingressing hwdom must be known for frames coming from a switchdev > regardless of whether they were forwarded in hardware or not > (basically, if the skb comes from a switchdev, skb->cb->src_hwdom now > always tracks which one). > > A typical example where this is relevant: the switchdev has a fixed > configuration to trap STP BPDUs, but STP is not running on the bridge > and the group_fwd_mask allows them to be forwarded. Say we have this > setup: > > br0 > / | \ > / | \ > swp0 swp1 swp2 > > A BPDU comes in on swp0 and is trapped to the CPU; the driver does not > set skb->offload_fwd_mark. The bridge determines that the frame should > be forwarded to swp{1,2}. It is imperative that forward offloading is > _not_ allowed in this case, as the source hwdom is already "poisoned". > > Recording the source hwdom allows this case to be handled properly. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > net/bridge/br_if.c | 2 +- > net/bridge/br_private.h | 10 +++++----- > net/bridge/br_switchdev.c | 16 ++++++++-------- > 3 files changed, 14 insertions(+), 14 deletions(-) > [...] Thank you. I very much like this patch by itself as it clarifies properly things which caused much headache (at least for me). I hope it can be moved forward regardless of the rest of the series. Minor comment - It will good to add in-code doc for added/renamed struct fields. Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index f7d2f472ae24..73fa703f8df5 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -643,7 +643,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev, if (err) goto err5; - err = nbp_switchdev_mark_set(p); + err = nbp_switchdev_hwdom_set(p); if (err) goto err6; diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 2b48b204205e..e16879caaaf3 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -329,7 +329,7 @@ struct net_bridge_port { struct netpoll *np; #endif #ifdef CONFIG_NET_SWITCHDEV - int offload_fwd_mark; + int hwdom; #endif u16 group_fwd_mask; u16 backup_redirected_cnt; @@ -476,7 +476,7 @@ struct net_bridge { u32 auto_cnt; #ifdef CONFIG_NET_SWITCHDEV - int offload_fwd_mark; + int last_hwdom; #endif struct hlist_head fdb_list; @@ -506,7 +506,7 @@ struct br_input_skb_cb { #endif #ifdef CONFIG_NET_SWITCHDEV - int offload_fwd_mark; + int src_hwdom; #endif }; @@ -1645,7 +1645,7 @@ static inline void br_sysfs_delbr(struct net_device *dev) { return; } /* br_switchdev.c */ #ifdef CONFIG_NET_SWITCHDEV -int nbp_switchdev_mark_set(struct net_bridge_port *p); +int nbp_switchdev_hwdom_set(struct net_bridge_port *p); void nbp_switchdev_frame_mark(const struct net_bridge_port *p, struct sk_buff *skb); bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, @@ -1665,7 +1665,7 @@ static inline void br_switchdev_frame_unmark(struct sk_buff *skb) skb->offload_fwd_mark = 0; } #else -static inline int nbp_switchdev_mark_set(struct net_bridge_port *p) +static inline int nbp_switchdev_hwdom_set(struct net_bridge_port *p) { return 0; } diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index d3adee0f91f9..833fd30482c2 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -8,20 +8,20 @@ #include "br_private.h" -static int br_switchdev_mark_get(struct net_bridge *br, struct net_device *dev) +static int br_switchdev_hwdom_get(struct net_bridge *br, struct net_device *dev) { struct net_bridge_port *p; /* dev is yet to be added to the port list. */ list_for_each_entry(p, &br->port_list, list) { if (netdev_port_same_parent_id(dev, p->dev)) - return p->offload_fwd_mark; + return p->hwdom; } - return ++br->offload_fwd_mark; + return ++br->last_hwdom; } -int nbp_switchdev_mark_set(struct net_bridge_port *p) +int nbp_switchdev_hwdom_set(struct net_bridge_port *p) { struct netdev_phys_item_id ppid = { }; int err; @@ -35,7 +35,7 @@ int nbp_switchdev_mark_set(struct net_bridge_port *p) return err; } - p->offload_fwd_mark = br_switchdev_mark_get(p->br, p->dev); + p->hwdom = br_switchdev_hwdom_get(p->br, p->dev); return 0; } @@ -43,15 +43,15 @@ int nbp_switchdev_mark_set(struct net_bridge_port *p) void nbp_switchdev_frame_mark(const struct net_bridge_port *p, struct sk_buff *skb) { - if (skb->offload_fwd_mark && !WARN_ON_ONCE(!p->offload_fwd_mark)) - BR_INPUT_SKB_CB(skb)->offload_fwd_mark = p->offload_fwd_mark; + if (p->hwdom) + BR_INPUT_SKB_CB(skb)->src_hwdom = p->hwdom; } bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, const struct sk_buff *skb) { return !skb->offload_fwd_mark || - BR_INPUT_SKB_CB(skb)->offload_fwd_mark != p->offload_fwd_mark; + BR_INPUT_SKB_CB(skb)->src_hwdom != p->hwdom; } /* Flags that can be offloaded to hardware */