Re: [PATCH net-next v2 00/13] Change DSA's FDB API and perform switchdev cleanup

Fri, Aug 04, 2017 at 12:39:02AM CEST, arkadis@mellanox.com wrote:

[...]

quoted

quoted

Now we have the "offload" read only flag, which is good to inform about
a successfully programmed hardware, but adds another level of complexity
to understand the interaction with the hardware.

I think iproute2 is getting more and more confusing. From what I
understood, respecting the "self" flag as described is not possible
anymore due to some retro-compatibility reasons.

Also Linux must use the hardware as an accelerator (so "self" or
"offload" must be the default), and always fall back to software
otherwise, hence "master" do not make sense here.

What do you think about this synopsis for bridge fdb add?

    # bridge fdb add LLADDR dev DEV [ offload { on | off } ]

Where offload defaults to "on". This option should also be ported to
other offloaded features like MDB and VLAN. Even though this is a bit
out of scope of this patchset, do you think this is feasible?

I agree completely that currently its confusing. The documentation
should be updated for sure. I think that 'self' was primarily introduced
(Commit 77162022a) for NIC embedded switches which are used for sriov, in
that case the self is related to the internal eswitch, which completely
diverge from the software one (clearly not swithcdev).

IMHO For switchdev devices 'self' should not be an option at all, or any
other arg regarding hardware. Furthermore, the 'offload' flag should be
only relevant during the dump as an indication to the user.

Unfortunately, the  lack of ability of syncing the sw with hw in DSA's
case introduces a problem for indicating that the entries are only
in hw, I mean marking it only as offloaded is not enough.

Hi,

It seems impossible currently to move the self to be the default, and
this introduces regression which you don't approve, so it seems few
options left:

a) Leave two ways to add fdb, through the bridge (by using the master
  flag) which is introduced in this patchset, and by using the self
  which is the legacy way. In this way no regression will be introduced,
  yet, it feels confusing a bit. The benefit is that we (DSA/mlxsw)
  will be synced.
b) Leave only the self (which means removing patch no 4,5).

I belive that option a) is the correct way to go. Introduction of self
inclusion was a mistake from the very beginning. I think that we should
just move one and correct this mistake.

Vivien, any arguments against a)?

Thanks!

In both cases the switchdev implementation of .ndo_fdb_add() will be
moved inside DSA in a similar way to the dump because its only used by
you.

Option b) actually turns this patchset into cosmetic one which does
only cleanup.

Thanks,
Arkadi