On 10/7/22 16:49, Eelco Chaudron wrote: > > > 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.
AFAICT, we use the same instance of the act_police in multiple datapath flows, so the statistics is aggregated across all of them, hence cannot be used as flow stats. > > 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. In normal OVS yes. But I was thinking in TC abstractions here. In TC stats are on each 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
