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