Hi Dumitru, Thanks for the review, new patch is submitted to the list.
Regards, Vladislav Odintsov > On 15 Jun 2021, at 18:50, Dumitru Ceara <[email protected]> wrote: > > 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 <[email protected] >> <mailto:[email protected]>> >> --- > > 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 <[email protected] <mailto:[email protected]>> > >> 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 > > <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 > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
