On Thu, Aug 04, 2022 at 10:24:52AM +0200, Ilya Maximets 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 ? > > 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?
Yes, I agree we should move forwards on applying this to branch-3.0, and tackle 2.17/2.16 separately. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
