Date:   Tue, 26 Apr 2022 23:17:56 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Hans Schultz <schultz.hans@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        George McCollister <george.mccollister@...il.com>
Subject: Re: [PATCH v2 net-next 07/10] net: dsa: request drivers to perform
 FDB isolation

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?

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?

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.

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?

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.

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.

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).

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.

Powered by blists - more mailing lists