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

> On Apr 5, 2016, at 1:13 PM, Darrell Ball <dlu998@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.

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

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

        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.

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

> +    /* 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".

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

-=-=-=-=-=-=-=-=-=-=-=-

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

Commit Message

Comments

Patch