[ovs-dev] [PATCH 14/17] rstp: shift learned MAC addresses to new Root port.

Jarno Rajahalme jrajahalme at nicira.com
Wed Nov 19 18:59:51 UTC 2014


Daniele,

Thanks for testing, pushed to master.

  Jarno

On Nov 17, 2014, at 9:13 AM, Daniele Venturino <daniele.venturino at m3s.it> wrote:

>> The problem must be that the port number and the hash value get out of sync, i.e., when the port number is changed. Please try the patch below on top of your patch and check if you still have the same problem.
> 
> With your modifications pertaining the hashmap those tests passed.
> I had to revert this change in ofproto-dpif:
> 
>> -        struct ofport_dpif *old_root;
>> -        struct ofport_dpif *new_root;
>>          if (rstp_shift_root_learned_address(ofproto->rstp)) {
>> -            old_root = rstp_get_old_root_aux(ofproto->rstp);
>> -            new_root = rstp_get_new_root_aux(ofproto->rstp);
>> -            bundle_move(old_root->bundle, new_root->bundle);
>> +            bundle_move(rstp_get_old_root_aux(ofproto->rstp),
>> +                        rstp_get_new_root_aux(ofproto->rstp));
>>              rstp_reset_root_changed(ofproto->rstp);
>>          }
> 
> since bundle_move() expects two “struct ofbundle" as arguments.
> 
> Regards,
> Daniele
> 
> 
>> Il giorno 14/nov/2014, alle ore 22:56, Jarno Rajahalme <jrajahalme at nicira.com> ha scritto:
>> 
>> 
>> On Nov 14, 2014, at 9:19 AM, Daniele Venturino <daniele.venturino at m3s.it> wrote:
>> 
>>> Why lose the ‘_WITH_HASH’? It seems pointless to iterate over all the ports, when we just care of the port with the ‘port_number’?
>>> 
>>> With the _WITH_HASH a test on learned addresses shifting fails. It seems like it doesn't even enter the FOR_EACH cycle when the port_number is not 1, maybe there is a problem with the hash lookup?
>>> 
>> 
>> The problem must be that the port number and the hash value get out of sync, i.e., when the port number is changed. Please try the patch below on top of your patch and check if you still have the same problem.
>> 
>>> Can the bundles really be from different bridges, as they both are managed by the same RSTP state machine? If both ofproto’s are the same, then this function can be simplified.
>>> 
>>> I guess they're from the same bridge. The shift is between two ports of the same bridge. Feel free to suggest any improvement on this.
>>> 
>> 
>> I’ll add an assert to that effect.
>> 
>>   Jarno
>> 
>> diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
>> index 705ab61..02e3f98 100644
>> --- a/lib/rstp-state-machines.c
>> +++ b/lib/rstp-state-machines.c
>> @@ -296,7 +296,7 @@ updt_roles_tree__(struct rstp *r)
>>  
>>      old_root_port_number = r->root_port_id & 0x00ff;
>>      if (old_root_port_number) {
>> -        r->old_root_aux = rstp_get_port_aux(r, old_root_port_number);
>> +        r->old_root_aux = rstp_get_port_aux__(r, old_root_port_number);
>>      }
>>      vsel = -1;
>>      best_vector = r->bridge_priority;
>> @@ -336,15 +336,15 @@ updt_roles_tree__(struct rstp *r)
>>               RSTP_ID_ARGS(r->root_priority.root_bridge_id));
>>      new_root_port_number = r->root_port_id & 0x00ff;
>>      if (new_root_port_number) {
>> -        r->new_root_aux = rstp_get_port_aux(r, new_root_port_number);
>> +        r->new_root_aux = rstp_get_port_aux__(r, new_root_port_number);
>>      }
>>      /* Shift learned MAC addresses from an old Root Port to an existing
>>       * Alternate Port. */
>>      if (!r->root_changed
>> -            && new_root_old_role == ROLE_ALTERNATE
>> -            && new_root_port_number
>> -            && old_root_port_number
>> -            && new_root_port_number != old_root_port_number) {
>> +        && new_root_old_role == ROLE_ALTERNATE
>> +        && new_root_port_number
>> +        && old_root_port_number
>> +        && new_root_port_number != old_root_port_number) {
>>          r->root_changed = true;
>>      }
>>      /* Letters d) e) */
>> diff --git a/lib/rstp.c b/lib/rstp.c
>> index e2420e8..05fa634 100644
>> --- a/lib/rstp.c
>> +++ b/lib/rstp.c
>> @@ -711,6 +711,8 @@ static void
>>  rstp_port_set_port_number__(struct rstp_port *port, uint16_t port_number)
>>      OVS_REQUIRES(rstp_mutex)
>>  {
>> +    int old_port_number = port->port_number;
>> +
>>      /* If new_port_number is available, use it, otherwise use the first free
>>       * available port number. */
>>      port->port_number =
>> @@ -718,14 +720,23 @@ rstp_port_set_port_number__(struct rstp_port *port, uint16_t port_number)
>>          ? port_number
>>          : rstp_first_free_number__(port->rstp, port);
>>  
>> -    set_port_id__(port);
>> -    /* [17.13] is not clear. I suppose that a port number change
>> -     * should trigger reselection like a port priority change. */
>> -    port->selected = false;
>> -    port->reselect = true;
>> +    if (port->port_number != old_port_number) {
>> +        set_port_id__(port);
>> +        /* [17.13] is not clear. I suppose that a port number change
>> +         * should trigger reselection like a port priority change. */
>> +        port->selected = false;
>> +        port->reselect = true;
>> +
>> +        /* Adjust the ports hmap. */
>> +        if (!hmap_node_is_null(&port->node)) {
>> +            hmap_remove(&port->rstp->ports, &port->node);
>> +        }
>> +        hmap_insert(&port->rstp->ports, &port->node,
>> +                    hash_int(port->port_number, 0));
>>  
>> -    VLOG_DBG("%s: set new RSTP port number %d", port->rstp->name,
>> -             port->port_number);
>> +        VLOG_DBG("%s: set new RSTP port number %d", port->rstp->name,
>> +                 port->port_number);
>> +    }
>>  }
>>  
>>  /* Converts the link speed to a port path cost [Table 17-3]. */
>> @@ -780,7 +791,11 @@ rstp_get_root_path_cost(const struct rstp *rstp)
>>   * pointer is returned if no port needs to flush its MAC learning table.
>>   * '*port' needs to be NULL in the first call to start the iteration.  If
>>   * '*port' is passed as non-NULL, it must be the value set by the last
>> - * invocation of this function. */
>> + * invocation of this function.
>> + *
>> + * This function may only be called by the thread that creates and deletes
>> + * ports.  Otherwise this function is not thread safe, as the returned
>> + * '*port' could become stale before it is used in the next invocation. */
>>  void *
>>  rstp_check_and_reset_fdb_flush(struct rstp *rstp, struct rstp_port **port)
>>      OVS_EXCLUDED(rstp_mutex)
>> @@ -819,8 +834,9 @@ out:
>>          (*port)->fdb_flush = false;
>>      }
>>      ovs_mutex_unlock(&rstp_mutex);
>> +
>>      return aux;
>> -    }
>> +}
>>  
>>  /* Finds a port whose state has changed, and returns the aux pointer set for
>>   * the port.  A NULL pointer is returned when no changed port is found.  On
>> @@ -866,40 +882,49 @@ rstp_get_next_changed_port_aux(struct rstp *rstp, struct rstp_port **portp)
>>      *portp = NULL;
>>  out:
>>      ovs_mutex_unlock(&rstp_mutex);
>> +
>>      return aux;
>>  }
>>  
>>  bool
>> -rstp_shift_root_learned_address(struct rstp *rstp) {
>> -    bool ret = false;
>> +rstp_shift_root_learned_address(struct rstp *rstp)
>> +{
>> +    bool ret;
>> +
>>      ovs_mutex_lock(&rstp_mutex);
>> -    if (rstp->root_changed) {
>> -        ret = true;
>> -    }
>> +    ret = rstp->root_changed;
>>      ovs_mutex_unlock(&rstp_mutex);
>> +
>>      return ret;
>>  }
>>  
>>  void *
>> -rstp_get_old_root_aux(struct rstp *rstp) {
>> -    void *aux = NULL;
>> +rstp_get_old_root_aux(struct rstp *rstp)
>> +{
>> +    void *aux;
>> +
>>      ovs_mutex_lock(&rstp_mutex);
>>      aux = rstp->old_root_aux;
>>      ovs_mutex_unlock(&rstp_mutex);
>> +
>>      return aux;
>>  }
>>  
>>  void *
>> -rstp_get_new_root_aux(struct rstp *rstp) {
>> -    void *aux = NULL;
>> +rstp_get_new_root_aux(struct rstp *rstp)
>> +{
>> +    void *aux;
>> +
>>      ovs_mutex_lock(&rstp_mutex);
>>      aux = rstp->new_root_aux;
>>      ovs_mutex_unlock(&rstp_mutex);
>> +
>>      return aux;
>>  }
>>  
>>  void
>> -rstp_reset_root_changed(struct rstp *rstp) {
>> +rstp_reset_root_changed(struct rstp *rstp)
>> +{
>>      ovs_mutex_lock(&rstp_mutex);
>>      rstp->root_changed = false;
>>      ovs_mutex_unlock(&rstp_mutex);
>> @@ -916,7 +941,8 @@ rstp_get_port__(struct rstp *rstp, uint16_t port_number)
>>  
>>      ovs_assert(rstp && port_number > 0 && port_number <= RSTP_MAX_PORTS);
>>  
>> -    HMAP_FOR_EACH (port, node, &rstp->ports) {
>> +    HMAP_FOR_EACH_WITH_HASH (port, node, hash_int(port_number, 0),
>> +                             &rstp->ports) {
>>          if (port->port_number == port_number) {
>>              return port;
>>          }
>> @@ -937,7 +963,7 @@ rstp_get_port(struct rstp *rstp, uint16_t port_number)
>>  }
>>  
>>  void *
>> -rstp_get_port_aux(struct rstp *rstp, uint16_t port_number)
>> +rstp_get_port_aux__(struct rstp *rstp, uint16_t port_number)
>>      OVS_REQUIRES(rstp_mutex)
>>  {
>>      struct rstp_port *p;
>> @@ -1117,6 +1143,7 @@ rstp_add_port(struct rstp *rstp)
>>      struct rstp_port *p = xzalloc(sizeof *p);
>>  
>>      ovs_refcount_init(&p->ref_cnt);
>> +    hmap_node_nullify(&p->node);
>>  
>>      ovs_mutex_lock(&rstp_mutex);
>>      p->rstp = rstp;
>> @@ -1128,7 +1155,6 @@ rstp_add_port(struct rstp *rstp)
>>               p->port_id);
>>  
>>      rstp_port_set_state__(p, RSTP_DISCARDING);
>> -    hmap_insert(&rstp->ports, &p->node, hash_int(p->port_number, 0));
>>      rstp->changes = true;
>>      move_rstp__(rstp);
>>      VLOG_DBG("%s: added port "RSTP_PORT_ID_FMT"", rstp->name, p->port_id);
>> @@ -1357,6 +1383,7 @@ rstp_get_root_port(struct rstp *rstp)
>>  /* Returns the state of port 'p'. */
>>  enum rstp_state
>>  rstp_port_get_state(const struct rstp_port *p)
>> +    OVS_EXCLUDED(rstp_mutex)
>>  {
>>      enum rstp_state state;
>>  
>> diff --git a/lib/rstp.h b/lib/rstp.h
>> index 3b40a00..8b57761 100644
>> --- a/lib/rstp.h
>> +++ b/lib/rstp.h
>> @@ -164,15 +164,13 @@ void *rstp_get_next_changed_port_aux(struct rstp *, struct rstp_port **)
>>  void rstp_port_set_mac_operational(struct rstp_port *,
>>                                     bool new_mac_operational)
>>      OVS_EXCLUDED(rstp_mutex);
>> -bool
>> -rstp_shift_root_learned_address(struct rstp *rstp)
>> +bool rstp_shift_root_learned_address(struct rstp *)
>>      OVS_EXCLUDED(rstp_mutex);
>>  void *rstp_get_old_root_aux(struct rstp *)
>>      OVS_EXCLUDED(rstp_mutex);
>>  void *rstp_get_new_root_aux(struct rstp *)
>>      OVS_EXCLUDED(rstp_mutex);
>> -void
>> -rstp_reset_root_changed(struct rstp *)
>> +void rstp_reset_root_changed(struct rstp *)
>>      OVS_EXCLUDED(rstp_mutex);
>>  
>>  /* Bridge setters */
>> @@ -242,8 +240,8 @@ void rstp_port_get_status(const struct rstp_port *, uint16_t *id,
>>                            int *rx_count, int *error_count, int *uptime)
>>      OVS_EXCLUDED(rstp_mutex);
>>  
>> -void * rstp_get_port_aux(struct rstp *rstp, uint16_t port_number)
>> -    OVS_EXCLUDED(rstp_mutex);
>> +void * rstp_get_port_aux__(struct rstp *rstp, uint16_t port_number)
>> +    OVS_REQUIRES(rstp_mutex);
>>  
>>  
>>  /* Internal API for rstp-state-machines.c */
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 0ec7b66..268aefa 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2133,8 +2133,8 @@ update_rstp_port_state(struct ofport_dpif *ofport)
>>              != rstp_learn_in_state(state)) {
>>              /* XXX: Learning action flows should also be flushed. */
>>              if (ofport->bundle) {
>> -                if(!rstp_shift_root_learned_address(ofproto->rstp)
>> -                    || ofport != rstp_get_old_root_aux(ofproto->rstp)) {
>> +                if (!rstp_shift_root_learned_address(ofproto->rstp)
>> +                    || rstp_get_old_root_aux(ofproto->rstp) != ofport) {
>>                      bundle_flush_macs(ofport->bundle, false);
>>                  }
>>              }
>> @@ -2184,20 +2184,16 @@ rstp_run(struct ofproto_dpif *ofproto)
>>           */
>>          while ((ofport = rstp_check_and_reset_fdb_flush(ofproto->rstp, &rp))) {
>>              if (!rstp_shift_root_learned_address(ofproto->rstp)
>> -                || ofport != rstp_get_old_root_aux(ofproto->rstp)) {
>> +                || rstp_get_old_root_aux(ofproto->rstp) != ofport) {
>>                  bundle_flush_macs(ofport->bundle, false);
>>              }
>>          }
>>  
>> -        struct ofport_dpif *old_root;
>> -        struct ofport_dpif *new_root;
>>          if (rstp_shift_root_learned_address(ofproto->rstp)) {
>> -            old_root = rstp_get_old_root_aux(ofproto->rstp);
>> -            new_root = rstp_get_new_root_aux(ofproto->rstp);
>> -            bundle_move(old_root->bundle, new_root->bundle);
>> +            bundle_move(rstp_get_old_root_aux(ofproto->rstp),
>> +                        rstp_get_new_root_aux(ofproto->rstp));
>>              rstp_reset_root_changed(ofproto->rstp);
>>          }
>> -
>>      }
>>  }
>>  
>> @@ -2544,21 +2540,23 @@ bundle_flush_macs(struct ofbundle *bundle, bool all_ofprotos)
>>      ovs_rwlock_unlock(&ml->rwlock);
>>  }
>>  
>> -static void bundle_move(struct ofbundle *old, struct ofbundle *new) {
>> -    struct ofproto_dpif *old_ofproto = old->ofproto;
>> -    struct ofproto_dpif *new_ofproto = new->ofproto;
>> -    struct mac_learning *old_ml = old_ofproto->ml;
>> +static void
>> +bundle_move(struct ofbundle *old, struct ofbundle *new)
>> +{
>> +    struct ofproto_dpif *ofproto = old->ofproto;
>> +    struct mac_learning *ml = ofproto->ml;
>>      struct mac_entry *mac, *next_mac;
>>  
>> -    old_ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> -    new_ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> -    ovs_rwlock_wrlock(&old_ml->rwlock);
>> -    LIST_FOR_EACH_SAFE (mac, next_mac, lru_node, &old_ml->lrus) {
>> +    ovs_assert(new->ofproto == old->ofproto);
>> +
>> +    ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> +    ovs_rwlock_wrlock(&ml->rwlock);
>> +    LIST_FOR_EACH_SAFE (mac, next_mac, lru_node, &ml->lrus) {
>>          if (mac->port.p == old) {
>>              mac->port.p = new;
>>          }
>>      }
>> -    ovs_rwlock_unlock(&old_ml->rwlock);
>> +    ovs_rwlock_unlock(&ml->rwlock);
>>  }
>>  
>>  static struct ofbundle *
>> 
>> 
>> 
>>> Daniele
>>> 
>>> 2014-11-14 1:11 GMT+01:00 Jarno Rajahalme <jrajahalme at nicira.com>:
>>> Daniele,
>>> 
>>> See comments inline.
>>> 
>>> Thanks!
>>> 
>>>   Jarno
>>> 
>>> On Nov 6, 2014, at 7:31 AM, Daniele Venturino <daniele.venturino at m3s.it> wrote:
>>> 
>>> >      All MAC addresses previously learned on a Root Port can be moved to an
>>> >      Alternate Port that becomes the new Root Port; i.e., Dynamic Filtering
>>> >      Entries for those addresses may be modified to show the new Root Port as
>>> >      their source, reducing the need to flood frames when recovering from
>>> >      some component failures.
>>> >
>>> 
>>> Indentation...
>>> 
>>> > Signed-off-by: Daniele Venturino <daniele.venturino at m3s.it>
>>> > ---
>>> > lib/rstp-common.h         |  4 ++++
>>> > lib/rstp-state-machines.c | 21 +++++++++++++++++++
>>> > lib/rstp.c                | 51 ++++++++++++++++++++++++++++++++++++++---------
>>> > lib/rstp.h                | 10 ++++++++++
>>> > ofproto/ofproto-dpif.c    | 39 ++++++++++++++++++++++++++++++++++--
>>> > 5 files changed, 114 insertions(+), 11 deletions(-)
>>> >
>>> > diff --git a/lib/rstp-common.h b/lib/rstp-common.h
>>> > index 6e56958..271db2a 100644
>>> > --- a/lib/rstp-common.h
>>> > +++ b/lib/rstp-common.h
>>> > @@ -868,6 +868,10 @@ struct rstp {
>>> >     /* Interface to client. */
>>> >     void (*send_bpdu)(struct ofpbuf *bpdu, void *port_aux, void *rstp_aux);
>>> >     void *aux;
>>> > +
>>> > +    bool root_changed;
>>> > +    void *old_root_aux;
>>> > +    void *new_root_aux;
>>> > };
>>> >
>>> > #endif /* rstp-common.h */
>>> > diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
>>> > index 516093f..40eeea5 100644
>>> > --- a/lib/rstp-state-machines.c
>>> > +++ b/lib/rstp-state-machines.c
>>> > @@ -287,7 +287,14 @@ updt_roles_tree__(struct rstp *r)
>>> >     struct rstp_port *p;
>>> >     int vsel;
>>> >     struct rstp_priority_vector best_vector, candidate_vector;
>>> > +    enum rstp_port_role new_root_old_role = ROLE_DESIGNATED;
>>> > +    uint16_t old_root_port_number = 0;
>>> > +    uint16_t new_root_port_number = 0;
>>> >
>>> > +    old_root_port_number = r->root_port_id & 0x00ff;
>>> > +    if (old_root_port_number) {
>>> > +        r->old_root_aux = rstp_get_port_aux(r, old_root_port_number);
>>> > +    }
>>> >     vsel = -1;
>>> >     best_vector = r->bridge_priority;
>>> >     /* Letter c1) */
>>> > @@ -317,12 +324,26 @@ updt_roles_tree__(struct rstp *r)
>>> >             r->root_times = p->port_times;
>>> >             r->root_times.message_age++;
>>> >             vsel = p->port_number;
>>> > +            new_root_old_role = p->role;
>>> >         }
>>> >     }
>>> >     r->root_priority = best_vector;
>>> >     r->root_port_id = best_vector.bridge_port_id;
>>> >     VLOG_DBG("%s: new Root is "RSTP_ID_FMT, r->name,
>>> >              RSTP_ID_ARGS(r->root_priority.root_bridge_id));
>>> > +    new_root_port_number = r->root_port_id & 0x00ff;
>>> > +    if (new_root_port_number) {
>>> > +        r->new_root_aux = rstp_get_port_aux(r, new_root_port_number);
>>> > +    }
>>> > +    /* Shift learned MAC addresses from an old Root Port to an existing
>>> > +     * Alternate Port. */
>>> > +    if (!r->root_changed
>>> > +            && new_root_old_role == ROLE_ALTERNATE
>>> > +            && new_root_port_number
>>> > +            && old_root_port_number
>>> > +            && new_root_port_number != old_root_port_number) {
>>> 
>>> Indentation above is wrong, the lines should line right after the opening parenthesis above.
>>> 
>>> > +        r->root_changed = true;
>>> > +    }
>>> >     /* Letters d) e) */
>>> >     HMAP_FOR_EACH (p, node, &r->ports) {
>>> >         p->designated_priority_vector.root_bridge_id =
>>> > diff --git a/lib/rstp.c b/lib/rstp.c
>>> > index fd42a7d..5256740 100644
>>> > --- a/lib/rstp.c
>>> > +++ b/lib/rstp.c
>>> > @@ -867,6 +867,42 @@ out:
>>> >     return aux;
>>> > }
>>> >
>>> > +bool
>>> > +rstp_shift_root_learned_address(struct rstp *rstp) {
>>> > +    bool ret = false;
>>> > +    ovs_mutex_lock(&rstp_mutex);
>>> > +    if (rstp->root_changed) {
>>> > +        ret = true;
>>> > +    }
>>> > +    ovs_mutex_unlock(&rstp_mutex);
>>> > +    return ret;
>>> > +}
>>> > +
>>> 
>>> Format function definitions like this:
>>> 
>>> bool
>>> rstp_shift_root_learned_address(struct rstp *rstp)
>>> {
>>>     bool ret;
>>> 
>>>     ovs_mutex_lock(&rstp_mutex);
>>>     ret = rstp->root_changed;
>>>     ovs_mutex_unlock(&rstp_mutex);
>>> 
>>>     return ret;
>>> }
>>> 
>>> > +void *
>>> > +rstp_get_old_root_aux(struct rstp *rstp) {
>>> > +    void *aux = NULL;
>>> > +    ovs_mutex_lock(&rstp_mutex);
>>> > +    aux = rstp->old_root_aux;
>>> > +    ovs_mutex_unlock(&rstp_mutex);
>>> > +    return aux;
>>> > +}
>>> > +
>>> > +void *
>>> > +rstp_get_new_root_aux(struct rstp *rstp) {
>>> > +    void *aux = NULL;
>>> > +    ovs_mutex_lock(&rstp_mutex);
>>> > +    aux = rstp->new_root_aux;
>>> > +    ovs_mutex_unlock(&rstp_mutex);
>>> > +    return aux;
>>> > +}
>>> > +
>>> > +void
>>> > +rstp_reset_root_changed(struct rstp *rstp) {
>>> > +    ovs_mutex_lock(&rstp_mutex);
>>> > +    rstp->root_changed = false;
>>> > +    ovs_mutex_unlock(&rstp_mutex);
>>> > +}
>>> > +
>>> > /* Returns the port in 'rstp' with number 'port_number'.
>>> >  *
>>> >  * XXX: May only be called while concurrent deletion of ports is excluded. */
>>> > @@ -878,8 +914,7 @@ rstp_get_port__(struct rstp *rstp, uint16_t port_number)
>>> >
>>> >     ovs_assert(rstp && port_number > 0 && port_number <= RSTP_MAX_PORTS);
>>> >
>>> > -    HMAP_FOR_EACH_WITH_HASH (port, node, hash_int(port_number, 0),
>>> > -                             &rstp->ports) {
>>> > +    HMAP_FOR_EACH (port, node, &rstp->ports) {
>>> 
>>> Why lose the ‘_WITH_HASH’? It seems pointless to iterate over all the ports, when we just care of the port with the ‘port_number’?
>>> 
>>> >         if (port->port_number == port_number) {
>>> >             return port;
>>> >         }
>>> > @@ -901,16 +936,14 @@ rstp_get_port(struct rstp *rstp, uint16_t port_number)
>>> >
>>> > void *
>>> > rstp_get_port_aux(struct rstp *rstp, uint16_t port_number)
>>> > -    OVS_EXCLUDED(rstp_mutex)
>>> > +    OVS_REQUIRES(rstp_mutex)
>>> > {
>>> >     struct rstp_port *p;
>>> > -    void *aux;
>>> > -
>>> > -    ovs_mutex_lock(&rstp_mutex);
>>> >     p = rstp_get_port__(rstp, port_number);
>>> > -    aux = p->aux;
>>> > -    ovs_mutex_unlock(&rstp_mutex);
>>> > -    return aux;
>>> > +    if (p) {
>>> > +        return p->aux;
>>> > +    }
>>> > +    return NULL;
>>> > }
>>> >
>>> > /* Updates the port_enabled parameter. */
>>> > diff --git a/lib/rstp.h b/lib/rstp.h
>>> > index b05cdf2..3b40a00 100644
>>> > --- a/lib/rstp.h
>>> > +++ b/lib/rstp.h
>>> > @@ -164,6 +164,16 @@ void *rstp_get_next_changed_port_aux(struct rstp *, struct rstp_port **)
>>> > void rstp_port_set_mac_operational(struct rstp_port *,
>>> >                                    bool new_mac_operational)
>>> >     OVS_EXCLUDED(rstp_mutex);
>>> > +bool
>>> > +rstp_shift_root_learned_address(struct rstp *rstp)
>>> > +    OVS_EXCLUDED(rstp_mutex);
>>> 
>>> Function declarations should have the return type and the function name on the same line.
>>> 
>>> > +void *rstp_get_old_root_aux(struct rstp *)
>>> > +    OVS_EXCLUDED(rstp_mutex);
>>> > +void *rstp_get_new_root_aux(struct rstp *)
>>> > +    OVS_EXCLUDED(rstp_mutex);
>>> > +void
>>> > +rstp_reset_root_changed(struct rstp *)
>>> > +    OVS_EXCLUDED(rstp_mutex);
>>> >
>>> 
>>> Here, too.
>>> 
>>> > /* Bridge setters */
>>> > void rstp_set_bridge_address(struct rstp *, rstp_identifier bridge_address)
>>> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> > index 2fd9896..2389a18 100644
>>> > --- a/ofproto/ofproto-dpif.c
>>> > +++ b/ofproto/ofproto-dpif.c
>>> > @@ -141,6 +141,7 @@ static void bundle_del_port(struct ofport_dpif *);
>>> > static void bundle_run(struct ofbundle *);
>>> > static void bundle_wait(struct ofbundle *);
>>> > static void bundle_flush_macs(struct ofbundle *, bool);
>>> > +static void bundle_move(struct ofbundle *, struct ofbundle *);
>>> >
>>> > static void stp_run(struct ofproto_dpif *ofproto);
>>> > static void stp_wait(struct ofproto_dpif *ofproto);
>>> > @@ -2096,11 +2097,15 @@ update_rstp_port_state(struct ofport_dpif *ofport)
>>> >                  netdev_get_name(ofport->up.netdev),
>>> >                  rstp_state_name(ofport->rstp_state),
>>> >                  rstp_state_name(state));
>>> > +
>>> >         if (rstp_learn_in_state(ofport->rstp_state)
>>> >                 != rstp_learn_in_state(state)) {
>>> >             /* xxx Learning action flows should also be flushed. */
>>> >             if (ofport->bundle) {
>>> > -                bundle_flush_macs(ofport->bundle, false);
>>> > +                if(!rstp_shift_root_learned_address(ofproto->rstp)
>>> > +                    || ofport != rstp_get_old_root_aux(ofproto->rstp)) {
>>> > +                    bundle_flush_macs(ofport->bundle, false);
>>> > +                }
>>> >             }
>>> >         }
>>> >         fwd_change = rstp_forward_in_state(ofport->rstp_state)
>>> > @@ -2147,8 +2152,21 @@ rstp_run(struct ofproto_dpif *ofproto)
>>> >          * p->fdb_flush) and not periodically.
>>> >          */
>>> >         while ((ofport = rstp_check_and_reset_fdb_flush(ofproto->rstp, &rp))) {
>>> > -            bundle_flush_macs(ofport->bundle, false);
>>> > +            if (!rstp_shift_root_learned_address(ofproto->rstp)
>>> > +                || ofport != rstp_get_old_root_aux(ofproto->rstp)) {
>>> > +                bundle_flush_macs(ofport->bundle, false);
>>> > +            }
>>> > +        }
>>> > +
>>> > +        struct ofport_dpif *old_root;
>>> > +        struct ofport_dpif *new_root;
>>> > +        if (rstp_shift_root_learned_address(ofproto->rstp)) {
>>> > +            old_root = rstp_get_old_root_aux(ofproto->rstp);
>>> > +            new_root = rstp_get_new_root_aux(ofproto->rstp);
>>> > +            bundle_move(old_root->bundle, new_root->bundle);
>>> > +            rstp_reset_root_changed(ofproto->rstp);
>>> >         }
>>> 
>>> Better write the latter part as:
>>> 
>>>         if (rstp_shift_root_learned_address(ofproto->rstp)) {
>>>             bundle_move(rstp_get_old_root_aux(ofproto->rstp),
>>>                         rstp_get_new_root_aux(ofproto->rstp));
>>>             rstp_reset_root_changed(ofproto->rstp);
>>>         }
>>> 
>>> 
>>> > +
>>> >     }
>>> > }
>>> >
>>> > @@ -2495,6 +2513,23 @@ bundle_flush_macs(struct ofbundle *bundle, bool all_ofprotos)
>>> >     ovs_rwlock_unlock(&ml->rwlock);
>>> > }
>>> >
>>> > +static void bundle_move(struct ofbundle *old, struct ofbundle *new) {
>>> > +    struct ofproto_dpif *old_ofproto = old->ofproto;
>>> > +    struct ofproto_dpif *new_ofproto = new->ofproto;
>>> 
>>> Can the bundles really be from different bridges, as they both are managed by the same RSTP state machine? If both ofproto’s are the same, then this function can be simplified.
>>> 
>>> > +    struct mac_learning *old_ml = old_ofproto->ml;
>>> > +    struct mac_entry *mac, *next_mac;
>>> > +
>>> > +    old_ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>> > +    new_ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>> > +    ovs_rwlock_wrlock(&old_ml->rwlock);
>>> > +    LIST_FOR_EACH_SAFE (mac, next_mac, lru_node, &old_ml->lrus) {
>>> > +        if (mac->port.p == old) {
>>> > +            mac->port.p = new;
>>> > +        }
>>> > +    }
>>> > +    ovs_rwlock_unlock(&old_ml->rwlock);
>>> > +}
>>> > +
>>> > static struct ofbundle *
>>> > bundle_lookup(const struct ofproto_dpif *ofproto, void *aux)
>>> > {
>>> > --
>>> > 1.8.1.2
>>> >
> 




More information about the dev mailing list