| Message ID | AHcAaQCwJMxxq99bfpFMu4qL.1.1693909054647.Hmail.mocan@ucloud.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v5,1/2] netdev-tc-offload: Add csum offload of IGMP/UDPLITE/SCTP in ip rewrite | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 5 Sep 2023, at 12:17, Faicker Mo via dev wrote: > The warning message is Hi Faicker, thanks for the patch! > |00001|tc(handler4)|WARN|can't offload rewrite of IP/IPV6 with ip_proto: X. > > IPIP and GRE only need the checksum recalculation of the IP header if the > IP header is rewritten. > > Eliminate the warning for IPIP and GRE protocols. Not that the subject needs a dot at the end: netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite What about the following commit message? Currently checksum recalculation is not supported with TC offload for IPIP and GRE packets. This patch adds support for TC offloading of IPIP and GRE packets by adding the correct csum action. Without this patch the following warning can be seen in the logging: Can't offload rewrite of IP/IPV6 with ip_proto: X. > Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc") > Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn> > --- > lib/tc.c | 4 +++- > tests/system-offloads-traffic.at | 35 ++++++++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/lib/tc.c b/lib/tc.c > index ae71390bc..f85703633 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -2975,7 +2975,9 @@ csum_update_flag(struct tc_flower *flower, > flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDP; > } else if (flower->key.ip_proto == IPPROTO_ICMP || > flower->key.ip_proto == IPPROTO_IGMP || > - flower->key.ip_proto == IPPROTO_SCTP) { > + flower->key.ip_proto == IPPROTO_SCTP || > + flower->key.ip_proto == IPPROTO_IPIP || > + flower->key.ip_proto == IPPROTO_GRE) { > flower->needs_full_ip_proto_mask = true; > } else if (flower->key.ip_proto == IPPROTO_ICMPV6) { > flower->needs_full_ip_proto_mask = true; > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index 47b5f1ff7..7f19f4144 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -879,3 +879,38 @@ AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "eth_type(0x0800) > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([offloads - Add IPIP/GRE protocols to offload in ip rewrite - offloads enabled]) Like in the previous patch use a short description. For example: AT_SETUP([offloads - IPIP with 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 See previous patch on checking actual flow. AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "eth_type(0x0800)" | DUMP_CLEAN_SORTED | strip_stats], [0], [dnl > +]) See previous patch, we might want to check the actual TC flow. > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.39.3
diff --git a/lib/tc.c b/lib/tc.c index ae71390bc..f85703633 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -2975,7 +2975,9 @@ csum_update_flag(struct tc_flower *flower, flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDP; } else if (flower->key.ip_proto == IPPROTO_ICMP || flower->key.ip_proto == IPPROTO_IGMP || - flower->key.ip_proto == IPPROTO_SCTP) { + flower->key.ip_proto == IPPROTO_SCTP || + flower->key.ip_proto == IPPROTO_IPIP || + flower->key.ip_proto == IPPROTO_GRE) { flower->needs_full_ip_proto_mask = true; } else if (flower->key.ip_proto == IPPROTO_ICMPV6) { flower->needs_full_ip_proto_mask = true; diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 47b5f1ff7..7f19f4144 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -879,3 +879,38 @@ AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "eth_type(0x0800) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([offloads - Add IPIP/GRE protocols to offload 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 +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP
The warning message is |00001|tc(handler4)|WARN|can't offload rewrite of IP/IPV6 with ip_proto: X. IPIP and GRE only need the checksum recalculation of the IP header if the IP header is rewritten. Eliminate the warning for IPIP and GRE protocols. Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc") Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn> --- lib/tc.c | 4 +++- tests/system-offloads-traffic.at | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-)