| Message ID | 20220404104826.1902292-1-mattias.forsblad@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: tc: dsa: Implement offload of matchall for bridged DSA ports | expand |
On Mon, 4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote: > 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. You should unregister your callback when last DSA port leaves and re-register when first joins. That way you'll get replay. Also the code needs to check the matchall is highest prio.
On 2022-04-06 03:09, Jakub Kicinski wrote: > On Mon, 4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote: >> 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. > > You should unregister your callback when last DSA port leaves and > re-register when first joins. That way you'll get replay. > So I've tried that and it partially works. I get the FLOW_BLOCK_BIND callback but tcf_action_reoffload_cb() bails out here (tc_act_bind() == 1): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/act_api.c?h=v5.18-rc1#n1819 B.c. that flag is set here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/cls_api.c?h=v5.18-rc1#n3088 I cannot say I fully understand this logic. Can you perhaps advise? > Also the code needs to check the matchall is highest prio. Isn't sufficient with this check? else if (flow_offload_has_one_action(&cls->rule->action) && cls->rule->action.entries[0].id == FLOW_ACTION_DROP) err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress); If it only has one action is must be the highest priority or am I missing something?
On 2022-04-06 11:24, Mattias Forsblad wrote: > On 2022-04-06 03:09, Jakub Kicinski wrote: >> On Mon, 4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote: >>> 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. >> >> You should unregister your callback when last DSA port leaves and >> re-register when first joins. That way you'll get replay. >> > > So I've tried that and it partially works. I get the FLOW_BLOCK_BIND > callback but tcf_action_reoffload_cb() bails out here (tc_act_bind() == 1): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/act_api.c?h=v5.18-rc1#n1819 > > B.c. that flag is set here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/cls_api.c?h=v5.18-rc1#n3088 > > I cannot say I fully understand this logic. Can you perhaps advise? > I cannot see that tcf_block_playback_offloads() -> mall_reoffload() is called at all in this case.
On Wed, 6 Apr 2022 11:24:46 +0200 Mattias Forsblad wrote: > On 2022-04-06 03:09, Jakub Kicinski wrote: > > On Mon, 4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote: > >> 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. > > > > You should unregister your callback when last DSA port leaves and > > re-register when first joins. That way you'll get replay. > > > > So I've tried that and it partially works. I get the FLOW_BLOCK_BIND > callback but tcf_action_reoffload_cb() bails out here (tc_act_bind() == 1): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/act_api.c?h=v5.18-rc1#n1819 > > B.c. that flag is set here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/cls_api.c?h=v5.18-rc1#n3088 > > I cannot say I fully understand this logic. Can you perhaps advise? tcf_action_reoffload_cb() is for action-as-first-class-citizen offload. I think you should get the reply thru tcf_block_playback_offloads(). But I haven't really kept up with the TC offloads, non-zero chance they got broken :/ > > Also the code needs to check the matchall is highest prio. > > Isn't sufficient with this check? > > else if (flow_offload_has_one_action(&cls->rule->action) && > cls->rule->action.entries[0].id == FLOW_ACTION_DROP) > err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress); > > If it only has one action is must be the highest priority or am I > missing something? That just checks there is a single action on the rule. There could be multiple rules, adding something like: if (flow->common.prio != 1) goto bail; is what I had in mind.
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 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 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. The first part of this serie uses the flow indirect framework to setup monitoring of tc qdisc and filters added to a bridge. The second 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>) Mattias Forsblad (2): net: tc: dsa: Add the matchall filter with drop action for bridged DSA ports. net: dsa: Implement tc offloading for drop target. drivers/net/dsa/mv88e6xxx/chip.c | 23 +++- include/net/dsa.h | 14 ++ net/dsa/dsa2.c | 5 + net/dsa/dsa_priv.h | 3 + net/dsa/port.c | 2 + net/dsa/slave.c | 224 ++++++++++++++++++++++++++++++- 6 files changed, 266 insertions(+), 5 deletions(-)