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