Thanks for reviewing
I’m sending a V2
On 3/3/16, 1:55 PM, "dev on behalf of Guru Shetty" <dev-boun...@openvswitch.org
on behalf of g...@ovn.org> wrote:
>On 12 February 2016 at 17:12, Darrell Ball <dlu...@gmail.com> wrote:
>
>> Signed-off-by: Darrell Ball <db...@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.entez...@gmail.com
>> 胡靖飞 hujingfei...@msn.com
>> 张伟 zhang...@126.com
>> 张强 zhangqi...@meizu.com
>> +darrell ball dlu...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
