From: Simon Horman <simon.horman@corigine.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "Andrew Lunn" <andrew@lunn.ch>,
"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
"Nikolay Aleksandrov" <razor@blackwall.org>,
"Kurt Kanzenbach" <kurt@linutronix.de>,
"Eric Dumazet" <edumazet@google.com>,
"Hans Schultz" <netdev@kapio-technology.com>,
"Ivan Vecera" <ivecera@redhat.com>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"moderated list:ETHERNET BRIDGE"
<bridge@lists.linux-foundation.org>,
"Russell King" <linux@armlinux.org.uk>,
"Roopa Prabhu" <roopa@nvidia.com>,
kuba@kernel.org, "Paolo Abeni" <pabeni@redhat.com>,
"Clément Léger" <clement.leger@bootlin.com>,
"Christian Marangi" <ansuelsmth@gmail.com>,
"Woojung Huh" <woojung.huh@microchip.com>,
"Landen Chao" <Landen.Chao@mediatek.com>,
"Jiri Pirko" <jiri@resnulli.us>,
"Hauke Mehrtens" <hauke@hauke-m.de>,
"Sean Wang" <sean.wang@mediatek.com>,
"DENG Qingfang" <dqfext@gmail.com>,
"Claudiu Manoil" <claudiu.manoil@nxp.com>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"moderated list:ARM/Mediatek SoC support"
<linux-arm-kernel@lists.infradead.org>,
netdev@vger.kernel.org,
"open list" <linux-kernel@vger.kernel.org>,
"maintainer:MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER"
<UNGLinuxDriver@microchip.com>,
"open list:RENESAS RZ/N1 A5PSW SWITCH DRIVER"
<linux-renesas-soc@vger.kernel.org>,
davem@davemloft.net
Subject: Re: [Bridge] [PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries
Date: Mon, 20 Feb 2023 15:11:19 +0100 [thread overview]
Message-ID: <Y/N/h5WMRzl0i2IT@corigine.com> (raw)
In-Reply-To: <20230217174431.bkkvfmtno56mfh5a@skbuf>
On Fri, Feb 17, 2023 at 07:44:31PM +0200, Vladimir Oltean wrote:
> On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz wrote:
> > On Mon, Feb 06, 2023 at 17:02, Simon Horman <simon.horman@corigine.com> wrote:
> > >
> > > Just to clarify my suggestion one last time, it would be along the lines
> > > of the following (completely untested!). I feel that it robustly covers
> > > all cases for fdb_flags. And as a bonus doesn't need to be modified
> > > if other (unsupported) flags are added in future.
> > >
> > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > > return -EOPNOTSUPP;
> > >
> > > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC)
> > > if (is_dynamic)
> > > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST;
> > >
> > >
> > > And perhaps for other drivers:
> > >
> > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > > return -EOPNOTSUPP;
> > > if (fdb_flags)
> > > return 0;
> > >
> > > Perhaps a helper would be warranted for the above.
> >
> > How would such a helper look? Inline function is not clean.
> >
> > >
> > > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP
> > > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha
> > > propagate, -EOPNOTSUPP.
> >
> > I looked at that, but changing the caller is also a bit ugly.
>
> Answering on behalf of Simon, and hoping he will agree.
Sorry for not responding earlier - I was on vacation last week.
TBH my idea was not nearly as well developed as the one you describe below.
But yes, I agree this is an interesting approach.
> You are missing a big opportunity to make the kernel avoid doing useless work.
> The dsa_slave_fdb_event() function runs in atomic switchdev notifier context,
> and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable
> context to program hardware.
>
> Only that scheduling a deferred work item is not exactly cheap, so we try to
> avoid doing that unless we know that we'll end up doing something with that
> FDB entry once the deferred work does get scheduled:
>
> /* Check early that we're not doing work in vain.
> * Host addresses on LAG ports still require regular FDB ops,
> * since the CPU port isn't in a LAG.
> */
> if (dp->lag && !host_addr) {
> if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del)
> return -EOPNOTSUPP;
> } else {
> if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del)
> return -EOPNOTSUPP;
> }
>
> What you should be doing is you should be using the pahole tool to find
> a good place for a new unsigned long field in struct dsa_switch, and add
> a new field ds->supported_fdb_flags. You should extend the early checking
> from dsa_slave_fdb_event() and exit without doing anything if the
> (fdb->flags & ~ds->supported_fdb_flags) expression is non-zero.
>
> This way you would kill 2 birds with 1 stone, since individual drivers
> would no longer need to check the flags; DSA would guarantee not calling
> them with unsupported flags.
>
> It would be also very good to reach an agreement with switchdev
> maintainers regarding the naming of the is_static/is_dyn field.
>
> It would also be excellent if you could rename "fdb_flags" to just
> "flags" within DSA.
next prev parent reply other threads:[~2023-02-20 14:11 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-30 17:34 [Bridge] [PATCH net-next 0/5] ATU and FDB synchronization on locked ports Hans J. Schultz
2023-01-30 17:34 ` [Bridge] [PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier Hans J. Schultz
2023-02-01 18:10 ` Ido Schimmel
2023-02-02 7:28 ` netdev
2023-02-02 16:11 ` Ido Schimmel
2023-02-02 16:38 ` netdev
2023-02-03 16:14 ` Ido Schimmel
2023-02-03 16:26 ` Vladimir Oltean
2023-02-03 16:27 ` netdev
2023-02-03 17:06 ` Ido Schimmel
2023-01-30 17:34 ` [Bridge] [PATCH net-next 2/5] net: dsa: propagate flags down towards drivers Hans J. Schultz
2023-01-30 17:34 ` [Bridge] [PATCH net-next 3/5] drivers: net: dsa: add fdb entry flags incoming to switchcore drivers Hans J. Schultz
2023-01-31 18:54 ` Simon Horman
2023-02-02 16:45 ` netdev
2023-02-03 8:17 ` Simon Horman
2023-02-03 18:41 ` netdev
2023-01-30 17:34 ` [Bridge] [PATCH net-next 4/5] net: bridge: ensure FDB offloaded flag is handled as needed Hans J. Schultz
2023-02-01 18:24 ` Ido Schimmel
2023-02-02 7:32 ` netdev
2023-01-30 17:34 ` [Bridge] [PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries Hans J. Schultz
2023-01-31 18:56 ` Simon Horman
2023-02-02 17:00 ` netdev
2023-02-03 8:20 ` Simon Horman
2023-02-03 20:44 ` Vladimir Oltean
2023-02-04 8:12 ` Simon Horman
2023-02-04 8:48 ` netdev
2023-02-06 16:02 ` Simon Horman
2023-02-14 21:14 ` Hans Schultz
2023-02-17 17:44 ` Vladimir Oltean
2023-02-20 14:11 ` Simon Horman [this message]
2023-01-31 19:25 ` [Bridge] [PATCH net-next 0/5] ATU and FDB synchronization on locked ports Ido Schimmel
2023-02-02 7:37 ` netdev
2023-02-02 15:43 ` Ido Schimmel
2023-02-02 16:19 ` netdev
2023-02-02 16:36 ` Ido Schimmel
2023-02-03 21:14 ` Vladimir Oltean
2023-02-02 17:18 ` netdev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y/N/h5WMRzl0i2IT@corigine.com \
--to=simon.horman@corigine.com \
--cc=Landen.Chao@mediatek.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=andrew@lunn.ch \
--cc=ansuelsmth@gmail.com \
--cc=bridge@lists.linux-foundation.org \
--cc=claudiu.manoil@nxp.com \
--cc=clement.leger@bootlin.com \
--cc=davem@davemloft.net \
--cc=dqfext@gmail.com \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=hauke@hauke-m.de \
--cc=ivecera@redhat.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=kurt@linutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=matthias.bgg@gmail.com \
--cc=netdev@kapio-technology.com \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=pabeni@redhat.com \
--cc=razor@blackwall.org \
--cc=roopa@nvidia.com \
--cc=sean.wang@mediatek.com \
--cc=woojung.huh@microchip.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).