On 23 Mar 2023, at 12:24, Chris Mi wrote:
> On 3/23/2023 5:47 PM, Eelco Chaudron wrote: >> >> On 23 Mar 2023, at 10:28, Chris Mi wrote: >> >>> On 3/22/2023 6:45 PM, Eelco Chaudron wrote: >>>> On 22 Mar 2023, at 7:15, Chris Mi wrote: >>>> >>>>> On 3/20/2023 6:04 PM, Eelco Chaudron wrote: >>>>>> On 20 Mar 2023, at 6:44, Chris Mi wrote: >>>>>> >>>>>>> On 3/16/2023 5:09 PM, Eelco Chaudron wrote: >>>>>>>> On 1 Mar 2023, at 8:22, Chris Mi wrote: >>>>>>>> >>>>>>>>> When offloading sample action to TC, userspace creates a unique ID >>>>>>>>> to map sFlow action and tunnel info and passes this ID to kernel >>>>>>>>> instead of the sFlow info. Psample will send this ID and sampled >>>>>>>>> packet to userspace. Using the ID, userspace can recover the sFlow >>>>>>>>> info and send sampled packet to the right sFlow monitoring host. >>>>>>>> See some comments inline below. >>>>>>>> >>>>>>>> //Eelco >>>>>>>> >>>>>>>>> Signed-off-by: Chris Mi<[email protected]> >>>>>>>>> Reviewed-by: Roi Dayan<[email protected]> >>>>>>>>> --- >>>>>>>>> lib/netdev-offload-tc.c | 233 >>>>>>>>> +++++++++++++++++++++++++++++++++++++++- >>>>>>>>> lib/tc.h | 1 + >>>>>>>>> 2 files changed, 232 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>>>>>>> index 4fb9d9f21..4214337d8 100644 >>>>>>>>> --- a/lib/netdev-offload-tc.c >>>>>>>>> +++ b/lib/netdev-offload-tc.c >>>>>>>>> @@ -41,6 +41,7 @@ >>>>>>>>> #include "unaligned.h" >>>>>>>>> #include "util.h" >>>>>>>>> #include "dpif-provider.h" >>>>>>>>> +#include "cmap.h" >>>>>>>>> >>>>>>>>> VLOG_DEFINE_THIS_MODULE(netdev_offload_tc); >>>>>>>>> >>>>>>>>> @@ -103,6 +104,226 @@ static void parse_tc_flower_to_stats(struct >>>>>>>>> tc_flower *flower, >>>>>>>>> static int get_ufid_adjust_stats(const ovs_u128 *ufid, >>>>>>>>> struct dpif_flow_stats *stats); >>>>>>>>> >>>>>>>>> +/* When offloading sample action to TC, userspace creates a unique ID >>>>>>>>> + * to map sFlow action and tunnel info and passes this ID to kernel >>>>>>>>> + * instead of the sFlow info. Psample will send this ID and sampled >>>>>>>>> + * packet to userspace. Using the ID, userspace can recover the sFlow >>>>>>>>> + * info and send sampled packet to the right sFlow monitoring host. >>>>>>>>> + */ >>>>>>>>> +struct offload_sflow { >>>>>>>> Should we maybe already give this a more general name, like >>>>>>>> offload_sample? >>>>>>>> This as you need the same structure later when you add support of >>>>>>>> offload sent to a controller? >>>>>>> OK. >>>>>>>> We might need an offload_type field. >>>>>>> OK. >>>>>>>>> + struct nlattr *action; /* SFlow action. Used in flow_get. */ >>>>>>>>> + struct nlattr *userdata; /* Struct user_action_cookie. */ >>>>>>>>> + struct nlattr *actions; /* All actions to get output tunnel. */ >>>>>>>> You should not need actions at all, see comments later. These are the >>>>>>>> next actions, which are not of interest to sflow offload. >>>>>>> As the comment describes, we need it to get the output tunnel. >>>>>>> Otherwise, we'll lose the following info using sflowtool after >>>>>>> offloading: >>>>>>> >>>>>>> extendedType out_VNI >>>>>>> out_VNI 4 >>>>>>> flowBlock_tag 0:1023 >>>>>>> flowSampleType tunnel_ipv4_out_IPV4 >>>>>>> tunnel_ipv4_out_sampledPacketSize 0 >>>>>>> tunnel_ipv4_out_IPSize 0 >>>>>>> tunnel_ipv4_out_srcIP 0.0.0.0 >>>>>>> tunnel_ipv4_out_dstIP 192.168.1.66 >>>>>>> tunnel_ipv4_out_IPProtocol 17 >>>>>>> tunnel_ipv4_out_IPTOS 0 >>>>>>> tunnel_ipv4_out_UDPSrcPort 0 >>>>>>> tunnel_ipv4_out_UDPDstPort 46354 >>>>>>> >>>>>>> I don't think we can get the output tunnel info without mapping >>>>>>> 'actions'. >>>>>> I’m confused as from the code it looks like actions are the outer >>>>>> actions, and your actions should come from the inner actions, i.e., they >>>>>> should be part of action. >>>>> In my understanding, 'actions' is all actions, for example: >>>>> "actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=935),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(df|key))),vxlan_sys_4789" >>>>> Not sure what do you mean by outer actions and inner actions. >>>> actions: >>>> [userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=935),actions)], >>>> [set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(df|key))),vxlan_sys_4789] >>>> >>>> +---------------------------------------------------------------- >>>> ------------------------------------------------------------------------------------------ >>>> | >>>> \ These are the remaining (outer) actions and the sflow implementation >>>> should not care about this at all. >>>> \ These are the sflow actions and this is all what sflow should >>>> care about! >>>> >>>> >>>> So sflow should only use the sFlow actions, and the remaining (outer) >>>> actions should not be touched by sFlow these should be implemented/using >>>> the existing TC offloads. >>>> If you supply these actions to the upcall they are also executed (again) >>>> in userspace, which they should not. >>>> >>>>>> Maybe I’m not understanding this correctly, and as this is a more >>>>>> complex case, we should have a test case fore it. >>>>>> >>>>>> If you can share the test case before the next revision of the patchset, >>>>>> I can revisit it to safe another version… >>>>> Our QA reported this bug using their test. It's written in python. I'm >>>>> afraid it doesn't help even if I can share it. >>>>> They said that sflowtool shows different results between >>>>> hw-offload="true" and non-offload. >>>>> >>>>> I read the code and found this: >>>>> dpif_sflow_received() >>>>> { >>>>> ... >>>>> /* Output tunnel. */ >>>>> if (sflow_actions >>>>> && sflow_actions->encap_depth == 1 >>>>> && !sflow_actions->tunnel_err >>>>> && dpif_sflow_cookie_num_outputs(cookie) == 1) { >>>>> tnlOutProto = sflow_actions->tunnel_ipproto; >>>>> >>>>> if (tnlOutProto == 0) { >>>>> .. >>>>> >>>>> After mapping the 'actions', the output tunnel appeared. I'm not sure if >>>>> we can create such complex test using m4. >>>>> And it needshttps://github.com/sflow/sflowtool. Usually I run sflowtool >>>>> manually to verify it. >>>>> Hopefully this test will not block the following reviews. >>>> I think we need to resolve this before you sent out the next revision, so >>>> we don’t end up with more revisions. >>>> >>>> There are already some tunnel cases for normal SFLOW which you could use >>>> as a base for your tests. >>>> >>>> ./ofproto-dpif.at:7523:AT_SETUP([ofproto-dpif - sFlow packet sampling - >>>> tunnel set]) >>>> ./ofproto-dpif.at:7592:AT_SETUP([ofproto-dpif - sFlow packet sampling - >>>> tunnel push]) >>>> ./ofproto-dpif.at:7694:AT_SETUP([ofproto-dpif - sFlow packet sampling - >>>> MPLS]) >>>> >>>> Please create a test case so I understand what’s going on, as for now, I >>>> do not see it from just reading code. >>> Hi Eelco, >>> >>> How about this test case? >>> >>> AT_SETUP([offloads - sflow with tunnel set - offloads enabled]) >>> OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . >>> other_config:hw-offload=true]) >>> >>> on_exit 'kill `cat test-sflow.pid`' >>> AT_CHECK([ovstest test-sflow --log-file --detach --no-chdir --pidfile >>> 0:127.0.0.1 > sflow.log], [0], [], [ignore]) >>> AT_CAPTURE_FILE([sflow.log]) >>> PARSE_LISTENING_PORT([test-sflow.log], [SFLOW_PORT]) >>> >>> AT_CHECK([ovs-appctl -t ovsdb-server exit]) >>> AT_CHECK([ovs-appctl -t ovs-vswitchd exit]) >>> AT_CHECK([rm -f ovsdb-server.pid]) >>> AT_CHECK([rm -f ovs-vswitchd.pid]) >>> AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file >>> --remote=punix:$OVS_RUNDIR/db.sock], [0], [], [stderr]) >>> AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn >>> -vofproto_dpif -vunixctl], [0], [], [stderr]) >> I forgot to ask in previous reviews, but why do you need to restart the >> process? Can you not enable the logging with the ovs-appctl commands? >> >>> on_exit "kill `cat ovsdb-server.pid`" >>> on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`" >>> >>> AT_CHECK([ovs-vsctl add-port br0 vxlan0 -- set Interface vxlan0 type=vxlan \ >>> options:remote_ip=192.168.1.1 options:key=4 >>> options:dst_port=4789 ofport_request=3]) >>> AT_CHECK([ovs-ofctl add-flow br0 "actions=3"]) >>> ADD_NAMESPACES(at_ns0, at_ns1) >>> ADD_VETH(p0, at_ns0, br0, "1.1.1.1/16") >>> mac=$(ip netns exec at_ns0 cat /sys/class/net/p0/address | sed 's/:/-/g') >>> hdr="FF-FF-FF-FF-FF-FF-$mac-08-06" >>> tunnel_hdr="tunnel4_out_length=0 tunnel4_out_protocol=17 >>> tunnel4_out_src=0.0.0.0 tunnel4_out_dst=192.168.1.1 tunnel4_out_src_port=0 >>> tunnel4_out_dst_port=46354 tunnel4_out_tcp_flags=0 tunnel4_out_tos=0 >>> tunnel_out_vni=4" >>> AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo >>> target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=1 polling=100 -- set >>> bridge br0 sflow=@sflow], [0], [ignore]) >>> NS_CHECK_EXEC([at_ns0], [arping -c 5 1.1.1.2], [1], [ignore]) >>> >>> OVS_TRAFFIC_VSWITCHD_STOP >>> OVS_APP_EXIT_AND_WAIT([test-sflow]) >>> count=`cat sflow.log | grep -i $hdr | grep "$tunnel_hdr" | wc -l` >>> AT_CHECK([[[[ $count -eq 5 ]]]]) >>> AT_CLEANUP >>> >>> 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! >> >> 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]) >>>>>>>>> + >>>>>>>>> +/* 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
