Message-ID: <878sa1h0bg.fsf@waldekranz.com>
Date:   Sun, 13 Dec 2020 22:18:27 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        j.vosburgh@...il.com, vfalico@...il.com, andy@...yhouse.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 2/4] net: dsa: Link aggregation support

On Sat, Dec 12, 2020 at 16:26, Vladimir Oltean <olteanv@...il.com> wrote:
> On Fri, Dec 11, 2020 at 09:50:24PM +0100, Tobias Waldekranz wrote:
>> 2. The issue Vladimir mentioned above. This is also a straight forward
>>    fix, I have patch for tag_dsa, making sure that offload_fwd_mark is
>>    never set for ports in standalone mode.
>>
>>    I am not sure if I should solve it like that or if we should just
>>    clear the mark in dsa_switch_rcv if the dp does not have a
>>    bridge_dev. I know both Vladimir and I were leaning towards each
>>    tagger solving it internally. But looking at the code, I get the
>>    feeling that all taggers will end up copying the same block of code
>>    anyway. What do you think?
>
> I am not sure what constitutes a good separation between DSA and taggers
> here. We have many taggers that just set skb->offload_fwd_mark = 1. We
> could have this as an opportunity to even let DSA take the decision
> altogether. What do you say if we stop setting skb->offload_fwd_mark
> from taggers, just add this:
>
> +#define DSA_SKB_TRAPPED	BIT(0)
> +
>  struct dsa_skb_cb {
>  	struct sk_buff *clone;
> +	unsigned long flags;
>  };
>
> and basically just reverse the logic. Make taggers just assign this flag
> for packets which are known to have reached software via data or control
> traps. Don't make the taggers set skb->offload_fwd_mark = 1 if they
> don't need to. Let DSA take that decision upon a more complex thought
> process, which looks at DSA_SKB_CB(skb)->flags & DSA_SKB_TRAPPED too,
> among other things.

What would the benefit of this over using the OFM directly? Would the
flag not carry the exact same bit of information, albeit inverted? Is it
about not giving the taggers any illusions about having the final say on
the OFM value?

>> As for this series, my intention is to make sure that (A) works as
>> intended, leaving (B) for another day. Does that seem reasonable?
>>
>> NOTE: In the offloaded case, (B) will of course also be supported.
>
> Yeah, ok, one can already tell that the way I've tested this setup was
> by commenting out skb->offload_fwd_mark = 1 altogether. It seems ok to
> postpone this a bit.
>
> For what it's worth, in the giant "RX filtering for DSA switches" fiasco
> https://patchwork.ozlabs.org/project/netdev/patch/20200521211036.668624-11-olteanv@gmail.com/
> we seemed to reach the conclusion that it would be ok to add a new NDO
> answering the question "can this interface do forwarding in hardware
> towards this other interface". We can probably start with the question
> being asked for L2 forwarding only.

Very interesting, though I did not completely understand the VXLAN
scenario laid out in that thread. I understand that OFM can not be 0,
because you might have successfully forwarded to some destinations. But
setting it to 1 does not smell right either. OFM=1 means "this has
already been forwarded according to your current configuration" which is
not completely true in this case. This is something in the middle, more
like skb->offload_fwd_mark = its_complicated;

Anyway, so we are essentially talking about replacing the question "do
you share a parent with this netdev?" with "do you share the same
hardware bridging domain as this netdev?" when choosing the port's OFM
in a bridge, correct? If so, great, that would also solve the software
LAG case. This would also get us one step closer to selectively
disabling bridge offloading on a switchdev port.

Powered by blists - more mailing lists