Phew, what a big patch! The approach looks good. I have some comments which
shouldn't be too hard to fix. Also it doesn't compile (likely due to a
rebasing issue). I didn't bother to figure out exactly why, I'm sure you will
make it compile next time you rebase -i the series.
Also note. For my convenience, I wrote my comments directly in the code.
Below is a diff of the patch containing my thoughts. If you would prefer a
more traditional email review I can convert it.
---
ofproto/ofproto.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
ofproto/ofproto.h | 2 ++
2 files changed, 52 insertions(+), 1 deletions(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7a741d5..3be8b37 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -825,6 +825,10 @@ ofproto_bundle_register(struct ofproto *ofproto, void *aux,
size_t i;
bool ok;
+ /* It seems like it would be more straightforward to just ignore
+ * bond_settings if n_slaves < 2. You could imagine the caller in the
+ * future may always provide bond_settings for simplicity. Doesn't really
+ * matter though, just a thought. */
assert((s->n_slaves > 1) == (s->bond != NULL));
bundle = ofproto_bundle_lookup(ofproto, aux);
@@ -890,6 +894,7 @@ ofproto_bundle_register(struct ofproto *ofproto, void *aux,
}
/* Get trunked VLANs. */
+ /* I think this should be s->trunks. */
trunks = s->vlan == -1 ? NULL : bundle->trunks;
if (!vlan_bitmap_equal(trunks, bundle->trunks)) {
free(bundle->trunks);
@@ -898,6 +903,10 @@ ofproto_bundle_register(struct ofproto *ofproto, void *aux,
}
/* LACP. */
+ /* You need to create bundle->lacp if it's NULL.
+ * This also seems a bit awkward to me. It might make sense to call
+ * lacp_slave_register during ofproto_bundle_add_port() and call
+ * lacp_create() beforehand. Not sure. */
if (s->lacp_slaves) {
for (i = 0; i < s->n_slaves; i++) {
uint32_t odp_port = ofp_port_to_odp_port(s->slaves[i]);
@@ -914,6 +923,12 @@ ofproto_bundle_register(struct ofproto *ofproto, void *aux,
/* Bonding. */
if (!list_is_short(&bundle->ports)) {
bundle->ofproto->has_bonded_bundles = true;
+ /* Bond_reconfigure is called incorrectly here. What we should do is
+ * create the bond if it's null, register all of the slaves, and then
+ * reconfigure. Again, I would be inclined to create the bond before
+ * registering any of the ports, call bond_slave_register() during
+ * ofproto_bundle_add_port(), and bond_reconfigure() when that process
+ * is done, destroying the bond if something went wrong. */
if (bundle->bond) {
if (bond_reconfigure(bundle->bond, s->bond)) {
ofproto->need_revalidate = true;
@@ -960,6 +975,10 @@ ofproto_bundle_destroy(struct ofbundle *bundle)
} else if (hmapx_find_and_delete(&m->srcs, bundle)
|| hmapx_find_and_delete(&m->dsts, bundle)) {
ofproto->need_revalidate = true;
+ } else {
+ /* Can this else branch be reached, or should we explicitly
+ * prevent it? */
+ NOT_REACHED();
}
}
}
@@ -1105,6 +1124,9 @@ ofproto_mirror_lookup(struct ofproto *ofproto, void *aux)
return NULL;
}
+/* This function isn't really mirror related. I would rename it
+ * ofproto_lookup_bundles(), and put it with its buddy
+ * ofproto_lookup_bundle(). */
static void
ofproto_mirror_lookup_bundles(struct ofproto *ofproto,
void **auxes, size_t n_auxes,
@@ -1136,6 +1158,13 @@ ofproto_mirror_register(struct ofproto *ofproto, void
*aux,
if (!mirror) {
int idx;
+ /* I don't think this patch is the correct place to address it, but why
+ * do we store mirrors in an array? Seems to me like a linked list or
+ * something would be a lot more straightforward. Was there some
+ * historical reason? I can see how the current implementation would
+ * be marginally more efficient during configuration.
+ *
+ * Just curious, shouldn't affect the patch. */
idx = ofproto_mirror_scan(ofproto);
if (idx < 0) {
VLOG_WARN("bridge %s: maximum of %d port mirrors reached, "
@@ -1166,6 +1195,10 @@ ofproto_mirror_register(struct ofproto *ofproto, void
*aux,
} else {
out = NULL;
}
+
+ /* Maybe a silly question, are you allowed to mirror into a non-bundled
+ * ofport? I suppose you would have to manually create a bundle if you
+ * wanted to do that (which is fine, just verifing). */
ofproto_mirror_lookup_bundles(ofproto, s->srcs, s->n_srcs, &srcs);
ofproto_mirror_lookup_bundles(ofproto, s->dsts, s->n_dsts, &dsts);
@@ -1173,6 +1206,8 @@ ofproto_mirror_register(struct ofproto *ofproto, void
*aux,
if (hmapx_equals(&srcs, &mirror->srcs)
&& hmapx_equals(&dsts, &mirror->dsts)
&& vlan_bitmap_equal(mirror->vlans, s->src_vlans)
+ /* Is it possible that out is null and mirror->out is non-null, but
+ * they have the same out_vlan? This if may miss that case. */
&& (out
? out == mirror->out
: s->out_vlan == mirror->out_vlan))
@@ -1213,6 +1248,8 @@ ofproto_mirror_register(struct ofproto *ofproto, void
*aux,
}
}
+ /* Do we need to say mirror->out = out somewhere in this function? */
+
ofproto->need_revalidate = true;
mac_learning_flush(ofproto->ml);
}
@@ -1263,7 +1300,7 @@ ofproto_set_flood_vlans(struct ofproto *ofproto, unsigned
long *flood_vlans)
}
}
-
+/* Extra newline. */
bool
ofproto_is_mirror_output_bundle(struct ofproto *ofproto, void *aux)
{
@@ -1958,6 +1995,7 @@ ofport_unregister(struct ofport *port)
bundle->bond = NULL;
}
}
+ /* Need to lacp_slave_unregister(). */
cfm_destroy(port->cfm);
port->cfm = NULL;
@@ -2451,6 +2489,12 @@ facet_account(struct ofproto *ofproto,
return;
}
NL_ATTR_FOR_EACH_UNSAFE (a, left, facet->actions, facet->actions_len) {
+ /* This may require a bit more thought now that we have the new
+ * autopath action. When using autopath, slave choices are stored in a
+ * register, and may continue through an arbitrarily complex flow table
+ * before finally being outputted, or dropped. This approach may be
+ * sufficient, or we may need to do something fancier. I'll put some
+ * thought into it. */
if (nl_attr_type(a) == ODP_ACTION_ATTR_OUTPUT) {
struct ofport *port = get_port(ofproto, nl_attr_get_u32(a));
if (port && port->bundle && port->bundle->bond) {
@@ -2748,6 +2792,7 @@ dst_is_duplicate(const struct dst_set *set, const struct
dst *test)
static bool
ofbundle_trunks_vlan(const struct ofbundle *bundle, uint16_t vlan)
{
+ /* Should this use vlan_bitmap_contains()? */
return (bundle->vlan < 0
&& (!bundle->trunks || bitmap_is_set(bundle->trunks, vlan)));
}
@@ -2795,6 +2840,7 @@ compose_dsts(struct action_xlate_ctx *ctx, uint16_t vlan,
static bool
vlan_is_mirrored(const struct ofmirror *m, int vlan)
{
+ /* Again, vlan_bitmap_contains? */
return !m->vlans || bitmap_is_set(m->vlans, vlan);
}
@@ -3518,6 +3564,9 @@ xlate_autopath(struct action_xlate_ctx *ctx,
uint16_t ofp_port = ntohl(naa->id);
struct ofport *port;
+ /* This isn't exactly the same as the previous behavior. If port is NULL,
+ * then ofp_port should be changed to ODPP_NONE. Otherwise, the logic
+ * looks correct. */
port = get_port(ctx->ofproto, ofp_port_to_odp_port(ofp_port));
if (port && port->bundle && port->bundle->bond) {
/* Autopath does not support VLAN hashing. */
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index cbdcd98..51c932c 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -178,6 +178,7 @@ struct ofproto_bundle_settings {
int vlan; /* VLAN if access port, -1 if trunk port. */
unsigned long *trunks; /* 4096-bit bitmap, NULL to trunk all VLANs. */
+ /* Comment should say it's a vlan_bitmap instead of 4096-bit bitmap. */
struct bond_settings *bond; /* Must be nonnull iff if n_slaves > 1. */
@@ -204,6 +205,7 @@ struct ofproto_mirror_settings {
/* VLANs of packets to select for mirroring. */
unsigned long *src_vlans; /* 4096-bit bitmap, NULL selects all VLANs. */
+ /* Same here about vlan_bitmap comment. */
/* Output (mutually exclusive). */
void *out_bundle; /* A registered ofbundle handle or NULL. */
--
1.7.4.4
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
