> On Apr 5, 2016, at 1:13 PM, Darrell Ball <dlu...@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
-=-=-=-=-=-=-=-=-=-=-=-
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);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
