On 9/15/22 13:56, Ilya Maximets wrote: > On 9/15/22 13:38, Tianyu Yuan wrote: >> >> On 9/15/22 19:28, Ilya Maximets wrote: >>> On 9/14/22 16:19, Simon Horman wrote: >>>> From: Tianyu Yuan <[email protected]> >>>> >>>> Since dd9881ed55e6 ("tc: Fix stats dump when using same meter table") >>>> rule stats update will ignore meter police. Correspondingly, the >>>> reference stats of the test should also be modified to ensure the test >>>> could pass correctly. >>>> >>>> Fixes: dd9881ed55e6 ("tc: Fix stats dump when using same meter table") >>>> Signed-off-by: Tianyu Yuan <[email protected]> >>>> Signed-off-by: Simon Horman <[email protected]> >>>> --- >>>> tests/system-offloads-traffic.at | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tests/system-offloads-traffic.at >>>> b/tests/system-offloads-traffic.at >>>> index d9b815a5ddf4..24e49d42f589 100644 >>>> --- a/tests/system-offloads-traffic.at >>>> +++ b/tests/system-offloads-traffic.at >>>> @@ -264,7 +264,7 @@ NS_CHECK_EXEC([at_ns0], [echo "mark" | nc >>>> $NC_EOF_OPT -u 10.1.1.2 5678 -p 6789]) done >>>> >>>> AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" | >>>> DUMP_CLEAN_SORTED], [0], [dnl >>>> -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), >>>> packets:10, bytes:330, used:0.001s, actions:meter(0),3 >>>> +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), >>>> +packets:1, bytes:33, used:0.001s, actions:meter(0),3 >>> >>> This looks very strange to me. The test does send 10 packets. >>> Why the flow should report only one? >>> >> Thanks for your review Ilya. >> The test does send 10 packets but 9 of them are dropped by >> meter action. As we descript in commit (dd9881ed55e6), the >> flow stats should not report the police stats.
"In previous patch, after parsing police action, the flower stats will be updated by dumped meter table stats" I suppose, that is the root cause. We should not mix these two completely different types of statistics. I didn't look at the code though to say why this was implemented in a way it is or how to fix that. >> In this case, only >> one packet passes the meter action and be used to update >> flow stats. > > The flow statistics counts how many packets hit the flow by > the match criteria, not how many of them survived after the > action execution. > > See the same test with offloads disabled (next to it in the file). > It correctly counts all the 10 packets. > > If we will not count packets, OVS will eventually remove the > flow from the datapath causing removal from TC and hardware > and triggering upcalls on the next packet. And OpenFlow > statistics will also be incorrect. > > Best regards, Ilya Maximets. > >>>> ]) >>>> >>>> AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | sed -e >>>> 's/duration:[[0-9]].[[0-9]]*s/duration:0.001s/'], [0], [dnl >>> >>> These meter stats are correctly reporting 11 packets, so the datapath flow >>> should report 10 (+1 on the upcall), AFAIU. >>> >>> Best regards, Ilya Maximets. >> >> Attach the tc filter information when running this test: >> filter protocol ip pref 3 flower chain 0 >> filter protocol ip pref 3 flower chain 0 handle 0x1 >> dst_mac f0:00:00:01:01:02 >> src_mac f0:00:00:01:01:01 >> eth_type ipv4 >> ip_proto udp >> ip_flags nofrag >> not_in_hw >> action order 1: police 0x10000000 rate 0bit burst 0b mtu 64Kb >> pkts_rate 1 pkts_burst 1 action drop/pipe overhead 0b linklayer unspec >> ref 2 bind 1 installed 2 sec used 0 sec firstused 0 sec >> Action statistics: >> Sent 330 bytes 10 pkt (dropped 9, overlimits 9 requeues 0) >> backlog 0b 0p requeues 0 >> >> action order 2: mirred (Egress Redirect to device ovs-p1) stolen >> index 8 ref 1 bind 1 installed 1 sec used 0 sec firstused 0 sec >> Action statistics: >> Sent 33 bytes 1 pkt (dropped 0, overlimits 0 requeues 0) >> backlog 0b 0p requeues 0 >> cookie 8455fe4dcd4e3677d0ca43b42684d1a6 >> no_percpu >> >> >> Best regards, >> Tianyu Yuan > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
