After some testing, here’s my ack. Acked-by: Daniele Venturino <daniele.ventur...@m3s.it
Il giorno 20/set/2014, alle ore 02:11, Jarno Rajahalme <jrajaha...@nicira.com> ha scritto: > > 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
