Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
"David S. Miller" <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
Andrew Lunn <andrew@lunn.ch>,
Vivien Didelot <vivien.didelot@gmail.com>,
Vladimir Oltean <olteanv@gmail.com>,
Kurt Kanzenbach <kurt@linutronix.de>,
Woojung Huh <woojung.huh@microchip.com>,
UNGLinuxDriver@microchip.com, Sean Wang <sean.wang@mediatek.com>,
Landen Chao <Landen.Chao@mediatek.com>,
DENG Qingfang <dqfext@gmail.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Claudiu Manoil <claudiu.manoil@nxp.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
George McCollister <george.mccollister@gmail.com>
Subject: [RFC PATCH v2 net-next 0/8] Remove the "dsa_to_port in a loop" antipattern
Date: Tue, 10 Aug 2021 19:14:40 +0300 [thread overview]
Message-ID: <20210810161448.1879192-1-vladimir.oltean@nxp.com> (raw)
v1->v2: more patches
The DSA core and drivers currently iterate too much through the port
list of a switch. For example, this snippet:
for (port = 0; port < ds->num_ports; port++) {
if (!dsa_is_cpu_port(ds, port))
continue;
ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
}
iterates through ds->num_ports once, and then calls dsa_is_cpu_port to
filter out the other types of ports. But that function has a hidden call
to dsa_to_port() in it, which contains:
list_for_each_entry(dp, &dst->ports, list)
if (dp->ds == ds && dp->index == p)
return dp;
where the only thing we wanted to know in the first place was whether
dp->type == DSA_PORT_TYPE_CPU or not.
So it seems that the problem is that we are not iterating with the right
variable. We have an "int port" but in fact need a "struct dsa_port *dp".
This has started being an issue since this patch series:
https://patchwork.ozlabs.org/project/netdev/cover/20191020031941.3805884-1-vivien.didelot@gmail.com/
The currently proposed set of changes iterates like this:
dsa_switch_for_each_cpu_port(cpu_dp, ds)
err = ds->ops->change_tag_protocol(ds, cpu_dp->index,
tag_ops->proto);
which iterates directly over ds->dst->ports, which is a list of struct
dsa_port *dp. This makes it much easier and more efficient to check
dp->type.
As a nice side effect, with the proposed driver API, driver writers are
now encouraged to use more efficient patterns, and not only due to less
iterations through the port list. For example, something like this:
for (port = 0; port < ds->num_ports; port++)
do_something();
probably does not need to do_something() for the ports that are disabled
in the device tree. But adding extra code for that would look like this:
for (port = 0; port < ds->num_ports; port++) {
if (!dsa_is_unused_port(ds, port))
continue;
do_something();
}
and therefore, it is understandable that some driver writers may decide
to not bother. This patch series introduces a "dsa_switch_for_each_available_port"
macro which comes at no extra cost in terms of lines of code / number of
braces to the driver writer, but it has the "dsa_is_unused_port" check
embedded within it.
I changed as much code as I could, probably not all, but a start anyway.
Vladimir Oltean (8):
net: dsa: introduce a dsa_port_is_unused helper
net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers
net: dsa: b53: express b53_for_each_port in terms of
dsa_switch_for_each_port
net: dsa: finish conversion to dsa_switch_for_each_port
net: dsa: remove gratuitous use of dsa_is_{user,dsa,cpu}_port
net: dsa: convert cross-chip notifiers to iterate using dp
net: dsa: tag_8021q: finish conversion to dsa_switch_for_each_port
drivers/net/dsa/b53/b53_common.c | 26 ++-
drivers/net/dsa/b53/b53_priv.h | 6 +-
drivers/net/dsa/bcm_sf2.c | 8 +-
drivers/net/dsa/hirschmann/hellcreek.c | 27 +--
.../net/dsa/hirschmann/hellcreek_hwtstamp.c | 19 +-
drivers/net/dsa/microchip/ksz9477.c | 19 +-
drivers/net/dsa/microchip/ksz_common.c | 19 +-
drivers/net/dsa/mt7530.c | 58 +++---
drivers/net/dsa/mv88e6xxx/chip.c | 48 ++---
drivers/net/dsa/mv88e6xxx/hwtstamp.c | 10 +-
drivers/net/dsa/mv88e6xxx/port.c | 12 +-
drivers/net/dsa/ocelot/felix.c | 79 +++-----
drivers/net/dsa/ocelot/felix_vsc9959.c | 11 +-
drivers/net/dsa/ocelot/seville_vsc9953.c | 14 +-
drivers/net/dsa/qca8k.c | 32 ++--
drivers/net/dsa/sja1105/sja1105_main.c | 176 ++++++++----------
drivers/net/dsa/sja1105/sja1105_mdio.c | 12 +-
drivers/net/dsa/xrs700x/xrs700x.c | 37 ++--
include/net/dsa.h | 47 ++++-
net/dsa/dsa.c | 22 +--
net/dsa/dsa2.c | 38 ++--
net/dsa/port.c | 11 +-
net/dsa/slave.c | 2 +-
net/dsa/switch.c | 170 ++++++++---------
net/dsa/tag_8021q.c | 121 ++++++------
25 files changed, 501 insertions(+), 523 deletions(-)
--
2.25.1
next reply other threads:[~2021-08-10 16:17 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-10 16:14 Vladimir Oltean [this message]
2021-08-10 16:14 ` [RFC PATCH v2 net-next 1/8] net: dsa: introduce a dsa_port_is_unused helper Vladimir Oltean
2021-08-10 16:14 ` [RFC PATCH v2 net-next 2/8] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core Vladimir Oltean
2021-08-10 16:14 ` [RFC PATCH v2 net-next 3/8] net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers Vladimir Oltean
2021-08-10 16:14 ` [RFC PATCH v2 net-next 4/8] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port Vladimir Oltean
2021-08-11 8:40 ` Florian Fainelli
2021-08-10 16:14 ` [RFC PATCH v2 net-next 5/8] net: dsa: finish conversion to dsa_switch_for_each_port Vladimir Oltean
2021-08-10 16:14 ` [RFC PATCH v2 net-next 6/8] net: dsa: remove gratuitous use of dsa_is_{user,dsa,cpu}_port Vladimir Oltean
2021-08-10 16:14 ` [RFC PATCH v2 net-next 7/8] net: dsa: convert cross-chip notifiers to iterate using dp Vladimir Oltean
2021-08-10 16:14 ` [RFC PATCH v2 net-next 8/8] net: dsa: tag_8021q: finish conversion to dsa_switch_for_each_port Vladimir Oltean
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=20210810161448.1879192-1-vladimir.oltean@nxp.com \
--to=vladimir.oltean@nxp.com \
--cc=Landen.Chao@mediatek.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=andrew@lunn.ch \
--cc=claudiu.manoil@nxp.com \
--cc=davem@davemloft.net \
--cc=dqfext@gmail.com \
--cc=f.fainelli@gmail.com \
--cc=george.mccollister@gmail.com \
--cc=kuba@kernel.org \
--cc=kurt@linutronix.de \
--cc=matthias.bgg@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=sean.wang@mediatek.com \
--cc=vivien.didelot@gmail.com \
--cc=woojung.huh@microchip.com \
--subject='Re: [RFC PATCH v2 net-next 0/8] Remove the "dsa_to_port in a loop" antipattern' \
/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
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 NNTP newsgroup(s).