On Mon, Mar 13, 2023 at 10:21:03AM +0100, Ales Musil wrote: > On Fri, Mar 10, 2023 at 11:11 AM Simon Horman <[email protected]> > wrote: > > > On Thu, Mar 09, 2023 at 07:17:15AM +0100, Ales Musil wrote: > > > There was a race within packet buffering that could > > > result in first packt being dropped. It could happen > > > under following conditions and topology: > > > S1 == R1 == public == R2 == S2 > > > SNAT on R1 and DGP on port connecting R1 with public. > > > > > > 1) The GARP is sent for the GDP SNAT > > > 2) The GARP is delayed on R2 because it's multicast > > > 3) Some traffic that get's buffered on S2 > > > 4) An ARP is sent as consequence of the buffering > > > 5) The delayed MAC binding is added to SB > > > 6) Response for the ARP is ignored because the MAC binding > > > already exists > > > 7) The buffered packet is never sent out and times out > > > > > > In order to prevent this behavior add new node that will > > > keep track of all recently changed MAC bindings. Those > > > recently changed MAC bindings are kept around for a longer > > > time than the buffered packets which should ensure that we > > > can find them even if they are created before the packet > > > is actually buffered. > > > > > > At the same time simplify the packet buffering process > > > and move it to mac-learn module. > > > > This patch is a bit long. > > It may be easier to review if it was broken up somehow. > > > > It can probably be easily split into two patches. One introducing > the new I-P node and second for the pinctrl and mac-learn part WDYT?
Yes, I think that would help. > > > Signed-off-by: Ales Musil <[email protected]> > > > > ... > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 7dcbfd252..c03b2af4c 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > > ... > > > > > @@ -4305,6 +4306,185 @@ pflow_output_debug_handler(struct engine_node > > *node, void *data) > > > return true; > > > } > > > > > > +static void > > > +recent_mac_bindings_add(struct hmap *recent_mbs, > > > + const struct sbrec_mac_binding *smb, > > > + const struct hmap *local_datapaths, > > > + struct ovsdb_idl_index > > *sbrec_port_binding_by_name) > > > +{ > > > + ovs_be32 ip4; > > > + struct in6_addr ip; > > > + struct eth_addr mac; > > > + const struct sbrec_port_binding *pb; > > > + > > > + if (!get_local_datapath(local_datapaths, > > smb->datapath->tunnel_key)) { > > > + return; > > > + } > > > + > > > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, > > smb->logical_port); > > > + if (!pb) { > > > + return; > > > + } > > > + > > > + if (!eth_addr_from_string(smb->mac, &mac)) { > > > + return; > > > + } > > > + > > > + if (strchr(smb->ip, '.')) { > > > + if (!ip_parse(smb->ip, &ip4)) { > > > + return; > > > + } > > > + ip = in6_addr_mapped_ipv4(ip4); > > > + } else { > > > + if (!ipv6_parse(smb->ip, &ip)) { > > > + return; > > > + } > > > + } > > > > Not strictly related to this patch, but this feels ripe for a helper. > > > > Might be, but probably for follow up. Thanks, I agree a follow-up would be an appropriate way to handle this. > > ... > > > > > +static void > > > +en_recent_mac_bindings_run(struct engine_node *node, void *data) > > > +{ > > > + struct hmap *recent_mbs = data; > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + const struct sbrec_mac_binding_table *mac_binding_table = > > > + EN_OVSDB_GET(engine_get_input("SB_mac_binding", node)); > > > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > > > + engine_ovsdb_node_get_index( > > > + engine_get_input("SB_port_binding", node), > > > + "name"); > > > + > > > + const struct sbrec_mac_binding *smb; > > > > Maybe it's just me, but the following seems somewhat easier to read: > > > > struct hmap *recent_mbs = data; > > const struct sbrec_mac_binding *smb; > > struct ed_type_runtime_data *rt_data; > > const struct sbrec_mac_binding_table *mac_binding_table; > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > > > rt_data = engine_get_input_data("runtime_data", node); > > mac_binding_table = EN_OVSDB_GET(engine_get_input("SB_mac_binding", > > node)); > > sbrec_port_binding_by_name = > > engine_ovsdb_node_get_index(engine_get_input("SB_port_binding", > > node), "name"); > > > > Also this setup and call invocatoion of SBREC_MAC_BINDING_TABLE_FOR_EACH > > seems to be repeated several times. Perhaps some sort of helper macro > > could be considered. > > > > The current version keeps the style of other I-P nodes in ovn-controller. I > don't have > a strong preference, but it should be probably aligned in the whole file. Keeping the style of the file is fine. I think you can ignore my earlier comment on this. ... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
