Message ID | 20220914141923.1725821-1-simon.horman@corigine.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev] tests: fix reference output for meter offload stats | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@corigine.com> Lines checked: 34, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 9/14/22 16:19, Simon Horman wrote: > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > 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 <tianyu.yuan@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > 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? > ]) > > 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.
On 9/15/22 19:28, Ilya Maximets wrote: > On 9/14/22 16:19, Simon Horman wrote: > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > 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 <tianyu.yuan@corigine.com> > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > --- > > 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. > > ]) > > > > 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
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 <tianyu.yuan@corigine.com> >>> >>> 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 <tianyu.yuan@corigine.com> >>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>> --- >>> 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. >>> ]) >>> >>> 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
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 <tianyu.yuan@corigine.com> >>>> >>>> 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 <tianyu.yuan@corigine.com> >>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>> --- >>>> 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 >
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 <tianyu.yuan@corigine.com> > >>> > >>> 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 <tianyu.yuan@corigine.com> > >>> Signed-off-by: Simon Horman <simon.horman@corigine.com> > >>> --- > >>> 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. > >>> ]) > >>> > >>> 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
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 <tianyu.yuan@corigine.com> >>>>> >>>>> 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 <tianyu.yuan@corigine.com> >>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>> --- >>>>> 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/ovs-dev@openvswitch.org/msg62960.html 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
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 <tianyu.yuan@corigine.com> >>>>>> >>>>>> 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 <tianyu.yuan@corigine.com> >>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>> --- >>>>>> 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/ovs-dev@openvswitch.org/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 >
On 16 Sep 2022, at 15:11, Ilya Maximets wrote: > 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 <tianyu.yuan@corigine.com> >>>>>>> >>>>>>> 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 <tianyu.yuan@corigine.com> >>>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>>> --- >>>>>>> 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/ovs-dev@openvswitch.org/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? TC does not have a specific count action. It’s part of each action, however some action type always return 0 (I was told). >> >> 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 >>
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 <tianyu.yuan@corigine.com> > >>>>> > >>>>> 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 <tianyu.yuan@corigine.com> > >>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> > >>>>> --- > >>>>> 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/ovs-dev@openvswitch.org/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
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 <tianyu.yuan@corigine.com> >>>>> >>>>> 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 <tianyu.yuan@corigine.com> >>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>> --- >>>>> 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? >>> 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 >>
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 <tianyu.yuan@corigine.com> >>>>>> >>>>>> 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 <tianyu.yuan@corigine.com> >>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>> --- >>>>>> 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. 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. 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. 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? 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 >>> >
(+TC folks and netdev@) On Fri, Oct 07, 2022 at 02:42:56PM +0200, 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 <tianyu.yuan@corigine.com> > >>>>>> > >>>>>> 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 <tianyu.yuan@corigine.com> > >>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> > >>>>>> --- > >>>>>> 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. > 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. > > 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. Thanks Ilya for the detailed explanation. > > 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. > > 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. I don't see how we could achieve this without breaking much of the user experience. > > - or create something like act_count - a dummy action that only > counts packets, and put it in every datapath action from OVS. This seems the easiest and best way forward IMHO. It's actually the 3rd option below but "on demand", considering that tc will already use the stats of the first action as the flow stats (in tcf_exts_dump_stats()), then we can patch ovs to add such action if a meter is also being used (or perhaps even always, because other actions may also drop packets, and for OVS we would really be at the 3rd option below). netfilter (with iptables or nft) has a way to do strict packet counting. It's useful. Thoughts? Marcelo > > - 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? > > 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 > >>> > > >
On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner <mleitner@redhat.com> wrote: > > (+TC folks and netdev@) > > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote: > > On 10/7/22 13:37, Eelco Chaudron wrote: [...] > I don't see how we could achieve this without breaking much of the > user experience. > > > > > - or create something like act_count - a dummy action that only > > counts packets, and put it in every datapath action from OVS. > > This seems the easiest and best way forward IMHO. It's actually the > 3rd option below but "on demand", considering that tc will already use > the stats of the first action as the flow stats (in > tcf_exts_dump_stats()), then we can patch ovs to add such action if a > meter is also being used (or perhaps even always, because other > actions may also drop packets, and for OVS we would really be at the > 3rd option below). Correct me if I'm wrong, but actually act_gact action with "pipe" control action should already do this counting job. any feedback appreciated, thanks!
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 <tianyu.yuan@corigine.com> >>>>>>> >>>>>>> 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 <tianyu.yuan@corigine.com> >>>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>>> --- >>>>>>> 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 >>>> >>
On Fri, Oct 07, 2022 at 04:39:25PM +0200, Davide Caratti wrote: > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner <mleitner@redhat.com> wrote: > > > > (+TC folks and netdev@) > > > > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote: > > > On 10/7/22 13:37, Eelco Chaudron wrote: > > [...] > > > I don't see how we could achieve this without breaking much of the > > user experience. > > > > > > > > - or create something like act_count - a dummy action that only > > > counts packets, and put it in every datapath action from OVS. > > > > This seems the easiest and best way forward IMHO. It's actually the > > 3rd option below but "on demand", considering that tc will already use > > the stats of the first action as the flow stats (in > > tcf_exts_dump_stats()), then we can patch ovs to add such action if a > > meter is also being used (or perhaps even always, because other > > actions may also drop packets, and for OVS we would really be at the > > 3rd option below). > > Correct me if I'm wrong, but actually act_gact action with "pipe" > control action should already do this counting job. act_gact is so transparent that I never see it/remembers about it :) Yup, although it's not offloadabe with pipe control actio AFAICT. Marcelo
On 7 Oct 2022, at 16:39, Davide Caratti wrote: > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner <mleitner@redhat.com> wrote: >> >> (+TC folks and netdev@) >> >> On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote: >>> On 10/7/22 13:37, Eelco Chaudron wrote: > > [...] > >> I don't see how we could achieve this without breaking much of the >> user experience. >> >>> >>> - or create something like act_count - a dummy action that only >>> counts packets, and put it in every datapath action from OVS. >> >> This seems the easiest and best way forward IMHO. It's actually the >> 3rd option below but "on demand", considering that tc will already use >> the stats of the first action as the flow stats (in >> tcf_exts_dump_stats()), then we can patch ovs to add such action if a >> meter is also being used (or perhaps even always, because other >> actions may also drop packets, and for OVS we would really be at the >> 3rd option below). Guess we have to add this extra action to each datapath flow offloaded due to the way flows back and forth translations are handled. Maybe we can do it selectively, but the code might become messier than it already is :( > Correct me if I'm wrong, but actually act_gact action with "pipe" > control action should already do this counting job. I think we could use that, as we only use TC_ACT_GOTO_CHAIN and TC_ACT_SHOT. And it looks like TC_ACT_SHOT is not decoded correctly :(
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 <tianyu.yuan@corigine.com> >>>>>>>> >>>>>>>> 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 <tianyu.yuan@corigine.com> >>>>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>>>> --- >>>>>>>> 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 >>>>> >>> >
On Fri, Oct 7, 2022 at 11:01 AM Marcelo Leitner <mleitner@redhat.com> wrote: > > On Fri, Oct 07, 2022 at 04:39:25PM +0200, Davide Caratti wrote: > > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner <mleitner@redhat.com> wrote: > > > > > > (+TC folks and netdev@) > > > > > > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote: > > > > On 10/7/22 13:37, Eelco Chaudron wrote: > > > > [...] > > > > > I don't see how we could achieve this without breaking much of the > > > user experience. > > > > > > > > > > > - or create something like act_count - a dummy action that only > > > > counts packets, and put it in every datapath action from OVS. > > > > > > This seems the easiest and best way forward IMHO. It's actually the > > > 3rd option below but "on demand", considering that tc will already use > > > the stats of the first action as the flow stats (in > > > tcf_exts_dump_stats()), then we can patch ovs to add such action if a > > > meter is also being used (or perhaps even always, because other > > > actions may also drop packets, and for OVS we would really be at the > > > 3rd option below). > > > > Correct me if I'm wrong, but actually act_gact action with "pipe" > > control action should already do this counting job. > > act_gact is so transparent that I never see it/remembers about it :) > Yup, although it's not offloadabe with pipe control actio AFAICT. > It's mostly how people who offload (not sure about OVS) do it; example some of the switches out there. The action index, whatever that action is, could be easily mapped to a stats index in hardware. If you are sharing action instances (eg policer index 32) across multiple flows then of course in hw you are using only that one instance of the meter. If you want to have extra stats that differentiate between the flows then add a gact (PIPE as Davide mentioned) and the only thing it will do is count and nothing else. cheers, jamal
On Fri, Oct 07, 2022 at 11:59:42AM -0400, Jamal Hadi Salim wrote: > On Fri, Oct 7, 2022 at 11:01 AM Marcelo Leitner <mleitner@redhat.com> wrote: > > > > On Fri, Oct 07, 2022 at 04:39:25PM +0200, Davide Caratti wrote: > > > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner <mleitner@redhat.com> wrote: > > > > > > > > (+TC folks and netdev@) > > > > > > > > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote: > > > > > On 10/7/22 13:37, Eelco Chaudron wrote: > > > > > > [...] > > > > > > > I don't see how we could achieve this without breaking much of the > > > > user experience. > > > > > > > > > > > > > > - or create something like act_count - a dummy action that only > > > > > counts packets, and put it in every datapath action from OVS. > > > > > > > > This seems the easiest and best way forward IMHO. It's actually the > > > > 3rd option below but "on demand", considering that tc will already use > > > > the stats of the first action as the flow stats (in > > > > tcf_exts_dump_stats()), then we can patch ovs to add such action if a > > > > meter is also being used (or perhaps even always, because other > > > > actions may also drop packets, and for OVS we would really be at the > > > > 3rd option below). > > > > > > Correct me if I'm wrong, but actually act_gact action with "pipe" > > > control action should already do this counting job. > > > > act_gact is so transparent that I never see it/remembers about it :) > > Yup, although it's not offloadabe with pipe control actio AFAICT. > > > > It's mostly how people who offload (not sure about OVS) do it; > example some of the switches out there. You mean with OK, DROP, TRAP or GOTO actions, right? Because for PIPE, it has: } else if (is_tcf_gact_pipe(act)) { NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is not supported"); return -EOPNOTSUPP; Or maybe I'm missing something. > The action index, whatever that action is, could be easily mapped > to a stats index in hardware. If you are sharing action instances > (eg policer index 32) across multiple flows then of course in hw > you are using only that one instance of the meter. If you want > to have extra stats that differentiate between the flows then > add a gact (PIPE as Davide mentioned) and the only thing it > will do is count and nothing else. Yup, makes sense. And talking with Ilya, that's what OVS already does for DPDK as well. Thanks, Marcelo
On Fri, Oct 7, 2022 at 1:37 PM Marcelo Leitner <mleitner@redhat.com> wrote: > > On Fri, Oct 07, 2022 at 11:59:42AM -0400, Jamal Hadi Salim wrote: > > On Fri, Oct 7, 2022 at 11:01 AM Marcelo Leitner <mleitner@redhat.com> wrote: > > > [..] > > > > It's mostly how people who offload (not sure about OVS) do it; > > example some of the switches out there. > > You mean with OK, DROP, TRAP or GOTO actions, right? > > Because for PIPE, it has: > } else if (is_tcf_gact_pipe(act)) { > NL_SET_ERR_MSG_MOD(extack, "Offload of > \"pipe\" action is not supported"); > return -EOPNOTSUPP; > I thought it was pipe but maybe it is OK(in my opinion that is a bad code for just "count"). We have some (at least NIC) hardware folks on the list. Note: we could create an alias to PIPE and call it COUNT if it helps. And yes, in retrospect we should probably have separated out accounting from the actions in tc. It makes a lot of sense in s/w - and would work fine for modern hardware but when you dont have as many counters as actions it's a challenge. Same thing with policers/meters. cheers, jamal
On 10 Oct 2022, at 13:40m, Eclco Chaudron wrote: > On 7 Oct 2022, at 16:39, Davide Caratti wrote: > > > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner <mleitner@redhat.com> > wrote: > >> > >> (+TC folks and netdev@) > >> > >> On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote: > >>> On 10/7/22 13:37, Eelco Chaudron wrote: > > > > [...] > > > >> I don't see how we could achieve this without breaking much of the > >> user experience. > >> > >>> > >>> - or create something like act_count - a dummy action that only > >>> counts packets, and put it in every datapath action from OVS. > >> > >> This seems the easiest and best way forward IMHO. It's actually the > >> 3rd option below but "on demand", considering that tc will already > >> use the stats of the first action as the flow stats (in > >> tcf_exts_dump_stats()), then we can patch ovs to add such action if a > >> meter is also being used (or perhaps even always, because other > >> actions may also drop packets, and for OVS we would really be at the > >> 3rd option below). > > Guess we have to add this extra action to each datapath flow offloaded due > to the way flows back and forth translations are handled. Maybe we can do it > selectively, but the code might become messier than it already is :( > Thanks for your and others' comments. According to your comments, I made a test by adding a gact with PIPE at the first place of each filter. It seems this gact will successfully record the stats hitting this rule in sw TC datapath. But this approach doesn't work for hw tc offload since the nic may not support offloading this action. In current OVS implementation, the flow stats is updated by action stats. Is that possible to collect filter stats in kernel TC when dumping actions stats and use this filter stats to update the flow stats in OVS. The filter stats could also be transmitted by flower option netlink messeage. I'm now trying to look if this will work. Best regards, Tianyu > > Correct me if I'm wrong, but actually act_gact action with "pipe" > > control action should already do this counting job. > > I think we could use that, as we only use TC_ACT_GOTO_CHAIN and > TC_ACT_SHOT. And it looks like TC_ACT_SHOT is not decoded correctly :(
On Fri, Oct 07, 2022 at 01:37:45PM +0200, 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: ... > >>>> 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? Thanks Eelco, there has been a bit of follow-on discussion in this thread regarding the root cause and possible fixes. We think it is fair to say that it is now clear that dd9881ed5 does not address the issue it seeks to address. Tianyu and I agree that while working on a solution it would be best to revert dd9881ed5. And we expect to post a patch to revert dd9881ed5 in the near future.
On 10/8/22 14:32, Jamal Hadi Salim wrote: > On Fri, Oct 7, 2022 at 1:37 PM Marcelo Leitner <mleitner@redhat.com> wrote: >> >> On Fri, Oct 07, 2022 at 11:59:42AM -0400, Jamal Hadi Salim wrote: >>> On Fri, Oct 7, 2022 at 11:01 AM Marcelo Leitner <mleitner@redhat.com> wrote: >>>> > > [..] >>> >>> It's mostly how people who offload (not sure about OVS) do it; >>> example some of the switches out there. >> >> You mean with OK, DROP, TRAP or GOTO actions, right? >> >> Because for PIPE, it has: >> } else if (is_tcf_gact_pipe(act)) { >> NL_SET_ERR_MSG_MOD(extack, "Offload of >> \"pipe\" action is not supported"); >> return -EOPNOTSUPP; >> > > I thought it was pipe but maybe it is OK(in my opinion that is a bad code > for just "count"). We have some (at least NIC) hardware folks on the list. IIRC, 'OK' action will stop the processing for the packet, so it can only be used as a last action in the list. But we need to count packets as a very first action in the list. So, that doesn't help. > Note: we could create an alias to PIPE and call it COUNT if it helps. Will that help with offloading of that action? Why the PIPE is not offloadable in the first place and will COUNT be offloadable? > And yes, in retrospect we should probably have separated out accounting > from the actions in tc. It makes a lot of sense in s/w - and would work fine for > modern hardware but when you dont have as many counters as actions > it's a challenge. Same thing with policers/meters. > > cheers, > jamal
On Fri, Oct 14, 2022 at 9:00 AM Ilya Maximets <i.maximets@ovn.org> wrote: > [..] > > I thought it was pipe but maybe it is OK(in my opinion that is a bad code > > for just "count"). We have some (at least NIC) hardware folks on the list. > > IIRC, 'OK' action will stop the processing for the packet, so it can > only be used as a last action in the list. But we need to count packets > as a very first action in the list. So, that doesn't help. > That's why i said it is a bad code - but i believe it's what some of the hardware people are doing. Note: it's only bad if you have more actions after because it aborts the processing pipeline. > > Note: we could create an alias to PIPE and call it COUNT if it helps. > > Will that help with offloading of that action? Why the PIPE is not > offloadable in the first place and will COUNT be offloadable? Offloadable is just a semantic choice in this case. If someone is using OK to count today - they could should be able to use PIPE instead (their driver needs to do some transformation of course). cheers, jamal
On Fri, Oct 14, 2022 at 10:40:30AM -0400, Jamal Hadi Salim wrote: > On Fri, Oct 14, 2022 at 9:00 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > > > [..] > > > I thought it was pipe but maybe it is OK(in my opinion that is a bad code > > > for just "count"). We have some (at least NIC) hardware folks on the list. > > > > IIRC, 'OK' action will stop the processing for the packet, so it can > > only be used as a last action in the list. But we need to count packets > > as a very first action in the list. So, that doesn't help. > > > > That's why i said it is a bad code - but i believe it's what some of > the hardware > people are doing. Note: it's only bad if you have more actions after because > it aborts the processing pipeline. > > > > Note: we could create an alias to PIPE and call it COUNT if it helps. > > > > Will that help with offloading of that action? Why the PIPE is not > > offloadable in the first place and will COUNT be offloadable? > > Offloadable is just a semantic choice in this case. If someone is > using OK to count today - they could should be able to use PIPE > instead (their driver needs to do some transformation of course). FWIIW, yes, that is my thinking too.
On 10/19/22 10:12, Simon Horman wrote: > On Fri, Oct 14, 2022 at 10:40:30AM -0400, Jamal Hadi Salim wrote: >> On Fri, Oct 14, 2022 at 9:00 AM Ilya Maximets <i.maximets@ovn.org> wrote: >>> >> >> [..] >>>> I thought it was pipe but maybe it is OK(in my opinion that is a bad code >>>> for just "count"). We have some (at least NIC) hardware folks on the list. >>> >>> IIRC, 'OK' action will stop the processing for the packet, so it can >>> only be used as a last action in the list. But we need to count packets >>> as a very first action in the list. So, that doesn't help. >>> >> >> That's why i said it is a bad code - but i believe it's what some of >> the hardware >> people are doing. Note: it's only bad if you have more actions after because >> it aborts the processing pipeline. >> >>>> Note: we could create an alias to PIPE and call it COUNT if it helps. >>> >>> Will that help with offloading of that action? Why the PIPE is not >>> offloadable in the first place and will COUNT be offloadable? >> >> Offloadable is just a semantic choice in this case. If someone is >> using OK to count today - they could should be able to use PIPE >> instead (their driver needs to do some transformation of course). > > FWIIW, yes, that is my thinking too. I don't know that code well, but I thought that tcf_gact_offload_act_setup() is a generic function. And since it explicitly forbids offload of PIPE action, no drivers can actually offload it even if they want to. So it's not really a driver's choice in the current kernel code. Or am I missing something? Best regards, Ilya Maximets.
On Wed, Oct 19, 2022 at 02:17:42PM +0200, Ilya Maximets wrote: > On 10/19/22 10:12, Simon Horman wrote: > > On Fri, Oct 14, 2022 at 10:40:30AM -0400, Jamal Hadi Salim wrote: > >> On Fri, Oct 14, 2022 at 9:00 AM Ilya Maximets <i.maximets@ovn.org> wrote: > >>> > >> > >> [..] > >>>> I thought it was pipe but maybe it is OK(in my opinion that is a bad code > >>>> for just "count"). We have some (at least NIC) hardware folks on the list. > >>> > >>> IIRC, 'OK' action will stop the processing for the packet, so it can > >>> only be used as a last action in the list. But we need to count packets > >>> as a very first action in the list. So, that doesn't help. > >>> > >> > >> That's why i said it is a bad code - but i believe it's what some of > >> the hardware > >> people are doing. Note: it's only bad if you have more actions after because > >> it aborts the processing pipeline. > >> > >>>> Note: we could create an alias to PIPE and call it COUNT if it helps. > >>> > >>> Will that help with offloading of that action? Why the PIPE is not > >>> offloadable in the first place and will COUNT be offloadable? > >> > >> Offloadable is just a semantic choice in this case. If someone is > >> using OK to count today - they could should be able to use PIPE > >> instead (their driver needs to do some transformation of course). > > > > FWIIW, yes, that is my thinking too. > > I don't know that code well, but I thought that tcf_gact_offload_act_setup() > is a generic function. And since it explicitly forbids offload of PIPE > action, no drivers can actually offload it even if they want to. Sure, but I would expect that can be changed. > So it's not really a driver's choice in the current kernel code. Or am I > missing something? > > Best regards, Ilya Maximets.
On Wed, Oct 19, 2022 at 02:28:29PM +0200, Simon Horman wrote: > On Wed, Oct 19, 2022 at 02:17:42PM +0200, Ilya Maximets wrote: > > On 10/19/22 10:12, Simon Horman wrote: > > > On Fri, Oct 14, 2022 at 10:40:30AM -0400, Jamal Hadi Salim wrote: > > >> On Fri, Oct 14, 2022 at 9:00 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > >>> > > >> > > >> [..] > > >>>> I thought it was pipe but maybe it is OK(in my opinion that is a bad code > > >>>> for just "count"). We have some (at least NIC) hardware folks on the list. > > >>> > > >>> IIRC, 'OK' action will stop the processing for the packet, so it can > > >>> only be used as a last action in the list. But we need to count packets > > >>> as a very first action in the list. So, that doesn't help. > > >>> > > >> > > >> That's why i said it is a bad code - but i believe it's what some of > > >> the hardware > > >> people are doing. Note: it's only bad if you have more actions after because > > >> it aborts the processing pipeline. > > >> > > >>>> Note: we could create an alias to PIPE and call it COUNT if it helps. > > >>> > > >>> Will that help with offloading of that action? Why the PIPE is not > > >>> offloadable in the first place and will COUNT be offloadable? > > >> > > >> Offloadable is just a semantic choice in this case. If someone is > > >> using OK to count today - they could should be able to use PIPE > > >> instead (their driver needs to do some transformation of course). > > > > > > FWIIW, yes, that is my thinking too. > > > > I don't know that code well, but I thought that tcf_gact_offload_act_setup() > > is a generic function. And since it explicitly forbids offload of PIPE > > action, no drivers can actually offload it even if they want to. > > Sure, but I would expect that can be changed. RFC kernel patch posted: * https://lore.kernel.org/netdev/20221122112020.922691-1-simon.horman@corigine.com/ > > So it's not really a driver's choice in the current kernel code. Or am I > > missing something? > > > > Best regards, Ilya Maximets.
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 ]) AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | sed -e 's/duration:[[0-9]].[[0-9]]*s/duration:0.001s/'], [0], [dnl