Daniele,
Thanks for testing, pushed to master.
Jarno
On Nov 17, 2014, at 9:13 AM, Daniele Venturino <daniele.ventur...@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
>> <jrajaha...@nicira.com> ha scritto:
>>
>>
>> On Nov 14, 2014, at 9:19 AM, Daniele Venturino <daniele.ventur...@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 <jrajaha...@nicira.com>:
>>> Daniele,
>>>
>>> See comments inline.
>>>
>>> Thanks!
>>>
>>> Jarno
>>>
>>> On Nov 6, 2014, at 7:31 AM, Daniele Venturino <daniele.ventur...@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.ventur...@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
>>> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
