* [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports
@ 2022-04-11 12:06 Mattias Forsblad
2022-04-11 12:06 ` [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them Mattias Forsblad
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Mattias Forsblad @ 2022-04-11 12:06 UTC (permalink / raw)
To: netdev
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz,
Mattias Forsblad
Greetings,
This series implements offloading of tc matchall filter to HW
for bridged DSA ports.
Background
When using a non-VLAN filtering bridge we want to be able to drop
traffic directed to the CPU port so that the CPU doesn't get unnecessary loaded.
This is specially important when we have disabled learning on user ports.
A basic sample configuration could be something like this:
br0
/ \
swp0 swp1
ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
ip link set swp0 master br0
ip link set swp1 master br0
ip link set swp0 type bridge_slave learning off
ip link set swp1 type bridge_slave learning off
ip link set swp0 up
ip link set swp1 up
ip link set br0 up
After discussions here: https://lore.kernel.org/netdev/YjMo9xyoycXgSWXS@shredder/
it was advised to use tc to set an ingress filter that could then
be offloaded to HW, like so:
tc qdisc add dev br0 clsact
tc filter add dev br0 ingress pref 1 proto all matchall action drop
Another situation that needs to be handled is when there is a
foreign interface in the bridge. In this case traffic must reach the
bridge, like the setup below.
br0
/ | \
swp0 swp1 veth0
Yet another case is when we have ports that are bonded with an
foreign interface added to the bridge.
br0
/ \
bond0 veth0
/ \
swp0 swp1
These examples highlight the need to evaluate the bridge stack
to be able to make the right decision about whetever we can
offload this to hw or not.
Limitations
If there is tc rules on a bridge and all the ports leave the bridge
and then joins the bridge again, the indirect framwork doesn't seem
to reoffload them at join. The tc rules need to be torn down and
re-added. This seems to be because of limitations in the tc
framework. A fix for this would need another patch-series in itself.
However we prepare for when this issue is fixed by registring and
deregistring when a dsa_bridge is created/destroyed so it should
work when it's solved.
The first patch in this series now include changes done by Vladimir
Oltean to cleanup netdev notifier code and check for foreign
interfaces. The second part uses the flow indirect framework to
setup monitoring of tc qdisc and filters added to a bridge.
The last part offloads the matchall filter to HW for Marvell
switches.
RFC -> v1: Monitor bridge join/leave and re-evaluate offloading (Vladimir Oltean)
v2: Fix code standard compliance (Jakub Kicinski)
v3: Fix warning from kernel test robot (<lkp@intel.com>)
v4: Check matchall priority (Jakub)
Use boolean type (Vladimir)
Use Vladimirs code for checking foreign interfaces
Drop unused argument (Vladimir)
Add switchdev notifier (Vladimir)
Only call ops when value have changed (Vladimir)
Add error check (Vladimir)
Mattias Forsblad (3):
net: dsa: track whetever bridges have foreign interfaces in them
net: dsa: Add support for offloading tc matchall with drop target
net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell
switches
drivers/net/dsa/mv88e6xxx/chip.c | 17 +-
include/net/dsa.h | 15 ++
include/net/switchdev.h | 2 +
net/dsa/dsa2.c | 2 +
net/dsa/dsa_priv.h | 3 +
net/dsa/port.c | 14 ++
net/dsa/slave.c | 321 +++++++++++++++++++++++++++++--
7 files changed, 361 insertions(+), 13 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them 2022-04-11 12:06 [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Mattias Forsblad @ 2022-04-11 12:06 ` Mattias Forsblad 2022-04-12 13:50 ` Vladimir Oltean 2022-04-11 12:06 ` [PATCH v4 net-next 2/3] net: dsa: Add support for offloading tc matchall with drop target Mattias Forsblad ` (2 subsequent siblings) 3 siblings, 1 reply; 8+ messages in thread From: Mattias Forsblad @ 2022-04-11 12:06 UTC (permalink / raw) To: netdev Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz, Mattias Forsblad Track if a bridge stack has any foreign interfaces in them. This patch is based on work done by Vlodimir Oltean. Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com> --- include/net/dsa.h | 1 + net/dsa/dsa_priv.h | 1 + net/dsa/slave.c | 88 +++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/include/net/dsa.h b/include/net/dsa.h index 934958fda962..52b6da7d45b3 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -242,6 +242,7 @@ struct dsa_bridge { unsigned int num; bool tx_fwd_offload; refcount_t refcount; + u8 have_foreign:1; }; struct dsa_port { diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 5d3f4a67dce1..d610776ecd76 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -320,6 +320,7 @@ void dsa_slave_setup_tagger(struct net_device *slave); int dsa_slave_change_mtu(struct net_device *dev, int new_mtu); int dsa_slave_manage_vlan_filtering(struct net_device *dev, bool vlan_filtering); +int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev); static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev) { diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 41c69a6e7854..feaf64564c6e 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2485,6 +2485,9 @@ static int dsa_slave_changeupper(struct net_device *dev, struct netlink_ext_ack *extack; int err = NOTIFY_DONE; + if (!dsa_slave_dev_check(dev)) + return err; + extack = netdev_notifier_info_to_extack(&info->info); if (netif_is_bridge_master(info->upper_dev)) { @@ -2539,6 +2542,9 @@ static int dsa_slave_prechangeupper(struct net_device *dev, { struct dsa_port *dp = dsa_slave_to_port(dev); + if (!dsa_slave_dev_check(dev)) + return NOTIFY_DONE; + if (netif_is_bridge_master(info->upper_dev) && !info->linking) dsa_port_pre_bridge_leave(dp, info->upper_dev); else if (netif_is_lag_master(info->upper_dev) && !info->linking) @@ -2559,6 +2565,9 @@ dsa_slave_lag_changeupper(struct net_device *dev, int err = NOTIFY_DONE; struct dsa_port *dp; + if (!netif_is_lag_master(dev)) + return err; + netdev_for_each_lower_dev(dev, lower, iter) { if (!dsa_slave_dev_check(lower)) continue; @@ -2588,6 +2597,9 @@ dsa_slave_lag_prechangeupper(struct net_device *dev, int err = NOTIFY_DONE; struct dsa_port *dp; + if (!netif_is_lag_master(dev)) + return err; + netdev_for_each_lower_dev(dev, lower, iter) { if (!dsa_slave_dev_check(lower)) continue; @@ -2605,6 +2617,18 @@ dsa_slave_lag_prechangeupper(struct net_device *dev, return err; } +static int dsa_bridge_changelower(struct net_device *dev, + struct netdev_notifier_changeupper_info *info) +{ + int err; + + if (!netif_is_bridge_master(info->upper_dev)) + return NOTIFY_DONE; + + err = dsa_bridge_foreign_dev_update(info->upper_dev); + return notifier_from_errno(err); +} + static int dsa_prevent_bridging_8021q_upper(struct net_device *dev, struct netdev_notifier_changeupper_info *info) @@ -2709,22 +2733,33 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, if (err != NOTIFY_DONE) return err; - if (dsa_slave_dev_check(dev)) - return dsa_slave_prechangeupper(dev, ptr); + err = dsa_slave_prechangeupper(dev, ptr); + if (notifier_to_errno(err)) + return err; - if (netif_is_lag_master(dev)) - return dsa_slave_lag_prechangeupper(dev, ptr); + err = dsa_slave_lag_prechangeupper(dev, ptr); + if (notifier_to_errno(err)) + return err; break; } - case NETDEV_CHANGEUPPER: - if (dsa_slave_dev_check(dev)) - return dsa_slave_changeupper(dev, ptr); + case NETDEV_CHANGEUPPER: { + int err; - if (netif_is_lag_master(dev)) - return dsa_slave_lag_changeupper(dev, ptr); + err = dsa_slave_changeupper(dev, ptr); + if (notifier_to_errno(err)) + return err; + + err = dsa_slave_lag_changeupper(dev, ptr); + if (notifier_to_errno(err)) + return err; + + err = dsa_bridge_changelower(dev, ptr); + if (notifier_to_errno(err)) + return err; break; + } case NETDEV_CHANGELOWERSTATE: { struct netdev_notifier_changelowerstate_info *info = ptr; struct dsa_port *dp; @@ -2877,6 +2912,41 @@ static bool dsa_foreign_dev_check(const struct net_device *dev, return true; } +int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev) +{ + struct net_device *first_slave, *lower; + struct dsa_bridge *bridge = NULL; + struct dsa_switch_tree *dst; + bool have_foreign = false; + struct list_head *iter; + struct dsa_port *dp; + + list_for_each_entry(dst, &dsa_tree_list, list) { + dsa_tree_for_each_user_port(dp, dst) { + if (dsa_port_offloads_bridge_dev(dp, bridge_dev)) { + bridge = dp->bridge; + first_slave = dp->slave; + break; + } + } + } + + /* Bridge with no DSA interface in it */ + if (!bridge) + return 0; + + netdev_for_each_lower_dev(bridge_dev, lower, iter) { + if (dsa_foreign_dev_check(first_slave, lower)) { + have_foreign = true; + break; + } + } + + bridge->have_foreign = have_foreign; + + return 0; +} + static int dsa_slave_fdb_event(struct net_device *dev, struct net_device *orig_dev, unsigned long event, const void *ctx, -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them 2022-04-11 12:06 ` [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them Mattias Forsblad @ 2022-04-12 13:50 ` Vladimir Oltean 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Oltean @ 2022-04-12 13:50 UTC (permalink / raw) To: Mattias Forsblad Cc: netdev, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz On Mon, Apr 11, 2022 at 02:06:31PM +0200, Mattias Forsblad wrote: > Track if a bridge stack has any foreign interfaces in them. > > This patch is based on work done by Vlodimir Oltean. Originally these were 2 patches which were squashed (and they were 2 patches for a reason), they're missing proper authorship (mine, my sign off, your sign off), and my name is misspelled. > > Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com> > --- > include/net/dsa.h | 1 + > net/dsa/dsa_priv.h | 1 + > net/dsa/slave.c | 88 +++++++++++++++++++++++++++++++++++++++++----- > 3 files changed, 81 insertions(+), 9 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 934958fda962..52b6da7d45b3 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -242,6 +242,7 @@ struct dsa_bridge { > unsigned int num; > bool tx_fwd_offload; > refcount_t refcount; > + u8 have_foreign:1; > }; > > struct dsa_port { > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 5d3f4a67dce1..d610776ecd76 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -320,6 +320,7 @@ void dsa_slave_setup_tagger(struct net_device *slave); > int dsa_slave_change_mtu(struct net_device *dev, int new_mtu); > int dsa_slave_manage_vlan_filtering(struct net_device *dev, > bool vlan_filtering); > +int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev); > > static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev) > { > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 41c69a6e7854..feaf64564c6e 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -2485,6 +2485,9 @@ static int dsa_slave_changeupper(struct net_device *dev, > struct netlink_ext_ack *extack; > int err = NOTIFY_DONE; > > + if (!dsa_slave_dev_check(dev)) > + return err; > + > extack = netdev_notifier_info_to_extack(&info->info); > > if (netif_is_bridge_master(info->upper_dev)) { > @@ -2539,6 +2542,9 @@ static int dsa_slave_prechangeupper(struct net_device *dev, > { > struct dsa_port *dp = dsa_slave_to_port(dev); > > + if (!dsa_slave_dev_check(dev)) > + return NOTIFY_DONE; > + > if (netif_is_bridge_master(info->upper_dev) && !info->linking) > dsa_port_pre_bridge_leave(dp, info->upper_dev); > else if (netif_is_lag_master(info->upper_dev) && !info->linking) > @@ -2559,6 +2565,9 @@ dsa_slave_lag_changeupper(struct net_device *dev, > int err = NOTIFY_DONE; > struct dsa_port *dp; > > + if (!netif_is_lag_master(dev)) > + return err; > + > netdev_for_each_lower_dev(dev, lower, iter) { > if (!dsa_slave_dev_check(lower)) > continue; > @@ -2588,6 +2597,9 @@ dsa_slave_lag_prechangeupper(struct net_device *dev, > int err = NOTIFY_DONE; > struct dsa_port *dp; > > + if (!netif_is_lag_master(dev)) > + return err; > + > netdev_for_each_lower_dev(dev, lower, iter) { > if (!dsa_slave_dev_check(lower)) > continue; > @@ -2605,6 +2617,18 @@ dsa_slave_lag_prechangeupper(struct net_device *dev, > return err; > } > > +static int dsa_bridge_changelower(struct net_device *dev, > + struct netdev_notifier_changeupper_info *info) > +{ > + int err; > + > + if (!netif_is_bridge_master(info->upper_dev)) > + return NOTIFY_DONE; > + > + err = dsa_bridge_foreign_dev_update(info->upper_dev); > + return notifier_from_errno(err); > +} > + > static int > dsa_prevent_bridging_8021q_upper(struct net_device *dev, > struct netdev_notifier_changeupper_info *info) > @@ -2709,22 +2733,33 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, > if (err != NOTIFY_DONE) > return err; > > - if (dsa_slave_dev_check(dev)) > - return dsa_slave_prechangeupper(dev, ptr); > + err = dsa_slave_prechangeupper(dev, ptr); > + if (notifier_to_errno(err)) > + return err; > > - if (netif_is_lag_master(dev)) > - return dsa_slave_lag_prechangeupper(dev, ptr); > + err = dsa_slave_lag_prechangeupper(dev, ptr); > + if (notifier_to_errno(err)) > + return err; > > break; > } > - case NETDEV_CHANGEUPPER: > - if (dsa_slave_dev_check(dev)) > - return dsa_slave_changeupper(dev, ptr); > + case NETDEV_CHANGEUPPER: { > + int err; > > - if (netif_is_lag_master(dev)) > - return dsa_slave_lag_changeupper(dev, ptr); > + err = dsa_slave_changeupper(dev, ptr); > + if (notifier_to_errno(err)) > + return err; > + > + err = dsa_slave_lag_changeupper(dev, ptr); > + if (notifier_to_errno(err)) > + return err; > + > + err = dsa_bridge_changelower(dev, ptr); > + if (notifier_to_errno(err)) > + return err; > > break; > + } > case NETDEV_CHANGELOWERSTATE: { > struct netdev_notifier_changelowerstate_info *info = ptr; > struct dsa_port *dp; > @@ -2877,6 +2912,41 @@ static bool dsa_foreign_dev_check(const struct net_device *dev, > return true; > } > > +int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev) > +{ > + struct net_device *first_slave, *lower; > + struct dsa_bridge *bridge = NULL; > + struct dsa_switch_tree *dst; > + bool have_foreign = false; > + struct list_head *iter; > + struct dsa_port *dp; > + > + list_for_each_entry(dst, &dsa_tree_list, list) { > + dsa_tree_for_each_user_port(dp, dst) { > + if (dsa_port_offloads_bridge_dev(dp, bridge_dev)) { > + bridge = dp->bridge; > + first_slave = dp->slave; > + break; > + } > + } > + } > + > + /* Bridge with no DSA interface in it */ > + if (!bridge) > + return 0; > + > + netdev_for_each_lower_dev(bridge_dev, lower, iter) { > + if (dsa_foreign_dev_check(first_slave, lower)) { > + have_foreign = true; > + break; > + } > + } > + > + bridge->have_foreign = have_foreign; > + > + return 0; > +} > + > static int dsa_slave_fdb_event(struct net_device *dev, > struct net_device *orig_dev, > unsigned long event, const void *ctx, > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 net-next 2/3] net: dsa: Add support for offloading tc matchall with drop target 2022-04-11 12:06 [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Mattias Forsblad 2022-04-11 12:06 ` [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them Mattias Forsblad @ 2022-04-11 12:06 ` Mattias Forsblad 2022-04-11 12:06 ` [PATCH v4 net-next 3/3] net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell switches Mattias Forsblad 2022-04-11 12:39 ` [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Vladimir Oltean 3 siblings, 0 replies; 8+ messages in thread From: Mattias Forsblad @ 2022-04-11 12:06 UTC (permalink / raw) To: netdev Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz, Mattias Forsblad Use the flow indirect framework on bridged DSA ports to be able to set up offloading of matchall filter with drop target. Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com> --- include/net/dsa.h | 14 +++ include/net/switchdev.h | 2 + net/dsa/dsa2.c | 2 + net/dsa/dsa_priv.h | 2 + net/dsa/port.c | 14 +++ net/dsa/slave.c | 233 +++++++++++++++++++++++++++++++++++++++- 6 files changed, 264 insertions(+), 3 deletions(-) diff --git a/include/net/dsa.h b/include/net/dsa.h index 52b6da7d45b3..009a03889d7c 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -171,6 +171,9 @@ struct dsa_switch_tree { /* Track the largest switch index within a tree */ unsigned int last_switch; + + /* For tc indirect bookkeeping */ + struct list_head tc_indr_block_list; }; /* LAG IDs are one-based, the dst->lags array is zero-based */ @@ -212,6 +215,7 @@ static inline int dsa_lag_id(struct dsa_switch_tree *dst, enum dsa_port_mall_action_type { DSA_PORT_MALL_MIRROR, DSA_PORT_MALL_POLICER, + DSA_PORT_MALL_DROP, }; /* TC mirroring entry */ @@ -220,6 +224,11 @@ struct dsa_mall_mirror_tc_entry { bool ingress; }; +/* TC drop entry */ +struct dsa_mall_drop_tc_entry { + bool enable; +}; + /* TC port policer entry */ struct dsa_mall_policer_tc_entry { u32 burst; @@ -234,13 +243,17 @@ struct dsa_mall_tc_entry { union { struct dsa_mall_mirror_tc_entry mirror; struct dsa_mall_policer_tc_entry policer; + struct dsa_mall_drop_tc_entry drop; }; }; struct dsa_bridge { struct net_device *dev; + struct dsa_switch_tree *dst; unsigned int num; bool tx_fwd_offload; + bool local_rcv:1; + bool local_rcv_effective:1; refcount_t refcount; u8 have_foreign:1; }; @@ -1035,6 +1048,7 @@ struct dsa_switch_ops { int (*port_policer_add)(struct dsa_switch *ds, int port, struct dsa_mall_policer_tc_entry *policer); void (*port_policer_del)(struct dsa_switch *ds, int port); + int (*bridge_local_rcv)(struct dsa_switch *ds, struct dsa_bridge *bridge); int (*port_setup_tc)(struct dsa_switch *ds, int port, enum tc_setup_type type, void *type_data); diff --git a/include/net/switchdev.h b/include/net/switchdev.h index aa0171d5786d..0dd9a870547a 100644 --- a/include/net/switchdev.h +++ b/include/net/switchdev.h @@ -31,6 +31,7 @@ enum switchdev_attr_id { SWITCHDEV_ATTR_ID_BRIDGE_MST, SWITCHDEV_ATTR_ID_MRP_PORT_ROLE, SWITCHDEV_ATTR_ID_VLAN_MSTI, + SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV, }; struct switchdev_mst_state { @@ -66,6 +67,7 @@ struct switchdev_attr { bool mc_disabled; /* MC_DISABLED */ u8 mrp_port_role; /* MRP_PORT_ROLE */ struct switchdev_vlan_msti vlan_msti; /* VLAN_MSTI */ + u8 local_rcv; /* BRIDGE_LOCAL_RCV */ } u; }; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index ca6af86964bc..2bb53ec436da 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -247,6 +247,8 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index) INIT_LIST_HEAD(&dst->list); list_add_tail(&dst->list, &dsa_tree_list); + INIT_LIST_HEAD(&dst->tc_indr_block_list); + kref_init(&dst->refcount); return dst; diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index d610776ecd76..bb3fc785731f 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -321,6 +321,8 @@ int dsa_slave_change_mtu(struct net_device *dev, int new_mtu); int dsa_slave_manage_vlan_filtering(struct net_device *dev, bool vlan_filtering); int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev); +int dsa_setup_bridge_tc_indr(struct dsa_bridge *bridge); +void dsa_cleanup_bridge_tc_indr(struct dsa_bridge *bridge); static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev) { diff --git a/net/dsa/port.c b/net/dsa/port.c index 32d472a82241..9dbeeea76ef3 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -361,6 +361,9 @@ static int dsa_port_bridge_create(struct dsa_port *dp, refcount_set(&bridge->refcount, 1); bridge->dev = br; + bridge->local_rcv = true; + bridge->local_rcv_effective = true; + bridge->dst = dp->ds->dst; bridge->num = dsa_bridge_num_get(br, ds->max_num_bridges); if (ds->max_num_bridges && !bridge->num) { @@ -372,6 +375,8 @@ static int dsa_port_bridge_create(struct dsa_port *dp, dp->bridge = bridge; + dsa_setup_bridge_tc_indr(bridge); + return 0; } @@ -388,6 +393,8 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp, if (bridge->num) dsa_bridge_num_put(br, bridge->num); + dsa_cleanup_bridge_tc_indr(bridge); + kfree(bridge); } @@ -602,8 +609,15 @@ int dsa_port_lag_join(struct dsa_port *dp, struct net_device *lag_dev, if (err) goto err_bridge_join; + err = dsa_bridge_foreign_dev_update(bridge_dev); + if (err) + goto err_foreign_update; + return 0; +err_foreign_update: + dsa_port_pre_bridge_leave(dp, bridge_dev); + dsa_port_bridge_leave(dp, bridge_dev); err_bridge_join: dsa_port_notify(dp, DSA_NOTIFIER_LAG_LEAVE, &info); err_lag_join: diff --git a/net/dsa/slave.c b/net/dsa/slave.c index feaf64564c6e..d3872fa0479d 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1246,6 +1246,82 @@ dsa_slave_add_cls_matchall_mirred(struct net_device *dev, return err; } +static void +dsa_slave_bridge_local_rcv_offload_notify(struct dsa_bridge *bridge, bool local_rcv) +{ + struct switchdev_attr attr = { + .orig_dev = bridge->dev, + .id = SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV, + .flags = SWITCHDEV_F_DEFER, + .u.local_rcv = local_rcv, + }; + + switchdev_port_attr_set(bridge->dev, &attr, NULL); +} + +static int dsa_slave_bridge_local_rcv_offload(struct net_device *dev, + struct dsa_mall_drop_tc_entry *drop) +{ + struct dsa_port *dp = NULL; + int new_local_rcv; + int err; + + if (!dsa_slave_dev_check(dev)) + return 0; + + dp = dsa_slave_to_port(dev); + if (!dp || !dp->bridge) + return 0; + + new_local_rcv = dp->bridge->local_rcv || dp->bridge->have_foreign; + if (new_local_rcv != dp->bridge->local_rcv_effective) { + dp->bridge->local_rcv_effective = new_local_rcv; + err = dp->ds->ops->bridge_local_rcv(dp->ds, dp->bridge); + if (err) + return err; + + dsa_slave_bridge_local_rcv_offload_notify(dp->bridge, new_local_rcv); + } + + return 0; +} + +static int +dsa_slave_add_cls_matchall_drop(struct net_device *dev, + struct tc_cls_matchall_offload *cls, + bool ingress) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + struct dsa_slave_priv *p = netdev_priv(dev); + struct dsa_mall_tc_entry *mall_tc_entry; + struct dsa_mall_drop_tc_entry *drop; + struct dsa_switch *ds = dp->ds; + int err; + + if (!ds->ops->bridge_local_rcv) + return -EOPNOTSUPP; + + mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL); + if (!mall_tc_entry) + return -ENOMEM; + + mall_tc_entry->cookie = cls->cookie; + mall_tc_entry->type = DSA_PORT_MALL_DROP; + drop = &mall_tc_entry->drop; + drop->enable = true; + dp->bridge->local_rcv = false; + err = dsa_slave_bridge_local_rcv_offload(dev, drop); + if (err) + goto out; + + list_add_tail(&mall_tc_entry->list, &p->mall_tc_list); + +out: + kfree(mall_tc_entry); + + return err; +} + static int dsa_slave_add_cls_matchall_police(struct net_device *dev, struct tc_cls_matchall_offload *cls, @@ -1320,6 +1396,10 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev, else if (flow_offload_has_one_action(&cls->rule->action) && cls->rule->action.entries[0].id == FLOW_ACTION_POLICE) err = dsa_slave_add_cls_matchall_police(dev, cls, ingress); + else if (flow_offload_has_one_action(&cls->rule->action) && + cls->rule->action.entries[0].id == FLOW_ACTION_DROP && + cls->common.prio == 1) + err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress); return err; } @@ -1347,6 +1427,13 @@ static void dsa_slave_del_cls_matchall(struct net_device *dev, if (ds->ops->port_policer_del) ds->ops->port_policer_del(ds, dp->index); break; + case DSA_PORT_MALL_DROP: + if (!dp->bridge) + return; + dp->bridge->local_rcv = true; + mall_tc_entry->drop.enable = false; + dsa_slave_bridge_local_rcv_offload(dev, &mall_tc_entry->drop); + break; default: WARN_ON(1); } @@ -1430,7 +1517,8 @@ static int dsa_slave_setup_tc_cls_flower(struct net_device *dev, } } -static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data, +static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, + void *cls, void *cb_priv, bool ingress) { struct net_device *dev = cb_priv; @@ -1440,9 +1528,9 @@ static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data, switch (type) { case TC_SETUP_CLSMATCHALL: - return dsa_slave_setup_tc_cls_matchall(dev, type_data, ingress); + return dsa_slave_setup_tc_cls_matchall(dev, cls, ingress); case TC_SETUP_CLSFLOWER: - return dsa_slave_setup_tc_cls_flower(dev, type_data, ingress); + return dsa_slave_setup_tc_cls_flower(dev, cls, ingress); default: return -EOPNOTSUPP; } @@ -1514,6 +1602,133 @@ static int dsa_slave_setup_ft_block(struct dsa_switch *ds, int port, return master->netdev_ops->ndo_setup_tc(master, TC_SETUP_FT, type_data); } +static LIST_HEAD(dsa_slave_block_indr_cb_list); + +struct dsa_slave_indr_block_cb_priv { + struct dsa_bridge *bridge; + struct list_head list; +}; + +static int dsa_slave_setup_bridge_block_cb(enum tc_setup_type type, + void *type_data, + void *cb_priv) +{ + struct dsa_slave_indr_block_cb_priv *priv = cb_priv; + struct tc_cls_matchall_offload *cls; + struct dsa_switch_tree *dst; + struct dsa_port *dp; + int ret = 0; + + cls = (struct tc_cls_matchall_offload *)type_data; + + list_for_each_entry(dst, &dsa_tree_list, list) { + dsa_tree_for_each_user_port(dp, dst) { + if (dsa_port_offloads_bridge_dev(dp, priv->bridge->dev)) + ret += dsa_slave_setup_tc_block_cb(type, cls, dp->slave, true); + } + } + + return ret; +} + +static struct dsa_slave_indr_block_cb_priv * +dsa_slave_tc_indr_block_cb_lookup(struct dsa_switch_tree *dst, struct net_device *netdev) +{ + struct dsa_slave_indr_block_cb_priv *cb_priv; + + list_for_each_entry(cb_priv, &dst->tc_indr_block_list, list) + if (cb_priv->bridge->dev == netdev) + return cb_priv; + + return NULL; +} + +static void dsa_slave_setup_tc_indr_rel(void *cb_priv) +{ + struct dsa_slave_indr_block_cb_priv *priv = cb_priv; + + list_del(&priv->list); + kfree(priv); +} + +static int +dsa_slave_setup_bridge_tc_indr_block(struct net_device *netdev, struct Qdisc *sch, + struct dsa_bridge *bridge, + struct flow_block_offload *f, void *data, + void (*cleanup)(struct flow_block_cb *block_cb)) +{ + struct dsa_slave_indr_block_cb_priv *cb_priv; + struct flow_block_cb *block_cb; + + if (f->binder_type != FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS) + return -EOPNOTSUPP; + + switch (f->command) { + case FLOW_BLOCK_BIND: + if (netdev != bridge->dev) + return 0; + + cb_priv = kmalloc(sizeof(*cb_priv), GFP_KERNEL); + if (!cb_priv) + return -ENOMEM; + + cb_priv->bridge = bridge; + list_add(&cb_priv->list, &bridge->dst->tc_indr_block_list); + + block_cb = flow_indr_block_cb_alloc(dsa_slave_setup_bridge_block_cb, + cb_priv, cb_priv, + dsa_slave_setup_tc_indr_rel, f, + netdev, sch, data, cb_priv, cleanup); + if (IS_ERR(block_cb)) { + list_del(&cb_priv->list); + kfree(cb_priv); + return PTR_ERR(block_cb); + } + + flow_block_cb_add(block_cb, f); + list_add_tail(&block_cb->driver_list, &dsa_slave_block_indr_cb_list); + break; + case FLOW_BLOCK_UNBIND: + cb_priv = dsa_slave_tc_indr_block_cb_lookup(bridge->dst, netdev); + if (!cb_priv) + return -ENOENT; + + block_cb = flow_block_cb_lookup(f->block, + dsa_slave_setup_bridge_block_cb, + cb_priv); + if (!block_cb) + return -ENOENT; + + flow_indr_block_cb_remove(block_cb, f); + list_del(&block_cb->driver_list); + break; + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static int dsa_slave_setup_bridge_tc_indr_cb(struct net_device *netdev, struct Qdisc *sch, + void *cb_priv, + enum tc_setup_type type, void *type_data, + void *data, + void (*cleanup)(struct flow_block_cb *block_cb)) +{ + if (!netdev || !netif_is_bridge_master(netdev)) + return -EOPNOTSUPP; + + switch (type) { + case TC_SETUP_BLOCK: + return dsa_slave_setup_bridge_tc_indr_block(netdev, sch, cb_priv, + type_data, data, cleanup); + default: + break; + } + + return -EOPNOTSUPP; +} + static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data) { @@ -1535,6 +1750,17 @@ static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type, return ds->ops->port_setup_tc(ds, dp->index, type, type_data); } +int dsa_setup_bridge_tc_indr(struct dsa_bridge *bridge) +{ + return flow_indr_dev_register(dsa_slave_setup_bridge_tc_indr_cb, bridge); +} + +void dsa_cleanup_bridge_tc_indr(struct dsa_bridge *bridge) +{ + flow_indr_dev_unregister(dsa_slave_setup_bridge_tc_indr_cb, + bridge, dsa_slave_setup_tc_indr_rel); +} + static int dsa_slave_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *nfc, u32 *rule_locs) { @@ -2943,6 +3169,7 @@ int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev) } bridge->have_foreign = have_foreign; + dsa_slave_bridge_local_rcv_offload(first_slave, NULL); return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 net-next 3/3] net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell switches 2022-04-11 12:06 [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Mattias Forsblad 2022-04-11 12:06 ` [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them Mattias Forsblad 2022-04-11 12:06 ` [PATCH v4 net-next 2/3] net: dsa: Add support for offloading tc matchall with drop target Mattias Forsblad @ 2022-04-11 12:06 ` Mattias Forsblad 2022-04-11 12:39 ` [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Vladimir Oltean 3 siblings, 0 replies; 8+ messages in thread From: Mattias Forsblad @ 2022-04-11 12:06 UTC (permalink / raw) To: netdev Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz, Mattias Forsblad Add the ability to handle tc matchall drop HW offloading for Marvell switches. Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 64f4fdd02902..cbffba18e240 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1436,7 +1436,7 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port) * bridge group. */ dsa_switch_for_each_port(other_dp, ds) - if (other_dp->type == DSA_PORT_TYPE_CPU || + if ((other_dp->type == DSA_PORT_TYPE_CPU && dp->bridge->local_rcv_effective) || other_dp->type == DSA_PORT_TYPE_DSA || dsa_port_bridge_same(dp, other_dp)) pvlan |= BIT(other_dp->index); @@ -6439,6 +6439,20 @@ static void mv88e6xxx_port_mirror_del(struct dsa_switch *ds, int port, mutex_unlock(&chip->reg_lock); } +static int mv88e6xxx_bridge_local_rcv(struct dsa_switch *ds, struct dsa_bridge *bridge) +{ + struct mv88e6xxx_chip *chip = ds->priv; + int err; + + mutex_lock(&chip->reg_lock); + + err = mv88e6xxx_bridge_map(chip, *bridge); + + mutex_unlock(&chip->reg_lock); + + return err; +} + static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port, struct switchdev_brport_flags flags, struct netlink_ext_ack *extack) @@ -6837,6 +6851,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .port_mdb_del = mv88e6xxx_port_mdb_del, .port_mirror_add = mv88e6xxx_port_mirror_add, .port_mirror_del = mv88e6xxx_port_mirror_del, + .bridge_local_rcv = mv88e6xxx_bridge_local_rcv, .crosschip_bridge_join = mv88e6xxx_crosschip_bridge_join, .crosschip_bridge_leave = mv88e6xxx_crosschip_bridge_leave, .port_hwtstamp_set = mv88e6xxx_port_hwtstamp_set, -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports 2022-04-11 12:06 [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Mattias Forsblad ` (2 preceding siblings ...) 2022-04-11 12:06 ` [PATCH v4 net-next 3/3] net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell switches Mattias Forsblad @ 2022-04-11 12:39 ` Vladimir Oltean 2022-04-11 12:48 ` Mattias Forsblad 3 siblings, 1 reply; 8+ messages in thread From: Vladimir Oltean @ 2022-04-11 12:39 UTC (permalink / raw) To: Mattias Forsblad Cc: netdev, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz On Mon, Apr 11, 2022 at 02:06:30PM +0200, Mattias Forsblad wrote: > RFC -> v1: Monitor bridge join/leave and re-evaluate offloading (Vladimir Oltean) > v2: Fix code standard compliance (Jakub Kicinski) > v3: Fix warning from kernel test robot (<lkp@intel.com>) > v4: Check matchall priority (Jakub) > Use boolean type (Vladimir) > Use Vladimirs code for checking foreign interfaces > Drop unused argument (Vladimir) > Add switchdev notifier (Vladimir) By switchdev notifier you mean SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV? I'm sorry, you must have misunderstood. I said, in reference to dp->ds->ops->bridge_local_rcv(): | Not to mention this should be a cross-chip notifier, maybe a | cross-tree notifier. https://patchwork.kernel.org/project/netdevbpf/patch/20220404104826.1902292-2-mattias.forsblad@gmail.com/#24805497 A cross-chip notifier is an event signaled using dsa_tree_notify() and handled in switch.c. Its purpose is to replicate an event exactly once towards all switches in a multi-switch topology. You could have explained that this isn't necessary, because dsa_slave_setup_bridge_tc_indr_block(netdev=bridge_dev) indirectly binds dsa_slave_setup_bridge_block_cb() which calls dsa_slave_setup_tc_block_cb() for each user port under said bridge. So replicating the ds->ops->bridge_local_rcv() towards each switch is already taken care of in another way, although suboptimally, because if there are 4 user ports under br0 in switch A and 4 user ports in switch B, ds->ops->bridge_local_rcv() will be called 4 times for switch A and 4 times for switch B. 6 out of those 8 calls are for nothing. Or you could have said that you don't understand the request and ask me to clarify. But I don't understand why you've added SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV which has no consumer. Initially I thought you'd go back to having the bridge monitor flow blocks binding to its ingress chain instead of this broken indirect stuff, then emit SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV on the switchdev notifier chain which DSA catches and offloads. And initial state would be synced/unsynced via attribute replays in dsa_port_switchdev_sync_attrs(). At least that would have worked. But nope. It really looks like SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV was added to appease an unclear comment even if it made no sense. > Only call ops when value have changed (Vladimir) > Add error check (Vladimir) > > Mattias Forsblad (3): > net: dsa: track whetever bridges have foreign interfaces in them > net: dsa: Add support for offloading tc matchall with drop target > net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell > switches > > drivers/net/dsa/mv88e6xxx/chip.c | 17 +- > include/net/dsa.h | 15 ++ > include/net/switchdev.h | 2 + > net/dsa/dsa2.c | 2 + > net/dsa/dsa_priv.h | 3 + > net/dsa/port.c | 14 ++ > net/dsa/slave.c | 321 +++++++++++++++++++++++++++++-- > 7 files changed, 361 insertions(+), 13 deletions(-) > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports 2022-04-11 12:39 ` [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Vladimir Oltean @ 2022-04-11 12:48 ` Mattias Forsblad 2022-04-11 13:16 ` Vladimir Oltean 0 siblings, 1 reply; 8+ messages in thread From: Mattias Forsblad @ 2022-04-11 12:48 UTC (permalink / raw) To: Vladimir Oltean Cc: netdev, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz On 2022-04-11 14:39, Vladimir Oltean wrote: > On Mon, Apr 11, 2022 at 02:06:30PM +0200, Mattias Forsblad wrote: >> RFC -> v1: Monitor bridge join/leave and re-evaluate offloading (Vladimir Oltean) >> v2: Fix code standard compliance (Jakub Kicinski) >> v3: Fix warning from kernel test robot (<lkp@intel.com>) >> v4: Check matchall priority (Jakub) >> Use boolean type (Vladimir) >> Use Vladimirs code for checking foreign interfaces >> Drop unused argument (Vladimir) >> Add switchdev notifier (Vladimir) > > By switchdev notifier you mean SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV? > I'm sorry, you must have misunderstood. I said, in reference to > dp->ds->ops->bridge_local_rcv(): > > | Not to mention this should be a cross-chip notifier, maybe a > | cross-tree notifier. > > https://patchwork.kernel.org/project/netdevbpf/patch/20220404104826.1902292-2-mattias.forsblad@gmail.com/#24805497 > > A cross-chip notifier is an event signaled using dsa_tree_notify() and > handled in switch.c. Its purpose is to replicate an event exactly once > towards all switches in a multi-switch topology. > > You could have explained that this isn't necessary, because > dsa_slave_setup_bridge_tc_indr_block(netdev=bridge_dev) indirectly binds > dsa_slave_setup_bridge_block_cb() which calls dsa_slave_setup_tc_block_cb() > for each user port under said bridge. So replicating the ds->ops->bridge_local_rcv() > towards each switch is already taken care of in another way, although > suboptimally, because if there are 4 user ports under br0 in switch A > and 4 user ports in switch B, ds->ops->bridge_local_rcv() will be called > 4 times for switch A and 4 times for switch B. 6 out of those 8 calls > are for nothing. > > Or you could have said that you don't understand the request and ask me > to clarify. > > But I don't understand why you've added SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV > which has no consumer. Initially I thought you'd go back to having the > bridge monitor flow blocks binding to its ingress chain instead of this > broken indirect stuff, then emit SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV on > the switchdev notifier chain which DSA catches and offloads. And initial > state would be synced/unsynced via attribute replays in > dsa_port_switchdev_sync_attrs(). At least that would have worked. > But nope. It really looks like SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV was > added to appease an unclear comment even if it made no sense. > My thinking was that the notifier I was aware of was the one I implemented and someway was a preparation for consumers (that didn't exist yes). I didn't even know about dsa_tree_notify(). So I'll remove that part, yes? Is that ok even if it's not optimal, like you say? >> Only call ops when value have changed (Vladimir) >> Add error check (Vladimir) >> >> Mattias Forsblad (3): >> net: dsa: track whetever bridges have foreign interfaces in them >> net: dsa: Add support for offloading tc matchall with drop target >> net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell >> switches >> >> drivers/net/dsa/mv88e6xxx/chip.c | 17 +- >> include/net/dsa.h | 15 ++ >> include/net/switchdev.h | 2 + >> net/dsa/dsa2.c | 2 + >> net/dsa/dsa_priv.h | 3 + >> net/dsa/port.c | 14 ++ >> net/dsa/slave.c | 321 +++++++++++++++++++++++++++++-- >> 7 files changed, 361 insertions(+), 13 deletions(-) >> >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports 2022-04-11 12:48 ` Mattias Forsblad @ 2022-04-11 13:16 ` Vladimir Oltean 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Oltean @ 2022-04-11 13:16 UTC (permalink / raw) To: Mattias Forsblad Cc: netdev, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, Paolo Abeni, Tobias Waldekranz On Mon, Apr 11, 2022 at 02:48:29PM +0200, Mattias Forsblad wrote: > My thinking was that the notifier I was aware of was the one I implemented > and someway was a preparation for consumers (that didn't exist yes). I didn't even > know about dsa_tree_notify(). So I'll remove that part, yes? Is that ok even > if it's not optimal, like you say? Optimal or sub-optimal, I think there are bigger problems which we're taking too lightly. Like this one: | If there is tc rules on a bridge and all the ports leave the bridge | and then joins the bridge again, the indirect framwork doesn't seem | to reoffload them at join. The tc rules need to be torn down and | re-added. This seems to be because of limitations in the tc | framework. A fix for this would need another patch-series in itself. | However we prepare for when this issue is fixed by registring and | deregistring when a dsa_bridge is created/destroyed so it should | work when it's solved. You've presented just the more convenient angle of the limitation (DSA interfaces present as bridge ports, then leave, then re-join). But the same problem is there even when the tc rule is added to the bridge before adding any port to it. Which is more likely to result in buggy user experience, because it doesn't say anywhere that there are restrictions to the order in which things should be set up. Personally, I would first try to ask for help, as some work was clearly put into the indirect flow block offload, and the reasonable expectation is that the filter replay (and not just the bind/unbind) works on register/unregister, yet for some unknown reason it doesn't. Then, if I get no answer/help, I would consider as an alternative implementing the flow block binding logic in the bridge, and re-notifying the relevant stuff through switchdev. Via this SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV we're talking about - hence the reason I'm mentioning it. After all, the flow block binding code is mostly bloatware, so we can reduce the duplicated code switchdev drivers have to write. But I wouldn't consider leaving this up in the air if there is a non-complicated way to address it, which it seems like there is. So in short, let's discuss what's optimal overall only when we see an implementation that fully works, ok? It was my mistake to point out during review minor optimizations that could be made even if they don't follow the overall direction that the patch set should go in. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-12 13:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-11 12:06 [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Mattias Forsblad 2022-04-11 12:06 ` [PATCH v4 net-next 1/3] net: dsa: track whetever bridges have foreign interfaces in them Mattias Forsblad 2022-04-12 13:50 ` Vladimir Oltean 2022-04-11 12:06 ` [PATCH v4 net-next 2/3] net: dsa: Add support for offloading tc matchall with drop target Mattias Forsblad 2022-04-11 12:06 ` [PATCH v4 net-next 3/3] net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell switches Mattias Forsblad 2022-04-11 12:39 ` [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports Vladimir Oltean 2022-04-11 12:48 ` Mattias Forsblad 2022-04-11 13:16 ` Vladimir Oltean
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.