On 19 Sep 2022, at 9:31, Eelco Chaudron wrote: > > On 16 Sep 2022, at 4:25, Tianyu Yuan wrote: > > > On 9/16/22 10:14, 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 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. > >> > > > > Thanks Ilya, I previously had some misunderstanding of flow stats. > > > > Now the problem is, once police action(meter) is the first action in a > > filter, flow stats will ignore the police action and only be updated > > using the survived packets, which means the flow stats either before > > or after the commit dd9881ed55e6 is incorrect. > > > > We will keep looking into this issue. > > > I while back before doing chk_pkt_len I suggested a patch, but some told me > not all actions are implemented, it might be worth looking into this again if > needed: > > https://www.mail-archive.com/[email protected]/msg62960.html > Thanks Eelco.
It is a wonderful idea to handle the action stats and rule stats when it comes to the first action in filter(rule). I'll try whether it could be used in this case. Best regards, Tianyu Yuan > I can’t remember how I fixed this for chk_pkt_len, as it does work there, as > there are some specific test cases for it :) > > > /ovs-master/tests/system-offloads-traffic.at > > # This test verifies the total packet counters work when individual branches # > are taken. > > > //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
