On 29 Jan 2025, at 15:09, Aaron Conole wrote:
> Eelco Chaudron <[email protected]> writes: > >> When TC parses the flow to install, it assumes that the datalink type >> mask is set. However, this may not always be the case, for example, >> when multiple VLANs exist but only one is enabled (vlan-limit). >> >> This patch will only process the dl_type if the mask is set. It also >> includes a unit test to verify that the TC rules are offloaded in this >> case. >> >> Fixes: 1be33d52af77 ("netdev-tc-offloads: Don't offload header modification >> on ip fragments.") >> Signed-off-by: Eelco Chaudron <[email protected]> >> >> --- >> v3: - Fixed all Ilya's style comments >> v2: - Adding is_ip_any() function that got deleted somehow >> - Fixed alignment >> --- >> lib/netdev-offload-tc.c | 11 +++++++---- >> tests/system-offloads-traffic.at | 32 +++++++++++++++++++++++++++++++- >> 2 files changed, 38 insertions(+), 5 deletions(-) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 9e163c2a6..38ea95bd4 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -2293,6 +2293,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> const struct flow_tnl *tnl = &match->flow.tunnel; >> struct flow_tnl *tnl_mask = &mask->tunnel; >> struct dpif_flow_stats adjust_stats; >> + bool exact_match_on_dl_type; >> bool recirc_act = false; >> uint32_t block_id = 0; >> struct tcf_id id; >> @@ -2310,6 +2311,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> >> memset(&flower, 0, sizeof flower); >> >> + exact_match_on_dl_type = mask->dl_type == htons(0xffff); > > I guess the only thing you could consider changing is putting this > assignment right at the point that we declare exact_match_on_dl_type. I > don't think it is too big a deal either way - consider it a very minor > nit. Thanks for reviewing this. I thought of this too, but it messes up the reverse Christmas tree style due to its dependence on another declaration. So I will keep it as is for now, as you marked it as a ‘very minor nit’. > Either way LGTM - > > Acked-by: Aaron Conole <[email protected]> > >> chain = key->recirc_id; >> mask->recirc_id = 0; >> >> @@ -2503,7 +2505,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> mask->dl_type = 0; >> mask->in_port.odp_port = 0; >> >> - if (key->dl_type == htons(ETH_P_ARP)) { >> + if (exact_match_on_dl_type && key->dl_type == htons(ETH_P_ARP)) { >> flower.key.arp.spa = key->nw_src; >> flower.key.arp.tpa = key->nw_dst; >> flower.key.arp.sha = key->arp_sha; >> @@ -2522,7 +2524,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> memset(&mask->arp_tha, 0, sizeof mask->arp_tha); >> } >> >> - if (is_ip_any(key) && !is_ipv6_fragment_and_masked(key, mask)) { >> + if (exact_match_on_dl_type && is_ip_any(key) >> + && !is_ipv6_fragment_and_masked(key, mask)) { >> flower.key.ip_proto = key->nw_proto; >> flower.mask.ip_proto = mask->nw_proto; >> mask->nw_proto = 0; >> @@ -2552,9 +2555,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> } else { >> /* This scenario should not occur. Currently, all installed IP >> DP >> * flows perform a fully masked match on the fragmentation bits. >> - * However, since TC depends on this behavior, we return >> ENOTSUPP >> + * However, since TC depends on this behavior, we return >> EOPNOTSUPP >> * for now in case this behavior changes in the future. */ >> - return EOPNOTSUPP; >> + return EOPNOTSUPP; >> } >> >> if (key->nw_proto == IPPROTO_TCP) { >> diff --git a/tests/system-offloads-traffic.at >> b/tests/system-offloads-traffic.at >> index 78c6f5d7e..32c0d2f2a 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -1016,4 +1016,34 @@ AT_CHECK( >> stdout]) >> >> OVS_TRAFFIC_VSWITCHD_STOP >> -AT_CLEANUP >> \ No newline at end of file >> +AT_CLEANUP >> + >> +AT_SETUP([offloads - 802.1ad should be offloaded]) >> +OVS_TRAFFIC_VSWITCHD_START( >> + [], [], [-- set Open_vSwitch . other_config:hw-offload=true]) >> +OVS_CHECK_8021AD() >> + >> +ADD_NAMESPACES(at_ns0, at_ns1) >> + >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> + >> +ADD_SVLAN(p0, at_ns0, 4094, "10.255.2.1/24") >> +ADD_SVLAN(p1, at_ns1, 4094, "10.255.2.2/24") >> + >> +ADD_CVLAN(p0.4094, at_ns0, 100, "10.2.2.1/24") >> +ADD_CVLAN(p1.4094, at_ns1, 100, "10.2.2.2/24") >> + >> +AT_CHECK([ovs-ofctl add-flow br0 "priority=1 action=normal"]) >> + >> +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.2.2.2]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep >> "eth_type(0x0800)" | DUMP_CLEAN_SORTED], [0], [dnl >> +in_port(2),eth(macs),eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x0800)), >> packets:0, bytes:0, used:0.001s, actions:output >> +in_port(3),eth(macs),eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x0800)), >> packets:0, bytes:0, used:0.001s, actions:output >> +]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows type=ovs | grep "eth_type(0x0800)" | >> DUMP_CLEAN_SORTED], [0], []) >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
