Thanks for the review!
It would be nice to have an Acked-by from you to the series. However, I plan to
squash trivial CodingStyle fixes in before pushing the series to master. Also,
I’ll add a News item stating that experimental RSTP is added, and more
compliance and interoperability testing is needed.
Responses below,
Jarno
On Sep 3, 2014, at 7:44 AM, Daniele Venturino <venturino.dani...@gmail.com>
wrote:
> 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.
>
Changed the language here to “Bad STP or RSTP BPDU received."
> /* 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
Thanks.
>
> + 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.
>
I will look into this.
> +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.
>
Removed the comment and added a note to this effect to vswitchd/vswitch.xml.
>
> 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;
>
I don’t seem to be able to reproduce the segfault. I tried this by adding
ovs-vsctl del-br and del-port commands to the new test cases, and they succeed
just fine.
The code right below will set the rstp_port member, if the new value is
different from the old value. So all the line you added above does is
circumvent the unref of the old pointer, and the ref of the new pointer. I see
that I missed the unref in xlate_xport_remove():
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index dd9536c..425b171 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct xport
*xport)
hmap_remove(&xport->xbridge->xports, &xport->ofp_node);
netdev_close(xport->netdev);
+ rstp_port_unref(xport->rstp_port);
cfm_unref(xport->cfm);
bfd_unref(xport->bfd);
free(xport);
Could you check if this resolves the segfault you get?
>
> 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.
I decided to initialize the of port->may_enable to false instead.
>
>
> 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.
>
That condition is checked as the last alternative in the conditional expression
above. I did not include a separate log message for that case, though.
I found the original conditional expression confusing, at the minimum it has
some dead logic, i.e.:
If:
v1->designated_bridge_id == v2->designated_bridge_id
&& v1->designated_port_id == v2->designated_port_id
then neither of the preceding “or" cases can be true and can be eliminated.
Also, “superior_same_des” still returns SUPERIOR, and looking at the comment
above the function, it seems to me that SAME is a subset of SUPERIOR (see also
better_or_same_info()) :
/* [17.6]
* This message priority vector is superior to the port priority vector and
* will replace it if, and only if, the message priority vector is better
* than the port priority vector, or the message has been transmitted from the
* same Designated Bridge and Designated Port as the port priority vector,
* i.e.,if the following is true:
* ((RD < RootBridgeID)) ||
* ((RD == RootBridgeID) && (RPCD < RootPathCost)) ||
* ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
* (D < designated_bridge_id)) ||
* ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
* (D == designated_bridge_id) && (PD < designated_port_id)) ||
* ((D == designated_bridge_id.BridgeAddress) &&
* (PD == designated_port_id.PortNumber))
*/
(By the "[17.6]" I believe this comment is a quotation from the standard(?))
Note that the main conditional expression in my proposed change was exactly
this, so it should be trivial to verify it for correctness. As SAME is a subset
of the above, I carved out the SAME prior to this. That leaves everything not
matching this to be INFERIOR. However, maybe this is even clearer:
/* compare_rstp_priority_vector() compares two struct rstp_priority_vectors
* and returns a value indicating if the first rstp_priority_vector is
* superior, same or inferior to the second one.
*
* Zero return value indicates INFERIOR, a non-zero return value indicates
* SUPERIOR. When it makes a difference the non-zero return value SAME
* indicates the priority vectors are identical (a subset of SUPERIOR).
*/
static enum vector_comparison
compare_rstp_priority_vectors(const struct rstp_priority_vector *v1,
const struct rstp_priority_vector *v2)
{
VLOG_DBG("v1: "RSTP_ID_FMT", %u, "RSTP_ID_FMT", %d",
RSTP_ID_ARGS(v1->root_bridge_id), v1->root_path_cost,
RSTP_ID_ARGS(v1->designated_bridge_id), v1->designated_port_id);
VLOG_DBG("v2: "RSTP_ID_FMT", %u, "RSTP_ID_FMT", %d",
RSTP_ID_ARGS(v2->root_bridge_id), v2->root_path_cost,
RSTP_ID_ARGS(v2->designated_bridge_id), v2->designated_port_id);
/* [17.6]
* This message priority vector is superior to the port priority vector and
* will replace it if, and only if, the message priority vector is better
* than the port priority vector, or the message has been transmitted from
* the same Designated Bridge and Designated Port as the port priority
* vector, i.e., if the following is true:
*
* ((RD < RootBridgeID)) ||
* ((RD == RootBridgeID) && (RPCD < RootPathCost)) ||
* ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
* (D < designated_bridge_id)) ||
* ((RD == RootBridgeID) && (RPCD == RootPathCost) &&
* (D == designated_bridge_id) && (PD < designated_port_id)) ||
* ((D == designated_bridge_id.BridgeAddress) &&
* (PD == designated_port_id.PortNumber))
*/
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)) {
/* SAME is a subset of SUPERIOR. */
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("superior_same");
return SAME;
}
VLOG_DBG("superior");
return SUPERIOR;
}
VLOG_DBG("inferior");
return INFERIOR;
}
Note, however that this should still be same logic as the original, just
clearer and closer to the one in the standard.
> 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;
> }
>
Fixed.
>
> Daniele
>
>
>
> 2014-08-21 1:57 GMT+02:00 Jarno Rajahalme <jrajaha...@nicira.com>:
> 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
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
