[ovs-dev] [PATCH] Patch v3: ovn-controller-vtep: Support BUM traffic for the VTEP Schema

Darrell Ball dlu998 at gmail.com
Tue Apr 26 02:39:34 UTC 2016


Thanks for the review;
The suggestions are fine;
I added a couple comments inline.

Darrell

On Mon, Apr 25, 2016 at 3:44 PM, Justin Pettit <jpettit at ovn.org> wrote:

>
> > On Apr 5, 2016, at 1:13 PM, Darrell Ball <dlu998 at gmail.com> wrote:
> >
> > This patch implements BUM support in the VTEP schema.  This relates to
> BUM
> > traffic flowing from a gateway towards HVs.  This code would be relevant
> to HW
> > gateways and the ovs-vtep simulator. In order to do this, the mcast macs
> remote
> > table in the VTEP schema is populated based on the OVN SB port binding.
> For
> > each logical switch, the SB port bindings are queried to find all the
> physical
> > locators to send BUM traffic to and the VTEP DB is updated.
> >
> > Some test packets were enabled in the HW gateway test case to exercise
> the
> > new code.
>
> I told you to limit the line length to 80 columns, but when you run "git
> log", it indents the message by four spaces, so it looks like better advice
> would be 75 or 76 columns.
>

acked



>
> The name of the patch shouldn't have "Patch v3:" be part of the title.
>

acked




>
> > diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
> > index 016c2e0..aa988c0 100644
> > --- a/ovn/controller-vtep/vtep.c
> > +++ b/ovn/controller-vtep/vtep.c
> > @@ -27,9 +27,20 @@
> > #include "openvswitch/vlog.h"
> > #include "ovn/lib/ovn-sb-idl.h"
> > #include "vtep/vtep-idl.h"
> > +#include "openvswitch/util.h"
>
> Is this additional #include needed?  It seems like builds fine without it.
>
> > +    if (n_locators_new) {
> > +        int i = 0;
> > +        struct vtep_rec_physical_locator_list_entry *ploc_entry;
> > +        const struct vteprec_physical_locator *pl;
> > +        LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
> > +            locators[i] = (struct vteprec_physical_locator *)
> > +                           ploc_entry->vteprec_ploc;
> > +            if(mmr_ext) {
> > +                pl = shash_find_data(&mmr_ext->physical_locators,
> > +                                     locators[i]->dst_ip);
> > +                if (!pl) {
> > +                    locator_lists_differ = true;
> > +                }
>
> I think the code can be slightly simplified since "pl" is never used other
> than to see if's not null like so:
>

Its not as ugly compressed as I had originally thought :-)



>
>         LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
>             locators[i] = (struct vteprec_physical_locator *)
>                            ploc_entry->vteprec_ploc;
>             if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
>                                             locators[i]->dst_ip)) {
>                     locator_lists_differ = true;
>             }
>
> > +/* Creates a new 'Mcast_Macs_Remote' entry. 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,
> > +                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 = ovs_list_size(locators_list);
> > +    bool mmr_changed = false;
> > +
> > +    locators = xmalloc(n_locators_new * sizeof *locators);
> > +
> > +    if (vtep_process_pls(locators_list, mmr_ext, locators)) {
> > +        mmr_changed = true;
> > +    }
>
> I don't think you need to initialize "mmr_changed" or have this "if"
> block, since vtep_process_pls() always returns true or false.
>

thanks for catching this one



>
> > -/* Updates the vtep 'Ucast_Macs_Remote' table based on non-vtep port
> > - * bindings. */
> > +/* Updates the vtep 'Ucast_Macs_Remote' and 'Mcast_Macs_Remote' tables
> based
> > + * on non-vtep port bindings. */
> > static void
> > vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash
> *ucast_macs_rmts,
> > -              struct shash *physical_locators, struct shash *vt  We
> sep_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;
> > +    const struct vteprec_physical_locator *pl;
>
> I believe this can be moved to a more speciifc code block.
>

yes, it slipped though the cracks


