Date:   Fri, 7 Oct 2022 06:21:01 -0700
From:   Marcelo Leitner <mleitner@...hat.com>
To:     Ilya Maximets <i.maximets@....org>
Cc:     Eelco Chaudron <echaudro@...hat.com>,
        Tianyu Yuan <tianyu.yuan@...igine.com>,
        Simon Horman <simon.horman@...igine.com>, dev@...nvswitch.org,
        oss-drivers <oss-drivers@...igine.com>, dcaratti@...hat.com,
        netdev@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Oz Shlomo <ozsh@...dia.com>, Paul Blakey <paulb@...dia.com>
Subject: Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

(+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@...igine.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@...igine.com>
> >>>>>> Signed-off-by: Simon Horman <simon.horman@...igine.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
> >>>
> >
>

Powered by blists - more mailing lists