On Thu 04 Aug 2022 at 10:24, Ilya Maximets <i.maxim...@ovn.org> wrote: > 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 ?
Tested and sent the fix for 2.17 branch. > > 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
