On 7 Oct 2022, at 14:42, Ilya Maximets wrote:
> On 10/7/22 13:37, Eelco Chaudron wrote: >> >> >> On 15 Sep 2022, at 14:03, Ilya Maximets wrote: >> >>> 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. >> >> Tianyu I might have missed this, but is there a follow on fix for this? >> Asking as the test in the current master is broken. >> >> Ilya/Simon should we undo “dd9881ed5 tc: Fix stats dump when using same >> meter table” and get a proper fix, fixing these counter issues? > > As far as I understand, kernel act_police is just not suitable > to represent an OpenFlow meter. The main problem is that with > OVS we need two separate entities - actual meter and the meter > action that is using some actual meter. And these two entities > should have separate sets of statistics. Actions should count > how many packets went through these actions (ideally, TC chain > as a whole should count the number of successful matches, but that > is anther inconsistency between TC and OVS) and the actual meter > should count how many packets went through/dropped/etc. Isn’t that the case? I think the basic statistics return the total number of packets hitting the rule, and the additional TCA_STATS_QUEUE stats give you the dropped packets. I’ll need to go over this, and the code again, as this is all from memory. > Multiple meter actions may reference the same actual meter. > Each of these actions should have statistics separate from the > statistics of the actual meter. Stats of actions should be > reported as flow stats, stats of the meter should be reported > as meter stats. This might be wrong, but I thought the stats are not per linked action, but from the rule that has the linked action. > But with the act_police the actual meter and the action are the > same entity, so the only statistics we have is the statistics > of the actual meter. So, there is no way to get the accurate > flow statistics if the meter is the first action. This is just > a broken API by design. > > Saying that, commit dd9881ed5 seems to be correct, because we > should not use statistics of the actual meter as flow stats. > At the same time we have no way to get flow stats. They are > broken either way but differently. If my story above not correct and we share the TCA_STATS_BASIC* with all instance where this meter is used we are out of luck. > For the time being we may "fix" the test by adding some action > before the meter in OpenFlow rules, so we can get accurate > stats from it. > > For the real solution - I don't see any that doesn't involve > kernel changes. We either need to: > > - separate act_police from the actual policer in the kernel, > so we can query stats for them independently. > > - or create something like act_count - a dummy action that only > counts packets, and put it in every datapath action from OVS. > > - or make flower chains count packets that successfully matched > and use that information as flows stats. > > In any case, we need to document somehow that flow stats with > meters are not correct. > > Any thoughts? I’ll try to find some time next week to experiment with this, but not sure if it will happen... > Best regards, Ilya Maximets. > >> >>>>> 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
