I looked and applied the patches. They’re good to me, I just have some
notes on patch 13/18 and 16/18.
@@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void
> *bpdu, size_t bpdu_size)
> memcpy(&p->received_bpdu_buffer, bpdu, sizeof(struct rstp_bpdu));
> rstp->changes = true;
> - move_rstp(rstp);
> + move_rstp__(rstp);
> } else {
> - VLOG_DBG("%s, port %u: Bad BPDU received", p->rstp->name,
> + VLOG_DBG("%s, port %u: Bad RSTP BPDU received", p->rstp->name,
> p->port_number);
> p->error_count++;
> }
The received BPDU could also be a STP BPDU.
/* Each RSTP port poits back to struct rstp without holding a
> + * reference for that pointer. This is OK as we never move
> + * ports from one bridge to another, and holders always
> + * release their ports before releasing the bridge. This
> + * means that there should be not ports at this time. */
> + ovs_assert(rstp->ports_count == 0);
Each RSTP port points back
+ rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
> + rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
> + rstp_set_bridge_forward_delay__(rstp,
> RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
> + rstp_set_bridge_hello_time__(rstp);
> + rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
> + rstp_set_bridge_migrate_time__(rstp);
> + rstp_set_bridge_transmit_hold_count__(rstp,
> +
> RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
> + rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
> + RSTP_BRIDGE_HELLO_TIME,
> + RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
>
>
These setters are the same in rstp_create() and reinitialize_rstp__(). We
could define a funcion like rstp_initialize_port_defaults__() for the
bridge.
> +static void
> +rstp_port_set_mcheck__(struct rstp_port *port, bool mcheck)
> + OVS_REQUIRES(rstp_mutex)
> {
> - struct rstp *rstp;
> + /* XXX: Should we also support setting this to false, i.e., when port
> + * configuration is changed? */
> + if (mcheck == true && port->rstp->force_protocol_version >= 2) {
> + port->mcheck = true;
802.1D-2004 standard claims mcheck to be set from management and cleared
from its procedure.
*17.19.13 mcheck*
*A boolean. May be set by management to force the Port Protocol Migration
state machine to transmit RST*
*BPDUs for a MigrateTime (17.13.9) period, to test whether all STP Bridges
(17.4) on the attached LAN*
*have been removed and the Port can continue to transmit RSTP BPDUs.
Setting mcheck has no effect if*
*stpVersion (17.20.12) is TRUE, i.e., the Bridge is operating in “STP
Compatibility” mode.*
However to use it twice, I need to reset it in the database (to make it
change when i want to invoke its setter), so i use the command with 0, with
the only purpouse to clear it in the db, no action is needed from rstp.
Then i can set it again and trigger the procedure.
static void
> xlate_xport_set(struct xport *xport, odp_port_t odp_port,
> const struct netdev *netdev, const struct cfm *cfm,
> - const struct bfd *bfd, int stp_port_no, int rstp_port_no,
> + const struct bfd *bfd, int stp_port_no,
> + const struct rstp_port* rstp_port,
> enum ofputil_port_config config, enum ofputil_port_state
> state,
> bool is_tunnel, bool may_enable)
> {
> xport->config = config;
> xport->state = state;
> xport->stp_port_no = stp_port_no;
> - xport->rstp_port_no = rstp_port_no;
I get a segfault when removing a port from a bridge. I don't if I add here
this line:
xport->rstp_port = rstp_port;
xport->is_tunnel = is_tunnel;
> xport->may_enable = may_enable;
> xport->odp_port = odp_port;
> + if (xport->rstp_port != rstp_port) {
> + rstp_port_unref(xport->rstp_port);
> + xport->rstp_port = rstp_port_ref(rstp_port);
> + }
> @@ -3133,16 +3088,15 @@ port_run(struct ofport_dpif *ofport)
> if (ofport->may_enable != enable) {
> struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(ofport->up.ofproto);
> - ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
> - }
> - ofport->may_enable = enable;
> + ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
> - if (ofport->rstp_port) {
> - if (rstp_port_get_mac_operational(ofport->rstp_port) != enable) {
> + if (ofport->rstp_port) {
> rstp_port_set_mac_operational(ofport->rstp_port, enable);
> }
> }
> +
> + ofport->may_enable = enable;
> }
rstp_port_set_mac_operational(ofport->rstp_port, enable) should be outside
if (ofport->may_enable != enable) otherwise ports remain disabled when
added.
In patch 16/18:
diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
> index e8b8438..5ae7124 100644
> --- a/lib/rstp-state-machines.c
> +++ b/lib/rstp-state-machines.c
> @@ -2015,42 +2015,33 @@ compare_rstp_priority_vector(struct
> rstp_priority_vector *v1,
> RSTP_ID_ARGS(v2->root_bridge_id), v2->root_path_cost,
> RSTP_ID_ARGS(v2->designated_bridge_id),
> v2->designated_port_id);
>
> - if (v1->root_bridge_id < v2->root_bridge_id
> - || (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost < v2->root_path_cost)
> - || (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost == v2->root_path_cost &&
> - v1->designated_bridge_id < v2->designated_bridge_id)
> - || (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost == v2->root_path_cost &&
> - v1->designated_bridge_id == v2->designated_bridge_id &&
> - v1->designated_port_id < v2->designated_port_id)) {
> - VLOG_DBG("superior_absolute");
> - return SUPERIOR;
> - } else if ((v1->root_bridge_id > v2->root_bridge_id
> - || (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost > v2->root_path_cost)
> - || (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost == v2->root_path_cost &&
> - v1->designated_bridge_id > v2->designated_bridge_id)
> - || (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost == v2->root_path_cost &&
> - v1->designated_bridge_id == v2->designated_bridge_id
> &&
> - v1->designated_port_id > v2->designated_port_id))
> - && v1->designated_bridge_id == v2->designated_bridge_id
> - && v1->designated_port_id == v2->designated_port_id) {
> - VLOG_DBG("superior_same_des");
> - return SUPERIOR;
> - } else if (v1->root_bridge_id == v2->root_bridge_id &&
> - v1->root_path_cost == v2->root_path_cost &&
> - v1->designated_bridge_id == v2->designated_bridge_id &&
> - v1->designated_port_id == v2->designated_port_id) {
> + /* Testing for SAME first, so the SUPERIOR test can follow the logic
> above
> + * as is. */
> + if (v1->root_bridge_id == v2->root_bridge_id
> + && v1->root_path_cost == v2->root_path_cost
> + && v1->designated_bridge_id == v2->designated_bridge_id
> + && v1->designated_port_id == v2->designated_port_id) {
> VLOG_DBG("same");
> return SAME;
> - } else {
> - VLOG_DBG("inferior");
> - return INFERIOR;
> }
> + if (v1->root_bridge_id < v2->root_bridge_id
> + || (v1->root_bridge_id == v2->root_bridge_id
> + && v1->root_path_cost < v2->root_path_cost)
> + || (v1->root_bridge_id == v2->root_bridge_id
> + && v1->root_path_cost == v2->root_path_cost
> + && v1->designated_bridge_id < v2->designated_bridge_id)
> + || (v1->root_bridge_id == v2->root_bridge_id
> + && v1->root_path_cost == v2->root_path_cost
> + && v1->designated_bridge_id == v2->designated_bridge_id
> + && v1->designated_port_id < v2->designated_port_id)
> + || (v1->designated_bridge_id == v2->designated_bridge_id
> + && v1->designated_port_id == v2->designated_port_id)) {
> + VLOG_DBG("superior");
> + return SUPERIOR;
> + }
> +
> + VLOG_DBG("inferior");
> + return INFERIOR;
> }
I think this is not correct, since the condition returning
"superior_same_des" is no longer checked.
802.1D-2004 is complex, and entails some keywords like "SUPERIOR" that are
used in misleading way. I would like to mantain
compare_rstp_priority_vector() as it is, since it is IMHO simpler to
compare it with the standard for correctness.
Also in lib/rstp-state-machines.c, it's probably too drastic to call
OVS_NOT_REACHED() when a BPDU is going to be transmitted on a port with
role disabled.
diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
> index 2a98f62..def9bdc 100644
> --- a/lib/rstp-state-machines.c
> +++ b/lib/rstp-state-machines.c
> @@ -848,7 +848,7 @@ tx_rstp(struct rstp_port *p)
> /* should not happen! */
> VLOG_ERR("%s transmitting bpdu in disabled role on port "
> ""RSTP_PORT_ID_FMT"", p->rstp->name, p->port_id);
> - OVS_NOT_REACHED();
> break;
> }
Daniele
2014-08-21 1:57 GMT+02:00 Jarno Rajahalme <[email protected]>:
> I took my time starting the review, so I decided address issues as I
> see them rather than just comment on them.
>
> The first patch of this series is a minimally rebased version of the
> v5 sent on ovs-dev on June 12th, 2014. Rest of the series is my
> proposal for fixes and enhancements.
>
> I could have reordered and squashed some of the patches together, but
> that would have been more work...
>
> Daniele Venturino (1):
> Rapid Spanning Tree Protocol (IEEE 802.1D).
>
> Jarno Rajahalme (17):
> lib/stp,rstp: Add unit more unit tests.
> lib/stp: Some debugging support.
> lib/rstp: Better debug messages, style fixes.
> vswitch.xml: Fix RSTP configuration documentation.
> lib/rstp: Remove unused struct rstp_priority_vector4
> lib/rstp: Coding style fixes.
> lib/rstp: Refactor priority vector recalculation.
> lib/rstp: Refactor port number allocation.
> lib/rstp: Refactor port initialization.
> lib/rstp: CodingStyle changes.
> lib/rstp: Inline trivial predicate functions.
> lib/rstp: More robust thread safety.
> lib/rstp: Remove lock recursion.
> lib/rstp: CodingStyle fixes.
> lib/rstp: Simplify priority vector comparison.
> lib/rstp: Eliminate ports_count.
> lib/rstp: Use hmap instead of a list for ports.
>
> AUTHORS | 1 +
> NOTICE | 3 +
> lib/automake.mk | 5 +
> lib/packets.h | 5 +
> lib/rstp-common.h | 879 ++++++++++++++++++
> lib/rstp-state-machines.c | 2025
> ++++++++++++++++++++++++++++++++++++++++++
> lib/rstp-state-machines.h | 46 +
> lib/rstp.c | 1369 ++++++++++++++++++++++++++++
> lib/rstp.h | 290 ++++++
> lib/stp.c | 7 +-
> lib/stp.h | 5 -
> ofproto/ofproto-dpif-xlate.c | 157 +++-
> ofproto/ofproto-dpif-xlate.h | 6 +-
> ofproto/ofproto-dpif.c | 255 +++++-
> ofproto/ofproto-provider.h | 47 +
> ofproto/ofproto.c | 84 ++
> ofproto/ofproto.h | 50 ++
> tests/.gitignore | 1 +
> tests/automake.mk | 3 +
> tests/ovs-vsctl.at | 4 +
> tests/rstp.at | 235 +++++
> tests/stp.at | 100 +++
> tests/test-rstp.c | 714 +++++++++++++++
> tests/testsuite.at | 1 +
> utilities/ovs-vsctl.8.in | 79 ++
> vswitchd/bridge.c | 295 ++++++
> vswitchd/vswitch.ovsschema | 15 +-
> vswitchd/vswitch.xml | 131 ++-
> 28 files changed, 6749 insertions(+), 63 deletions(-)
> create mode 100644 lib/rstp-common.h
> create mode 100644 lib/rstp-state-machines.c
> create mode 100644 lib/rstp-state-machines.h
> create mode 100644 lib/rstp.c
> create mode 100644 lib/rstp.h
> create mode 100644 tests/rstp.at
> create mode 100644 tests/test-rstp.c
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
