Message-ID: <20220427103209.luyfereepqaha7dw@skbuf>
Date:   Wed, 27 Apr 2022 10:32:11 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Hans Schultz <schultz.hans@...il.com>
CC:     Andrew Lunn <andrew@...n.ch>,
        "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 Wed, Apr 27, 2022 at 10:38:18AM +0200, Hans Schultz wrote:
> > 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.

As a disadvantage to your proposal, such a change would not only modify
the port_fdb_add prototype of these functions once again, but it would
also modify the way in which they *access* their arguments (instead of
accessing "addr" they now need to access "fdb->addr" for example).

You can argue that this would be the change to end all changes, but what
if we then need to add more unrelated arguments to port_fdb_add, like an
"extack". You still need to modify the prototypes for all drivers again.
I think it's a non-goal, you may disagree.

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

They tend to copy-paste bad coding patterns from each other. The
SWITCHDEV_FDB_ADD_TO_DEVICE and SWITCHDEV_FDB_DEL_TO_DEVICE handlers are
quite a big mess, but that's a story for another time.

Otherwise, generally speaking, they have access from the atomic notifier
to the struct switchdev_notifier_fdb_info *fdb_info, then they allocate
a work item on a private work queue, copy the stuff from this notifier
object that they find relevant, and use that private structure from the
deferred context.

DSA does basically the same thing, except for the fact that the hardware
access is abstracted behind an indirect call to port_fdb_add().

> > 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?

I don't know about 64-bit MAC addresses, do you have more information?

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

Yeah, I should update the DSA documentation with clear info about that,
it's just the commit messages 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.

My advice is just add what you need to add. The prototype changes for
port_fdb_add took place in:

2022-02-25 c26933639b54 ("net: dsa: request drivers to perform FDB isolation")
2017-08-06 1b6dd556c304 ("net: dsa: Remove prepare phase for FDB")
2017-08-06 6c2c1dcb185f ("net: dsa: Change DSA slave FDB API to be switchdev independent")
2016-04-06 8497aa618dd6 ("net: dsa: make the FDB add function return void")
2015-10-08 1f36faf26943 ("net: dsa: push prepare phase in port_fdb_add")
2015-08-10 2a778e1b5899 ("net: dsa: change FDB routines prototypes")
2015-08-06 55045ddded0f ("net: dsa: add support for switchdev FDB objects")
2015-03-26 339d82626d22 ("net: dsa: Add basic framework to support ndo_fdb functions")

As you can see, those aren't a lot of changes. I'm happy to see new
development in this area, but there was a long period of stability,
which is likely to continue.

Also, if you study the phylink code, you'll notice that it does't have
"a slower development cycle", quite the contrary, it is even aggressively
converting drivers to make use of new API, and marking unconverted drivers
as deprecated/legacy.

Powered by blists - more mailing lists