[ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

Daniele Venturino daniele.venturino at m3s.it
Thu Sep 25 14:52:09 UTC 2014


After some testing, here’s my ack.

Acked-by: Daniele Venturino <daniele.venturino at m3s.it

Il giorno 20/set/2014, alle ore 02:11, Jarno Rajahalme <jrajahalme at nicira.com> ha scritto:

> 
> On Sep 19, 2014, at 8:26 AM, Daniele Venturino <daniele.venturino at m3s.it> wrote:
> 
>> 
>> Il giorno 11/set/2014, alle ore 19:09, Jarno Rajahalme <jrajahalme at nicira.com> ha scritto:
>> 
>>> 
>>> On Sep 11, 2014, at 5:49 AM, Daniele Venturino <daniele.venturino at m3s.it> wrote:
>>> 
>>>> 
>>>> Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino <daniele.venturino at m3s.it> ha scritto:
>>>> 
>>>>> 
>>>>> Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme <jrajahalme at nicira.com> ha scritto:
>>>>> 
>>>>>> 
>>>>>> On Sep 9, 2014, at 3:53 AM, Daniele Venturino <venturino.daniele at 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.daniele at 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




More information about the dev mailing list