On Tue, Aug 3, 2021 at 3:29 PM Han Zhou <[email protected]> wrote: > > On Tue, Jul 27, 2021 at 10:06 AM Lorenzo Bianconi < > [email protected]> wrote: > > > > Introduce check_pkt_larger action for ingress traffic > > entering the cluster from a distributed gw router port > > or from a gw router. This patch enables pMTU discovery > > for ingress traffic. > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > Thanks for the patch. I hit a problem while rebasing on top of this patch. > It is committed as: > With the current master (already has this patch committed), the DDlog test > case fails (100% failure when I run it individually, but sometimes passes > when I run in parallel with other cases): > router - check packet length - icmp defrag -- ovn-northd-ddlog -- > dp-groups=yes > > Reverting the commit (1c9e46ab5 northd: add check_pkt_larger lflows for > ingress traffic) makes test pass. I haven't debugged why this happened. > > And I also have a question on the patch. Please see my comment below. > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > index d1f7dbbed..838f72824 100644 > > --- a/northd/ovn_northd.dl > > +++ b/northd/ovn_northd.dl > > @@ -4728,7 +4728,12 @@ for (&RouterPort(.lrp = lrp, > > * This will save us from having to match on inport further down in > > * the pipeline. > > */ > > - var actions = "${rEG_INPORT_ETH_ADDR()} = ${lrp_networks.ea}; next;" > in { > > + var gw_mtu = lrp.options.get_int_def("gateway_mtu", 0) in > > + var mtu = gw_mtu + vLAN_ETH_HEADER_LEN() in > > + var actions = "${rEG_INPORT_ETH_ADDR()} = ${lrp_networks.ea}; " ++ > > + if (gw_mtu > 0) { > > + "${rEGBIT_PKT_LARGER()} = check_pkt_larger(${mtu}); next;" > > + } else { "next;" } in { > > Flow(.logical_datapath = router._uuid, > > .stage = s_ROUTER_IN_ADMISSION(), > > .priority = 50, > > @@ -7558,11 +7563,12 @@ Flow(.logical_datapath = lr_uuid, > > var mtu = gw_mtu + vLAN_ETH_HEADER_LEN(). > > MeteredFlow(.logical_datapath = lr_uuid, > > .stage = s_ROUTER_IN_LARGER_PKTS(), > > - .priority = 50, > > - .__match = "inport == ${rp.json_name} && outport == > ${l3dgw_port_json_name} && " > > - "ip4 && ${rEGBIT_PKT_LARGER()}", > > + .priority = 150, > > + .__match = "inport == ${rp.json_name} && ip4 && " > > + "${rEGBIT_PKT_LARGER()} && > ${rEGBIT_EGRESS_LOOPBACK()} == 0", > > .actions = "icmp4_error {" > > "${rEGBIT_EGRESS_LOOPBACK()} = 1; " > > + "${rEGBIT_PKT_LARGER()} = 0; " > > "eth.dst = ${rp.networks.ea}; " > > "ip4.dst = ip4.src; " > > "ip4.src = ${first_ipv4.addr}; " > > @@ -7585,13 +7591,46 @@ MeteredFlow(.logical_datapath = lr_uuid, > > rp in &RouterPort(.router = r), > > rp.lrp != l3dgw_port, > > Some{var first_ipv4} = rp.networks.ipv4_addrs.nth(0). > > + > > +MeteredFlow(.logical_datapath = lr_uuid, > > + .stage = s_ROUTER_IN_IP_INPUT(), > > + .priority = 150, > > + .__match = "inport == ${rp.json_name} && ip4 && " > > + "${rEGBIT_PKT_LARGER()} && > ${rEGBIT_EGRESS_LOOPBACK()} == 0", > > + .actions = "icmp4_error {" > > + "${rEGBIT_EGRESS_LOOPBACK()} = 1; " > > + "${rEGBIT_PKT_LARGER()} = 0; " > > + "eth.dst = ${rp.networks.ea}; " > > + "ip4.dst = ip4.src; " > > + "ip4.src = ${first_ipv4.addr}; " > > + "ip.ttl = 255; " > > + "icmp4.type = 3; /* Destination > Unreachable. */ " > > + "icmp4.code = 4; /* Frag Needed and DF > was Set. */ " > > + /* Set icmp4.frag_mtu to gw_mtu */ > > + "icmp4.frag_mtu = ${gw_mtu}; " > > + "next(pipeline=ingress, table=0); " > > + "};", > > + .tags = map_empty(), > > + .controller_meter = r.copp.get(cOPP_ICMP4_ERR()), > > + .external_ids = stage_hint(rp.lrp._uuid)) :- > > + r in &Router(._uuid = lr_uuid), > > + Some{var l3dgw_port} = r.l3dgw_port, > > + var l3dgw_port_json_name = json_string_escape(l3dgw_port.name), > > + r.redirect_port_name != "", > > + var gw_mtu = l3dgw_port.options.get_int_def("gateway_mtu", 0), > > + gw_mtu > 0, > > + rp in &RouterPort(.router = r), > > + rp.lrp == l3dgw_port, > > + Some{var first_ipv4} = rp.networks.ipv4_addrs.nth(0). > > + > > This flow is exactly the same as the one immediately above it, except that > "rp.lrp == l3dgw_port" instead of "rp.lrp != l3dgw_port". Does it mean > adding the flow for all LRPs, whether it is l3dgw_port or not? If so, why > not combine the code just by removing the "rp.lrp == l3dgw_port" or "rp.lrp > != l3dgw_port" check? Did I misunderstand something? Same for the ipv6 flow > below.
I added the ddlog part of the code. Let me check and come back. Thanks Numan > > Thanks, > Han > _______________________________________________ > 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
