On Mon, Oct 17, 2011 at 10:31:49PM -0700, Justin Pettit wrote:
> Add 802.1D spanning tree protocol support to ovs-vswitchd. Still needs
> improvements in the following areas:
>
> * Does not work over bonds.
> * Does not expire entries generated by the learning action when STP
> state changes occur.
> * Needs more interoperability testing. Only tested against the
> Linux bridge.
"git am" says:
Applying: ovs-vswitchd: Add support for 802.1D STP.
/home/blp/db/.git/rebase-apply/patch:1223: trailing whitespace.
warning: 1 line adds whitespace errors.
ofproto-dpif.c
--------------
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.
stp_msec_in_state is IMO misnamed, since it's actually the time at
which the state was entered. "stp_state_entered" maybe?
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/?
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.
This code here is a totally weird micro-optimization that I must have
come up with years ago. You can drop the clause before the &&:
+ 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)
+ ? htonl(OFPPC_NO_RECV_STP)
+ : htonl(OFPPC_NO_RECV))) {
+ return false;
+ }
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.
bridge.c
--------
There's a double blank line above port_configure_stp().
You could use "!list_is_short(&port->ifaces)" instead of
"list_size(&port->ifaces) != 1".
"CONTAINER_OF(list_front(&port->ifaces), struct iface, port_elem);"
appears twice, a helper might be nice.
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?
port_refresh_stp_status() contains a double blank line.
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.
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 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.
ovs-vsctl.8.in
--------------
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.
vswitch.xml
-----------
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.
It would be good to document how other-config:forward-bpdu interacts
with the STP options.
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.
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'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.
I think that stp_sec_in_state could have type='{"type": "integer"}'.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
