Re: [PATCH v3 net-next 2/4] net: dsa: Link aggregation support

[Tobias Waldekranz <tobias@waldekranz.com>]

On Thu, Dec 03, 2020 at 18:24, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Dec 02, 2020 at 10:13:54AM +0100, Tobias Waldekranz wrote:
>> +static inline bool dsa_lag_offloading(struct dsa_switch_tree *dst)
>> +{
>> +	return dst->lags.num > 0;
>> +}
>
> You assume that the DSA switch, when it sets a non-zero number of LAGs,
> can offload any type of LAG TX type, when in fact the switch might be
> able to offload just NETDEV_LAG_TX_TYPE_HASH.

Right you are, I had this on my TODO but I most have lost track of
it. Good catch!

> I like the fact that we revert to a software-based implementation for
> features the hardware can't offload. So rejecting other TX types is out
> of the question.

Well if we really want to be precise, we must also ensure that the exact
hash type is supported by the hardware. mv88e6xxx only supports
NETDEV_LAG_HASH_L2 for example. There is a needle to thread here I
think. Story time ('tis the season after all):

    A user, Linus, has just installed OpenWRT on his gateway. Finally,
    he can unlock the full potential of his 802.11 AP by setting up a
    LAG to it.

    He carefully studies teamd.conf(5) and rightfully comes to the
    conclusion that he should set up the tx_hash to include the full
    monty of available keys. Teamd gets nothing but praise from the
    kernel when applying the configuration.

    And yet, Linus is not happy - the throughput between his NAS and his
    smart TV is now lower than before. It is enough for Linus to start
    working on his OS. It won't be big and professional like Linux of
    course, but it will at least get this bit right.

One could argue that if Linus had received an error instead, adapted his
teamd config and tried again, he would be a happier user and we might
not have to compete with his OS.

I am not sure which way is the correct one, but I do not think that it
necessarily _always_ correct to silently fallback to a non-offloaded
mode.

> However we still have to prevent hardware bridging.

The idea behind checking for dsa_lag_offloading in
dsa_slave_lag_changeupper was exactly this. If the LAG itself is not
offloaded, we should never call dsa_port_bridge_join on the lowers.

> Should we add an array of supported TX types that the switch port can
> offload, and that should be checked by DSA in dsa_lag_offloading?

That would work. We could also create a new DSA op that we would call
for each chip from PRECHANGEUPPER to verify that it is supported. One
advantage with this approach is that we can just pass along the `struct
netdev_lag_upper_info` so drivers always have access to all information,
in the event that new fields are added to it for example.