On 29 Mar 2023, at 13:41, Chris Mi wrote:
> 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. 🙁 Will try to look at the v26 next week. >>>>> 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 ]]]]) Will do some testing, but are you sure this will fill the struct flow_tnl *tunnel attribute? //eelco >>>>>>>>>>>> + >>>>>>>>>>>> +/* 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
