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? > > > 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. > > ... > > > +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. > > > + SBREC_MAC_BINDING_TABLE_FOR_EACH (smb, mac_binding_table) { > > + recent_mac_bindings_add(recent_mbs, smb, > &rt_data->local_datapaths, > > + sbrec_port_binding_by_name); > > + } > > +} > > > +static bool > > +recent_mac_bindings_mac_binding_handler(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; > > + SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, mac_binding_table) { > > + /* The deleted entry will expire eventually. */ > > + if (sbrec_mac_binding_is_deleted(smb)) { > > + continue; > > + } > > + > > + recent_mac_bindings_add(recent_mbs, smb, > &rt_data->local_datapaths, > > + sbrec_port_binding_by_name); > > + } > > + > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > +static bool > > +recent_mac_bindings_runtime_data_handler(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); > > + struct ovsdb_idl_index *smb_by_datapath = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_mac_binding", node), > > + "datapath"); > > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_port_binding", node), > > + "name"); > > + > > + if (!rt_data->tracked) { > > + return false; > > + } > > + > > + struct tracked_datapath *tdp; > > + HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) { > > + if (tdp->tracked_type != TRACKED_RESOURCE_NEW) { > > + continue; > > + } > > + > > + struct sbrec_mac_binding *mb_index_row = > > + sbrec_mac_binding_index_init_row(smb_by_datapath); > > + sbrec_mac_binding_index_set_datapath(mb_index_row, tdp->dp); > > + > > + const struct sbrec_mac_binding *smb; > > + SBREC_MAC_BINDING_FOR_EACH_EQUAL (smb, mb_index_row, > smb_by_datapath) { > > + recent_mac_bindings_add(recent_mbs, smb, > &rt_data->local_datapaths, > > + sbrec_port_binding_by_name); > > + } > > + > > + sbrec_mac_binding_index_destroy_row(mb_index_row); > > + } > > + > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > static void * > > en_controller_output_init(struct engine_node *node OVS_UNUSED, > > struct engine_arg *arg OVS_UNUSED) > > ... > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
