Re: [PATCH v2 net-next 07/10] net: dsa: request drivers to perform FDB isolation

[Hans Schultz <schultz.hans@gmail.com>]

On tis, apr 26, 2022 at 23:17, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Tue, Apr 26, 2022 at 06:14:23PM +0200, Andrew Lunn wrote:
>> > > @@ -941,23 +965,29 @@ struct dsa_switch_ops {
>> > >  	 * Forwarding database
>> > >  	 */
>> > >  	int	(*port_fdb_add)(struct dsa_switch *ds, int port,
>> > > -				const unsigned char *addr, u16 vid);
>> > > +				const unsigned char *addr, u16 vid,
>> > > +				struct dsa_db db);
>> > 
>> > Hi! Wouldn't it be better to have a struct that has all the functions
>> > parameters in one instead of adding further parameters to these
>> > functions?
>> > 
>> > I am asking because I am also needing to add a parameter to
>> > port_fdb_add(), and it would be more future oriented to have a single
>> > function parameter as a struct, so that it is easier to add parameters
>> > to these functions without havíng to change the prototype of the
>> > function every time.
>> 
>> Hi Hans
>> 
>> Please trim the text to only what is relevant when replying. It is
>> easy to miss comments when having to Page Down, Page Down, Page down,
>> to find comments.
>> 
>>    Andrew
>
> Agreed, had to scroll too much.
>
> Hans, what extra argument do you want to add to port_fdb_add?
> A static/dynamic, I suppose, similar to what exists in port_fdb_dump?

Yes, a static/dynamic bool, or could be called a flag field.

>
> But we surely wouldn't pass _all_ parameters of port_fdb_add through
> some giant struct args_of_port_fdb_add, would we?  Not ds, port, db,
> just what is naturally grouped together as an FDB entry: addr, vid,
> maybe your new static/dynamic thing.
>
> If we group the addr and vid in port_fdb_add into a structure that
> represents an FDB entry, what do we do about those in port_fdb_del?
> Group those as well, maybe for consistency?

I think the 'old' interface that several other functions use should have
one struct... e.g. port, addr and vid. But somehow it would be good to
have something more dynamic. There could be two layer of structs, but
generally i think that for these op functions now in relation to fdb
should only have structs as parameters in a logical way that is
expandable and thus future oriented.

Something else to consider is what do switchcore drivers that don't use
'include/net/dsa.h' do and why?

>
> Same question for port_fdb_dump and its dsa_fdb_dump_cb_t: would you
> change it for uniformity, or would you keep it the way it is to reduce
> the churn? I mean it's a perfectly viable candidate for argument
> grouping, but your stated goal _is_ to reduce churn.

I think port_fdb_dump() is maybe the last one that I would change, but
all those functions where you have added the struct dsa_db would be 
candidates.

>
> But if we add the static/dynamic boolean to this structure, does it make
> sense on deletion? And if it doesn't, why have we changed the prototype
> of port_fdb_del to include it?
>

True a static/dynamic boolean doesn't make much sense on deletion, only
if it came in because it is part of a generic struct.

> Restated: do we want to treat the "static/dynamic" info as a property of
> an FDB entry (i.e. a member of the structure), or as the way in which a
> certain FDB entry can be added to hardware (case in which it is relevant
> only to _add and to _dump)?  After all, an FDB entry for {addr, vid}
> learned statically, and another FDB entry for the same {addr, vid} but
> learned dynamically, are fundamentally the same object.
>

I cannot answer for the workings of all switchcores, but for my sake I
use a debug tool to show the age of a dynamic entry in the ATU, so I
don't think that it has much relevance outside of that.

> And if we don't go with a big struct args_of_port_fdb_add (which would
> be silly if we did), who guarantees that the argument list of port_fdb_add
> won't grow in the future anyway? Like in the example I just gave above,
> where "static/dynamic" doesn't fully appear to group naturally with
> "addr" and "vid", and would probably still be a separate boolean,
> rendering the whole point moot.
>
> And even grouping only those last 2 together is maybe debatable due to
> practical reasons - where do we declare this small structure? We have a
> struct switchdev_notifier_fdb_info with some more stuff that we
> deliberately do not want to expose, and {addr, vid} are all that remain.
>
> Although maybe there are benefits to having a small {addr, vid} structure
> defined somewhere publicly, too, and referenced consistently by driver
> code. Like for example to avoid bad patterns from proliferating.
> Currently we have "const unsigned char *addr, u16 vid", so on a 64 bit
> machine, this is a pointer plus an unsigned short, 10 bytes, padded up
> by the compiler, maybe to 16. But the Ethernet addresses are 6 bytes,
> those are shorter than the pointer itself, so on a 64-bit machine,
> having "unsigned char addr[ETH_ALEN], u16 vid" makes a lot more space,
> saves some real memory.

I see that there is definitions for 64bit mac addresses out there, which
might also be needed to be supported at some point?

>
> Anyway, I'm side tracking. If you want to make changes, propose a
> patch, but come up with a more real argument than "reducing churn"
> (while effectively producing churn).

Unfortunately I don't have the time to make such a patch suggestion for
some time to come as I also have other patch sets coming up, and I
should study a bit what your patch set with the dsa_db is about also, so
maybe I must just add the bool to port_fdb_add() for now.

>
> To give you a counter example, phylink_mac_config() used to have the
> opposite problem, the arguments were all neatly packed into a struct
> phylink_link_state, but as the kerneldocs from include/linux/phylink.h
> put it, not all members of that structure were guaranteed to contain
> valid values. So there were bugs due to people not realizing this, and
> consequently, phylink_mac_link_up() took the opposite approach, of
> explicitly passing all known parameters of the resolved link state as
> individual arguments. Now there are 10 arguments to that function, but
> somehow at least this appears to have worked better, in the sense that
> there isn't an explanatory note saying what's valid and what isn't.

Yes, I can see the danger of it, but something like phylink is also
different as it is more hardware related, which has a slower development
cycle than feature/protocol stuff.