On Nov 19, 2014, at 12:21 PM, Daniele Venturino <daniele.ventur...@m3s.it>
wrote:
> 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.
Please do, and sorry for missing this.
Jarno
>
> Regards,
> Daniele
>
>> Il giorno 19/nov/2014, alle ore 19:59, Jarno Rajahalme
>> <jrajaha...@nicira.com> ha scritto:
>>
>> 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
