On Tue, Sep 13, 2022 at 12:41:33PM +0200, Ilya Maximets wrote:
> On 9/13/22 09:51, Simon Horman wrote:
> > On Mon, Sep 12, 2022 at 03:02:06PM +0200, Eelco Chaudron wrote:
> >>
> >>
> >> On 7 Sep 2022, at 11:36, Simon Horman wrote:
> >>
> >>> From: Tianyu Yuan <tianyu.y...@corigine.com>
> >>>
> >>> When we apply meter police on both directions of TCP traffic, the
> >>> dumped stats is shown same (as shown below). This issue is introduced
> >>> by modifying the stats update strategy.
> >>>
> >>> ...,in_port(6),eth(),eth_type(0x0800),ipv4(frag=no), packets:1488557,
> >>> bytes:2089059644, used:0.040s, actions:meter(0),9
> >>> ...,in_port(9),eth(),eth_type(0x0800),ipv4(frag=no), packets:1488557,
> >>> bytes:2089059644, used:0.040s, actions:meter(0),6
> >>>
> >>> In previous patch, after parsing police action, the flower stats will
> >>> be updated by dumped meter table stats, which will result in the issue
> >>> above.
> >>>
> >>> Thus, the stats of meter table should not be used when dumping flow
> >>> stats. Ignore the stats update when police.index belongs to meter.
> >>>
> >>> Fixes: a9b8cdde69de ("tc: Add support parsing tc police action")
> >>> Signed-off-by: Tianyu Yuan <tianyu.y...@corigine.com>
> >>> Reviewed-by: Baowen Zheng <baowen.zh...@corigine.com>
> >>> Signed-off-by: Simon Horman <simon.hor...@corigine.com>
> >
> > ...
> >
> >>> + /*
> >>> + * Skip the stats update when act_police is meter, since there are
> >>> always
> >>> + * some other actions following meter. For other potential kind of
> >>> police,
> >>> + * whose stats could not be skipped (e.g. filter has only one police
> >>> + * action), update the action stats to flow rule
> >>> + */
> >>
> >> Small nit:
> >>
> >>
> >> + /* Skip the stats update when act_police is meter since there are
> >> always
> >> + * some other actions following meter. For other potential kinds of
> >> + * act_police actions, whose stats could not be skipped (e.g. filter
> >> has
> >> + * only one police action), update the action stats to the flow
> >> rule. */
> >>
> >> The rest looks good to me.
> >>
> >> Acked-by: Eelco Chaudron <echau...@redhat.com>
> >
> > Thanks Eelco,
> >
> > I've made the change suggested above and applied the patch to master and
> > branch-3.0.
>
> Hi, Simon and Eelco.
>
> This commit made offload tests consistently fail due to incorrectly counted
> flow stats:
>
> # make check-offloads TESTSUITEFLAGS='8'
> 8. system-offloads-traffic.at:230: testing offloads - check interface meter
> offloading - offloads enabled ...
>
> ./system-offloads-traffic.at:266: ovs-appctl dpctl/dump-flows | grep "meter"
> | sed -e 's/used:\([0-9.]*s\|never\)/used:0.001s/;s/eth(
> src=[a-z0-9:]*,dst=[a-z0-9:]*)/eth(macs)/;s/actions:[0-9,]\+/actions:output/;s/recirc_id(0),//'
> | sort
> --- - 2022-09-13 06:37:19.306764602 -0400
> +++ /root/ovs/tests/system-offloads-testsuite.dir/at-groups/8/stdout
> 2022-09-13 06:37:19.304198787 -0400
> @@ -1,2 +1,2 @@
> -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
>
> Could you, please, take a look?
>
> I see this on my rhel8 VM.
Thanks for letting me know and sorry about this.
We'll look into it.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
