[ovs-dev] [PATCH v6 17/18] lib/rstp: Eliminate ports_count.

Jarno Rajahalme jrajahalme at nicira.com
Wed Aug 20 23:57:29 UTC 2014


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




More information about the dev mailing list