| Message ID | 20231030140031.75157-1-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] netdev-offload-tc: Fix offload of tunnel key tp_src. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <odivlad@gmail.com> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> 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 <echaudro@redhat.com>
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. 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 <echaudro@redhat.com> 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 <odivlad@gmail.com> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > 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 <echaudro@redhat.com <mailto:echaudro@redhat.com>> > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
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 <echaudro@redhat.com> 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 <odivlad@gmail.com> >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> >> 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 <echaudro@redhat.com <mailto:echaudro@redhat.com>> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Regards, > Vladislav Odintsov
> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. > > >> 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 <echaudro@redhat.com> 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 <odivlad@gmail.com> >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> >>> 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 <echaudro@redhat.com <mailto:echaudro@redhat.com> <mailto:echaudro@redhat.com>> >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org <mailto:dev@openvswitch.org> <mailto:dev@openvswitch.org> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> Regards, >> Vladislav Odintsov > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: >> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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 <echaudro@redhat.com> 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 <odivlad@gmail.com> >>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>> >>>> 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 <echaudro@redhat.com <mailto:echaudro@redhat.com> <mailto:echaudro@redhat.com>> >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org <mailto:dev@openvswitch.org> <mailto:dev@openvswitch.org> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >>> Regards, >>> Vladislav Odintsov >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Regards, > Vladislav Odintsov
On 11/13/23 13:13, Eelco Chaudron wrote: > > > On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: > >>> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. I don't think there is a direct way to force the tp_src into the action. OVS is making decision based on the set of flows it has, but I'm not sure why exactly. Vladislav, could you try running ofproto/trace for the packet of interest on your setup? This may shed some light on why exactly OVS wants this field to be part of the action. But, as Eelco said, as long as this field is part of the action, we can't allow it to be offloaded, just because TC doesn't support it. Best regards, Ilya Maximets.
(re-send as for some reason gmail rejected the previous attempt and it wasn't delivered to Vladislav) On 11/13/23 20:25, Ilya Maximets wrote: > On 11/13/23 13:13, Eelco Chaudron wrote: >> >> >> On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: >> >>>> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. > > I don't think there is a direct way to force the tp_src into the action. > OVS is making decision based on the set of flows it has, but I'm not > sure why exactly. > > Vladislav, could you try running ofproto/trace for the packet of interest > on your setup? This may shed some light on why exactly OVS wants this > field to be part of the action. > > But, as Eelco said, as long as this field is part of the action, we > can't allow it to be offloaded, just because TC doesn't support it. > > Best regards, Ilya Maximets.
On Mon, Oct 30, 2023 at 03:00:29PM +0100, 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: Late comment but, yes.. thanks for fixing this. I also have never seen a flow that ended up matching on the tunnel src port, but still, it was an issue. Marcelo
Hi Ilya, > On 13 Nov 2023, at 22:25, Ilya Maximets <i.maximets@ovn.org> wrote: > > On 11/13/23 13:13, Eelco Chaudron wrote: >> >> >> On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: >> >>>> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. > > I don't think there is a direct way to force the tp_src into the action. > OVS is making decision based on the set of flows it has, but I'm not > sure why exactly. > > Vladislav, could you try running ofproto/trace for the packet of interest > on your setup? This may shed some light on why exactly OVS wants this > field to be part of the action. There were no DP flows with tp_src in action in ovs-appctl dpctl/dump-flows output, so I grab such flow (with tp_src set in action) from ovs-vswitchd logs with enabled dpif_netlink dbg loglevel: 2023-11-15T12:52:36.279Z|00056|dpif_netlink(handler3)|DBG|added flow 2023-11-15T12:52:36.279Z|00057|dpif_netlink(handler3)|DBG|system@ovs-system: put[create] ufid:23548c16-67c6-47af-a710-2d80cab0a361 recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00), actions:set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 and tracing: # ovs-appctl ofproto/trace 'recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00)' Flow: arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 bridge("br-int") ---------------- 0. in_port=17, priority 100 move:NXM_NX_TUN_ID[0..23]->OXM_OF_METADATA[0..23] -> OXM_OF_METADATA[0..23] is now 0x6 move:NXM_NX_TUN_METADATA0[16..30]->NXM_NX_REG14[0..14] -> NXM_NX_REG14[0..14] is now 0x4 move:NXM_NX_TUN_METADATA0[0..15]->NXM_NX_REG15[0..15] -> NXM_NX_REG15[0..15] is now 0x3 resubmit(,38) 38. reg15=0x3,metadata=0x6, priority 100, cookie 0xdb490628 set_field:0x2->reg15 set_field:0x2->reg11 set_field:0x6->reg12 resubmit(,39) 39. priority 0 set_field:0->reg0 set_field:0->reg1 set_field:0->reg2 set_field:0->reg3 set_field:0->reg4 set_field:0->reg5 set_field:0->reg6 set_field:0->reg7 set_field:0->reg8 set_field:0->reg9 resubmit(,40) 40. metadata=0x6, priority 0, cookie 0xc3547f56 set_field:0/0x10->xreg4 resubmit(,41) 41. metadata=0x6, priority 0, cookie 0x64834c48 resubmit(,42) 42. metadata=0x6, priority 0, cookie 0xc1a1d8a2 resubmit(,43) 43. metadata=0x6, priority 0, cookie 0x20edcb50 resubmit(,44) 44. metadata=0x6, priority 0, cookie 0xb22206a8 resubmit(,45) 45. metadata=0x6, priority 0, cookie 0xe07a9d12 resubmit(,46) 46. reg15=0x2,metadata=0x6, priority 100, cookie 0x1261fd96 resubmit(,64) 64. priority 0 resubmit(,65) 65. reg15=0x2,metadata=0x6, priority 100, cookie 0x903b6c1d clone(ct_clear,set_field:0->reg11,set_field:0->reg12,set_field:0->reg13,set_field:0xc->reg11,set_field:0x9->reg12,set_field:0xff0002->metadata,set_field:0x55->reg14,set_field:0->reg10,set_field:0->reg15,set_field:0->reg0,set_field:0->reg1,set_field:0->reg2,set_field:0->reg3,set_field:0->reg4,set_field:0->reg5,set_field:0->reg6,set_field:0->reg7,set_field:0->reg8,set_field:0->reg9,resubmit(,8)) ct_clear set_field:0->reg11 set_field:0->reg12 set_field:0->reg13 set_field:0xc->reg11 set_field:0x9->reg12 set_field:0xff0002->metadata set_field:0x55->reg14 set_field:0->reg10 set_field:0->reg15 set_field:0->reg0 set_field:0->reg1 set_field:0->reg2 set_field:0->reg3 set_field:0->reg4 set_field:0->reg5 set_field:0->reg6 set_field:0->reg7 set_field:0->reg8 set_field:0->reg9 resubmit(,8) 8. metadata=0xff0002, priority 50, cookie 0x577856d6 set_field:0/0x1000->reg10 resubmit(,73) 73. No match. drop move:NXM_NX_REG10[12]->NXM_NX_XXREG0[111] -> NXM_NX_XXREG0[111] is now 0 resubmit(,9) 9. metadata=0xff0002, priority 0, cookie 0x5ccc67cb resubmit(,10) 10. metadata=0xff0002, priority 0, cookie 0xfa1655a9 resubmit(,11) 11. metadata=0xff0002, priority 0, cookie 0x573cb297 resubmit(,12) 12. metadata=0xff0002, priority 0, cookie 0x9484d696 resubmit(,13) 13. metadata=0xff0002,dl_dst=01:00:00:00:00:00/01:00:00:00:00:00, priority 110, cookie 0x318d5cd4 resubmit(,14) 14. metadata=0xff0002, priority 0, cookie 0x2e0a4e13 resubmit(,15) 15. metadata=0xff0002, priority 65535, cookie 0x9f411194 resubmit(,16) 16. metadata=0xff0002, priority 65535, cookie 0x83a5ca2c resubmit(,17) 17. metadata=0xff0002, priority 0, cookie 0xe87f9407 resubmit(,18) 18. metadata=0xff0002, priority 0, cookie 0x75403eb9 resubmit(,19) 19. metadata=0xff0002, priority 0, cookie 0xc18625c0 resubmit(,20) 20. metadata=0xff0002, priority 0, cookie 0xb81b324 resubmit(,21) 21. metadata=0xff0002, priority 0, cookie 0x1d3cdf0d resubmit(,22) 22. metadata=0xff0002, priority 0, cookie 0xe7e06c1e resubmit(,23) 23. metadata=0xff0002, priority 0, cookie 0x46d2ac7d resubmit(,24) 24. metadata=0xff0002, priority 0, cookie 0x3b63f9de resubmit(,25) 25. metadata=0xff0002, priority 0, cookie 0x3a0b4c7c resubmit(,26) 26. metadata=0xff0002, priority 0, cookie 0x4868c582 resubmit(,27) 27. metadata=0xff0002, priority 0, cookie 0xe247626 resubmit(,28) 28. metadata=0xff0002, priority 0, cookie 0xd0f0fe80 resubmit(,29) 29. metadata=0xff0002, priority 0, cookie 0xa1decea3 resubmit(,30) 30. metadata=0xff0002, priority 0, cookie 0x72a20311 resubmit(,31) 31. arp,metadata=0xff0002,dl_src=00:00:c9:99:bd:0e,arp_op=1, priority 75, cookie 0x1baa2454 set_field:0x8004->reg15 resubmit(,37) 37. reg15=0x8004,metadata=0xff0002, priority 100, cookie 0xfef4a15e set_field:0x8e->reg15 set_field:0xff0002/0xffffff->tun_id set_field:0x8e->tun_metadata0 move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30] -> NXM_NX_TUN_METADATA0[16..30] is now 0x55 output:9 -> output to kernel tunnel set_field:0x8004->reg15 Final flow: arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 Megaflow: recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1 Datapath actions: set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 The traffic is an OVN interconnection usecase, where there is a transit switch and two LRs are connected to it on different Availability Zones. LR on AZ1 tries to resolve ARP for LR in another AZ. I’d like to add here, that I see if there is a tp_src field in match portion, there will be tp_src in action part. And vice versa. > > But, as Eelco said, as long as this field is part of the action, we > can't allow it to be offloaded, just because TC doesn't support it. Do I understand correctly, that this patch introduced a kind of regression in my case because earlier kernel ignored tp_src, which was set by OVS and the tc rule was installed and traffic was HW-offloaded and worked fine. And now OVS sees it tries to offload a flow with tp_src set and it decides such flow is not allowed to be offloaded? > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
On 11/15/23 14:13, Vladislav Odintsov wrote: > Hi Ilya, > >> On 13 Nov 2023, at 22:25, Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 11/13/23 13:13, Eelco Chaudron wrote: >>> >>> >>> On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: >>> >>>>> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. >> >> I don't think there is a direct way to force the tp_src into the action. >> OVS is making decision based on the set of flows it has, but I'm not >> sure why exactly. >> >> Vladislav, could you try running ofproto/trace for the packet of interest >> on your setup? This may shed some light on why exactly OVS wants this >> field to be part of the action. > > There were no DP flows with tp_src in action in ovs-appctl dpctl/dump-flows > output, so I grab such flow (with tp_src set in action) from ovs-vswitchd logs > with enabled dpif_netlink dbg loglevel: > > 2023-11-15T12:52:36.279Z|00056|dpif_netlink(handler3)|DBG|added flow > 2023-11-15T12:52:36.279Z|00057|dpif_netlink(handler3)|DBG|system@ovs-system: put[create] ufid:23548c16-67c6-47af-a710-2d80cab0a361 recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00), actions:set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 > > and tracing: > > # ovs-appctl ofproto/trace 'recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00)' > Flow: arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 > > bridge("br-int") > ---------------- > 0. in_port=17, priority 100 > move:NXM_NX_TUN_ID[0..23]->OXM_OF_METADATA[0..23] > -> OXM_OF_METADATA[0..23] is now 0x6 > move:NXM_NX_TUN_METADATA0[16..30]->NXM_NX_REG14[0..14] > -> NXM_NX_REG14[0..14] is now 0x4 > move:NXM_NX_TUN_METADATA0[0..15]->NXM_NX_REG15[0..15] > -> NXM_NX_REG15[0..15] is now 0x3 > resubmit(,38) > 38. reg15=0x3,metadata=0x6, priority 100, cookie 0xdb490628 > set_field:0x2->reg15 > set_field:0x2->reg11 > set_field:0x6->reg12 > resubmit(,39) > 39. priority 0 > set_field:0->reg0 > set_field:0->reg1 > set_field:0->reg2 > set_field:0->reg3 > set_field:0->reg4 > set_field:0->reg5 > set_field:0->reg6 > set_field:0->reg7 > set_field:0->reg8 > set_field:0->reg9 > resubmit(,40) > 40. metadata=0x6, priority 0, cookie 0xc3547f56 > set_field:0/0x10->xreg4 > resubmit(,41) > 41. metadata=0x6, priority 0, cookie 0x64834c48 > resubmit(,42) > 42. metadata=0x6, priority 0, cookie 0xc1a1d8a2 > resubmit(,43) > 43. metadata=0x6, priority 0, cookie 0x20edcb50 > resubmit(,44) > 44. metadata=0x6, priority 0, cookie 0xb22206a8 > resubmit(,45) > 45. metadata=0x6, priority 0, cookie 0xe07a9d12 > resubmit(,46) > 46. reg15=0x2,metadata=0x6, priority 100, cookie 0x1261fd96 > resubmit(,64) > 64. priority 0 > resubmit(,65) > 65. reg15=0x2,metadata=0x6, priority 100, cookie 0x903b6c1d > clone(ct_clear,set_field:0->reg11,set_field:0->reg12,set_field:0->reg13,set_field:0xc->reg11,set_field:0x9->reg12,set_field:0xff0002->metadata,set_field:0x55->reg14,set_field:0->reg10,set_field:0->reg15,set_field:0->reg0,set_field:0->reg1,set_field:0->reg2,set_field:0->reg3,set_field:0->reg4,set_field:0->reg5,set_field:0->reg6,set_field:0->reg7,set_field:0->reg8,set_field:0->reg9,resubmit(,8)) > ct_clear > set_field:0->reg11 > set_field:0->reg12 > set_field:0->reg13 > set_field:0xc->reg11 > set_field:0x9->reg12 > set_field:0xff0002->metadata > set_field:0x55->reg14 > set_field:0->reg10 > set_field:0->reg15 > set_field:0->reg0 > set_field:0->reg1 > set_field:0->reg2 > set_field:0->reg3 > set_field:0->reg4 > set_field:0->reg5 > set_field:0->reg6 > set_field:0->reg7 > set_field:0->reg8 > set_field:0->reg9 > resubmit(,8) > 8. metadata=0xff0002, priority 50, cookie 0x577856d6 > set_field:0/0x1000->reg10 > resubmit(,73) > 73. No match. > drop > move:NXM_NX_REG10[12]->NXM_NX_XXREG0[111] > -> NXM_NX_XXREG0[111] is now 0 > resubmit(,9) > 9. metadata=0xff0002, priority 0, cookie 0x5ccc67cb > resubmit(,10) > 10. metadata=0xff0002, priority 0, cookie 0xfa1655a9 > resubmit(,11) > 11. metadata=0xff0002, priority 0, cookie 0x573cb297 > resubmit(,12) > 12. metadata=0xff0002, priority 0, cookie 0x9484d696 > resubmit(,13) > 13. metadata=0xff0002,dl_dst=01:00:00:00:00:00/01:00:00:00:00:00, priority 110, cookie 0x318d5cd4 > resubmit(,14) > 14. metadata=0xff0002, priority 0, cookie 0x2e0a4e13 > resubmit(,15) > 15. metadata=0xff0002, priority 65535, cookie 0x9f411194 > resubmit(,16) > 16. metadata=0xff0002, priority 65535, cookie 0x83a5ca2c > resubmit(,17) > 17. metadata=0xff0002, priority 0, cookie 0xe87f9407 > resubmit(,18) > 18. metadata=0xff0002, priority 0, cookie 0x75403eb9 > resubmit(,19) > 19. metadata=0xff0002, priority 0, cookie 0xc18625c0 > resubmit(,20) > 20. metadata=0xff0002, priority 0, cookie 0xb81b324 > resubmit(,21) > 21. metadata=0xff0002, priority 0, cookie 0x1d3cdf0d > resubmit(,22) > 22. metadata=0xff0002, priority 0, cookie 0xe7e06c1e > resubmit(,23) > 23. metadata=0xff0002, priority 0, cookie 0x46d2ac7d > resubmit(,24) > 24. metadata=0xff0002, priority 0, cookie 0x3b63f9de > resubmit(,25) > 25. metadata=0xff0002, priority 0, cookie 0x3a0b4c7c > resubmit(,26) > 26. metadata=0xff0002, priority 0, cookie 0x4868c582 > resubmit(,27) > 27. metadata=0xff0002, priority 0, cookie 0xe247626 > resubmit(,28) > 28. metadata=0xff0002, priority 0, cookie 0xd0f0fe80 > resubmit(,29) > 29. metadata=0xff0002, priority 0, cookie 0xa1decea3 > resubmit(,30) > 30. metadata=0xff0002, priority 0, cookie 0x72a20311 > resubmit(,31) > 31. arp,metadata=0xff0002,dl_src=00:00:c9:99:bd:0e,arp_op=1, priority 75, cookie 0x1baa2454 > set_field:0x8004->reg15 > resubmit(,37) > 37. reg15=0x8004,metadata=0xff0002, priority 100, cookie 0xfef4a15e > set_field:0x8e->reg15 > set_field:0xff0002/0xffffff->tun_id > set_field:0x8e->tun_metadata0 > move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30] > -> NXM_NX_TUN_METADATA0[16..30] is now 0x55 > output:9 > -> output to kernel tunnel > set_field:0x8004->reg15 > > Final flow: arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 > Megaflow: recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1 > Datapath actions: set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 > > The traffic is an OVN interconnection usecase, where there is a transit switch > and two LRs are connected to it on different Availability Zones. LR on AZ1 > tries to resolve ARP for LR in another AZ. > > I’d like to add here, that I see if there is a tp_src field in match portion, > there will be tp_src in action part. And vice versa. OK, I see what is going on here. The traffic is received from one tunnel and is being sent to a different one. It's technically not necessary to set the tp_src in this particular case, so we may be abe to improve this and allow offloading. But some very careful checks needed, since the filled value may come not only from the previous tunnel metadata, but also can be set through other actions, in which case we can't simply drop it. Needs some investigation. BTW, is this only for ARP traffic, or some heavy datapath traffic like TCP/UDP is also flowing through this path and you actually care for it to be offloaded? > >> >> But, as Eelco said, as long as this field is part of the action, we >> can't allow it to be offloaded, just because TC doesn't support it. > > Do I understand correctly, that this patch introduced a kind of regression in > my case because earlier kernel ignored tp_src, which was set by OVS and the tc > rule was installed and traffic was HW-offloaded and worked fine. > And now OVS sees it tries to offload a flow with tp_src set and it decides such > flow is not allowed to be offloaded? Yes. Currently the vaue is just getting ignored, because there is no way to deliver it to TC and offloading kind of "works". With the fix applied, OVS doesn't allow that to happen. > >> >> Best regards, Ilya Maximets. >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Regards, > Vladislav Odintsov >
Hi Vladislav, On Wed, Nov 15, 2023 at 04:13:13PM +0300, Vladislav Odintsov wrote: ... > Final flow: arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 > Megaflow: recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1 > Datapath actions: set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 > > The traffic is an OVN interconnection usecase, where there is a transit switch > and two LRs are connected to it on different Availability Zones. LR on AZ1 > tries to resolve ARP for LR in another AZ. In the flow here I see in_port=17 and output to port 5. Can you please confirm which ports are those? The 2 ports of the CX6, maybe? Asking because I'm a bit surprised that hairpin traffic with such encap operations actually works. Thanks, Marcelo
Hi Marcelo, PSB. > On 16 Nov 2023, at 15:02, Marcelo Ricardo Leitner <mleitner@redhat.com> wrote: > > Hi Vladislav, > > On Wed, Nov 15, 2023 at 04:13:13PM +0300, Vladislav Odintsov wrote: > ... >> Final flow: arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 >> Megaflow: recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1 >> Datapath actions: set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 >> >> The traffic is an OVN interconnection usecase, where there is a transit switch >> and two LRs are connected to it on different Availability Zones. LR on AZ1 >> tries to resolve ARP for LR in another AZ. > > In the flow here I see in_port=17 and output to port 5. Can you please > confirm which ports are those? The 2 ports of the CX6, maybe? > Asking because I'm a bit surprised that hairpin traffic with such > encap operations actually works. Port 5 is a geneve tunnel OVS port. The next flow is working fine on my setup (first packet reaches system and others don’t; traffic is flowing offloaded): 2023-11-20T10:29:25.220Z|00034|dpif_netlink(handler1)|DBG|system@ovs-system: put[create] ufid:417d13c0-d5d9-4832-a155-8e83c99c1fe4 recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.107,dst=10.1.0.109,ttl=64/0,tp_src=19915/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x50003}),flags(-df+csum+key)),in_port(4),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=00:01:c9:99:bd:0e),eth_type(0x0800),ipv4(src=172.31.32.6/0.0.0.0,dst=172.31.0.6/0.0.0.0,proto=1,tos=0/0x3,ttl=63/0,frag=no),icmp(type=8/0,code=0/0), actions:set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=19915,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),4 (port 4 is genev_sys_6081 here). > > Thanks, > Marcelo > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
Hi Ilya, > On 15 Nov 2023, at 21:51, Ilya Maximets <i.maximets@ovn.org> wrote: > > On 11/15/23 14:13, Vladislav Odintsov wrote: >> Hi Ilya, >> >>> On 13 Nov 2023, at 22:25, Ilya Maximets <i.maximets@ovn.org> wrote: >>> >>> On 11/13/23 13:13, Eelco Chaudron wrote: >>>> >>>> >>>> On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: >>>> >>>>>> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. >>> >>> I don't think there is a direct way to force the tp_src into the action. >>> OVS is making decision based on the set of flows it has, but I'm not >>> sure why exactly. >>> >>> Vladislav, could you try running ofproto/trace for the packet of interest >>> on your setup? This may shed some light on why exactly OVS wants this >>> field to be part of the action. >> >> There were no DP flows with tp_src in action in ovs-appctl dpctl/dump-flows >> output, so I grab such flow (with tp_src set in action) from ovs-vswitchd logs >> with enabled dpif_netlink dbg loglevel: >> >> 2023-11-15T12:52:36.279Z|00056|dpif_netlink(handler3)|DBG|added flow >> 2023-11-15T12:52:36.279Z|00057|dpif_netlink(handler3)|DBG|system@ovs-system: put[create] ufid:23548c16-67c6-47af-a710-2d80cab0a361 recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00), actions:set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 >> >> and tracing: >> >> # ovs-appctl ofproto/trace 'recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00)' >> Flow: arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 >> >> bridge("br-int") >> ---------------- >> 0. in_port=17, priority 100 >> move:NXM_NX_TUN_ID[0..23]->OXM_OF_METADATA[0..23] >> -> OXM_OF_METADATA[0..23] is now 0x6 >> move:NXM_NX_TUN_METADATA0[16..30]->NXM_NX_REG14[0..14] >> -> NXM_NX_REG14[0..14] is now 0x4 >> move:NXM_NX_TUN_METADATA0[0..15]->NXM_NX_REG15[0..15] >> -> NXM_NX_REG15[0..15] is now 0x3 >> resubmit(,38) >> 38. reg15=0x3,metadata=0x6, priority 100, cookie 0xdb490628 >> set_field:0x2->reg15 >> set_field:0x2->reg11 >> set_field:0x6->reg12 >> resubmit(,39) >> 39. priority 0 >> set_field:0->reg0 >> set_field:0->reg1 >> set_field:0->reg2 >> set_field:0->reg3 >> set_field:0->reg4 >> set_field:0->reg5 >> set_field:0->reg6 >> set_field:0->reg7 >> set_field:0->reg8 >> set_field:0->reg9 >> resubmit(,40) >> 40. metadata=0x6, priority 0, cookie 0xc3547f56 >> set_field:0/0x10->xreg4 >> resubmit(,41) >> 41. metadata=0x6, priority 0, cookie 0x64834c48 >> resubmit(,42) >> 42. metadata=0x6, priority 0, cookie 0xc1a1d8a2 >> resubmit(,43) >> 43. metadata=0x6, priority 0, cookie 0x20edcb50 >> resubmit(,44) >> 44. metadata=0x6, priority 0, cookie 0xb22206a8 >> resubmit(,45) >> 45. metadata=0x6, priority 0, cookie 0xe07a9d12 >> resubmit(,46) >> 46. reg15=0x2,metadata=0x6, priority 100, cookie 0x1261fd96 >> resubmit(,64) >> 64. priority 0 >> resubmit(,65) >> 65. reg15=0x2,metadata=0x6, priority 100, cookie 0x903b6c1d >> clone(ct_clear,set_field:0->reg11,set_field:0->reg12,set_field:0->reg13,set_field:0xc->reg11,set_field:0x9->reg12,set_field:0xff0002->metadata,set_field:0x55->reg14,set_field:0->reg10,set_field:0->reg15,set_field:0->reg0,set_field:0->reg1,set_field:0->reg2,set_field:0->reg3,set_field:0->reg4,set_field:0->reg5,set_field:0->reg6,set_field:0->reg7,set_field:0->reg8,set_field:0->reg9,resubmit(,8)) >> ct_clear >> set_field:0->reg11 >> set_field:0->reg12 >> set_field:0->reg13 >> set_field:0xc->reg11 >> set_field:0x9->reg12 >> set_field:0xff0002->metadata >> set_field:0x55->reg14 >> set_field:0->reg10 >> set_field:0->reg15 >> set_field:0->reg0 >> set_field:0->reg1 >> set_field:0->reg2 >> set_field:0->reg3 >> set_field:0->reg4 >> set_field:0->reg5 >> set_field:0->reg6 >> set_field:0->reg7 >> set_field:0->reg8 >> set_field:0->reg9 >> resubmit(,8) >> 8. metadata=0xff0002, priority 50, cookie 0x577856d6 >> set_field:0/0x1000->reg10 >> resubmit(,73) >> 73. No match. >> drop >> move:NXM_NX_REG10[12]->NXM_NX_XXREG0[111] >> -> NXM_NX_XXREG0[111] is now 0 >> resubmit(,9) >> 9. metadata=0xff0002, priority 0, cookie 0x5ccc67cb >> resubmit(,10) >> 10. metadata=0xff0002, priority 0, cookie 0xfa1655a9 >> resubmit(,11) >> 11. metadata=0xff0002, priority 0, cookie 0x573cb297 >> resubmit(,12) >> 12. metadata=0xff0002, priority 0, cookie 0x9484d696 >> resubmit(,13) >> 13. metadata=0xff0002,dl_dst=01:00:00:00:00:00/01:00:00:00:00:00, priority 110, cookie 0x318d5cd4 >> resubmit(,14) >> 14. metadata=0xff0002, priority 0, cookie 0x2e0a4e13 >> resubmit(,15) >> 15. metadata=0xff0002, priority 65535, cookie 0x9f411194 >> resubmit(,16) >> 16. metadata=0xff0002, priority 65535, cookie 0x83a5ca2c >> resubmit(,17) >> 17. metadata=0xff0002, priority 0, cookie 0xe87f9407 >> resubmit(,18) >> 18. metadata=0xff0002, priority 0, cookie 0x75403eb9 >> resubmit(,19) >> 19. metadata=0xff0002, priority 0, cookie 0xc18625c0 >> resubmit(,20) >> 20. metadata=0xff0002, priority 0, cookie 0xb81b324 >> resubmit(,21) >> 21. metadata=0xff0002, priority 0, cookie 0x1d3cdf0d >> resubmit(,22) >> 22. metadata=0xff0002, priority 0, cookie 0xe7e06c1e >> resubmit(,23) >> 23. metadata=0xff0002, priority 0, cookie 0x46d2ac7d >> resubmit(,24) >> 24. metadata=0xff0002, priority 0, cookie 0x3b63f9de >> resubmit(,25) >> 25. metadata=0xff0002, priority 0, cookie 0x3a0b4c7c >> resubmit(,26) >> 26. metadata=0xff0002, priority 0, cookie 0x4868c582 >> resubmit(,27) >> 27. metadata=0xff0002, priority 0, cookie 0xe247626 >> resubmit(,28) >> 28. metadata=0xff0002, priority 0, cookie 0xd0f0fe80 >> resubmit(,29) >> 29. metadata=0xff0002, priority 0, cookie 0xa1decea3 >> resubmit(,30) >> 30. metadata=0xff0002, priority 0, cookie 0x72a20311 >> resubmit(,31) >> 31. arp,metadata=0xff0002,dl_src=00:00:c9:99:bd:0e,arp_op=1, priority 75, cookie 0x1baa2454 >> set_field:0x8004->reg15 >> resubmit(,37) >> 37. reg15=0x8004,metadata=0xff0002, priority 100, cookie 0xfef4a15e >> set_field:0x8e->reg15 >> set_field:0xff0002/0xffffff->tun_id >> set_field:0x8e->tun_metadata0 >> move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30] >> -> NXM_NX_TUN_METADATA0[16..30] is now 0x55 >> output:9 >> -> output to kernel tunnel >> set_field:0x8004->reg15 >> >> Final flow: arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 >> Megaflow: recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1 >> Datapath actions: set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 >> >> The traffic is an OVN interconnection usecase, where there is a transit switch >> and two LRs are connected to it on different Availability Zones. LR on AZ1 >> tries to resolve ARP for LR in another AZ. >> >> I’d like to add here, that I see if there is a tp_src field in match portion, >> there will be tp_src in action part. And vice versa. > > OK, I see what is going on here. The traffic is received from one tunnel > and is being sent to a different one. It's technically not necessary to > set the tp_src in this particular case, so we may be abe to improve this > and allow offloading. But some very careful checks needed, since the > filled value may come not only from the previous tunnel metadata, but also > can be set through other actions, in which case we can't simply drop it. > Needs some investigation. > > BTW, is this only for ARP traffic, or some heavy datapath traffic like > TCP/UDP is also flowing through this path and you actually care for it > to be offloaded? ICMP/tcp also not offloaded with patch applied, but offload is working on current master branch. > >> >>> >>> But, as Eelco said, as long as this field is part of the action, we >>> can't allow it to be offloaded, just because TC doesn't support it. >> >> Do I understand correctly, that this patch introduced a kind of regression in >> my case because earlier kernel ignored tp_src, which was set by OVS and the tc >> rule was installed and traffic was HW-offloaded and worked fine. >> And now OVS sees it tries to offload a flow with tp_src set and it decides such >> flow is not allowed to be offloaded? > > Yes. Currently the vaue is just getting ignored, because there is no > way to deliver it to TC and offloading kind of "works". With the fix > applied, OVS doesn't allow that to happen. > >> >>> >>> Best regards, Ilya Maximets. >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> Regards, >> Vladislav Odintsov >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
On 11/20/23 11:42, Vladislav Odintsov wrote: > Hi Ilya, > >> On 15 Nov 2023, at 21:51, Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 11/15/23 14:13, Vladislav Odintsov wrote: >>> Hi Ilya, >>> >>>> On 13 Nov 2023, at 22:25, Ilya Maximets <i.maximets@ovn.org> wrote: >>>> >>>> On 11/13/23 13:13, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 13 Nov 2023, at 12:43, Vladislav Odintsov wrote: >>>>> >>>>>>> On 13 Nov 2023, at 14:17, Eelco Chaudron <echaudro@redhat.com> 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. >>>> >>>> I don't think there is a direct way to force the tp_src into the action. >>>> OVS is making decision based on the set of flows it has, but I'm not >>>> sure why exactly. >>>> >>>> Vladislav, could you try running ofproto/trace for the packet of interest >>>> on your setup? This may shed some light on why exactly OVS wants this >>>> field to be part of the action. >>> >>> There were no DP flows with tp_src in action in ovs-appctl dpctl/dump-flows >>> output, so I grab such flow (with tp_src set in action) from ovs-vswitchd logs >>> with enabled dpif_netlink dbg loglevel: >>> >>> 2023-11-15T12:52:36.279Z|00056|dpif_netlink(handler3)|DBG|added flow >>> 2023-11-15T12:52:36.279Z|00057|dpif_netlink(handler3)|DBG|system@ovs-system: put[create] ufid:23548c16-67c6-47af-a710-2d80cab0a361 recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00), actions:set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 >>> >>> and tracing: >>> >>> # ovs-appctl ofproto/trace 'recirc_id(0),dp_hash(0/0),skb_priority(0/0),tunnel(tun_id=0x6,src=10.1.0.103,dst=10.1.0.109,ttl=64/0,tp_src=3275/0,tp_dst=6081/0,geneve({class=0x102,type=0x80,len=4,0x40003}),flags(-df+csum+key)),in_port(5),skb_mark(0/0),ct_state(0/0x2f),ct_zone(0/0),ct_mark(0/0),ct_label(0/0x3),eth(src=00:00:c9:99:bd:0e,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=169.254.100.1/0.0.0.0,tip=169.254.100.3,op=1,sha=00:00:c9:99:bd:0e/00:00:00:00:00:00,tha=00:00:00:00:00:00/00:00:00:00:00:00)' >>> Flow: arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 >>> >>> bridge("br-int") >>> ---------------- >>> 0. in_port=17, priority 100 >>> move:NXM_NX_TUN_ID[0..23]->OXM_OF_METADATA[0..23] >>> -> OXM_OF_METADATA[0..23] is now 0x6 >>> move:NXM_NX_TUN_METADATA0[16..30]->NXM_NX_REG14[0..14] >>> -> NXM_NX_REG14[0..14] is now 0x4 >>> move:NXM_NX_TUN_METADATA0[0..15]->NXM_NX_REG15[0..15] >>> -> NXM_NX_REG15[0..15] is now 0x3 >>> resubmit(,38) >>> 38. reg15=0x3,metadata=0x6, priority 100, cookie 0xdb490628 >>> set_field:0x2->reg15 >>> set_field:0x2->reg11 >>> set_field:0x6->reg12 >>> resubmit(,39) >>> 39. priority 0 >>> set_field:0->reg0 >>> set_field:0->reg1 >>> set_field:0->reg2 >>> set_field:0->reg3 >>> set_field:0->reg4 >>> set_field:0->reg5 >>> set_field:0->reg6 >>> set_field:0->reg7 >>> set_field:0->reg8 >>> set_field:0->reg9 >>> resubmit(,40) >>> 40. metadata=0x6, priority 0, cookie 0xc3547f56 >>> set_field:0/0x10->xreg4 >>> resubmit(,41) >>> 41. metadata=0x6, priority 0, cookie 0x64834c48 >>> resubmit(,42) >>> 42. metadata=0x6, priority 0, cookie 0xc1a1d8a2 >>> resubmit(,43) >>> 43. metadata=0x6, priority 0, cookie 0x20edcb50 >>> resubmit(,44) >>> 44. metadata=0x6, priority 0, cookie 0xb22206a8 >>> resubmit(,45) >>> 45. metadata=0x6, priority 0, cookie 0xe07a9d12 >>> resubmit(,46) >>> 46. reg15=0x2,metadata=0x6, priority 100, cookie 0x1261fd96 >>> resubmit(,64) >>> 64. priority 0 >>> resubmit(,65) >>> 65. reg15=0x2,metadata=0x6, priority 100, cookie 0x903b6c1d >>> clone(ct_clear,set_field:0->reg11,set_field:0->reg12,set_field:0->reg13,set_field:0xc->reg11,set_field:0x9->reg12,set_field:0xff0002->metadata,set_field:0x55->reg14,set_field:0->reg10,set_field:0->reg15,set_field:0->reg0,set_field:0->reg1,set_field:0->reg2,set_field:0->reg3,set_field:0->reg4,set_field:0->reg5,set_field:0->reg6,set_field:0->reg7,set_field:0->reg8,set_field:0->reg9,resubmit(,8)) >>> ct_clear >>> set_field:0->reg11 >>> set_field:0->reg12 >>> set_field:0->reg13 >>> set_field:0xc->reg11 >>> set_field:0x9->reg12 >>> set_field:0xff0002->metadata >>> set_field:0x55->reg14 >>> set_field:0->reg10 >>> set_field:0->reg15 >>> set_field:0->reg0 >>> set_field:0->reg1 >>> set_field:0->reg2 >>> set_field:0->reg3 >>> set_field:0->reg4 >>> set_field:0->reg5 >>> set_field:0->reg6 >>> set_field:0->reg7 >>> set_field:0->reg8 >>> set_field:0->reg9 >>> resubmit(,8) >>> 8. metadata=0xff0002, priority 50, cookie 0x577856d6 >>> set_field:0/0x1000->reg10 >>> resubmit(,73) >>> 73. No match. >>> drop >>> move:NXM_NX_REG10[12]->NXM_NX_XXREG0[111] >>> -> NXM_NX_XXREG0[111] is now 0 >>> resubmit(,9) >>> 9. metadata=0xff0002, priority 0, cookie 0x5ccc67cb >>> resubmit(,10) >>> 10. metadata=0xff0002, priority 0, cookie 0xfa1655a9 >>> resubmit(,11) >>> 11. metadata=0xff0002, priority 0, cookie 0x573cb297 >>> resubmit(,12) >>> 12. metadata=0xff0002, priority 0, cookie 0x9484d696 >>> resubmit(,13) >>> 13. metadata=0xff0002,dl_dst=01:00:00:00:00:00/01:00:00:00:00:00, priority 110, cookie 0x318d5cd4 >>> resubmit(,14) >>> 14. metadata=0xff0002, priority 0, cookie 0x2e0a4e13 >>> resubmit(,15) >>> 15. metadata=0xff0002, priority 65535, cookie 0x9f411194 >>> resubmit(,16) >>> 16. metadata=0xff0002, priority 65535, cookie 0x83a5ca2c >>> resubmit(,17) >>> 17. metadata=0xff0002, priority 0, cookie 0xe87f9407 >>> resubmit(,18) >>> 18. metadata=0xff0002, priority 0, cookie 0x75403eb9 >>> resubmit(,19) >>> 19. metadata=0xff0002, priority 0, cookie 0xc18625c0 >>> resubmit(,20) >>> 20. metadata=0xff0002, priority 0, cookie 0xb81b324 >>> resubmit(,21) >>> 21. metadata=0xff0002, priority 0, cookie 0x1d3cdf0d >>> resubmit(,22) >>> 22. metadata=0xff0002, priority 0, cookie 0xe7e06c1e >>> resubmit(,23) >>> 23. metadata=0xff0002, priority 0, cookie 0x46d2ac7d >>> resubmit(,24) >>> 24. metadata=0xff0002, priority 0, cookie 0x3b63f9de >>> resubmit(,25) >>> 25. metadata=0xff0002, priority 0, cookie 0x3a0b4c7c >>> resubmit(,26) >>> 26. metadata=0xff0002, priority 0, cookie 0x4868c582 >>> resubmit(,27) >>> 27. metadata=0xff0002, priority 0, cookie 0xe247626 >>> resubmit(,28) >>> 28. metadata=0xff0002, priority 0, cookie 0xd0f0fe80 >>> resubmit(,29) >>> 29. metadata=0xff0002, priority 0, cookie 0xa1decea3 >>> resubmit(,30) >>> 30. metadata=0xff0002, priority 0, cookie 0x72a20311 >>> resubmit(,31) >>> 31. arp,metadata=0xff0002,dl_src=00:00:c9:99:bd:0e,arp_op=1, priority 75, cookie 0x1baa2454 >>> set_field:0x8004->reg15 >>> resubmit(,37) >>> 37. reg15=0x8004,metadata=0xff0002, priority 100, cookie 0xfef4a15e >>> set_field:0x8e->reg15 >>> set_field:0xff0002/0xffffff->tun_id >>> set_field:0x8e->tun_metadata0 >>> move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30] >>> -> NXM_NX_TUN_METADATA0[16..30] is now 0x55 >>> output:9 >>> -> output to kernel tunnel >>> set_field:0x8004->reg15 >>> >>> Final flow: arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x0000,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00 >>> Megaflow: recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1 >>> Datapath actions: set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5 >>> >>> The traffic is an OVN interconnection usecase, where there is a transit switch >>> and two LRs are connected to it on different Availability Zones. LR on AZ1 >>> tries to resolve ARP for LR in another AZ. >>> >>> I’d like to add here, that I see if there is a tp_src field in match portion, >>> there will be tp_src in action part. And vice versa. >> >> OK, I see what is going on here. The traffic is received from one tunnel >> and is being sent to a different one. It's technically not necessary to >> set the tp_src in this particular case, so we may be abe to improve this >> and allow offloading. But some very careful checks needed, since the >> filled value may come not only from the previous tunnel metadata, but also >> can be set through other actions, in which case we can't simply drop it. >> Needs some investigation. >> >> BTW, is this only for ARP traffic, or some heavy datapath traffic like >> TCP/UDP is also flowing through this path and you actually care for it >> to be offloaded? > > ICMP/tcp also not offloaded with patch applied, but offload is working on current master branch. Ack. I posted a v2 adding a test case for the issue: https://patchwork.ozlabs.org/project/openvswitch/patch/20231201230836.3093792-1-i.maximets@ovn.org/ I also posted a potential fix that would enable offloading: https://patchwork.ozlabs.org/project/openvswitch/patch/20231201210523.3085560-1-i.maximets@ovn.org/ > >> >>> >>>> >>>> But, as Eelco said, as long as this field is part of the action, we >>>> can't allow it to be offloaded, just because TC doesn't support it. >>> >>> Do I understand correctly, that this patch introduced a kind of regression in >>> my case because earlier kernel ignored tp_src, which was set by OVS and the tc >>> rule was installed and traffic was HW-offloaded and worked fine. >>> And now OVS sees it tries to offload a flow with tp_src set and it decides such >>> flow is not allowed to be offloaded? >> >> Yes. Currently the vaue is just getting ignored, because there is no >> way to deliver it to TC and offloading kind of "works". With the fix >> applied, OVS doesn't allow that to happen. >> >>> >>>> >>>> Best regards, Ilya Maximets. >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>> >>> >>> Regards, >>> Vladislav Odintsov >>> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > Regards, > Vladislav Odintsov >
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index b846a63c2..164c7eef6 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action, } break; case OVS_TUNNEL_KEY_ATTR_TP_SRC: { - action->encap.tp_src = nl_attr_get_be16(tun_attr); + /* There is no corresponding attribute in TC. */ + VLOG_DBG_RL(&rl, "unsupported tunnel key attribute TP_SRC"); + return EOPNOTSUPP; } break; case OVS_TUNNEL_KEY_ATTR_TP_DST: { diff --git a/lib/tc.h b/lib/tc.h index 06707ffa4..fdbcf4b7c 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -213,7 +213,8 @@ enum nat_type { struct tc_action_encap { bool id_present; ovs_be64 id; - ovs_be16 tp_src; + /* ovs_be16 tp_src; Could have been here, but there is no + * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */ ovs_be16 tp_dst; uint8_t tos; uint8_t ttl;
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 <odivlad@gmail.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/netdev-offload-tc.c | 4 +++- lib/tc.h | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-)