>
> > +    /* Collects 'Mcast_Macs_Remote's. */
> > +    VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, ctx->vtep_idl) {
> > +        struct mmr_hash_node_data *mmr_ext = xmalloc(sizeof *mmr_ext);;
> > +        char *mac_tnlkey =
> > +            xasprintf("%s_%"PRId64, mmr->MAC,
> > +                      mmr->logical_switch &&
> mmr->logical_switch->n_tunnel_key
> > +                          ? mmr->logical_switch->tunnel_key[0] :
> INT64_MAX);
> > +
> > +        shash_add(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
> > +        mmr_ext->mmr = mmr;
> > +
> > +        shash_init(&mmr_ext->physical_locators);
> > +        for (size_t i = 0; i < mmr_ext->mmr->locator_set->n_locators;
> i++) {
> > +            shash_add(&mmr_ext->physical_locators,
> > +                      mmr_ext->mmr->locator_set->locators[i]->dst_ip,
> > +                      mmr_ext->mmr->locator_set->locators[i]);
>
> Minor, but I think it's clearer if you just access these through "mmr"
> rather than "mmr_ext->mmr".
>

yes, reduce one indirection



>
> I made a few other minor style changes.  If you agree with all the changes
> at the end of this message, I'll go ahead and apply the patch with those
> changes.
>
> Thanks,
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
>
> diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
> index aa988c0..9e11e28 100644
> --- a/ovn/controller-vtep/vtep.c
> +++ b/ovn/controller-vtep/vtep.c
> @@ -27,7 +27,6 @@
>  #include "openvswitch/vlog.h"
>  #include "ovn/lib/ovn-sb-idl.h"
>  #include "vtep/vtep-idl.h"
> -#include "openvswitch/util.h"
>
>  VLOG_DEFINE_THIS_MODULE(vtep);
>
> @@ -93,9 +92,8 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char
> *chas
>
>      return new_pl;
>  }
> -
>  ^L
> -/* Creates a new 'Mcast_Macs_Remote' entry. */
> +/* Creates a new 'Mcast_Macs_Remote'. */
>  static void
>  vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
>                  const struct vteprec_logical_switch *vtep_ls,
> @@ -109,14 +107,13 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
> const
>      vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
>  }
>
> -/* Compares prev and new mmr locator sets and return true if they differ
> and
> - * false otherwise. This function also preps new locator set for database
> - * write.
> +/* Compares previous and new mmr locator sets and returns true if they
> + * differ and false otherwise. This function also preps a new locator
> + * set for database write.
>   *
> - * locators_list is the new set of locators for the associated
> - * 'Mcast_Macs_Remote' entry passed in and is queried to generate the new
> set
> - * of locators in vtep database format.
> - */
> + * 'locators_list' is the new set of locators for the associated
> + * 'Mcast_Macs_Remote' entry passed in and is queried to generate the
> + * new set of locators in vtep database format. */
>  static bool
>  vtep_process_pls(const struct ovs_list *locators_list,
>                   const struct mmr_hash_node_data *mmr_ext,
> @@ -136,16 +133,12 @@ vtep_process_pls(const struct ovs_list
> *locators_list,
>      if (n_locators_new) {
>          int i = 0;
>          struct vtep_rec_physical_locator_list_entry *ploc_entry;
> -        const struct vteprec_physical_locator *pl;
>          LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
>              locators[i] = (struct vteprec_physical_locator *)
>                             ploc_entry->vteprec_ploc;
> -            if(mmr_ext) {
> -                pl = shash_find_data(&mmr_ext->physical_locators,
> -                                     locators[i]->dst_ip);
> -                if (!pl) {
> +            if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
> +                                            locators[i]->dst_ip)) {
>                      locator_lists_differ = true;
> -                }
>              }
>              i++;
>          }
> @@ -154,7 +147,7 @@ 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
> +/* Creates a new 'Mcast_Macs_Remote' entry 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,
> @@ -164,13 +157,11 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>  {
>      struct vteprec_physical_locator **locators = NULL;
>      size_t n_locators_new = ovs_list_size(locators_list);
> -    bool mmr_changed = false;
> +    bool mmr_changed;
>
>      locators = xmalloc(n_locators_new * sizeof *locators);
>
> -    if (vtep_process_pls(locators_list, mmr_ext, locators)) {
> -        mmr_changed = true;
> -    }
> +    mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
>
>      if (mmr_ext && !n_locators_new) {
>          vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
> @@ -262,7 +253,6 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct sha
>  {
>      struct shash_node *node;
>      struct hmap ls_map;
> -    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
> @@ -338,11 +328,13 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct s
>              continue;
>          }
>
> -        pl = shash_find_data(physical_locators, chassis_ip);
> +        const struct vteprec_physical_locator *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);
>          }
> +
>          const struct vteprec_physical_locator *ls_pl =
>              shash_find_data(&ls_node->physical_locators, chassis_ip);
>          if (!ls_pl) {
> @@ -420,7 +412,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct sha
>          free(iter);
>      }
>      hmap_destroy(&ls_map);
> -    /* Clean stale 'mcast_macs_remote' */
> +
> +    /* Clean stale 'Mcast_Macs_Remote' */
>      struct mmr_hash_node_data *mmr_ext;
>      SHASH_FOR_EACH (node, mcast_macs_rmts) {
>          mmr_ext = node->data;
> @@ -531,10 +524,10 @@ vtep_run(struct controller_vtep_ctx *ctx)
>          mmr_ext->mmr = mmr;
>
>          shash_init(&mmr_ext->physical_locators);
> -        for (size_t i = 0; i < mmr_ext->mmr->locator_set->n_locators;
> i++) {
> +        for (size_t i = 0; i < mmr->locator_set->n_locators; i++) {
>              shash_add(&mmr_ext->physical_locators,
> -                      mmr_ext->mmr->locator_set->locators[i]->dst_ip,
> -                      mmr_ext->mmr->locator_set->locators[i]);
> +                      mmr->locator_set->locators[i]->dst_ip,
> +                      mmr->locator_set->locators[i]);
>          }
>
>          free(mac_tnlkey);
>
>
>



More information about the dev mailing list