Message-ID: <875z54hghp.fsf@waldekranz.com>
Date:   Mon, 14 Dec 2020 10:41:22 +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 Sun, Dec 13, 2020 at 22:18, Tobias Waldekranz <tobias@...dekranz.com> wrote:
> 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?

On second thought, does this even matter if we solve the issue with
properly separating the different L2 domains? I.e. in this setup:

      br0
     /   \
  team0   \
   / \     \
swp0 swp1 swp2

If team0 is not offloaded, and our new fancy ndo were to relay that to
the bridge, then team0 and swp2 would no longer share OFM. In that case
traffic will flow between them indepent of OFM, just like it would
between ports from two different switchdevs.

With that in mind, I will leave this patch out of v4 as case (A) works
without it, and including it does not solve (B). I suppose there could
be other reasons to accurately convey the OFM in these cases, but I
think we can revisit that once everything else is in place.

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