Thanks!
On patch 14/17 there’s a little change to add, which I anticipated in my
previous mail.
bundle_move() expects two “struct ofbundle” as arguments. With the current
patch the two tests pertaining the MAC learning shifting fail.
I just tested the merged version and this seems to be a good solution:
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8778ed2..3f8768d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2190,8 +2190,8 @@ rstp_run(struct ofproto_dpif *ofproto)
}
if (rstp_shift_root_learned_address(ofproto->rstp)) {
- bundle_move(rstp_get_old_root_aux(ofproto->rstp),
- rstp_get_new_root_aux(ofproto->rstp));
+ bundle_move(((struct ofport_dpif
*)rstp_get_old_root_aux(ofproto->rstp))->bundle,
+ ((struct ofport_dpif
*)rstp_get_new_root_aux(ofproto->rstp))->bundle);
rstp_reset_root_changed(ofproto->rstp);
}
}
If you want I have already prepared a patch for this.
Regards,
Daniele
> Il giorno 19/nov/2014, alle ore 19:59, Jarno Rajahalme
> <[email protected]> ha scritto:
>
> Daniele,
>
> Thanks for testing, pushed to master.
>
> Jarno
>
> On Nov 17, 2014, at 9:13 AM, Daniele Venturino <[email protected]
> <mailto:[email protected]>> 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
>>> <[email protected] <mailto:[email protected]>> ha scritto:
>>>
>>>
>>> On Nov 14, 2014, at 9:19 AM, Daniele Venturino <[email protected]
>>> <mailto:[email protected]>> 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 <[email protected]
>>>> <mailto:[email protected]>>:
>>>> Daniele,
>>>>
>>>> See comments inline.
>>>>
>>>> Thanks!
>>>>
>>>> Jarno
>>>>
>>>> On Nov 6, 2014, at 7:31 AM, Daniele Venturino <[email protected]
>>>> <mailto:[email protected]>> 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 <[email protected]
>>>> > <mailto:[email protected]>>
>>>> > ---
>>>> > 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
