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. > > Jarno > > Daniele _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
