On 5 Sep 2023, at 12:17, Faicker Mo via dev wrote: Hi Faicker,
Thanks for this patch series! I have some comments below. > When the IP header is rewritten like NAT or ttl/tos changed, > the csum of IP header need recalculation. The UDPLITE checksum > needs recalculation if src or dst changed in IP header. > The tc-csum action is for this. The subject needs a dot at the end: netdev-tc-offload: Add csum offload of IGMP/UDPLITE/SCTP in ip rewrite. What about the following commit messages to make it a bit more clear? When the IP header is modified, for example, by NAT or a ToS/TTL change, the IP header checksum needs recalculation. In addition to the IP header checksum, for UDPLITE, its checksum also needs recalculation when any of the addresses change. This patch adds support for TC offloading of IGMP, UDP-lite, and SCTP packets by adding the correct csum action. > Signed-off-by: Faicker Mo <[email protected]> > --- > lib/tc.c | 7 ++++++- > tests/system-offloads-traffic.at | 24 ++++++++++++++++++++++++ > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/lib/tc.c b/lib/tc.c > index f49048cda..ae71390bc 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -2973,11 +2973,16 @@ csum_update_flag(struct tc_flower *flower, > } else if (flower->key.ip_proto == IPPROTO_UDP) { > flower->needs_full_ip_proto_mask = true; > flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDP; > - } else if (flower->key.ip_proto == IPPROTO_ICMP) { > + } else if (flower->key.ip_proto == IPPROTO_ICMP || > + flower->key.ip_proto == IPPROTO_IGMP || > + flower->key.ip_proto == IPPROTO_SCTP) { > flower->needs_full_ip_proto_mask = true; > } else if (flower->key.ip_proto == IPPROTO_ICMPV6) { > flower->needs_full_ip_proto_mask = true; > flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP; > + } else if (flower->key.ip_proto == IPPROTO_UDPLITE) { > + flower->needs_full_ip_proto_mask = true; > + flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE; > } else { > VLOG_WARN_RL(&error_rl, > "can't offload rewrite of IP/IPV6 with ip_proto: > %d", > diff --git a/tests/system-offloads-traffic.at > b/tests/system-offloads-traffic.at > index 7215e36e2..47b5f1ff7 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -855,3 +855,27 @@ AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded > | grep "eth_type(0x0800) > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([offloads - Add IGMP/UDPLITE/SCTP protocols to offload in ip > rewrite - offloads enabled]) Description can be shorter, i.e. just indicate what you are testing: AT_SETUP([offloads - IGMP with ip rewrite - offloads enabled]) > +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . > other_config:hw-offload=true]) > + > +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") > + > +dnl Set up the ip field modify flow. Mayeb 'dnl Set up the ip field modify flow.' > +AT_CHECK([ovs-ofctl add-flow br0 "priority=100 in_port=ovs-p0,ip > actions=mod_nw_tos:12,output:ovs-p1"]) > + > +dnl add and del multicast address to send IGMP packet. Captial A for add. > +NS_CHECK_EXEC([at_ns0], [ip addr add dev p0 224.10.10.10/24 autojoin > 2>/dev/null], [0]) > +NS_CHECK_EXEC([at_ns0], [ip addr del dev p0 224.10.10.10/24 2>/dev/null], > [0]) > + > +sleep 1 We should avoid adding sleep in new tests, as it would add additional delay. In my setup it was working fine without the delay, but it might varray on other systems. Would somehting like this work? OVS_WAIT_UNTIL([test `ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "eth_type(0x0800)" | wc -l` -ge 1]) > +dnl Check the offloaded flow. > +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep > "eth_type(0x0800)" | wc -l], [0], [dnl > +1 > +]) I prefer to have a direct flow compare: dnl Check the offloaded flow. -AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "eth_type(0x0800)" | wc -l], [0], [dnl -1 +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "eth_type(0x0800)" | DUMP_CLEAN_SORTED | strip_stats], [0], [dnl +in_port(2),eth(),eth_type(0x0800),ipv4(proto=2,tos=0xc0/0xfc,frag=no), packets:0, bytes:0, used:0.001s, actions:set(ipv4(tos=0xc/0xfc)),3 ]) In addition we should also check if checksum was really added in the TC rule? Guess you can filer the following: filter protocol ip pref 2 flower chain 0 handle 0x1 eth_type ipv4 ip_proto 02 ip_tos 192/0xfc ip_flags nofrag not_in_hw action order 1: pedit action pipe keys 1 index 1 ref 1 bind 1 key #0 at ipv4+0: val 000c0000 mask ff03ffff action order 2: csum (iph) action pipe index 1 ref 1 bind 1 no_percpu action order 3: mirred (Egress Redirect to device ovs-p1) stolen index 1 ref 1 bind 1 cookie 354b590815442cd5b99fe5a79a1e85fc no_percpu >From the 'tc -d filter show dev ovs-p0 ingress' command. > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.39.3 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
