[ovs-dev] tests: fix reference output for meter offload stats

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(-)

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.