On 3/29/2023 6:03 PM, Eelco Chaudron wrote:
On 23 Mar 2023, at 13:07, Eelco Chaudron wrote:On 23 Mar 2023, at 12:24, Chris Mi wrote:<SNIP>I ran it 100 times. All passed.Visually inspecting this, it looks fine, however, is this failing without passing the actions parts?Yes. Only the first sampled packet has tunnel header without passing actions. And without offload. This test also passes.ACK, ok will do some investigation next week, and let you know!I was a bit surprised that you already sent out a v25, I was hoping to first wrap up the discussion. Anyhow, I’ll ignore the v25 and wait for v26 based on my comments below. So after some testing, I realized what was going on here, and we do need those actions in the offload_sflow structure (getting them from the kernel each time we need this might add additional delay). However, to give more clarity I would rename it from actions to userspace_actions. So it's clear what they are. In parse_sample_action() and parse_userspace_action() I would rename “const struct nlattr *actions, size_t actions_len” to next_actions, next_actions_len so it’s more clear what they are (or remaining_actions).
Sorry for the hurry. 🙁 Done.
Also, I think for the final version I would make sure a remote VXLAN port exists, so you can send actual traffic and verify you do not receive duplicate packets.That's not duplicate packets. arping sends 5 packets. So we receive 5 sampled packets. It seems it doesn't resend. Do we need a remote vxlan port? Is there any existing test to do that?I want to make sure we receive exactly five packets on the other side of the tunnel. I guess there is this one: AT_SETUP([datapath - ping over vxlan tunnel])I think we should also add a test that utilizes the “struct flow_tnl *tunnel;” data in struct offload_sflow.
Will add the following lines in this test:in_tunnel_hdr="tunnel4_in_protocol=17 tunnel4_in_src=172.31.1.1 tunnel4_in_dst=172.31.1.100"
in_count=`grep "$in_tunnel_hdr" sflow.log | wc -l` AT_CHECK([[[[ $in_count -ge 999 ]]]])
+ +/* Allocate a unique group id for the given set of flow metadata and + * optional actions. + */So assuming the ufid is part of the offload_sflow data and we only support a single sample per flow we will probably not re-use the metadata. So I guess this only makes sense if we support multiple offloads per flow in the future am I right?Actually, ufid was introduced just for easy hashing and comparing. I didn't noticed that with this change, metadata can't be reused. Without ufid, arp and ip metedata can be reused, for example: recirc_id(0),in_port(enp8s0f0_1),eth(src=02:25:d0:65:01:02,dst=24:25:d0:e1:00:00),eth_type(0x0800),ipv4(tos=0,frag=no), packets:16, bytes:2330, used:0.110s, actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=2429),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789 recirc_id(0),in_port(enp8s0f0_1),eth(src=02:25:d0:65:01:02,dst=24:25:d0:e1:00:00),eth_type(0x0806), packets:0, bytes:0, used:never, actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=2429),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789 Maybe we should remove ufid to re-use meterdata. Or maybe keep it. So there is a 1:1 mapping for sample group id and ufid. What do you think, Eelco?I’m ok with both designs if you use the 1:1 mapping you can remove the ref counting, if you remove it you need to fix the above refcount comment.OK, I'll use 1:1 mapping.ACK <SNIP>
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
