| Message ID | CAM_3v9JSsP9ek3UrrEng7bqNV-YZ7G4AHT3-d8v_wA6RnN1cdA@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Thanks for reviewing I’m sending a V2 On 3/3/16, 1:55 PM, "dev on behalf of Guru Shetty" <dev-bounces@openvswitch.org on behalf of guru@ovn.org> wrote: >On 12 February 2016 at 17:12, Darrell Ball <dlu998@gmail.com> wrote: > >> Signed-off-by: Darrell Ball <dball@vmware.com> >> --- >> AUTHORS | 1 + >> ovn/controller-vtep/vtep.c | 207 >> ++++++++++++++++++++++++++++++++++++++++----- >> 2 files changed, 186 insertions(+), 22 deletions(-) >> >> diff --git a/AUTHORS b/AUTHORS >> index 5c3643a..b709482 100644 >> --- a/AUTHORS >> +++ b/AUTHORS >> @@ -417,6 +417,7 @@ rahim entezari rahim.entezari@gmail.com >> 胡靖飞 hujingfei914@msn.com >> 张伟 zhangwqh@126.com >> 张强 zhangqiang@meizu.com >> +darrell ball dlu998@gmail.com > > >I think you have added your name to the wring list. The list is also >alphabetically ordered. So please add it at the right place. Thanks; Russell pointed this out as well > > > >--snip-- > >> >> >> + } >> + free(locators); >> +} >> + >> /* Updates the vtep Logical_Switch table entries' tunnel keys based >> * on the port bindings. */ >> > >You will have to update the comment above. Done > > >> static void >> @@ -153,12 +255,15 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset >> *vtep_pswitches, >> * bindings. */ >> > > > > >> static void >> vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash >> *ucast_macs_rmts, >> - struct shash *physical_locators, struct shash >> *vtep_lswitches, >> - struct shash *non_vtep_pbs) >> + struct shash *mcast_macs_rmts, struct shash >> *physical_locators, >> + struct shash *vtep_lswitches, struct shash *non_vtep_pbs) >> { >> struct shash_node *node; >> struct hmap ls_map; >> >> + struct vtep_rec_physical_locator_list_entry *ploc_entry; >> + const struct vteprec_physical_locator *pl; >> + >> /* Maps from ovn logical datapath tunnel key (which is also the vtep >> * logical switch tunnel key) to the corresponding vtep logical switch >> * instance. Also, the shash map 'added_macs' is used for checking >> @@ -168,6 +273,9 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >> struct shash *ucast_macs_rmts, >> >> >The structure has some comments about added_macs above. Consider adding for >the new ones too. Sure; I added comments > >> const struct vteprec_logical_switch *vtep_ls; >> struct shash added_macs; >> + >> + struct ovs_list locators_list; >> + struct mmr_hash_node_data *mmr_ext; >> }; >> >> hmap_init(&ls_map); >> @@ -181,6 +289,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >> struct shash *ucast_macs_rmts, >> ls_node = xmalloc(sizeof *ls_node); >> ls_node->vtep_ls = vtep_ls; >> shash_init(&ls_node->added_macs); >> + list_init(&ls_node->locators_list); >> + ls_node->mmr_ext = NULL; >> hmap_insert(&ls_map, &ls_node->hmap_node, >> hash_uint64((uint64_t) vtep_ls->tunnel_key[0])); >> } >> @@ -222,18 +332,31 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >> struct shash *ucast_macs_rmts, >> continue; >> } >> >> + pl = shash_find_data(physical_locators, chassis_ip); >> + if(!pl){ >> + pl = create_pl(vtep_idl_txn, chassis_ip); >> + shash_add(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); >> > > >Please add a comment below on why we delete the shash node. There is a >similar comment for >shash_find_and_delete(ucast_macs_rmts, mac_ip_tnlkey) and I think it is >useful while reading code. I had already added a comment regarding cleaning up stale MMRs for the hash table; I can duplicate it here to make it more obvious > > >> + if(ls_node->mmr_ext && >> + ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) { >> + shash_find_and_delete(mcast_macs_rmts, mac_tnlkey); >> + } >> + free(mac_tnlkey); >> + ploc_entry = xmalloc(sizeof *ploc_entry); >> + ploc_entry->vteprec_ploc = pl; >> + list_push_back(&ls_node->locators_list, >> + &ploc_entry->locators_node); >> + >> >Are we inserting duplicate physical_locators above to the list? Is that >Okay? It looked like update_mmr was adding a record for even the duplicate >ones. Yes, its needs a check; back in Dec, I had some some misconceptions regarding OVN; thanks > > > >> for (i = 0; i < port_binding_rec->n_mac; i++) { >> const struct vteprec_ucast_macs_remote *umr; >> - const struct vteprec_physical_locator *pl; >> const struct sbrec_port_binding *conflict; >> char *mac = port_binding_rec->mac[i]; >> >> - /* xxx Need to address this later when we support >> - * update of 'Mcast_Macs_Remote' table in VTEP. */ >> - if (!strcmp(mac, "unknown")) { >> - continue; >> - } >> - >> >The above piece of code that is being removed is for cases wherein we don't >know the mac addresses of logical ports that reside in a hypervisor. Can >they still be pinged via VTEP emulator? I wonder whether VTEP emulator, >since it does not know this mac address, handle the case properly? The name was changed to unknown-dst in the vtep schema and its covered by this patch > > > >> /* Checks for duplicate MAC in the same vtep logical switch. >> */ >> conflict = shash_find_data(&ls_node->added_macs, mac); >> if (conflict) { >> @@ -257,19 +380,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >> struct shash *ucast_macs_rmts, >> shash_find_and_delete(ucast_macs_rmts, mac_ip_tnlkey); >> } else { >> const struct vteprec_ucast_macs_remote *new_umr; >> - >> new_umr = create_umr(vtep_idl_txn, mac, ls_node->vtep_ls); >> - pl = shash_find_data(physical_locators, chassis_ip); >> - if (pl) { >> - vteprec_ucast_macs_remote_set_locator(new_umr, pl); >> - } else { >> - const struct vteprec_physical_locator *new_pl; >> - >> - new_pl = create_pl(vtep_idl_txn, chassis_ip); >> - vteprec_ucast_macs_remote_set_locator(new_umr, >> new_pl); >> - /* Updates the 'physical_locators'. */ >> - shash_add(physical_locators, chassis_ip, new_pl); >> - } >> + vteprec_ucast_macs_remote_set_locator(new_umr, pl); >> } >> free(mac_ip_tnlkey); >> } >> @@ -281,11 +393,25 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >> struct shash *ucast_macs_rmts, >> } >> struct ls_hash_node *iter, *next; >> HMAP_FOR_EACH_SAFE (iter, next, hmap_node, &ls_map) { >> + vtep_update_mmr(vtep_idl_txn,&iter->locators_list, >> + iter->vtep_ls, iter->mmr_ext); >> + LIST_FOR_EACH_POP(ploc_entry,locators_node, >> + &iter->locators_list) { >> + free(ploc_entry); >> + } >> hmap_remove(&ls_map, &iter->hmap_node); >> shash_destroy(&iter->added_macs); >> free(iter); >> } >> hmap_destroy(&ls_map); >> + /* Clean stale MMRs */ >> + struct mmr_hash_node_data *mmr_ext; >> + SHASH_FOR_EACH (node, mcast_macs_rmts) { >> + mmr_ext = node->data; >> + shash_destroy(&mmr_ext->physical_locators); >> + vteprec_mcast_macs_remote_delete(mmr_ext->mmr); >> + free(mmr_ext); >> + } >> > >The above looks to do a proper cleanup of mmr_ext that still exists in >shash. It looks like for the ones that were deleted from shash with >'shash_find_and_delete()', its memory is not cleaned up, right? The new code using shash_find_and_delete() for mcast_macs_rmts is correct. I added a big comment there. However, I did review the patch again and found the cleanup above is not exactly correct. This cleanup above was meant only for the VTEP DB entries; the physical_locators hash and mmr_ext cleanups belong at the end of vtep_run(). I split out the code accordingly. > >vtep_macs_cleanup() needs comment update. updated > >For the rest of the code, there is some CodingStyle violations. I think the >following is the usual practice. Thanks; hopefully I updated most of these space and parentheses issues > >iff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c >index 04251c4..29c4192 100644 >--- a/ovn/controller-vtep/vtep.c >+++ b/ovn/controller-vtep/vtep.c >@@ -30,7 +30,7 @@ > > VLOG_DEFINE_THIS_MODULE(vtep); > >-struct vtep_rec_physical_locator_list_entry{ >+struct vtep_rec_physical_locator_list_entry { > struct ovs_list locators_node; > const struct vteprec_physical_locator *vteprec_ploc; > }; >@@ -106,17 +106,16 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, > > 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); >+ vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set); > } > > /* Compares prev and new mmr locator sets and return true if they > * differ; false otherwise; also preps new locator set > * for database write */ > static bool >-vtep_process_pls( >- const struct ovs_list *locators_list, >- const struct mmr_hash_node_data *mmr_ext, >- struct vteprec_physical_locator **locators) >+vtep_process_pls(const struct ovs_list *locators_list, >+ const struct mmr_hash_node_data *mmr_ext, >+ struct vteprec_physical_locator **locators) > { > int i; > size_t n_locators_prev = 0; >@@ -125,22 +124,22 @@ vtep_process_pls( > const struct vteprec_physical_locator *pl = NULL; > bool prev_and_new_locator_lists_differ = false; > >- if(mmr_ext) { >+ if (mmr_ext) { > n_locators_prev = mmr_ext->mmr->locator_set->n_locators; > } >- if(n_locators_prev != n_locators_new) { >+ if (n_locators_prev != n_locators_new) { > prev_and_new_locator_lists_differ = true; > } > >- if(n_locators_new) { >+ if (n_locators_new) { > i = 0; > LIST_FOR_EACH (ploc_entry, locators_node, locators_list) { > locators[i] = (struct vteprec_physical_locator *) > ploc_entry->vteprec_ploc; >- if(mmr_ext) { >+ if (mmr_ext) { > pl = shash_find_data(&mmr_ext->physical_locators, > locators[i]->dst_ip); >- if(!pl) { >+ if (!pl) { > prev_and_new_locator_lists_differ = true; > } > } >@@ -148,7 +147,7 @@ vtep_process_pls( > } > } > >- return(prev_and_new_locator_lists_differ); >+ return prev_and_new_locator_lists_differ; > } > > /* Creates a new 'Mcast_Macs_Remote' entry if needed. >@@ -159,21 +158,20 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, > const struct vteprec_logical_switch *vtep_ls, > const struct mmr_hash_node_data *mmr_ext) > { >- struct vteprec_physical_locator **locators = NULL; > size_t n_locators_new = list_size(locators_list); > bool mmr_changed = false; > const struct vteprec_physical_locator_set *ploc_set; > >- locators = xmalloc(n_locators_new * sizeof(*locators)); >+ locators = xmalloc(n_locators_new * sizeof *locators); > >- if(vtep_process_pls(locators_list, mmr_ext, locators)) { >+ if (vtep_process_pls(locators_list, mmr_ext, locators)) { > mmr_changed = true; > } > >- if(mmr_ext && (!n_locators_new)) { >+ 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)) { >+ (!mmr_ext && n_locators_new)) { > > ploc_set = vteprec_physical_locator_set_insert(vtep_idl_txn); > >@@ -181,7 +179,6 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, > > vteprec_physical_locator_set_set_locators(ploc_set, locators, > n_locators_new); >- > } > free(locators); > } >@@ -333,7 +330,7 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >struct shash *ucast_macs_rmts, > } > > pl = shash_find_data(physical_locators, chassis_ip); >- if(!pl){ >+ if (!pl) { > pl = create_pl(vtep_idl_txn, chassis_ip); > shash_add(physical_locators, chassis_ip, pl); > } >@@ -342,11 +339,13 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >struct shash *ucast_macs_rmts, > 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) { > shash_find_and_delete(mcast_macs_rmts, mac_tnlkey); > } > free(mac_tnlkey); >+ > ploc_entry = xmalloc(sizeof *ploc_entry); > ploc_entry->vteprec_ploc = pl; > list_push_back(&ls_node->locators_list, >@@ -391,11 +390,12 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, >struct shash *ucast_macs_rmts, > SHASH_FOR_EACH (node, ucast_macs_rmts) { > vteprec_ucast_macs_remote_delete(node->data); > } >+ > struct ls_hash_node *iter, *next; > HMAP_FOR_EACH_SAFE (iter, next, hmap_node, &ls_map) { >- vtep_update_mmr(vtep_idl_txn,&iter->locators_list, >+ vtep_update_mmr(vtep_idl_txn, &iter->locators_list, > iter->vtep_ls, iter->mmr_ext); >- LIST_FOR_EACH_POP(ploc_entry,locators_node, >+ LIST_FOR_EACH_POP(ploc_entry, locators_node, > &iter->locators_list) { > free(ploc_entry); > } >@@ -513,7 +513,7 @@ vtep_run(struct controller_vtep_ctx *ctx) > locators_list = locator_set->locators; > > shash_init(&mmr_ext->physical_locators); >- for(i = 0; i < n_locators; i++) { >+ for (i = 0; i < n_locators; i++) { > shash_add(&mmr_ext->physical_locators, > locators_list[i]->dst_ip, > locators_list[i]); >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev
--- a/ovn/controller-vtep/vtep.c +++ b/ovn/controller-vtep/vtep.c @@ -30,7 +30,7 @@ VLOG_DEFINE_THIS_MODULE(vtep); -struct vtep_rec_physical_locator_list_entry{ +struct vtep_rec_physical_locator_list_entry { struct ovs_list locators_node; const struct vteprec_physical_locator *vteprec_ploc; }; @@ -106,17 +106,16 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, 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); + vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set); } /* Compares prev and new mmr locator sets and return true if they * differ; false otherwise; also preps new locator set * for database write */ static bool -vtep_process_pls( - const struct ovs_list *locators_list, - const struct mmr_hash_node_data *mmr_ext, - struct vteprec_physical_locator **locators) +vtep_process_pls(const struct ovs_list *locators_list, + const struct mmr_hash_node_data *mmr_ext, + struct vteprec_physical_locator **locators) { int i; size_t n_locators_prev = 0; @@ -125,22 +124,22 @@ vtep_process_pls( const struct vteprec_physical_locator *pl = NULL; bool prev_and_new_locator_lists_differ = false; - if(mmr_ext) { + if (mmr_ext) { n_locators_prev = mmr_ext->mmr->locator_set->n_locators; } - if(n_locators_prev != n_locators_new) { + if (n_locators_prev != n_locators_new) { prev_and_new_locator_lists_differ = true; } - if(n_locators_new) { + if (n_locators_new) { i = 0; LIST_FOR_EACH (ploc_entry, locators_node, locators_list) { locators[i] = (struct vteprec_physical_locator *) ploc_entry->vteprec_ploc; - if(mmr_ext) { + if (mmr_ext) { pl = shash_find_data(&mmr_ext->physical_locators, locators[i]->dst_ip); - if(!pl) { + if (!pl) { prev_and_new_locator_lists_differ = true; } } @@ -148,7 +147,7 @@ vtep_process_pls( } } - return(prev_and_new_locator_lists_differ); + return prev_and_new_locator_lists_differ; } /* Creates a new 'Mcast_Macs_Remote' entry if needed. @@ -159,21 +158,20 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const struct vteprec_logical_switch *vtep_ls, const struct mmr_hash_node_data *mmr_ext) { - struct vteprec_physical_locator **locators = NULL; size_t n_locators_new = list_size(locators_list); bool mmr_changed = false; const struct vteprec_physical_locator_set *ploc_set; - locators = xmalloc(n_locators_new * sizeof(*locators)); + locators = xmalloc(n_locators_new * sizeof *locators); - if(vtep_process_pls(locators_list, mmr_ext, locators)) { + if (vtep_process_pls(locators_list, mmr_ext, locators)) { mmr_changed = true; } - if(mmr_ext && (!n_locators_new)) { + 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)) { + (!mmr_ext && n_locators_new)) { ploc_set = vteprec_physical_locator_set_insert(vtep_idl_txn); @@ -181,7 +179,6 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, vteprec_physical_locator_set_set_locators(ploc_set, locators, n_locators_new); - } free(locators); } @@ -333,7 +330,7 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash *ucast_macs_rmts, } pl = shash_find_data(physical_locators, chassis_ip); - if(!pl){ + if (!pl) { pl = create_pl(vtep_idl_txn, chassis_ip); shash_add(physical_locators, chassis_ip, pl);