About the second change,
A large part of ip protocols do not need the checksum recalculation of themself.
There are only 6 protocols in tc csum action except for the ip header.
If a protocol need the recalculation of the checksum, may be it should add the
special handling in TC,
because each has the different checksum calculation.
From: Ilya Maximets <i.maxim...@ovn.org>
Date: 2023-07-21 22:20:55
To: Aaron Conole <acon...@redhat.com>,ovs-dev <ovs-dev@openvswitch.org>
Cc: simon.hor...@corigine.com,pa...@nvidia.com,Faicker Mo
<faicker...@ucloud.cn>,i.maxim...@ovn.org
Subject: Re: [ovs-dev] [PATCH] netdev-tc-offload: Fix ip protocols not
offloaded in ip rewrite>On 7/20/23 17:32, Aaron Conole wrote:
>> Faicker Mo via dev <ovs-dev@openvswitch.org> 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 <faicker...@ucloud.cn>
>>> ---
>>
>> 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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
