On Oct 18, 2011, at 1:23 PM, Ben Pfaff wrote:
I've dropped references to the minor, obviously correct issues you pointed out.
> I'd be tempted to use a "struct stp_port *" in place of stp_port_num.
> It might not be worth it. Up to you.
That does make the code cleaner, so I changed it.
> Some ofproto-dpif.c functions' names start with "stp_". I found this
> confusing since I expected them to be in stp.c; that's our usual
> convention. Would you mind renaming them, e.g. s/stp_run/run_stp/?
As we discussed in person, I think the correct solution is to move a lot of
those "stp_" functions into the STP library. That will require more
restructuring than I want to do for this commit, but I'll put that on my to-do
list after this series is committed.
> set_stp() always sets need_revalidate, even if nothing changes.
> That's more expensive than I'd like; it means that every change to the
> database causes all flows to be revalidated.
Good point. I've changed the behavior.
> I think that the ofpbuf_clear() in do_xlate_actions() is going to make
> sFlow really mad. We need to avoid clearing the action added by
> add_sflow_action(), or you could call add_sflow_action() again I
> guess.
Thanks for pointing that out. It's now calling add_sflow_action() right after
the clear call.
> You could use "!list_is_short(&port->ifaces)" instead of
> "list_size(&port->ifaces) != 1".
That allows a zero count, which also seemed problematic. I've changed the code
to list_is_singleton(), is that sufficient?
> "CONTAINER_OF(list_front(&port->ifaces), struct iface, port_elem);"
> appears twice, a helper might be nice.
Those are only done because STP doesn't work over bonds. I hope that's
addressed soon, so I'll leave it for now unless you feel passionately.
> When stp-port-num isn't set, and a port gets added or deleted, it
> seems like the STP port numbers of all the ports can change. Is this
> good enough?
Yeah, that doesn't seem great. I'm not sure how stable port numbers need to be
or whether people even need to set them manually. Let's leave it for now, and
I'll do some research, since I'm generally not happy with how port numbers are
handled.
> It is a little questionable to use a shash in br_refresh_stp_status()
> and port_refresh_stp_status(). These functions could each just keep
> two local arrays of 3 or 4 elements, respectively, one for the keys
> and one for the values, and avoid doing as much dynamic allocation as
> they do. But it may be a little easier to maintain this way, so maybe
> it is not worth changing.
I don't have a strong preference, so I changed it.
> The stp status will only be refreshed every 5 seconds. Is that good
> enough? (Do we need to refresh port state immediately upon state
> change?)
I used the database rate-limiting method used elsewhere that should make it
much more responsive.
> I think that br_refresh_stp_status() and port_refresh_stp_status()
> should clear the status column when STP is disabled for whatever
> reason. Otherwise, if STP is enabled and then disabled, then the last
> STP status will persist.
As we discussed in person, I think it's only a problem for
port_refresh_stp_status() if the port gets moved to a bond when it was
previously active. I now clear the status in that case.
> Could you choose some value other than 0x8100 for the STP priority
> example? It makes me think of 802.1Q even though it's completely
> unrelated.
Sure. I changed it to 0x7800.
> Please add new option groups to vswitch.xml instead of mixing the STP
> options with Other Features (in the Bridge table) or with LACP
> Configuration (in the Port table). It would be nice to add a little
> introduction about spanning tree to the new groups, too.
I'll clean that up before I commit.
> It would be good to document how other-config:forward-bpdu interacts
> with the STP options.
I'll do that before I commit.
> Instead of saying that the default priority is 0x8000, I'd be inclined
> to say 32768, since we don't suggest using hexadecimal elsewhere.
> Similarly, 128 instead of 0x80.
I'd prefer to leave them in hexadecimal, since that's how we display them in
the bridge/port ids. I couldn't find consensus on how to display priorities,
but hexadecimal seemed the most common. What do you think?
> Regarding Port's other-config:stp-enable, I think that STP cannot even
> be enabled explicitly for bond, internal, and mirror ports, but the
> description does not clearly say that.
I'll add that before I commit.
> I'm pretty sure we only refresh stats every 5 seconds, but the text
> under Bridge Status and Port Status says that it's once a second.
Whoops that was from an older version. Now that it's generally instantaneous,
I removed references to time.
Below, you'll find the incremental diff from the previous version. Let me know
if you want the commit as a whole.
Thanks for the review!
--Justin
>From a403bd44a535ae8ce61b62a5ce1dd27772375eaf Mon Sep 17 00:00:00 2001
From: Justin Pettit <jpet...@nicira.com>
Date: Wed, 19 Oct 2011 11:57:33 -0700
Subject: [PATCH] STP fixes based on BLP suggestions.
---
ofproto/ofproto-dpif.c | 70 +++++++++++++++++----------------------------
utilities/ovs-vsctl.8.in | 4 +-
vswitchd/bridge.c | 68 +++++++++++++++++++++++---------------------
vswitchd/vswitch.xml | 10 +++---
4 files changed, 69 insertions(+), 83 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 025cc5b..065acf0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -318,9 +318,9 @@ struct ofport_dpif {
uint32_t bond_stable_id; /* stable_id to use as bond slave, or 0. */
bool may_enable; /* May be enabled in bonds. */
- int stp_port_num; /* -1 indicates disabled. */
+ struct stp_port *stp_port; /* Spanning Tree Protocol, if any. */
enum stp_state stp_state; /* Always STP_DISABLED if STP not in use. */
- long long int stp_msec_in_state;
+ long long int stp_state_entered;
};
static struct ofport_dpif *
@@ -797,7 +797,7 @@ port_construct(struct ofport *port_)
port->cfm = NULL;
port->tag = tag_create_random();
port->may_enable = true;
- port->stp_port_num = -1;
+ port->stp_port = NULL;
port->stp_state = STP_DISABLED;
if (ofproto->sflow) {
@@ -967,6 +967,11 @@ set_stp(struct ofproto *ofproto_, const struct
ofproto_stp_settings *s)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+ /* Only revalidate flows if the configuration changed. */
+ if (!s != !ofproto->stp) {
+ ofproto->need_revalidate = true;
+ }
+
if (s) {
if (!ofproto->stp) {
ofproto->stp = stp_create(ofproto_->name, s->system_id,
@@ -983,7 +988,6 @@ set_stp(struct ofproto *ofproto_, const struct
ofproto_stp_settings *s)
stp_destroy(ofproto->stp);
ofproto->stp = NULL;
}
- ofproto->need_revalidate = true;
return 0;
}
@@ -1006,21 +1010,14 @@ get_stp_status(struct ofproto *ofproto_, struct
ofproto_stp_status *s)
}
static void
-stp_update_port_state(struct ofport_dpif *ofport)
+update_stp_port_state(struct ofport_dpif *ofport)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
enum stp_state state;
/* Figure out new state. */
- if (ofport->stp_port_num != -1) {
- struct stp_port *sp;
-
- sp = stp_get_port(ofproto->stp, ofport->stp_port_num);
- state = stp_port_get_state(sp);
- } else {
- /* STP is disabled on the port. */
- state = STP_DISABLED;
- }
+ state = ofport->stp_port ? stp_port_get_state(ofport->stp_port)
+ : STP_DISABLED;
/* Update state. */
if (ofport->stp_state != state) {
@@ -1041,7 +1038,7 @@ stp_update_port_state(struct ofport_dpif *ofport)
ofproto->need_revalidate = true;
ofport->stp_state = state;
- ofport->stp_msec_in_state = time_msec();
+ ofport->stp_state_entered = time_msec();
if (fwd_change) {
bundle_update(ofport->bundle);
@@ -1067,34 +1064,29 @@ set_stp_port(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
- struct stp_port *sp = NULL;
-
- if (ofport->stp_port_num != -1) {
- sp = stp_get_port(ofproto->stp, ofport->stp_port_num);
- }
+ struct stp_port *sp = ofport->stp_port;
if (!s || !s->enable) {
if (sp) {
- ofport->stp_port_num = -1;
+ ofport->stp_port = NULL;
stp_port_disable(sp);
}
return 0;
- } else if (ofport->stp_port_num != s->port_num
- && sp && ofport == stp_port_get_aux(sp)) {
+ } else if (sp && stp_port_no(sp) != s->port_num
+ && ofport == stp_port_get_aux(sp)) {
/* The port-id changed, so disable the old one if it's not
* already in use by another port. */
stp_port_disable(sp);
}
- ofport->stp_port_num = s->port_num;
- sp = stp_get_port(ofproto->stp, ofport->stp_port_num);
+ sp = ofport->stp_port = stp_get_port(ofproto->stp, s->port_num);
stp_port_enable(sp);
stp_port_set_aux(sp, ofport);
stp_port_set_priority(sp, s->priority);
stp_port_set_path_cost(sp, s->path_cost);
- stp_update_port_state(ofport);
+ update_stp_port_state(ofport);
return 0;
}
@@ -1105,19 +1097,17 @@ get_stp_port_status(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
- struct stp_port *sp;
+ struct stp_port *sp = ofport->stp_port;
- if (!ofproto->stp || ofport->stp_port_num == -1) {
+ if (!ofproto->stp || !sp) {
s->enabled = false;
return 0;
}
- sp = stp_get_port(ofproto->stp, ofport->stp_port_num);
-
s->enabled = true;
s->port_id = stp_port_get_id(sp);
s->state = stp_port_get_state(sp);
- s->sec_in_state = (time_msec() - ofport->stp_msec_in_state) / 1000;
+ s->sec_in_state = (time_msec() - ofport->stp_state_entered) / 1000;
s->role = stp_port_get_role(sp);
return 0;
@@ -1139,7 +1129,7 @@ stp_run(struct ofproto_dpif *ofproto)
struct ofport_dpif *ofport = stp_port_get_aux(sp);
if (ofport) {
- stp_update_port_state(ofport);
+ update_stp_port_state(ofport);
}
}
}
@@ -1161,21 +1151,15 @@ stp_should_process_flow(const struct flow *flow)
}
static void
-stp_process_packet(const struct ofproto_dpif *ofproto,
- const struct ofport_dpif *ofport,
+stp_process_packet(const struct ofport_dpif *ofport,
const struct ofpbuf *packet)
{
struct ofpbuf payload = *packet;
struct eth_header *eth = payload.data;
- struct stp_port *sp;
+ struct stp_port *sp = ofport->stp_port;
/* Sink packets on ports that have STP disabled when the bridge has
* STP enabled. */
- if (ofport->stp_port_num == -1) {
- return;
- }
-
- sp = stp_get_port(ofproto->stp, ofport->stp_port_num);
if (!sp || stp_port_get_state(sp) == STP_DISABLED) {
return;
}
@@ -2120,7 +2104,7 @@ process_special(struct ofproto_dpif *ofproto, const
struct flow *flow,
return true;
} else if (ofproto->stp && stp_should_process_flow(flow)) {
if (packet) {
- stp_process_packet(ofproto, ofport, packet);
+ stp_process_packet(ofport, packet);
}
return true;
}
@@ -4095,8 +4079,7 @@ xlate_learn_action(struct action_xlate_ctx *ctx,
static bool
may_receive(const struct ofport_dpif *port, struct action_xlate_ctx *ctx)
{
- if (port->up.opp.config & htonl(OFPPC_NO_RECV | OFPPC_NO_RECV_STP) &&
- port->up.opp.config & (eth_addr_equals(ctx->flow.dl_dst, eth_addr_stp)
+ if (port->up.opp.config & (eth_addr_equals(ctx->flow.dl_dst, eth_addr_stp)
? htonl(OFPPC_NO_RECV_STP)
: htonl(OFPPC_NO_RECV))) {
return false;
@@ -4282,6 +4265,7 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
* so drop it now if forwarding is disabled. */
if (port && !stp_forward_in_state(port->stp_state)) {
ofpbuf_clear(ctx->odp_actions);
+ add_sflow_action(ctx);
}
}
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index f2607ee..64255b5 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -835,9 +835,9 @@ Configure bridge \fBbr0\fR to participate in an 802.1D
spanning tree:
.IP
.B "ovs\-vsctl set Bridge br0 stp_enable=true"
.PP
-Set the bridge priority of \fBbr0\fR to 0x8100:
+Set the bridge priority of \fBbr0\fR to 0x7800:
.IP
-.B "ovs\-vsctl set Bridge br0 other_config:stp-priority=0x8100"
+.B "ovs\-vsctl set Bridge br0 other_config:stp-priority=0x7800"
.PP
Set the path cost of port \fBeth0\fR to 10:
.IP
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 8bf6eed..3a19235 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -725,7 +725,6 @@ bridge_configure_sflow(struct bridge *br, int
*sflow_bridge_number)
sset_destroy(&oso.targets);
}
-
static void
port_configure_stp(const struct ofproto *ofproto, struct port *port,
struct ofproto_port_stp_settings *port_s,
@@ -743,7 +742,7 @@ port_configure_stp(const struct ofproto *ofproto, struct
port *port,
}
/* STP over bonds is not supported. */
- if (list_size(&port->ifaces) != 1) {
+ if (!list_is_singleton(&port->ifaces)) {
VLOG_ERR("port %s: cannot enable STP on bonds, disabling",
port->name);
port_s->enable = false;
@@ -1566,9 +1565,8 @@ br_refresh_stp_status(struct bridge *br)
{
struct ofproto *ofproto = br->ofproto;
struct ofproto_stp_status status;
- struct shash sh;
- size_t n;
- char **keys, **values;
+ char *keys[3], *values[3];
+ size_t i;
if (ofproto_get_stp_status(ofproto, &status)) {
return;
@@ -1579,20 +1577,18 @@ br_refresh_stp_status(struct bridge *br)
return;
}
- shash_init(&sh);
- shash_add(&sh, "stp_bridge_id",
- xasprintf(STP_ID_FMT, STP_ID_ARGS(status.bridge_id)));
- shash_add(&sh, "stp_designated_root",
- xasprintf(STP_ID_FMT, STP_ID_ARGS(status.designated_root)));
- shash_add(&sh, "stp_root_path_cost",
- xasprintf("%d", status.root_path_cost));
+ keys[0] = "stp_bridge_id",
+ values[0] = xasprintf(STP_ID_FMT, STP_ID_ARGS(status.bridge_id));
+ keys[1] = "stp_designated_root",
+ values[1] = xasprintf(STP_ID_FMT, STP_ID_ARGS(status.designated_root));
+ keys[2] = "stp_root_path_cost",
+ values[2] = xasprintf("%d", status.root_path_cost);
- shash_to_ovs_idl_map(&sh, &keys, &values, &n);
- ovsrec_bridge_set_status(br->cfg, keys, values, n);
+ ovsrec_bridge_set_status(br->cfg, keys, values, ARRAY_SIZE(values));
- shash_destroy_free_data(&sh);
- free(keys);
- free(values);
+ for (i = 0; i < ARRAY_SIZE(values); i++) {
+ free(values[i]);
+ }
}
static void
@@ -1601,13 +1597,12 @@ port_refresh_stp_status(struct port *port)
struct ofproto *ofproto = port->bridge->ofproto;
struct iface *iface;
struct ofproto_port_stp_status status;
- struct shash sh;
- size_t n;
- char **keys, **values;
-
+ char *keys[4], *values[4];
+ size_t i;
/* STP doesn't currently support bonds. */
if (!list_is_singleton(&port->ifaces)) {
+ ovsrec_port_set_status(port->cfg, NULL, NULL, 0);
return;
}
@@ -1622,18 +1617,20 @@ port_refresh_stp_status(struct port *port)
return;
}
- shash_init(&sh);
- shash_add(&sh, "stp_port_id", xasprintf(STP_PORT_ID_FMT, status.port_id));
- shash_add(&sh, "stp_state", xstrdup(stp_state_name(status.state)));
- shash_add(&sh, "stp_sec_in_state", xasprintf("%u", status.sec_in_state));
- shash_add(&sh, "stp_role", xstrdup(stp_role_name(status.role)));
+ keys[0] = "stp_port_id";
+ values[0] = xasprintf(STP_PORT_ID_FMT, status.port_id);
+ keys[1] = "stp_state";
+ values[1] = xstrdup(stp_state_name(status.state));
+ keys[2] = "stp_sec_in_state";
+ values[2] = xasprintf("%u", status.sec_in_state);
+ keys[3] = "stp_role";
+ values[3] = xstrdup(stp_role_name(status.role));
- shash_to_ovs_idl_map(&sh, &keys, &values, &n);
- ovsrec_port_set_status(port->cfg, keys, values, n);
+ ovsrec_port_set_status(port->cfg, keys, values, ARRAY_SIZE(values));
- shash_destroy_free_data(&sh);
- free(keys);
- free(values);
+ for (i = 0; i < ARRAY_SIZE(values); i++) {
+ free(values[i]);
+ }
}
static bool
@@ -1823,11 +1820,9 @@ bridge_run(void)
HMAP_FOR_EACH (br, node, &all_bridges) {
struct port *port;
- br_refresh_stp_status(br);
HMAP_FOR_EACH (port, hmap_node, &br->ports) {
struct iface *iface;
- port_refresh_stp_status(port);
LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
iface_refresh_stats(iface);
iface_refresh_status(iface);
@@ -1849,6 +1844,13 @@ bridge_run(void)
txn = ovsdb_idl_txn_create(idl);
HMAP_FOR_EACH (br, node, &all_bridges) {
struct iface *iface;
+ struct port *port;
+
+ br_refresh_stp_status(br);
+
+ HMAP_FOR_EACH (port, hmap_node, &br->ports) {
+ port_refresh_stp_status(port);
+ }
HMAP_FOR_EACH (iface, name_node, &br->iface_by_name) {
const char *link_state;
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index c427d50..83dfa57 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -531,7 +531,7 @@
<group title="Bridge Status">
<p>
- Status information about bridges, updated every second.
+ Status information about bridges.
</p>
<column name="status">
Key-value pairs that report bridge status.
@@ -851,7 +851,7 @@
changed, all flows will be assigned different hash values possibly
causing slave selection decisions to change.
</column>
-
+
<column name="other_config" key="stp-enable"
type='{"type": "boolean"}'>
If spanning tree is enabled on the bridge, member ports are
@@ -936,8 +936,7 @@
<group title="Port Status">
<p>
- Status information about ports attached to bridges, updated every
- second.
+ Status information about ports attached to bridges.
</p>
<column name="status">
Key-value pairs that report port status.
@@ -958,7 +957,8 @@
STP state of the port.
</p>
</column>
- <column name="status" key="stp_sec_in_state">
+ <column name="status" key="stp_sec_in_state"
+ type='{"type": "integer", "minInteger": 0}'>
<p>
The amount of time (in seconds) port has been in the current
STP state.
--
1.7.1
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
