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 (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? > It would be really nice if you could modify the rstp.at to show this behavior. Dummy interfaces are special in that they start up as admin down, but they forward packets anyway. RSTP explicitly asks for the carrier status, so the dummy interface status must be changed for RSTP to work on them. You could also test if this helps: 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 Regards, Jarno > Daniele _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
