| Message ID | 20210426170411.1789186-3-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: Forward offloading | 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 |
On Mon, Apr 26, 2021 at 07:04:04PM +0200, Tobias Waldekranz wrote: > - skb->cb->offload_fwd_mark becomes skb->cb->src_hwdom. There is a > slight change here: Whereas previously this was only set for > offloaded packets, we now always track the incoming hwdom. As all > uses where 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 both for > offloaded and non-offloaded frames. [...] > @@ -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; > } I assume you are referring to this change? "src_hwdom" sounds weird if it's expected to be valid for non-offloaded frames. Can you elaborate about "future changes where the ingressing hwdom must be known both for offloaded and non-offloaded frames"? Probably best to split this change to a different patch given the rest of the changes are mechanical. > > 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 */ > -- > 2.25.1 >
On Sun, May 02, 2021 at 18:00, Ido Schimmel <idosch@idosch.org> wrote: > On Mon, Apr 26, 2021 at 07:04:04PM +0200, Tobias Waldekranz wrote: >> - skb->cb->offload_fwd_mark becomes skb->cb->src_hwdom. There is a >> slight change here: Whereas previously this was only set for >> offloaded packets, we now always track the incoming hwdom. As all >> uses where 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 both for >> offloaded and non-offloaded frames. > > [...] > >> @@ -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; >> } > > I assume you are referring to this change? "src_hwdom" sounds weird if > it's expected to be valid for non-offloaded frames. Perhaps "non-offloaded" was a sloppy description on my part. I was trying to describe frames that originate from a switchdev, but have not been forwarded by hardware; e.g. STP BPDUs, IGMP reports, etc. So nbp_switchdev_frame_mark now basically says: "If this skb came in from a switchdev, make sure to note which one". > Can you elaborate about "future changes where the ingressing hwdom must > be known both for offloaded and non-offloaded frames"? Typical example: 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. > Probably best to split this change to a different patch given the rest > of the changes are mechanical. Right, but I think the change in name to warrants a change in semantics. It is being renamed to src_hwdom because it now holds just that information. Again, there is no functional change introduced by this since nbp_switchdev_allowed_egress always checks for the presence of skb->offload_fwd_mark anyway. But if you feel strongly about it, I will split it up. >> >> 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 */ >> -- >> 2.25.1 >>
On Mon, May 03, 2021 at 10:49:12AM +0200, Tobias Waldekranz wrote: > On Sun, May 02, 2021 at 18:00, Ido Schimmel <idosch@idosch.org> wrote: > > On Mon, Apr 26, 2021 at 07:04:04PM +0200, Tobias Waldekranz wrote: > >> - skb->cb->offload_fwd_mark becomes skb->cb->src_hwdom. There is a > >> slight change here: Whereas previously this was only set for > >> offloaded packets, we now always track the incoming hwdom. As all > >> uses where 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 both for > >> offloaded and non-offloaded frames. > > > > [...] > > > >> @@ -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; > >> } > > > > I assume you are referring to this change? "src_hwdom" sounds weird if > > it's expected to be valid for non-offloaded frames. > > Perhaps "non-offloaded" was a sloppy description on my part. I was > trying to describe frames that originate from a switchdev, but have not > been forwarded by hardware; e.g. STP BPDUs, IGMP reports, etc. So > nbp_switchdev_frame_mark now basically says: "If this skb came in from a > switchdev, make sure to note which one". > > > Can you elaborate about "future changes where the ingressing hwdom must > > be known both for offloaded and non-offloaded frames"? > > Typical example: 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. OK, thanks for the explanation. If it is allowed, then the packet will be transmitted from swp0, from which it was received. > > > Probably best to split this change to a different patch given the rest > > of the changes are mechanical. > > Right, but I think the change in name to warrants a change in > semantics. It is being renamed to src_hwdom because it now holds just > that information. Again, there is no functional change introduced by > this since nbp_switchdev_allowed_egress always checks for the presence > of skb->offload_fwd_mark anyway. But if you feel strongly about it, I > will split it up. If you put the explanation above in the changelog, then it should be fine to keep it as one patch. > > >> > >> 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 */ > >> -- > >> 2.25.1 > >>
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 7ce8a77cc6b6..53248715f631 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -327,7 +327,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; @@ -472,7 +472,7 @@ struct net_bridge { u32 auto_cnt; #ifdef CONFIG_NET_SWITCHDEV - int offload_fwd_mark; + int last_hwdom; #endif struct hlist_head fdb_list; @@ -502,7 +502,7 @@ struct br_input_skb_cb { #endif #ifdef CONFIG_NET_SWITCHDEV - int offload_fwd_mark; + int src_hwdom; #endif }; @@ -1593,7 +1593,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, @@ -1613,7 +1613,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 a5e601e41cb9..bc085077ae71 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 */
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. There is a slight change here: Whereas previously this was only set for offloaded packets, we now always track the incoming hwdom. As all uses where 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 both for offloaded and non-offloaded frames. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.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(-)