On 8/4/22 09:52, Simon Horman wrote: > On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote: >> On 8/4/22 09:18, Simon Horman wrote: >>> On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: >>>> Referenced commit changed policer action type from TC_ACT_UNSPEC (continue) >>>> to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 >>>> driver at the time validated action type and always assumed 'continue', the >>>> breakage wasn't caught until later validation code was added. The change >>>> also broke valid configuration when sending from offload-capable device to >>>> non-offload capable. For example, when sending from mlx5 VF to OvS bridge >>>> netdevice the traffic that passed matchall classifier with policer could no >>>> longer match the following flower rule in software: >>> >>> ... >>> >>>> Notes: >>>> Changes V2 -> V3: >>>> >>>> - Refactored the fix to set action TC_ACT_UNSPEC only for >>>> matchall/basic >>>> classifiers. >>>> >>>> Changes V1 -> V2: >>>> >>>> - Rebase on latest master. >>> >>> ... >>> >>> Hi Vlad, >>> >>> thanks for addressing our concerns. We have reviewed and tested this patch >>> and do not observe any problems for our use-cases: in particular mirred >>> after meter (police). >>> >>> Acked-by: Simon Horman <simon.hor...@corigine.com> >>> >>> I'd also be happy to apply this to the upstream tree >>> if there are no objections from others. >> >> No objections from my side. >> >> Regarding backports, AFAICT, we'll need that change down to 2.16, right? >> Can code from v1 be used on 2.17 and 2.16 or do we need a separate backport >> patch? > > I think that the complexity that v3 addresses arises from metering support > which allows other actions to follow a police action (meter). > > So, assuming metering is not present in 2.17 and 2.16, and given that it > turns out that UNSPEC is fine for both PPS and BPS, then I suspect we > can go for a simple backport-patch which simply changes TC_ACT_PIPE > to TC_ACT_UNSPEC in nl_msg_act_police_end_nest(). > > This would need some testing, IMHO, and I may end up eating the words > above.
If that works, sounds good to me. Vlad, could you also test this approach with 2.17 ? I think, the best way forward will be to apply v3 to master and branch-3.0 now. Once the approach above for 2.17/2.16 is tested to work or some other solution is chosen, one of you could post the backport patch, so it can be reviewed/applied. What do you think? Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
