On Mon, May 03, 2021 at 10:49:12AM +0200, Tobias Waldekranz wrote:

On Sun, May 02, 2021 at 18:00, Ido Schimmel [off-list ref] wrote:

quoted

On Mon, Apr 26, 2021 at 07:04:04PM +0200, Tobias Waldekranz wrote:

quoted

- 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.

[...]

quoted

@@ -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".

quoted

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.

quoted

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.

quoted

quoted

 
 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