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 = >>>> 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. >>>> xport->is_tunnel = is_tunnel; >>> >>> I applied this and have the impression that this is ok if ports are added >>> to a bridge after rstp is turned on. Otherwise, if a bridge already has >>> some ports and rstp is enabled, ports remain disabled. >> >> I changed the test case at the end of tests/rstp.at to exercise this, and >> could not see what you expected. Maybe you check this out when the code is >> merged to master? > > I’ll test the merged version as soon as I can. I still have this problem with the merged version. If a bridge has already some ports and I enable rstp, those ports remain in ROLE_DISABLED. My guess is that when I enable rstp ofport->may_enable and enable have the same value, so rstp_port_set_mac_operational() is not executed. Maybe the tests in tests/rstp.at don’t show this behavior because they use dummy interfaces? Daniele _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
