Hi Ilya,
> On 15 Nov 2023, at 21:51, Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 11/15/23 14:13, Vladislav Odintsov wrote:
>> Hi Ilya,
>>
>>> On 13 Nov 2023, at 22:25, Ilya Maximets <i.maxim...@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 <echau...@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
>>> d...@openvswitch.org <mailto:d...@openvswitch.org>
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>> Regards,
>> Vladislav Odintsov
>>
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org <mailto:d...@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Regards,
Vladislav Odintsov
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
