[ovs-dev] [PATCH 1/3] VTEP Schema - Support mcast macs remote table updates

Guru Shetty guru at ovn.org
Thu Mar 3 21:55:52 UTC 2016


On 12 February 2016 at 17:12, Darrell Ball <dlu998 at gmail.com> wrote:

> Signed-off-by: Darrell Ball <dball at 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 at gmail.com
>  胡靖飞                  hujingfei914 at msn.com
>  张伟                     zhangwqh at 126.com
>  张强                    zhangqiang at meizu.com
> +darrell ball            dlu998 at 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.



--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.


>  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.

>          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.


> +        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.



>          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?



>              /* 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?

vtep_macs_cleanup() needs comment update.

For the rest of the code, there is some CodingStyle violations. I think the
following is the usual practice.

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]);



More information about the dev mailing list