Hi Chris, I hope you enjoyed the holiday. And don't worry about the delay, as I try to only spend time reviewing stuff on Fridays.
Thanks for fixing the comments, and I will re-review once it's integrated into the sflow patchset. //Eelco On 8 Oct 2021, at 8:23, Chris Mi wrote: > Hi Eelco, > > Sorry for the late reply due to the long National Day holiday in China. > > On 10/1/2021 10:34 PM, Eelco Chaudron wrote: >> Thanks for working on the test cases. I have two small requests below to >> make the tests also verify the datapath actions. >> >> And as you already discussed with Simon this should be part of the generic >> TC sFlow patchset. >> >> //Eelco >> >> On 16 Sep 2021, at 10:38, Chris Mi wrote: >> >>> Add two sFlow offload test caes: >>> >>> 3: sflow offloads with sampling=1 - ping between two ports - offloads >>> enabled ok >>> 4: sflow offloads with sampling=2 - ping between two ports - offloads >>> enabled ok >>> >>> Signed-off-by: Chris Mi <[email protected]> >>> --- >>> tests/system-offloads-traffic.at | 84 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 84 insertions(+) >>> >>> diff --git a/tests/system-offloads-traffic.at >>> b/tests/system-offloads-traffic.at >>> index be63057bb..f263c9183 100644 >>> --- a/tests/system-offloads-traffic.at >>> +++ b/tests/system-offloads-traffic.at >>> @@ -5,6 +5,7 @@ AT_BANNER([datapath offloads]) >>> # Normilizes output ports, recirc_id, packets and macs. >>> # >>> m4_define([DUMP_CLEAN_SORTED], [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/actions:[[0-9,]]*/actions:output/;s/recirc_id(0),//' >>> | sort]) >>> +m4_define([DUMP_CLEAN_SORTED_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/actions:.*$/actions:output/;s/recirc_id(0),//' >>> | sort]) >>> >>> AT_SETUP([offloads - ping between two ports - offloads disabled]) >>> OVS_TRAFFIC_VSWITCHD_START() >>> @@ -71,6 +72,89 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded >>> flows : [[1-9]]"], [0], [i >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> +AT_SETUP([sflow offloads with sampling=1 - ping between two ports - >>> offloads enabled]) >>> +OVS_TRAFFIC_VSWITCHD_START() >>> + >>> +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 set Open_vSwitch . other_config:hw-offload=true]) >>> + >>> +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]) >>> +on_exit "kill `cat ovsdb-server.pid`" >>> +on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`" >>> + >>> +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]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0], >>> [dnl >>> +10 packets transmitted, 10 received, 0% packet loss, time 0ms >>> +]) >>> +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 -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0], >>> [dnl >>> +10 packets transmitted, 10 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep >>> "actions:userspace" |grep "sFlow" | grep "eth_type(0x0800)" | >>> DUMP_CLEAN_SORTED_SFLOW], [0], [dnl >>> +in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:10, >>> bytes:840, used:0.001s, actions:output >>> +in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:10, >>> bytes:840, used:0.001s, actions:output >>> +]) >> I think your filter to much in this case, as we would like to verify the >> correct datapath format for actions, so I think it should be something like: >> >> actions:userspace(pid=XXX,sFlow(vid=0,pcp=0,output=341),actions),3 >> >> Where you can ignore/replace the actual pid value, but you need to find a >> way to map the correct if_index. > Done. >> >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +OVS_APP_EXIT_AND_WAIT([test-sflow]) >>> +count=`cat sflow.log | wc -l` >>> +AT_CHECK([[[[ $count -le 22 && $count -ge 18 ]]]]) >>> +AT_CLEANUP >>> + >>> +AT_SETUP([sflow offloads with sampling=2 - ping between two ports - >>> offloads enabled]) >>> +OVS_TRAFFIC_VSWITCHD_START() >>> + >>> +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 set Open_vSwitch . other_config:hw-offload=true]) >>> + >>> +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]) >>> +on_exit "kill `cat ovsdb-server.pid`" >>> +on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`" >>> + >>> +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 -c 1000 -i 0.01 -w 12 10.1.1.2], [0], >>> [ignore]) >>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep >>> "actions:sample" | grep "sFlow" | grep "eth_type(0x0800)"], [0], [ignore]) >> Why is this ignore? It should match flow/action,see also above: >> >> >> sample(sample=50.0%,actions(userspace(pid=XXX,sFlow(vid=0,pcp=0,output=361),actions))),2 > Because I think we have enough grep, so I ignored it. Anyway, it's done now. > I will address your other comments in generic TC sFlow patchset and put this > patch at the end of it. > > Thanks, > Chris >> >>> + >>> +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 - set ingress_policing_rate and ingress_policing_burst >>> - offloads disabled]) >>> AT_KEYWORDS([ingress_policing]) >>> AT_SKIP_IF([test $HAVE_TC = "no"]) >>> -- >>> 2.27.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
