On 9/16/22 11:25, 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
For the rte_flow case, the similar issue is solved by always adding an RTE_FLOW_ACTION_TYPE_COUNT action at the beginning of the action list. Is there some similar action in TC? > > 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
