[ovs-dev] [PATCH v6 17/18] lib/rstp: Eliminate ports_count.
Daniele Venturino
venturino.daniele at gmail.com
Tue Sep 9 10:38:36 UTC 2014
Acked-by: Daniele Venturino <daniele.venturino at m3s.it>
2014-08-21 1:57 GMT+02:00 Jarno Rajahalme <jrajahalme at nicira.com>:
> It was only used to guard against unintialized list.
>
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> ---
> lib/rstp-common.h | 1 -
> lib/rstp-state-machines.c | 273
> ++++++++++++++++++++-------------------------
> lib/rstp.c | 89 +++++++--------
> 3 files changed, 164 insertions(+), 199 deletions(-)
>
> diff --git a/lib/rstp-common.h b/lib/rstp-common.h
> index d798ba2..2be5861 100644
> --- a/lib/rstp-common.h
> +++ b/lib/rstp-common.h
> @@ -867,7 +867,6 @@ struct rstp {
>
> /* Ports */
> struct list ports OVS_GUARDED_BY(rstp_mutex);
> - uint16_t ports_count OVS_GUARDED_BY(rstp_mutex);
>
> struct ovs_refcount ref_cnt;
>
> diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
> index 5ae7124..c40449d 100644
> --- a/lib/rstp-state-machines.c
> +++ b/lib/rstp-state-machines.c
> @@ -190,18 +190,16 @@ move_rstp__(struct rstp *rstp)
>
> rstp->changes = false;
> port_role_selection_sm(rstp);
> - if (rstp->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &rstp->ports) {
> - if (p->rstp_state != RSTP_DISABLED) {
> - port_receive_sm(p);
> - bridge_detection_sm(p);
> - port_information_sm(p);
> - port_role_transition_sm(p);
> - port_state_transition_sm(p);
> - topology_change_sm(p);
> - port_transmit_sm(p);
> - port_protocol_migration_sm(p);
> - }
> + LIST_FOR_EACH (p, node, &rstp->ports) {
> + if (p->rstp_state != RSTP_DISABLED) {
> + port_receive_sm(p);
> + bridge_detection_sm(p);
> + port_information_sm(p);
> + port_role_transition_sm(p);
> + port_state_transition_sm(p);
> + topology_change_sm(p);
> + port_transmit_sm(p);
> + port_protocol_migration_sm(p);
> }
> }
> num_iterations++;
> @@ -219,19 +217,17 @@ void decrease_rstp_port_timers__(struct rstp *r)
> {
> struct rstp_port *p;
>
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - decrement_timer(&p->hello_when);
> - decrement_timer(&p->tc_while);
> - decrement_timer(&p->fd_while);
> - decrement_timer(&p->rcvd_info_while);
> - decrement_timer(&p->rr_while);
> - decrement_timer(&p->rb_while);
> - decrement_timer(&p->mdelay_while);
> - decrement_timer(&p->edge_delay_while);
> - decrement_timer(&p->tx_count);
> - p->uptime += 1;
> - }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + decrement_timer(&p->hello_when);
> + decrement_timer(&p->tc_while);
> + decrement_timer(&p->fd_while);
> + decrement_timer(&p->rcvd_info_while);
> + decrement_timer(&p->rr_while);
> + decrement_timer(&p->rb_while);
> + decrement_timer(&p->mdelay_while);
> + decrement_timer(&p->edge_delay_while);
> + decrement_timer(&p->tx_count);
> + p->uptime += 1;
> }
> r->changes = true;
> move_rstp__(r);
> @@ -254,10 +250,8 @@ updt_role_disabled_tree(struct rstp *r)
> {
> struct rstp_port *p;
>
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - p->selected_role = ROLE_DISABLED;
> - }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + p->selected_role = ROLE_DISABLED;
> }
> }
>
> @@ -267,10 +261,8 @@ clear_reselect_tree(struct rstp *r)
> {
> struct rstp_port *p;
>
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - p->reselect = false;
> - }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + p->reselect = false;
> }
> }
>
> @@ -287,32 +279,30 @@ updt_roles_tree__(struct rstp *r)
> /* Letter c1) */
> r->root_times = r->bridge_times;
> /* Letters a) b) c) */
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - uint32_t old_root_path_cost;
> - uint32_t root_path_cost;
> + LIST_FOR_EACH (p, node, &r->ports) {
> + uint32_t old_root_path_cost;
> + uint32_t root_path_cost;
>
> - if (p->info_is != INFO_IS_RECEIVED) {
> - continue;
> - }
> - /* [17.6] */
> - candidate_vector = p->port_priority;
> - candidate_vector.bridge_port_id = p->port_id;
> - old_root_path_cost = candidate_vector.root_path_cost;
> - root_path_cost = old_root_path_cost + p->port_path_cost;
> - candidate_vector.root_path_cost = root_path_cost;
> -
> - if ((candidate_vector.designated_bridge_id &
> 0xffffffffffffULL) ==
> - (r->bridge_priority.designated_bridge_id &
> 0xffffffffffffULL)) {
> - break;
> - }
> - if (compare_rstp_priority_vector(&candidate_vector,
> &best_vector)
> - == SUPERIOR) {
> - best_vector = candidate_vector;
> - r->root_times = p->port_times;
> - r->root_times.message_age++;
> - vsel = p->port_number;
> - }
> + if (p->info_is != INFO_IS_RECEIVED) {
> + continue;
> + }
> + /* [17.6] */
> + candidate_vector = p->port_priority;
> + candidate_vector.bridge_port_id = p->port_id;
> + old_root_path_cost = candidate_vector.root_path_cost;
> + root_path_cost = old_root_path_cost + p->port_path_cost;
> + candidate_vector.root_path_cost = root_path_cost;
> +
> + if ((candidate_vector.designated_bridge_id & 0xffffffffffffULL) ==
> + (r->bridge_priority.designated_bridge_id &
> 0xffffffffffffULL)) {
> + break;
> + }
> + if (compare_rstp_priority_vector(&candidate_vector, &best_vector)
> + == SUPERIOR) {
> + best_vector = candidate_vector;
> + r->root_times = p->port_times;
> + r->root_times.message_age++;
> + vsel = p->port_number;
> }
> }
> r->root_priority = best_vector;
> @@ -320,63 +310,59 @@ updt_roles_tree__(struct rstp *r)
> VLOG_DBG("%s: new Root is "RSTP_ID_FMT"", r->name,
> RSTP_ID_ARGS(r->root_priority.root_bridge_id));
> /* Letters d) e) */
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - p->designated_priority_vector.root_bridge_id =
> - r->root_priority.root_bridge_id;
> - p->designated_priority_vector.root_path_cost =
> - r->root_priority.root_path_cost;
> - p->designated_priority_vector.designated_bridge_id =
> - r->bridge_identifier;
> - p->designated_priority_vector.designated_port_id =
> - p->port_id;
> - p->designated_times = r->root_times;
> - p->designated_times.hello_time = r->bridge_times.hello_time;
> - }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + p->designated_priority_vector.root_bridge_id =
> + r->root_priority.root_bridge_id;
> + p->designated_priority_vector.root_path_cost =
> + r->root_priority.root_path_cost;
> + p->designated_priority_vector.designated_bridge_id =
> + r->bridge_identifier;
> + p->designated_priority_vector.designated_port_id =
> + p->port_id;
> + p->designated_times = r->root_times;
> + p->designated_times.hello_time = r->bridge_times.hello_time;
> }
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - switch (p->info_is) {
> - case INFO_IS_DISABLED:
> - p->selected_role = ROLE_DISABLED;
> - break;
> - case INFO_IS_AGED:
> + LIST_FOR_EACH (p, node, &r->ports) {
> + switch (p->info_is) {
> + case INFO_IS_DISABLED:
> + p->selected_role = ROLE_DISABLED;
> + break;
> + case INFO_IS_AGED:
> + p->updt_info = true;
> + p->selected_role = ROLE_DESIGNATED;
> + break;
> + case INFO_IS_MINE:
> + p->selected_role = ROLE_DESIGNATED;
> + if (compare_rstp_priority_vector(&p->port_priority,
> +
> &p->designated_priority_vector)
> + != SAME
> + || !rstp_times_equal(&p->designated_times,
> &r->root_times)) {
> p->updt_info = true;
> - p->selected_role = ROLE_DESIGNATED;
> - break;
> - case INFO_IS_MINE:
> - p->selected_role = ROLE_DESIGNATED;
> - if (compare_rstp_priority_vector(&p->port_priority,
> -
> &p->designated_priority_vector)
> - != SAME
> - || !rstp_times_equal(&p->designated_times,
> &r->root_times)) {
> - p->updt_info = true;
> - }
> - break;
> - case INFO_IS_RECEIVED:
> - if (vsel == p->port_number) { /* Letter i) */
> - p->selected_role = ROLE_ROOT;
> + }
> + break;
> + case INFO_IS_RECEIVED:
> + if (vsel == p->port_number) { /* Letter i) */
> + p->selected_role = ROLE_ROOT;
> + p->updt_info = false;
> + } else if (compare_rstp_priority_vector(
> + &p->designated_priority_vector,
> &p->port_priority)
> + == INFERIOR) {
> + if (p->port_priority.designated_bridge_id !=
> + r->bridge_identifier) {
> + p->selected_role = ROLE_ALTERNATE;
> p->updt_info = false;
> - } else if (compare_rstp_priority_vector(
> - &p->designated_priority_vector,
> &p->port_priority)
> - == INFERIOR) {
> - if (p->port_priority.designated_bridge_id !=
> - r->bridge_identifier) {
> - p->selected_role = ROLE_ALTERNATE;
> - p->updt_info = false;
> - } else {
> - p->selected_role = ROLE_BACKUP;
> - p->updt_info = false;
> - }
> } else {
> - p->selected_role = ROLE_DESIGNATED;
> - p->updt_info = true;
> + p->selected_role = ROLE_BACKUP;
> + p->updt_info = false;
> }
> - break;
> - default:
> - OVS_NOT_REACHED();
> - /* no break */
> + } else {
> + p->selected_role = ROLE_DESIGNATED;
> + p->updt_info = true;
> }
> + break;
> + default:
> + OVS_NOT_REACHED();
> + /* no break */
> }
> }
> seq_change(connectivity_seq_get());
> @@ -388,16 +374,14 @@ set_selected_tree(struct rstp *r)
> {
> struct rstp_port *p;
>
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - if (p->reselect) {
> - return;
> - }
> - }
> - LIST_FOR_EACH (p, node, &r->ports) {
> - p->selected = true;
> + LIST_FOR_EACH (p, node, &r->ports) {
> + if (p->reselect) {
> + return;
> }
> }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + p->selected = true;
> + }
> }
>
> static int
> @@ -432,13 +416,11 @@ port_role_selection_sm(struct rstp *r)
> PORT_ROLE_SELECTION_SM_ROLE_SELECTION;
> /* no break */
> case PORT_ROLE_SELECTION_SM_ROLE_SELECTION:
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - if (p->reselect) {
> - r->port_role_selection_sm_state =
> - PORT_ROLE_SELECTION_SM_ROLE_SELECTION_EXEC;
> - break;
> - }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + if (p->reselect) {
> + r->port_role_selection_sm_state =
> + PORT_ROLE_SELECTION_SM_ROLE_SELECTION_EXEC;
> + break;
> }
> }
> break;
> @@ -1278,10 +1260,8 @@ set_re_root_tree(struct rstp_port *p)
> struct rstp_port *p1;
>
> r = p->rstp;
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p1, node, &r->ports) {
> - p1->re_root = true;
> - }
> + LIST_FOR_EACH (p1, node, &r->ports) {
> + p1->re_root = true;
> }
> }
>
> @@ -1293,10 +1273,8 @@ set_sync_tree(struct rstp_port *p)
> struct rstp_port *p1;
>
> r = p->rstp;
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p1, node, &r->ports) {
> - p1->sync = true;
> - }
> + LIST_FOR_EACH (p1, node, &r->ports) {
> + p1->sync = true;
> }
> }
>
> @@ -1383,11 +1361,9 @@ re_rooted(struct rstp_port *p)
> struct rstp_port *p1;
>
> r = p->rstp;
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p1, node, &r->ports) {
> - if ((p1 != p) && (p1->rr_while != 0)) {
> - return false;
> - }
> + LIST_FOR_EACH (p1, node, &r->ports) {
> + if ((p1 != p) && (p1->rr_while != 0)) {
> + return false;
> }
> }
> return true;
> @@ -1399,12 +1375,10 @@ all_synced(struct rstp *r)
> {
> struct rstp_port *p;
>
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &r->ports) {
> - if (!(p->selected && p->role == p->selected_role &&
> - (p->role == ROLE_ROOT || p->synced == true))) {
> - return false;
> - }
> + LIST_FOR_EACH (p, node, &r->ports) {
> + if (!(p->selected && p->role == p->selected_role &&
> + (p->role == ROLE_ROOT || p->synced == true))) {
> + return false;
> }
> }
> return true;
> @@ -1859,14 +1833,11 @@ set_tc_prop_tree(struct rstp_port *p)
> struct rstp_port *p1;
>
> r = p->rstp;
> - if (r->ports_count > 0) {
> - LIST_FOR_EACH (p1, node, &r->ports) {
> - /* Set tc_prop on every port, except the one calling this
> - * function.
> - */
> - if (p1->port_number != p->port_number) {
> - p1->tc_prop = true;
> - }
> + LIST_FOR_EACH (p1, node, &r->ports) {
> + /* Set tc_prop on every port, except the one calling this
> + * function. */
> + if (p1->port_number != p->port_number) {
> + p1->tc_prop = true;
> }
> }
> }
> diff --git a/lib/rstp.c b/lib/rstp.c
> index 223df01..d5abd20 100644
> --- a/lib/rstp.c
> +++ b/lib/rstp.c
> @@ -171,12 +171,12 @@ rstp_unref(struct rstp *rstp)
> if (rstp && ovs_refcount_unref(&rstp->ref_cnt) == 1) {
> ovs_mutex_lock(&rstp_mutex);
>
> - /* Each RSTP port poits back to struct rstp without holding a
> + /* Each RSTP port points back to struct rstp without holding a
> * reference for that pointer. This is OK as we never move
> * ports from one bridge to another, and holders always
> * release their ports before releasing the bridge. This
> * means that there should be not ports at this time. */
> - ovs_assert(rstp->ports_count == 0);
> + ovs_assert(list_is_empty(&rstp->ports));
>
> list_remove(&rstp->node);
> ovs_mutex_unlock(&rstp_mutex);
> @@ -251,6 +251,10 @@ rstp_create(const char *name, rstp_identifier
> bridge_address,
> rstp = xzalloc(sizeof *rstp);
> rstp->name = xstrdup(name);
>
> + /* Initialize the ports list before calling any setters,
> + * so that the state machines will see an empty ports list. */
> + list_init(&rstp->ports);
> +
> ovs_mutex_lock(&rstp_mutex);
> /* Set bridge address. */
> rstp_set_bridge_address__(rstp, bridge_address);
> @@ -272,8 +276,6 @@ rstp_create(const char *name, rstp_identifier
> bridge_address,
> rstp->changes = false;
> rstp->begin = true;
>
> - /* Initialize the ports list. */
> - list_init(&rstp->ports);
> ovs_refcount_init(&rstp->ref_cnt);
>
> list_push_back(all_rstps, &rstp->node);
> @@ -291,6 +293,8 @@ static void
> set_bridge_priority__(struct rstp *rstp)
> OVS_REQUIRES(rstp_mutex)
> {
> + struct rstp_port *p;
> +
> rstp->bridge_priority.root_bridge_id = rstp->bridge_identifier;
> rstp->bridge_priority.designated_bridge_id = rstp->bridge_identifier;
> VLOG_DBG("%s: new bridge identifier: "RSTP_ID_FMT"", rstp->name,
> @@ -299,13 +303,9 @@ set_bridge_priority__(struct rstp *rstp)
> /* [17.13] When the bridge address changes, recalculates all priority
> * vectors.
> */
> - if (rstp->ports_count > 0) {
> - struct rstp_port *p;
> -
> - LIST_FOR_EACH (p, node, &rstp->ports) {
> - p->selected = false;
> - p->reselect = true;
> - }
> + LIST_FOR_EACH (p, node, &rstp->ports) {
> + p->selected = false;
> + p->reselect = true;
> }
> rstp->changes = true;
> updt_roles_tree__(rstp);
> @@ -417,6 +417,7 @@ reinitialize_rstp__(struct rstp *rstp)
> {
> struct rstp temp;
> static struct list ports;
> + struct rstp_port *p;
>
> /* Copy rstp in temp */
> temp = *rstp;
> @@ -427,6 +428,11 @@ reinitialize_rstp__(struct rstp *rstp)
>
> /* Initialize rstp. */
> rstp->name = temp.name;
> +
> + /* Initialize the ports list before calling any setters,
> + * so that the state machines will see an empty ports list. */
> + list_init(&rstp->ports);
> +
> /* Set bridge address. */
> rstp_set_bridge_address__(rstp, temp.address);
> /* Set default parameters values. */
> @@ -447,16 +453,14 @@ reinitialize_rstp__(struct rstp *rstp)
> rstp->node = temp.node;
> rstp->changes = false;
> rstp->begin = true;
> - rstp->ports = ports;
> - rstp->ports_count = temp.ports_count;
>
> - if (rstp->ports_count > 0) {
> - struct rstp_port *p;
> + /* Restore ports. */
> + rstp->ports = ports;
>
> - LIST_FOR_EACH (p, node, &rstp->ports) {
> - reinitialize_port__(p);
> - }
> + LIST_FOR_EACH (p, node, &rstp->ports) {
> + reinitialize_port__(p);
> }
> +
> rstp->ref_cnt = temp.ref_cnt;
> }
>
> @@ -570,19 +574,17 @@ rstp_set_bridge_transmit_hold_count__(struct rstp
> *rstp,
> int new_transmit_hold_count)
> OVS_REQUIRES(rstp_mutex)
> {
> - struct rstp_port *p;
> -
> if (new_transmit_hold_count >= RSTP_MIN_TRANSMIT_HOLD_COUNT
> && new_transmit_hold_count <= RSTP_MAX_TRANSMIT_HOLD_COUNT) {
> + struct rstp_port *p;
> +
> VLOG_DBG("%s: set RSTP Transmit Hold Count to %d", rstp->name,
> new_transmit_hold_count);
> /* Resetting txCount on all ports [17.13]. */
>
> rstp->transmit_hold_count = new_transmit_hold_count;
> - if (rstp->ports_count > 0) {
> - LIST_FOR_EACH (p, node, &rstp->ports) {
> - p->tx_count = 0;
> - }
> + LIST_FOR_EACH (p, node, &rstp->ports) {
> + p->tx_count = 0;
> }
> }
> }
> @@ -777,15 +779,13 @@ rstp_check_and_reset_fdb_flush(struct rstp *rstp)
> needs_flush = false;
>
> ovs_mutex_lock(&rstp_mutex);
> - if (rstp->ports_count > 0){
> - LIST_FOR_EACH (p, node, &rstp->ports) {
> - if (p->fdb_flush) {
> - needs_flush = true;
> - /* fdb_flush should be reset by the filtering database
> - * once the entries are removed if rstp_version is TRUE,
> and
> - * immediately if stp_version is TRUE.*/
> - p->fdb_flush = false;
> - }
> + LIST_FOR_EACH (p, node, &rstp->ports) {
> + if (p->fdb_flush) {
> + needs_flush = true;
> + /* fdb_flush should be reset by the filtering database
> + * once the entries are removed if rstp_version is TRUE, and
> + * immediately if stp_version is TRUE.*/
> + p->fdb_flush = false;
> }
> }
> ovs_mutex_unlock(&rstp_mutex);
> @@ -850,11 +850,9 @@ rstp_get_port__(struct rstp *rstp, uint16_t
> port_number)
>
> ovs_assert(rstp && port_number > 0 && port_number <= RSTP_MAX_PORTS);
>
> - if (rstp->ports_count > 0) {
> - LIST_FOR_EACH (port, node, &rstp->ports) {
> - if (port->port_number == port_number) {
> - return port;
> - }
> + LIST_FOR_EACH (port, node, &rstp->ports) {
> + if (port->port_number == port_number) {
> + return port;
> }
> }
> return NULL;
> @@ -1054,7 +1052,6 @@ rstp_add_port(struct rstp *rstp)
>
> rstp_port_set_state__(p, RSTP_DISCARDING);
> list_push_back(&rstp->ports, &p->node);
> - rstp->ports_count++;
> rstp->changes = true;
> move_rstp__(rstp);
> VLOG_DBG("%s: added port "RSTP_PORT_ID_FMT"", rstp->name, p->port_id);
> @@ -1088,8 +1085,8 @@ rstp_port_unref(struct rstp_port *rp)
> rstp = rp->rstp;
> rstp_port_set_state__(rp, RSTP_DISABLED);
> list_remove(&rp->node);
> - rstp->ports_count--;
> - VLOG_DBG("%s: removed port "RSTP_PORT_ID_FMT"", rstp->name,
> rp->port_id);
> + VLOG_DBG("%s: removed port "RSTP_PORT_ID_FMT"", rstp->name,
> + rp->port_id);
> ovs_mutex_unlock(&rstp_mutex);
> free(rp);
> }
> @@ -1240,12 +1237,10 @@ rstp_get_root_port(struct rstp *rstp)
> struct rstp_port *p;
>
> ovs_mutex_lock(&rstp_mutex);
> - if (rstp->ports_count > 0){
> - LIST_FOR_EACH (p, node, &rstp->ports) {
> - if (p->port_id == rstp->root_port_id) {
> - ovs_mutex_unlock(&rstp_mutex);
> - return p;
> - }
> + LIST_FOR_EACH (p, node, &rstp->ports) {
> + if (p->port_id == rstp->root_port_id) {
> + ovs_mutex_unlock(&rstp_mutex);
> + return p;
> }
> }
> ovs_mutex_unlock(&rstp_mutex);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list