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). >>> 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. >>>>>>>>>> + >>>>>>>>>> +/* 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
