On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote:
>> On 13 Nov 2023, at 14:17, Eelco Chaudron <[email protected]> wrote: >> >> >> >> On 8 Nov 2023, at 14:39, Vladislav Odintsov wrote: >> >>> Hi Ilya, Eelco, >>> >>> I’ve tried this patch against 3.1 and latest master branch. There are no >>> warnings anymore, >>> but it seems that in my installation it has broken offload capability. >> >> Yes, this is expected, this specific flow can not be offloaded with TC and >> therefore will be processed by the kernel datapath. > > But why did it work before the patch? The traffic was offloaded to it was > flowing correctly. It seemed to work, but your rule had an action to set the source port to 59507, however, this is not happening with tc. actions:set(tunnel(tun_id=0xff0011,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=59507,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x18000b}),flags(df|csum|key))),4 If you still want to offload this flow, you should not configure the tp_src for this action, and it will be offloaded. Ilya, is this done by OVN? If so, it might need a change there also. >>> In tcpdump I see packets, which I expect to be offloaded, >>> so not to appear in tcpdump output, also there is an empty output >>> of ovs-appctl dpctl/dump-flows type=offloaded. >>> >>> On ovs without this patch the same host offloads this traffic but with >>> warnings. >>> Let me know which debug information you need from me. >>> >>> As a reminder, the test system runs CentOS 8.4 with >>> 6.5.7-1.el8.elrepo.x86_64 elrepo kernel. >>> SmartNIC is Mellanox Technologies MT2892 Family [ConnectX-6 Dx]. >>> >>>> On 31 Oct 2023, at 11:33, Eelco Chaudron <[email protected]> wrote: >>>> >>>> >>>> >>>> On 30 Oct 2023, at 15:00, Ilya Maximets wrote: >>>> >>>>> There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload >>>>> should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested >>>>> by OVS. Current code just ignores the attribute in the tunnel(set()) >>>>> action leading to a flow mismatch and potential incorrect datapath >>>>> behavior: >>>>> >>>>> |tc(handler21)|DBG|tc flower compare failed action compare >>>>> ... >>>>> Action 0 mismatch: >>>>> - Expected Action: >>>>> 00000010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11 >>>>> 00000020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12 >>>>> 00000050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>>> 00000060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00 >>>>> ... >>>>> - Received Action: >>>>> 00000010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11 >>>>> 00000020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12 >>>>> 00000050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>>> 00000060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00 >>>>> ... >>>>> >>>>> In the tc_action dump above we can see the difference on the second >>>>> line. The action dumped from a kernel is missing 'c0 5b' - source >>>>> port for a tunnel(set()) action on the second line. >>>>> >>>>> Removing the field from the tc_action_encap structure entirely to >>>>> avoid any potential confusion. >>>>> >>>>> Note: In general, the source port number in the tunnel(set()) action >>>>> is not particularly useful for most tunnels, because they may just >>>>> ignore the value. Specs for Geneve and VXLAN suggest using a value >>>>> based on the headers of the inner packet as a source port. >>>>> And I'm not really sure how to make OVS to generate the action with >>>>> a source port in it, so the commit doesn't include the test case. >>>>> In vast majority of scenarios the source port doesn't actually end >>>>> up in the action itself. >>>>> Having a mismatch between the userspace and TC leads to constant >>>>> revalidation of the flow and warnings in the log, and might >>>>> potentially cause mishandling of the traffic, even though the impact >>>>> is unclear at the moment. >>>>> >>>>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using >>>>> tc interface") >>>>> Reported-at: >>>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html >>>>> Reported-by: Vladislav Odintsov <[email protected]> >>>>> Signed-off-by: Ilya Maximets <[email protected]> >>>> >>>> Thanks Ilya, this change seems correct as the kernel does not seem to >>>> support setting the source port. I did some additional tests, and found no >>>> problems. >>>> >>>> Acked-by: Eelco Chaudron <[email protected] <mailto:[email protected]> >>>> <mailto:[email protected]>> >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> [email protected] <mailto:[email protected]> >>>> <mailto:[email protected]> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >>> Regards, >>> Vladislav Odintsov >> >> _______________________________________________ >> dev mailing list >> [email protected] <mailto:[email protected]> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Regards, > Vladislav Odintsov _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
