| Message ID | 20230329114259.110297-9-cmi@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add offload support for sFlow | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 29 Mar 2023, at 13:42, Chris Mi wrote: > Add three sFlow offload test cases: > > 3: offloads - sflow with sampling=1 - offloads enabled ok > 4: offloads - sflow with sampling=2 - offloads enabled ok > 5: offloads - ping over vxlan tunnel with sflow - offloads enabled ok See some comments inline. This concluded the v26 review... //Eelco > Signed-off-by: Chris Mi <cmi@nvidia.com> > Reviewed-by: Roi Dayan <roid@nvidia.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > tests/system-offloads-traffic.at | 125 +++++++++++++++++++++++++++++++ > 1 file changed, 125 insertions(+) > > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index eb331d6ce..3a596eb19 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -93,6 +93,131 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([offloads - sflow with sampling=1 - 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-ofctl add-flow br0 "actions=normal"]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > + > +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], [ping -q -c 1000 -i 0.01 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl > +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms > +]) > + > +P1_IFINDEX=$(cat /sys/class/net/ovs-p1/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=$P1_IFINDEX/output=1/"]) > +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:userspace" | grep "sFlow" | DUMP_SFLOW], [0], [dnl > +recirc_id(0),in_port(2),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),3 > +]) > + > +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? > +AT_CLEANUP > + > +AT_SETUP([offloads - sflow with sampling=2 - 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-ofctl add-flow br0 "actions=normal"]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > +AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore]) Guess this dump can be removed? > + > +AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=2 polling=100 -- set bridge br0 sflow=@sflow], [0], [ignore]) > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1000 -i 0.01 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl > +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms > +]) > + > +P1_IFINDEX=$(cat /sys/class/net/ovs-p1/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=$P1_IFINDEX/output=1/"]) > +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:sample" | grep "sFlow" | DUMP_SFLOW], [0], [dnl > +recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:999, bytes:83916, used:0.001s, actions:sample(sample=50.0%,actions(userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions))),3 > +]) > + > +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:sample" | 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:sample(sample=50.0%,actions(userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions))),2 > +]) > + > +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 ]]]]) > +AT_CLEANUP > + > +AT_SETUP([offloads - ping over vxlan tunnel with sflow - offloads enabled]) > +OVS_CHECK_TUNNEL_TSO() > +OVS_CHECK_VXLAN() > + > +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true]) > +ADD_BR([br-underlay]) > + > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) > + > +ADD_NAMESPACES(at_ns0) > + > +dnl Set up underlay link from host into the namespace using veth pair. > +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") > +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) > +AT_CHECK([ip link set dev br-underlay up]) > + > +dnl Set up tunnel endpoints on OVS outside the namespace and with a native > +dnl linux device inside the namespace. > +ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24]) > +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], [10.1.1.1/24], > + [id 0 dstport 4789]) > + > +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-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]) > + > +dnl First, check the underlay > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | FORMAT_PING], [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +dnl Okay, now check the overlay with different packet sizes Guess this comment needs an update. > +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? 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. 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. > + > +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
On 4/12/2023 10:10 PM, Eelco Chaudron wrote: > On 29 Mar 2023, at 13:42, Chris Mi wrote: > >> Add three sFlow offload test cases: >> >> 3: offloads - sflow with sampling=1 - offloads enabled ok >> 4: offloads - sflow with sampling=2 - offloads enabled ok >> 5: offloads - ping over vxlan tunnel with sflow - offloads enabled ok > See some comments inline. This concluded the v26 review... > > //Eelco > >> Signed-off-by: Chris Mi<cmi@nvidia.com> >> Reviewed-by: Roi Dayan<roid@nvidia.com> >> Acked-by: Eelco Chaudron<echaudro@redhat.com> >> --- >> tests/system-offloads-traffic.at | 125 +++++++++++++++++++++++++++++++ >> 1 file changed, 125 insertions(+) >> >> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at >> index eb331d6ce..3a596eb19 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -93,6 +93,131 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([offloads - sflow with sampling=1 - 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-ofctl add-flow br0 "actions=normal"]) >> + >> +ADD_NAMESPACES(at_ns0, at_ns1) >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> + >> +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], [ping -q -c 1000 -i 0.01 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl >> +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms >> +]) >> + >> +P1_IFINDEX=$(cat /sys/class/net/ovs-p1/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=$P1_IFINDEX/output=1/"]) >> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:userspace" | grep "sFlow" | DUMP_SFLOW], [0], [dnl >> +recirc_id(0),in_port(2),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),3 >> +]) >> + >> +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 ]]]]) >> +AT_CLEANUP >> + >> +AT_SETUP([offloads - sflow with sampling=2 - 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-ofctl add-flow br0 "actions=normal"]) >> + >> +ADD_NAMESPACES(at_ns0, at_ns1) >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> +AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore]) > Guess this dump can be removed? Done. >> + >> +AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=2 polling=100 -- set bridge br0 sflow=@sflow], [0], [ignore]) >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1000 -i 0.01 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl >> +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms >> +]) >> + >> +P1_IFINDEX=$(cat /sys/class/net/ovs-p1/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=$P1_IFINDEX/output=1/"]) >> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:sample" | grep "sFlow" | DUMP_SFLOW], [0], [dnl >> +recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:999, bytes:83916, used:0.001s, actions:sample(sample=50.0%,actions(userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions))),3 >> +]) >> + >> +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:sample" | 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:sample(sample=50.0%,actions(userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions))),2 >> +]) >> + >> +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 ]]]]) >> +AT_CLEANUP >> + >> +AT_SETUP([offloads - ping over vxlan tunnel with sflow - offloads enabled]) >> +OVS_CHECK_TUNNEL_TSO() >> +OVS_CHECK_VXLAN() >> + >> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true]) >> +ADD_BR([br-underlay]) >> + >> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) >> + >> +ADD_NAMESPACES(at_ns0) >> + >> +dnl Set up underlay link from host into the namespace using veth pair. >> +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") >> +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) >> +AT_CHECK([ip link set dev br-underlay up]) >> + >> +dnl Set up tunnel endpoints on OVS outside the namespace and with a native >> +dnl linux device inside the namespace. >> +ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24]) >> +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], [10.1.1.1/24], >> + [id 0 dstport 4789]) >> + >> +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-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]) >> + >> +dnl First, check the underlay >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | FORMAT_PING], [0], [dnl >> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >> +]) >> + >> +dnl Okay, now check the overlay with different packet sizes > Guess this comment needs an update. Done. >> +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]) > 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. 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. > >> + >> +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
>> 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? Forgot to address this comment. I found this comment: /* XXX: This is wrong! We're ignoring DF and CSUM flags configuration * requested by the user. However, TC for now has no way to pass * these flags in a flower key and their masks are set by default, * meaning tunnel offloading will not work at all if not cleared. * Keeping incorrect behavior for now. */ tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); Maybe it's a known issue.
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? <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 ]]]]) ^^^^^^^^^^^^^^^^^^ <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? >> 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? >>> + >>> +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
On 27 Apr 2023, at 3:27, Chris Mi wrote: >>> 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? > Forgot to address this comment. I found this comment: > > /* XXX: This is wrong! We're ignoring DF and CSUM flags configuration > * requested by the user. However, TC for now has no way to pass > * these flags in a flower key and their masks are set by default, > * meaning tunnel offloading will not work at all if not cleared. > * Keeping incorrect behavior for now. */ > tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); > > Maybe it's a known issue. Yes, this looks like the problem. I guess we should add a note to the tc-offload.rst document’s ‘Known TC flow offload limitations’ section for both the general tunnel offload and sflow section. This concludes my comments to your comments on my comments ;) And sorry for the late response but I was on PTO. Cheers, Eelco
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. > <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. >>> 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 ]]]]) >>>> + >>>> +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
On 5/10/2023 10:57 PM, Eelco Chaudron wrote: > On 27 Apr 2023, at 3:27, Chris Mi wrote: > >>>> 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? >> Forgot to address this comment. I found this comment: >> >> /* XXX: This is wrong! We're ignoring DF and CSUM flags configuration >> * requested by the user. However, TC for now has no way to pass >> * these flags in a flower key and their masks are set by default, >> * meaning tunnel offloading will not work at all if not cleared. >> * Keeping incorrect behavior for now. */ >> tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); >> >> Maybe it's a known issue. > Yes, this looks like the problem. I guess we should add a note to the tc-offload.rst document’s ‘Known TC flow offload limitations’ section for both the general tunnel offload and sflow section. Done. Maybe no need to add it for sflow section. Because tunnel offload limitation is common. Otherwise, maybe we need to add it everywhere. > This concludes my comments to your comments on my comments ;) 😂 Eelco, I have addressed all your comments. Thanks for your review. Regards, Chris
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
On 26 May 2023, at 10:24, Chris Mi wrote: > On 5/10/2023 10:57 PM, Eelco Chaudron wrote: >> On 27 Apr 2023, at 3:27, Chris Mi wrote: >> >>>>> 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? >>> Forgot to address this comment. I found this comment: >>> >>> /* XXX: This is wrong! We're ignoring DF and CSUM flags configuration >>> * requested by the user. However, TC for now has no way to pass >>> * these flags in a flower key and their masks are set by default, >>> * meaning tunnel offloading will not work at all if not cleared. >>> * Keeping incorrect behavior for now. */ >>> tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); >>> >>> Maybe it's a known issue. >> Yes, this looks like the problem. I guess we should add a note to the tc-offload.rst document’s ‘Known TC flow offload limitations’ section for both the general tunnel offload and sflow section. > Done. Maybe no need to add it for sflow section. Because tunnel offload limitation is common. Otherwise, maybe we need to add it everywhere. Yes that should be good enough! >> This concludes my comments to your comments on my comments ;) > 😂 > > > Eelco, > > I have addressed all your comments. Thanks for your review. Thanks, looking forward to v27. > Regards, > Chris
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index eb331d6ce..3a596eb19 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -93,6 +93,131 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([offloads - sflow with sampling=1 - 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-ofctl add-flow br0 "actions=normal"]) + +ADD_NAMESPACES(at_ns0, at_ns1) +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +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], [ping -q -c 1000 -i 0.01 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms +]) + +P1_IFINDEX=$(cat /sys/class/net/ovs-p1/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=$P1_IFINDEX/output=1/"]) +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:userspace" | grep "sFlow" | DUMP_SFLOW], [0], [dnl +recirc_id(0),in_port(2),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),3 +]) + +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]) +AT_CLEANUP + +AT_SETUP([offloads - sflow with sampling=2 - 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-ofctl add-flow br0 "actions=normal"]) + +ADD_NAMESPACES(at_ns0, at_ns1) +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") +AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore]) + +AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=2 polling=100 -- set bridge br0 sflow=@sflow], [0], [ignore]) +NS_CHECK_EXEC([at_ns0], [ping -q -c 1000 -i 0.01 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl +1000 packets transmitted, 1000 received, 0% packet loss, time 0ms +]) + +P1_IFINDEX=$(cat /sys/class/net/ovs-p1/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=$P1_IFINDEX/output=1/"]) +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:sample" | grep "sFlow" | DUMP_SFLOW], [0], [dnl +recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:999, bytes:83916, used:0.001s, actions:sample(sample=50.0%,actions(userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions))),3 +]) + +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:sample" | 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:sample(sample=50.0%,actions(userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions))),2 +]) + +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 ]]]]) +AT_CLEANUP + +AT_SETUP([offloads - ping over vxlan tunnel with sflow - offloads enabled]) +OVS_CHECK_TUNNEL_TSO() +OVS_CHECK_VXLAN() + +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true]) +ADD_BR([br-underlay]) + +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) + +ADD_NAMESPACES(at_ns0) + +dnl Set up underlay link from host into the namespace using veth pair. +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) +AT_CHECK([ip link set dev br-underlay up]) + +dnl Set up tunnel endpoints on OVS outside the namespace and with a native +dnl linux device inside the namespace. +ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24]) +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], [10.1.1.1/24], + [id 0 dstport 4789]) + +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-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]) + +dnl First, check the underlay +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | FORMAT_PING], [0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +dnl Okay, now check the overlay with different packet sizes +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 +]) + +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 ]]]]) + +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"])