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. > 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]>> >> >> _______________________________________________ >> 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
