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) { >>>>>>> + 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. > > > >> # >> >> # Turn RSTP on in br1 after the ports have been added. >> >> # >> >> AT_CHECK([ovs-vsctl set bridge br1 rstp_enable=true]) > > I see you already enable RSTP after adding some ports in rstp.at. > >> >> 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 > > 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
