On 11/15/23 14:13, Vladislav Odintsov wrote: > Hi Ilya, > >> On 13 Nov 2023, at 22:25, Ilya Maximets <[email protected]> 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 <[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. >> >> 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 >> [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
