On 6/11/21 1:44 PM, Vladislav Odintsov wrote: > Before this patch ovn-controller-vtep created Mcast_Macs_Remote > record for each Port Binding of the OVN Logical Switch, to which > VTEP Logical Switch was attached. > With this patch there is only one Mcast_Macs_Remote record per datapath. > Physical Locator Set is created every time when physical locators for > associated datapath are changed. Next, this newly-created physical > locator set is updated in the MMR record. > > Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> > ---
Thanks for this new revision! I think the changes are OK, I just have a few minor style comments. If you address them, feel free to add my ack in v4. Acked-by: Dumitru Ceara <dce...@redhat.com> > controller-vtep/vtep.c | 60 ++++++++++++++++++------------- > tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 105 insertions(+), 25 deletions(-) > > diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c > index f3b02f63f..49723b39d 100644 > --- a/controller-vtep/vtep.c > +++ b/controller-vtep/vtep.c > @@ -48,7 +48,7 @@ struct mmr_hash_node_data { > * database. > * > */ > - > + These are actually part of the coding guidelines [0], we should probably leave them as they are: "Use form feeds (control+L) to divide long source files into logical pieces. A form feed should appear as the only character on a line." [0] https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#basics > /* Searches the 'chassis_rec->encaps' for the first vtep tunnel > * configuration, returns the 'ip'. Unless duplicated, the returned > * pointer cannot live past current vtep_run() execution. */ > @@ -94,7 +94,7 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char > *chassis_ip) > > return new_pl; > } > - > + Same here. > /* Creates a new 'Mcast_Macs_Remote'. */ > static void > vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac, > @@ -104,6 +104,7 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const > char *mac, > struct vteprec_mcast_macs_remote *new_mmr = > vteprec_mcast_macs_remote_insert(vtep_idl_txn); > > + VLOG_DBG("Inserting MMR for LS '%s'", vtep_ls->name); > vteprec_mcast_macs_remote_set_MAC(new_mmr, mac); > vteprec_mcast_macs_remote_set_logical_switch(new_mmr, vtep_ls); > vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set); > @@ -140,7 +141,7 @@ vtep_process_pls(const struct ovs_list *locators_list, > ploc_entry->vteprec_ploc; > if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators, > locators[i]->dst_ip)) { > - locator_lists_differ = true; > + locator_lists_differ = true; > } > i++; > } > @@ -149,8 +150,8 @@ vtep_process_pls(const struct ovs_list *locators_list, > return locator_lists_differ; > } > > -/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up > - * out-dated remote mcast mac entries as needed. */ > +/* Creates a new 'Mcast_Macs_Remote' entry or modifies existing if needed > + * and also cleans up out-dated remote mcast mac entries as needed. */ > static void > vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, > struct ovs_list *locators_list, > @@ -162,22 +163,29 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, > bool mmr_changed; > > locators = xmalloc(n_locators_new * sizeof *locators); > - > mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators); > > - if (mmr_ext && !n_locators_new) { > - vteprec_mcast_macs_remote_delete(mmr_ext->mmr); > - } else if ((mmr_ext && mmr_changed) || > - (!mmr_ext && n_locators_new)) { > + if (mmr_changed) { > + if (n_locators_new) { > + const struct vteprec_physical_locator_set *ploc_set = > + vteprec_physical_locator_set_insert(vtep_idl_txn); > > - const struct vteprec_physical_locator_set *ploc_set = > - vteprec_physical_locator_set_insert(vtep_idl_txn); > + vteprec_physical_locator_set_set_locators(ploc_set, locators, > + n_locators_new); > > - vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set); > + if (!mmr_ext) { /* create new mmr */ > + vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, > + ploc_set); > + } else { /* update existing mmr */ > + vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr, > + ploc_set); > + } > > - vteprec_physical_locator_set_set_locators(ploc_set, locators, > - n_locators_new); > + } else if (mmr_ext) { /* remove old mmr */ > + vteprec_mcast_macs_remote_delete(mmr_ext->mmr); > + } > } > + > free(locators); > } > > @@ -353,17 +361,19 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, > struct shash *ucast_macs_rmts, > shash_add(&ls_node->physical_locators, chassis_ip, pl); > } > > - char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key); > - ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey); > + if (!ls_node->mmr_ext) { > + char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", > tnl_key); > + ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey); > > - if (ls_node->mmr_ext && > - ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) { > + if (ls_node->mmr_ext && > + ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) { > > - /* Delete the entry from the hash table so the mmr does not get > - * removed from the DB later on during stale checking. */ > - shash_find_and_delete(mcast_macs_rmts, mac_tnlkey); > + /* Delete the entry from the hash table so the mmr does not > get > + * removed from the DB later on during stale checking. */ > + shash_find_and_delete(mcast_macs_rmts, mac_tnlkey); > + } > + free(mac_tnlkey); > } > - free(mac_tnlkey); > > for (i = 0; i < port_binding_rec->n_mac; i++) { > const struct vteprec_ucast_macs_remote *umr; > @@ -481,7 +491,7 @@ vtep_mcast_macs_cleanup(struct ovsdb_idl *vtep_idl) > > return true; > } > - > + Form feed can stay. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev