On Wed, Nov 10, 2021 at 5:19 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 11/9/21 8:24 PM, Numan Siddique wrote:
> > On Fri, Nov 5, 2021 at 5:21 AM Dumitru Ceara <dce...@redhat.com> wrote:
> >>
> >> There are various costs (e.g., not being able to perform hardware
> >> offload in some cases) when using check_pkt_larger() so the CMS
> >> can now limit the impact by bypassing the packet length checks for
> >> specific types of traffic (e.g., TCP).
> >>
> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2011779
> >> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> >
> >
> > Hi Dumitru,
> >
>
> Hi Numan,
>
> Thanks for reviewing this!
>
> > Thanks for the patch. Instead of adding a bypass option, how about adding
> > the option to check for the extra matches ?
>
> We could do that too, even if it's just for making it more natural to
> configure.
>
> >
> > i.e new option can be - gateway_mtu_match. And ovn-northd would append
> > this match to the existing logical flows with the action - check_pkt_larger
> > ?
>
> That wouldn't work directly. That's because we already append
> check_pkt_larger() to flows that perform other actions.
Ah I see. I missed this. Thanks.
>
> Let's assume we start with gateway_mtu not configured, then we'd have
> (at least) the following flows:
>
> - table=rtr_in_admission,prio=50, eth.mcast && inport == <rtr-port>,
> actions='REG_INPORT_ETH_ADDR = <rtr-port>.eth, next;'
> - table=rtr_in_admission,prio=50, eth.dst == <rtr-port> && inport ==
> <rtr-port> && is_chassis_resident(<rtr-port>),
> actions='REG_INPORT_ETH_ADDR = <rtr-port>.eth, next;'
>
> If we now configure gateway_mtu=1400 these flows get changed to:
>
> - table=rtr_in_admission,prio=50, eth.mcast && inport == <rtr-port>,
> actions='REG_INPORT_ETH_ADDR = <rtr-port>.eth;
> reg9[1]=check_pkt_larger(1428); next;'
> - table=rtr_in_admission,prio=50, eth.dst == <rtr-port> && inport ==
> <rtr-port> && is_chassis_resident(<rtr-port>),
> actions='REG_INPORT_ETH_ADDR = <rtr-port>.eth;
> reg9[1]=check_pkt_larger(1428); next;'
>
> Just adding an extra gateway_mtu_match to the matches of the two flows
> above would be wrong as we would not be performing the other actions
> ("REG_INPORT_ETH_ADDR = <rtr-port>; next;") for all traffic that doesn't
> need check_pkt_larger.
>
> >
> > This would not increase the number of lflows ? It's possible that I may
> > have
> > missed a few details/scenarios. What do you think?
>
> Due to the reasons above, this other approach you're suggesting would
> unfortunately also have to increase the number of flows. However, we're
> not talking about a huge increase because we currently have at most 3
> flows per logical router port that has gateway_mtu enabled. I don't
> expect the number of such ports to be huge.
Agree.
>
> >
> > Otherwise the patch LGTM.
> >
>
> If you think it makes more sense to configure "gateway_mtu_match"
> instead of "gateway_mtu_bypass", I'm ok with that and I can send a v3
> but the implementation would be very similar to the one in v2.
Having a whitelist match seems more natural to me. So my suggestion
would be for "gateway_mtu_match".
Thanks
Numan
>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
