Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
On Sep 25, 2014, at 7:52 AM, Daniele Venturino daniele.ventur...@m3s.it wrote: After some testing, here’s my ack. Acked-by: Daniele Venturino daniele.ventur...@m3s.it Pushed to master, thanks! Jarno diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d3e527a..d3973e5 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2386,6 +2386,8 @@ set_rstp_port(struct ofport *ofport_, rstp_port_set(rp, s-port_num, s-priority, s-path_cost, s-admin_edge_port, s-auto_edge, s-mcheck, ofport); update_rstp_port_state(ofport); +/* Synchronize operational status. */ +rstp_port_set_mac_operational(rp, ofport-may_enable); } static void This helps, physical interfaces are no longer blocked in role disabled if RSTP is enabled after they are added to a bridge. I’ll test this more thoroughly soon. I’ll merge this when I get your “Acked-by:”, Jarno ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
Il giorno 11/set/2014, alle ore 19:09, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 11, 2014, at 5:49 AM, Daniele Venturino daniele.ventur...@m3s.it wrote: Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino daniele.ventur...@m3s.it ha scritto: Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com wrote: 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 Ok. I sent my acks. Thanks! I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. Nice to hear about the IXIA validation work. I’ll merge the series to master soon, so that we have less different versions around. Thanks to you for your reviews! I’ll let you have my other patches soon. 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. (snip) +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. Later. (snip) 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? It appears to be solved. Good :-) @@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void *bpdu, size_t bpdu_size) /* Each RSTP port poits back to struct rstp without holding a +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY); +static void static void 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 =
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
On Sep 19, 2014, at 8:26 AM, Daniele Venturino daniele.ventur...@m3s.it wrote: Il giorno 11/set/2014, alle ore 19:09, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 11, 2014, at 5:49 AM, Daniele Venturino daniele.ventur...@m3s.it wrote: Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino daniele.ventur...@m3s.it ha scritto: Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com wrote: 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 Ok. I sent my acks. Thanks! I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. Nice to hear about the IXIA validation work. I’ll merge the series to master soon, so that we have less different versions around. Thanks to you for your reviews! I’ll let you have my other patches soon. 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. (snip) +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. Later. (snip) 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? It appears to be solved. Good :-) @@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void *bpdu, size_t bpdu_size) /* Each RSTP port poits back to struct rstp without holding a +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY); +static void static void xport-may_enable = may_enable; xport-odp_port = odp_port; +if (xport-rstp_port != rstp_port) {
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino daniele.ventur...@m3s.it ha scritto: Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com wrote: 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 Ok. I sent my acks. Thanks! I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. Nice to hear about the IXIA validation work. I’ll merge the series to master soon, so that we have less different versions around. Thanks to you for your reviews! I’ll let you have my other patches soon. 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. (snip) +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. Later. (snip) 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? It appears to be solved. Good :-) @@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void *bpdu, size_t bpdu_size) /* Each RSTP port poits back to struct rstp without holding a +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY); +static void static void 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 =
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
On Sep 11, 2014, at 5:49 AM, Daniele Venturino daniele.ventur...@m3s.it wrote: Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino daniele.ventur...@m3s.it ha scritto: Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com wrote: 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 Ok. I sent my acks. Thanks! I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. Nice to hear about the IXIA validation work. I’ll merge the series to master soon, so that we have less different versions around. Thanks to you for your reviews! I’ll let you have my other patches soon. 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. (snip) +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. Later. (snip) 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? It appears to be solved. Good :-) @@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void *bpdu, size_t bpdu_size) /* Each RSTP port poits back to struct rstp without holding a +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY); +static void static void 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
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
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 Ok. I sent my acks. I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. 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. 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. + * 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_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. +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. 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
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com wrote: 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 Ok. I sent my acks. Thanks! I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. Nice to hear about the IXIA validation work. I’ll merge the series to master soon, so that we have less different versions around. 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. (snip) +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. Later. (snip) 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? It appears to be solved. Good :-) @@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void *bpdu, size_t bpdu_size) /* Each RSTP port poits back to struct rstp without holding a +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY); +static void static void 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) { +
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme jrajaha...@nicira.com ha scritto: On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com wrote: 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 Ok. I sent my acks. Thanks! I did some minor changes from June to August, that you can see here: https://github.com/M3S/ovs-rstp/compare/b946221...c910081 About the compliance and interoperability testing, I've been working with an IXIA validation software for a couple of weeks now, and I already have some patches. I'm still solving some problems and I expect to have some more fixes. This should take a couple of weeks to obtain a compliant version, so maybe it would be better to wait for a compliant version of the protocol before merging it to the master. Anyway, if you still want to merge it and mark it as experimental is still fine by me, since I'll be able to rebase my patches on your version. Nice to hear about the IXIA validation work. I’ll merge the series to master soon, so that we have less different versions around. Thanks to you for your reviews! I’ll let you have my other patches soon. 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. (snip) +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. Later. (snip) 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? It appears to be solved. Good :-) @@ -108,9 +121,9 @@ process_received_bpdu(struct rstp_port *p, const void *bpdu, size_t bpdu_size) /* Each RSTP port poits back to struct rstp without holding a +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY); +static void static void 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; -} -
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
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
Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
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
[ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol
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