On Wed, Nov 10, 2021 at 12:24 PM Numan Siddique <[email protected]> wrote: > > On Wed, Nov 10, 2021 at 5:19 AM Dumitru Ceara <[email protected]> wrote: > > > > On 11/9/21 8:24 PM, Numan Siddique wrote: > > > On Fri, Nov 5, 2021 at 5:21 AM Dumitru Ceara <[email protected]> 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 <[email protected]> > > > > > > > > > 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".
On second thought, I think "gateway_mtu_bypass" makes more sense. If a user wants to bypass "tcp" protocol, then with gateway_mtu_match user has to add a match like - "(udp || icmp)" which would result in more openflows. I'll take a closer look into v2 tomorrow. Thanks Numan > > Thanks > Numan > > > > > Thanks, > > Dumitru > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
