On 7/20/23 17:32, Aaron Conole wrote: > Faicker Mo via dev <[email protected]> writes: > >> The warning message is >> |00001|tc(handler4)|WARN|can't offload rewrite of IP/IPV6 with ip_proto: X. >> >> Some ip protocols like ipip, gre and so on do not need the recalculation of >> the checksum of themself except for the ip header checksum recalculation >> in the ip header rewrite case. >> The tc csum action also supports IGMP, UDPLITE and SCTP. >> Enable the offload of the ip protocols besides the TCP, UDP and ICMP. >> >> Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc") >> Signed-off-by: Faicker Mo <[email protected]> >> --- > > Hi Faicker, > >> lib/tc.c | 17 +++++++--------- >> tests/system-offloads-traffic.at | 34 ++++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+), 10 deletions(-) >> >> diff --git a/lib/tc.c b/lib/tc.c >> index f49048cda..501c7ceb8 100644 >> --- a/lib/tc.c >> +++ b/lib/tc.c >> @@ -2967,22 +2967,19 @@ csum_update_flag(struct tc_flower *flower, >> case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6: >> case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP: >> case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP: >> + flower->needs_full_ip_proto_mask = true; >> if (flower->key.ip_proto == IPPROTO_TCP) { >> - flower->needs_full_ip_proto_mask = true; >> flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_TCP; >> } 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) { >> - 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 { >> - VLOG_WARN_RL(&error_rl, >> - "can't offload rewrite of IP/IPV6 with ip_proto: >> %d", >> - flower->key.ip_proto); >> - break; >> + } else if (flower->key.ip_proto == IPPROTO_IGMP) { >> + flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP; >> + } else if (flower->key.ip_proto == IPPROTO_UDPLITE) { >> + flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE; >> + } else if (flower->key.ip_proto == IPPROTO_SCTP) { >> + flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP; > > This seems like two changes to me. > > First change is adding additional support for these other csum update > flags. This could be argued as feature.
Sounds like a feature to me. > > Second change is dropping the WARN message and always setting > needs_full_ip_proto_mask. I'm not really sure we can do this second change. There are protocols that require L4 checksum recalculation on IP header changes. These are not only well known TCP/UDP. For example, the DCCP seems to be such a protocol. And who knows what other protocols need that as well. So, we should not blindly allow all the protocols. We should only allow ones that are known to not require any special handling, or ones for which the special handling is supported by TC. Best regards, Ilya Maximets. > >> } >> /* Fall through. */ >> case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH: >> diff --git a/tests/system-offloads-traffic.at >> b/tests/system-offloads-traffic.at >> index 7215e36e2..49b145e82 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -855,3 +855,37 @@ AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded >> | grep "eth_type(0x0800) >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([offloads - fix ip protocols not offloaded in ip rewrite - >> offloads enabled]) >> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . >> other_config:hw-offload=true]) >> + >> +AT_CHECK([ovs-ofctl add-flow br0 "priority=0 actions=normal"]) >> + >> +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. >> +AT_CHECK([ovs-ofctl add-flow br0 "priority=100 >> in_port=ovs-p0,ip,nw_dst=10.1.1.2 actions=dec_ttl,output:ovs-p1"]) >> +AT_CHECK([ovs-ofctl add-flow br0 "priority=100 >> in_port=ovs-p1,ip,nw_dst=10.1.1.1 actions=dec_ttl,output:ovs-p0"]) >> + >> +dnl Set up ipip tunnel in NS. >> +NS_CHECK_EXEC([at_ns0], [ip tunnel add ipip0 remote 10.1.1.2 2>/dev/null], >> [0]) >> +NS_CHECK_EXEC([at_ns0], [ip link set dev ipip0 up 2>/dev/null], [0]) >> +NS_CHECK_EXEC([at_ns0], [ip addr add dev ipip0 192.168.1.1/30 2>/dev/null], >> [0]) >> +NS_CHECK_EXEC([at_ns1], [ip tunnel add ipip0 remote 10.1.1.1 2>/dev/null], >> [0]) >> +NS_CHECK_EXEC([at_ns1], [ip link set dev ipip0 up 2>/dev/null], [0]) >> +NS_CHECK_EXEC([at_ns1], [ip addr add dev ipip0 192.168.1.2/30 2>/dev/null], >> [0]) >> + >> +dnl Check the tunnel. >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 192.168.1.2 | >> FORMAT_PING], [0], [dnl >> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >> +]) >> + >> +dnl Check the offloaded flow. >> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep >> "eth_type(0x0800)" | wc -l], [0], [dnl >> +2 >> +]) > > In general there is a check for the specific flows by doing '| > DUMP_CLEAN_SORTED' but I also see that there are some tests which are > simply piping through 'wc -l'. I prefer to see the actual flows in the > test, but I guess it isn't as big a deal here. > >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
