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

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

[Marcelo Leitner]

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

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

[Davide Caratti]

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!
-- 
davide

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

[Marcelo Leitner]

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

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

[Jamal Hadi Salim]

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

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

[Marcelo Leitner]

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

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

[Jamal Hadi Salim]

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

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

[Ilya Maximets]

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

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

[Jamal Hadi Salim]

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

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

[Simon Horman]

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.

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

[Ilya Maximets]

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.

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

[Simon Horman]

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.

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

[Simon Horman]

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.

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

[Eelco Chaudron]

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

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

[Tianyu Yuan]

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