On Sun, May 02, 2021 at 18:00, Ido Schimmel <idosch@xxxxxxxxxx> 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
>>
