Date:   Thu, 10 Dec 2020 00:21:03 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.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 Wed, Dec 09, 2020 at 11:01:25PM +0100, Tobias Waldekranz wrote:
> It is not the Fibonacci sequence or anything, it is an integer in the
> range 0..num_lags-1. I realize that some hardware probably allocate IDs
> from some shared (and thus possibly non-contiguous) pool. Maybe ocelot
> works like that. But it seems reasonable to think that at least some
> other drivers could make use of a linear range.

In the ocelot RFC patches that I've sent to the list yesterday, you
could see that the ports within the same bond must have the same logical
port ID (as opposed to regular mode, when each port has a logical ID
equal to its physical ID, i.e. swp0 -> 0, swp1 -> 1, etc). We can't use
the contiguous LAG ID assignment that you do in DSA, because maybe we
have swp1 and swp2 in a bond, and the LAG ID you give that bond is 0.
But if we assign logical port ID 0 to physical ports 1 and 2, then we
end up also bonding with swp0... So what is done in ocelot is that the
LAG ID is derived from the index of the first port that is part of the
bond, and the logical port IDs are all assigned to that value. It's
really simple when you think about it. It would have probably been the
same for Marvell too if it weren't for that cross-chip thing.

If I were to take a look at Florian's b53-bond branch, I do see that
Broadcom switches also expect a contiguous range of LAG IDs:
https://github.com/ffainelli/linux/tree/b53-bond

So ok, maybe ocelot is in the minority. Not an issue. If you add that
lookup table in the DSA layer, then you could get your linear "LAG ID"
by searching through it using the struct net_device *bond as key.
Drivers which don't need this linear array will just not use it.

> > I think that there is a low practical risk that the assumption will not
> > hold true basically forever. But I also see why you might like your
> > approach more. Maybe Vivien, Andrew, Florian could also chime in and we
> > can see if struct dsa_lag "bothers" anybody else except me (bothers in
> > the sense that it's an unnecessary complication to hold in DSA). We
> > could, of course, also take the middle ground, which would be to keep
> > the 16-entry array of bonding net_device pointers in DSA, and you could
> > still call your dsa_lag_dev_by_id() and pretend it's generic, and that
> > would just look up that table. Even with this middle ground, we are
> > getting rid of the port lists and of the reference counting, which is
> > still a welcome simplification in my book.
>
> Yeah I agree that we can trim it down to just the array. Going beyond
> that point, i.e. doing something like how sja1105 works, is more painful
> but possible if Andrew can live with it.

I did not get the reference to sja1105 here. That does not support
bonding offload, but does work properly with software bridging thanks to
your patches.

Powered by blists - more mailing lists