On 26 May 2023, at 10:21, Chris Mi wrote:
> On 5/10/2023 10:52 PM, Eelco Chaudron wrote: >> On 26 Apr 2023, at 4:47, Chris Mi wrote: >> >> <SNIP> >> >>>>> + >>>>> +P0_IFINDEX=$(cat /sys/class/net/ovs-p0/ifindex) >>>>> +m4_define([DUMP_SFLOW], [sed -e >>>>> "s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P0_IFINDEX/output=1/"]) >>>>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep >>>>> "in_port(3)" | grep "eth_type(0x0800)" | grep "actions:userspace" | grep >>>>> "sFlow" | DUMP_SFLOW], [0], [dnl >>>>> +recirc_id(0),in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), >>>>> packets:999, bytes:83916, used:0.001s, >>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),2 >>>>> +]) >>>>> + >>>>> +OVS_TRAFFIC_VSWITCHD_STOP >>>>> +OVS_APP_EXIT_AND_WAIT([test-sflow]) >>>> There is no check to verify the content of the sflow log? >>> How about the following check? >>> >>> +hdr="in_ifindex=$p0_ifindex in_format=0 out_ifindex=$p1_ifindex >>> out_format=0 hdr_prot=1 pkt_len=102 stripped=4 hdr_len=98" >>> +count=`grep "$hdr" sflow.log | wc -l` >>> +AT_CHECK([[[[ $count -ge 996 ]]]]) >>> +hdr="in_ifindex=$p1_ifindex in_format=0 out_ifindex=$p0_ifindex >>> out_format=0 hdr_prot=1 pkt_len=102 stripped=4 hdr_len=98" >>> +count=`grep "$hdr" sflow.log | wc -l` >>> +AT_CHECK([[[[ $count -ge 996 ]]]]) >> Did not verify, but this looks right. Do we need a -le check, as you have >> below? > It's a little different. In this case sample rate is 1, we can't receive more > than 1000 packets. > So checking less and equal than 1000 seems unnecessary. I’m ok with leaving it as is, but I was thinking about checking against future bugs. >> <SNIP> >> >>>>> + >>>>> +OVS_TRAFFIC_VSWITCHD_STOP >>>>> +OVS_APP_EXIT_AND_WAIT([test-sflow]) >>>>> +count=`cat sflow.log | wc -l` >>>>> +AT_CHECK([[[[ $count -le 1100 && $count -ge 900 ]]]]) >> ^^^^^^^^^^^^^^^^^^ > In this case, sample rate is 2, we may receive more than 1000 packets. >> <SNIP> >> >>>>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1000 -i 0.01 10.1.1.100 | >>>>> FORMAT_PING], [0], [dnl >>>>> +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms >>>>> +]) >>>>> + >>>> Can we also add dpctl/dumpflows checking here (3x), as the tunnels stuff >>>> is complex? >>> How about the following check? >>> >>> +dnl Check encap flow >>> +match="recirc_id(0),in_port(br0),eth(src=$br0_mac,dst=$vxlan1_mac),eth_type(0x0800),ipv4(tos=0/0x3,frag=no)" >>> +action="actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=$vxlan_sys_ifindex),actions),set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(0))),vxlan_sys_4789" >>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep $match | grep $action >>> > /dev/null]) >>> + >>> +dnl Check decap flow >>> +match="recirc_id(0),tunnel(tun_id=0x0,src=172.31.1.1,dst=172.31.1.100,tp_dst=4789,flags(+key)),in_port(vxlan_sys_4789),eth(src=$vxlan1_mac,dst=$br0_mac),eth_type(0x0800),ipv4(frag=no)" >>> +action="actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=$br0_ifindex),actions),br0" >>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep $match | grep >>> $action> /dev/null]) >> Did not verify, but it looks good. Can you make sure it also passes with the >> kernel datapath, i.e. it has the same output? > Not the same, there are some differences, I'll write that in the > tc-offload.rst as known issues: > > +Tunnel offload > +++++++++++++++ > + > +Current tunnel offload ignores DF and CSUM flags configuration requested by > +the user. TC for now has no way to pass these flags in a flower key and their > +masks are set by default. To make tunnel offload work, DF and CSUM flags > +are cleared. So please be aware of the following differences. > + > +Dumping vxlan decap match without offload, it shows:: > + > + > recirc_id(0),tunnel(tun_id=0x4,src=192.168.1.1,dst=192.168.1.2,flags(-df+csum+key)),in_port(vxlan_sys_4789) > + > +Dumping vxlan decap match with offload, it shows:: > + > + > recirc_id(0),tunnel(tun_id=0x4,src=192.168.1.1,dst=192.168.1.2,tp_dst=4789,flags(+key)),in_port(vxlan_sys_4789) > + > +Dumping vxlan encap action without offload, it shows:: > + > + > actions:set(tunnel(tun_id=0x4,dst=192.168.1.1,ttl=64,tp_dst=4789,flags(df|key))),vxlan_sys_4789 > + > +Dumping vxlan encap action with offload, it shows:: > + > + > actions:set(tunnel(tun_id=0x4,dst=192.168.1.64,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789 > + > > Except this, other info are the same. Thanks for adding this. >>>> recirc_id(0),in_port(1),eth(src=06:9c:de:63:c9:40,dst=01:00:5e:00:00:fb),eth_type(0x0800),ipv4(tos=0/0x3,frag=no), >>>> packets:13, bytes:1883, used:5.700s, >>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=20360),actions),set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(0))),4 >>>> recirc_id(0),in_port(1),eth(src=06:9c:de:63:c9:40,dst=1e:51:4f:cd:c3:0e),eth_type(0x0800),ipv4(tos=0/0x3,frag=no), >>>> packets:999, bytes:97902, used:0.010s, >>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=20360),actions),set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(0))),4 >>>> recirc_id(0),tunnel(tun_id=0x0,src=172.31.1.1,dst=172.31.1.100,tp_dst=4789,flags(+key)),in_port(4),eth(src=1e:51:4f:cd:c3:0e,dst=06:9c:de:63:c9:40),eth_type(0x0800),ipv4(frag=no), >>>> packets:999, bytes:83916, used:0.010s, >>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=20356),actions),1 >>>> >>>> I did notice a little difference between TC and Kernel, below is the >>>> kernel and it has flags as (df) where tc does not have this. Is this a bug >>>> in tc? >>>> >>>> recirc_id(0),in_port(1),eth(src=52:3c:6f:15:be:40,dst=01:00:5e:00:00:fb),eth_type(0x0800),ipv4(tos=0/0x3,frag=no), >>>> packets:13, bytes:1883, used:5.324s, >>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=20369),actions),set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(df))),4 >>>> recirc_id(0),in_port(1),eth(src=52:3c:6f:15:be:40,dst=0a:54:8d:56:5f:e0),eth_type(0x0800),ipv4(tos=0/0x3,frag=no), >>>> packets:999, bytes:97902, used:0.017s, >>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=20369),actions),set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(df))),4 >>>> recirc_id(0),tunnel(tun_id=0x0,src=172.31.1.1,dst=172.31.1.100,flags(-df+csum+key)),in_port(4),eth(src=0a:54:8d:56:5f:e0,dst=52:3c:6f:15:be:40),eth_type(0x0800),ipv4(frag=no), >>>> packets:999, bytes:97902, used:0.017s, >>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=20365),actions),1 >>>> >>>>> +dnl Vni is 0, test-sflow doesn't show it >>>>> +out_tunnel_hdr="tunnel4_out_protocol=17 tunnel4_out_src=0.0.0.0 >>>>> tunnel4_out_dst=172.31.1.1 tunnel4_out_src_port=0 >>>>> tunnel4_out_dst_port=46354" >>>>> +out_count=`grep "$out_tunnel_hdr" sflow.log | wc -l` >>>>> +AT_CHECK([[[[ $out_count -ge 999 ]]]]) >>>>> + >>>>> +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 ]]]]) >>>> Note there seems to be a bug in tc, as it's missing the in ifindex. >>> I think that's because tunnel vtep is in a namespace, so ovs/tc can't get >>> in_ifindex from namespace. >>> >>> I changed the code to only get sample group id and sampled packets from tc. >>> All other information, >>> like input ifindex, output tunnel and so on, will be got from group id >>> mapping. >>> After this change, we'll not have such error. >> Ok, thanks for looking into this and fixing it. >> >>> Eelco, >>> >>> Now I finished all your comments in v26. Will send a new version after you >>> ack all of them. >>> Thanks again for the review. >>> >>> -Chris >>>> TC: >>>> >>>> >>>> HEADER dgramSeqNo=345 ds=127.0.0.1>2:1000 fsSeqNo=2025 >>>> tunnel4_out_length=0 tunnel4_out_protocol=17 \ >>>> tunnel4_out_src=0.0.0.0 tunnel4_out_dst=172.31.1.1 >>>> tunnel4_out_src_port=0 tunnel4_out_dst_port=46354 \ >>>> tunnel4_out_tcp_flags=0 tunnel4_out_tos=0 in_vlan=0 in_priority=0 >>>> out_vlan=0 out_priority=0 meanSkip=1 \ >>>> samplePool=2025 dropEvents=0 in_ifindex=0 in_format=0 >>>> out_ifindex=20351 out_format=0 hdr_prot=1 pkt_len=102... >>>> ^^^^^^^^^^^^ >>>> Kernel: >>>> >>>> HEADER dgramSeqNo=348 ds=127.0.0.1>2:1000 fsSeqNo=2027 >>>> tunnel4_out_length=0 tunnel4_out_protocol=17 \ >>>> tunnel4_out_src=0.0.0.0 tunnel4_out_dst=172.31.1.1 >>>> tunnel4_out_src_port=0 tunnel4_out_dst_port=46354 \ >>>> tunnel4_out_tcp_flags=0 tunnel4_out_tos=0 in_vlan=0 in_priority=0 >>>> out_vlan=0 out_priority=0 meanSkip=1 \ >>>> samplePool=2027 dropEvents=0 in_ifindex=20365 in_format=0 >>>> out_ifindex=20369 out_format=0 hdr_prot=1 pkt_len=102... >>>> ^^^^^^^^^^^^^^^^ >>>> >>>> Guess this needs to be fixed, and we should also add a check to make sure >>>> no in_ifindex=0 or out_ifindex=0 exists. >> Did you add this check to the tests? > Yes. > > dnl Vni is 0, test-sflow doesn't show it > out_tunnel_hdr="tunnel4_out_protocol=17 tunnel4_out_src=0.0.0.0 > tunnel4_out_dst=172.31.1.1 tunnel4_out_src_port=0 tunnel4_out_dst_port=46354" > ifindex="in_ifindex=$br0_ifindex in_format=0 out_ifindex=$vxlan_sys_ifindex" > out_count=`grep "$out_tunnel_hdr" sflow.log | grep "$ifindex" | wc -l` > AT_CHECK([[[[ $out_count -ge 999 ]]]]) > > in_tunnel_hdr="tunnel4_in_protocol=17 tunnel4_in_src=172.31.1.1 > tunnel4_in_dst=172.31.1.100" > ifindex="in_ifindex=$vxlan_sys_ifindex in_format=0 out_ifindex=$br0_ifindex" > in_count=`grep "$in_tunnel_hdr" sflow.log | grep "$ifindex" | wc -l` > AT_CHECK([[[[ $in_count -ge 999 ]]]]) Cool, thanks!! >>>>> + >>>>> +OVS_TRAFFIC_VSWITCHD_STOP >>>>> +OVS_APP_EXIT_AND_WAIT([test-sflow]) >>>>> +AT_CLEANUP >>>>> + >>>>> AT_SETUP([offloads - set ingress_policing_rate and >>>>> ingress_policing_burst - offloads disabled]) >>>>> AT_KEYWORDS([ingress_policing]) >>>>> AT_SKIP_IF([test $HAVE_TC = "no"]) >>>>> -- >>>>> 2.26.3 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
