Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Tobias Waldekranz <tobias@waldekranz.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, andrew@lunn.ch,
	vivien.didelot@gmail.com, f.fainelli@gmail.com,
	j.vosburgh@gmail.com, vfalico@gmail.com, andy@greyhouse.net,
	netdev@vger.kernel.org
Subject: Re: [PATCH v3 net-next 2/4] net: dsa: Link aggregation support
Date: Fri, 11 Dec 2020 21:50:24 +0100
Message-ID: <87a6uk5apb.fsf@waldekranz.com> (raw)
In-Reply-To: <20201208112350.kuvlaxqto37igczk@skbuf>

On Tue, Dec 08, 2020 at 13:23, Vladimir Oltean <olteanv@gmail.com> wrote:
> Sorry it took so long. I wanted to understand:
> (a) where are the challenged for drivers to uniformly support software
>     bridging when they already have code for bridge offloading. I found
>     the following issues:
>     - We have taggers that unconditionally set skb->offload_fwd_mark = 1,
>       which kind of prevents software bridging. I'm not sure what the
>       fix for these should be.

I took a closer look at the software fallback mode for LAGs and I've
found three issues that prevent this from working in a bridged setup,
two of which are easy to fix. This is the setup (team0 is _not_
offloaded):

(A)  br0
     /
  team0
   / \
swp0 swp1


1. DSA tries to offload port attributes for standalone ports. So in this
   setup, if vlan filtering is enabled on br0, we will enable it in
   hardware which on mv88e6xxx causes swp0/1 to drop all packets on
   ingress due to a VTU violation. This is a very easy fix, I will
   include it in v4.

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?

With these two patches in place, setup (A) works as expected. But if you
extend it to (team0 still not offloaded):

(B)   br0
     /   \
  team0   \
   / \     \
swp0 swp1  swp2

You instantly run into:

3. Only traffic which does _not_ have offload_fwd_mark set is allowed to
   pass from swp2 to team0. This is because the bridge uses
   dev_get_port_parent_id to figure out which ports belong to the same
   switch. This will recurse down through all lowers and find swp0/1
   which will answer with the same ID as swp2.

   In the case where team0 is offloaded, this is exactly what we want,
   but in a setup like (B) they do not have the same "logical" parent in
   the sense that br0 is led to believe. I.e. the hardware will never
   forward packets between swp0/1 and swp2.

   I do not see an obvious solution to this. Refusing to divulge the
   parent just because you are a part of a software LAG seems fraught
   with danger as there are other users of those APIs. Adding yet
   another ndo would theoretically be possible, but not
   desirable. Ideas?

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.

  parent reply index

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-02  9:13 [PATCH v3 net-next 0/4] " Tobias Waldekranz
2020-12-02  9:13 ` [PATCH v3 net-next 1/4] net: bonding: Notify ports about their initial state Tobias Waldekranz
2020-12-02 19:09   ` Jay Vosburgh
2020-12-02 21:52     ` Tobias Waldekranz
2020-12-03  0:39       ` Jay Vosburgh
2020-12-03  8:16         ` Tobias Waldekranz
2020-12-02  9:13 ` [PATCH v3 net-next 2/4] net: dsa: Link aggregation support Tobias Waldekranz
2020-12-02 10:07   ` Vladimir Oltean
2020-12-02 10:51     ` Tobias Waldekranz
2020-12-02 18:58   ` Jakub Kicinski
2020-12-02 21:29     ` Tobias Waldekranz
2020-12-02 21:32       ` Vladimir Oltean
2020-12-03 16:24   ` Vladimir Oltean
2020-12-03 20:53     ` Tobias Waldekranz
2020-12-03 21:09       ` Andrew Lunn
2020-12-03 21:35         ` Tobias Waldekranz
2020-12-04  0:35           ` Vladimir Oltean
2020-12-03 21:57       ` Vladimir Oltean
2020-12-03 23:12         ` Tobias Waldekranz
2020-12-04  0:56           ` Vladimir Oltean
2020-12-07 21:49             ` Tobias Waldekranz
2020-12-04  1:33         ` Andrew Lunn
2020-12-04  4:18           ` Florian Fainelli
2020-12-07 21:56             ` Tobias Waldekranz
2020-12-03 20:48   ` Vladimir Oltean
2020-12-04  2:20   ` Andrew Lunn
2020-12-07 21:19     ` Tobias Waldekranz
2020-12-07 23:26       ` Andrew Lunn
2020-12-09  8:57         ` Tobias Waldekranz
2020-12-09 14:27           ` Andrew Lunn
2020-12-09 15:21             ` Tobias Waldekranz
2020-12-09 23:03               ` Andrew Lunn
2020-12-04  4:04   ` Florian Fainelli
2020-12-08 11:23   ` Vladimir Oltean
2020-12-08 15:33     ` Tobias Waldekranz
2020-12-08 16:37       ` Vladimir Oltean
2020-12-09  8:37         ` Tobias Waldekranz
2020-12-09 10:53           ` Vladimir Oltean
2020-12-09 14:11             ` Tobias Waldekranz
2020-12-09 16:04               ` Vladimir Oltean
2020-12-09 22:01                 ` Tobias Waldekranz
2020-12-09 22:21                   ` Vladimir Oltean
2020-12-10 10:18                     ` Tobias Waldekranz
2020-12-09 22:59                 ` Andrew Lunn
2020-12-10  1:05                   ` Vladimir Oltean
2020-12-09 14:23             ` Andrew Lunn
2020-12-09 23:17               ` Vladimir Oltean
2020-12-08 17:26     ` Andrew Lunn
2020-12-11 20:50     ` Tobias Waldekranz [this message]
2020-12-12 14:26       ` Vladimir Oltean
2020-12-13 21:18         ` Tobias Waldekranz
2020-12-14  0:12           ` Vladimir Oltean
2020-12-14 11:42             ` Ido Schimmel
2020-12-16 15:15               ` Tobias Waldekranz
2020-12-16 18:48                 ` Ido Schimmel
2020-12-14  9:41           ` Tobias Waldekranz
2020-12-02  9:13 ` [PATCH v3 net-next 3/4] net: dsa: mv88e6xxx: " Tobias Waldekranz
2020-12-02  9:13 ` [PATCH v3 net-next 4/4] net: dsa: tag_dsa: Support reception of packets from LAG devices Tobias Waldekranz
2020-12-04  3:58   ` Florian Fainelli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a6uk5apb.fsf@waldekranz.com \
    --to=tobias@waldekranz.com \
    --cc=andrew@lunn.ch \
    --cc=andy@greyhouse.net \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=j.vosburgh@gmail.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=vfalico@gmail.com \
    --cc=vivien.didelot@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git