[ovs-dev] [PATCH 14/17] rstp: shift learned MAC addresses to new Root port.
Daniele Venturino
daniele.venturino at m3s.it
Fri Nov 14 17:19:44 UTC 2014
>
> 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?
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.
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