[ovs-dev] [PATCH] ofproto: Recycle least recently used ofport.

Gurucharan Shetty shettyg at nicira.com
Mon Aug 26 17:25:39 UTC 2013


If there is a lot of churn in creation and deletion of
interfaces, we may end up recycling the ofport value of a
recently deleted interface for a newly created interface.
This may result in an old stale openflow rule applying
on the newly created interface.

With this commit, when a new port is added, try and provide
it an ofport value that has not been used during the current
run. If such value does not exist, provide the least
recently used ofport value.

Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
---
 ofproto/ofproto-provider.h |   18 ++++++++++-
 ofproto/ofproto.c          |   75 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index d8b6a79..659e135 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -63,10 +63,10 @@ struct ofproto {
     /* Datapath. */
     struct hmap ports;          /* Contains "struct ofport"s. */
     struct shash port_by_name;
-    unsigned long *ofp_port_ids;/* Bitmap of used OpenFlow port numbers. */
     struct simap ofp_requests;  /* OpenFlow port number requests. */
     uint16_t alloc_port_no;     /* Last allocated OpenFlow port number. */
     uint16_t max_ports;         /* Max possible OpenFlow port num, plus one. */
+    struct hmap ofport_usage;   /* Map ofport to last used time. */
 
     /* Flow tables. */
     struct oftable *tables;
@@ -134,6 +134,22 @@ struct ofport {
     int mtu;
 };
 
+long long int ofport_get_usage(const struct ofproto *,
+                               ofp_port_t ofp_port);
+void ofport_set_usage(struct ofproto *, ofp_port_t ofp_port,
+                      long long int last_used);
+
+/* Ofport usage.
+ *
+ * Keeps track of the currently used and recently used ofport values and is
+ * used to prevent immediate recycling of ofport values. */
+struct ofport_usage {
+    struct hmap_node hmap_node; /* In struct ofproto's "ofport_usage" hmap. */
+    ofp_port_t ofp_port;        /* OpenFlow port number. */
+    long long int last_used;    /* Last time the 'ofp_port' was used. LLONG_MAX
+                                   represents in-use ofports. */
+};
+
 void ofproto_port_set_state(struct ofport *, enum ofputil_port_state);
 
 /* OpenFlow table flags:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 709fd07..916a0bb 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -430,6 +430,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     ofproto->dp_desc = NULL;
     ofproto->frag_handling = OFPC_FRAG_NORMAL;
     hmap_init(&ofproto->ports);
+    hmap_init(&ofproto->ofport_usage);
     shash_init(&ofproto->port_by_name);
     simap_init(&ofproto->ofp_requests);
     ofproto->max_ports = ofp_to_u16(OFPP_MAX);
@@ -459,11 +460,6 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
         return error;
     }
 
-    /* The "max_ports" member should have been set by ->construct(ofproto).
-     * Port 0 is not a valid OpenFlow port, so mark that as unavailable. */
-    ofproto->ofp_port_ids = bitmap_allocate(ofproto->max_ports);
-    bitmap_set1(ofproto->ofp_port_ids, 0);
-
     /* Check that hidden tables, if any, are at the end. */
     ovs_assert(ofproto->n_tables);
     for (i = 0; i + 1 < ofproto->n_tables; i++) {
@@ -1127,8 +1123,8 @@ ofproto_destroy__(struct ofproto *ofproto)
     free(ofproto->serial_desc);
     free(ofproto->dp_desc);
     hmap_destroy(&ofproto->ports);
+    hmap_destroy(&ofproto->ofport_usage);
     shash_destroy(&ofproto->port_by_name);
-    bitmap_free(ofproto->ofp_port_ids);
     simap_destroy(&ofproto->ofp_requests);
 
     OFPROTO_FOR_EACH_TABLE (table, ofproto) {
@@ -1148,6 +1144,7 @@ void
 ofproto_destroy(struct ofproto *p)
 {
     struct ofport *ofport, *next_ofport;
+    struct ofport_usage *usage, *next_usage;
 
     if (!p) {
         return;
@@ -1158,6 +1155,11 @@ ofproto_destroy(struct ofproto *p)
         ofport_destroy(ofport);
     }
 
+    HMAP_FOR_EACH_SAFE (usage, next_usage, hmap_node, &p->ofport_usage) {
+        hmap_remove(&p->ofport_usage, &usage->hmap_node);
+        free(usage);
+    }
+
     p->ofproto_class->destruct(p);
     ofproto_destroy__(p);
 }
@@ -1748,35 +1750,45 @@ alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
     port_idx = port_idx ? port_idx : UINT16_MAX;
 
     if (port_idx >= ofproto->max_ports
-        || bitmap_is_set(ofproto->ofp_port_ids, port_idx)) {
-        uint16_t end_port_no = ofproto->alloc_port_no;
+        || (ofport_get_usage(ofproto, u16_to_ofp(port_idx)) == LLONG_MAX)) {
+        uint16_t lru_ofport = 0, end_port_no = ofproto->alloc_port_no;
+        long long int last_used_at, lru = LLONG_MAX;
 
         /* Search for a free OpenFlow port number.  We try not to
          * immediately reuse them to prevent problems due to old
          * flows. */
         for (;;) {
             if (++ofproto->alloc_port_no >= ofproto->max_ports) {
-                ofproto->alloc_port_no = 0;
+                ofproto->alloc_port_no = 1;
             }
-            if (!bitmap_is_set(ofproto->ofp_port_ids,
-                               ofproto->alloc_port_no)) {
+            last_used_at = ofport_get_usage(ofproto,
+                                         u16_to_ofp(ofproto->alloc_port_no));
+            if (!last_used_at) {
                 port_idx = ofproto->alloc_port_no;
                 break;
+            } else if (last_used_at < lru) {
+                lru = last_used_at;
+                lru_ofport = ofproto->alloc_port_no;
             }
+
             if (ofproto->alloc_port_no == end_port_no) {
+                if (lru_ofport) {
+                    port_idx = lru_ofport;
+                    break;
+                }
                 return OFPP_NONE;
             }
         }
     }
-    bitmap_set1(ofproto->ofp_port_ids, port_idx);
+    ofport_set_usage(ofproto, u16_to_ofp(port_idx), LLONG_MAX);
     return u16_to_ofp(port_idx);
 }
 
 static void
-dealloc_ofp_port(const struct ofproto *ofproto, ofp_port_t ofp_port)
+dealloc_ofp_port(struct ofproto *ofproto, ofp_port_t ofp_port)
 {
     if (ofp_to_u16(ofp_port) < ofproto->max_ports) {
-        bitmap_set0(ofproto->ofp_port_ids, ofp_to_u16(ofp_port));
+        ofport_set_usage(ofproto, ofp_port, time_msec());
     }
 }
 
@@ -2001,6 +2013,41 @@ ofproto_get_port(const struct ofproto *ofproto, ofp_port_t ofp_port)
     return NULL;
 }
 
+long long int
+ofport_get_usage(const struct ofproto *ofproto, ofp_port_t ofp_port)
+{
+    struct ofport_usage *usage;
+
+    HMAP_FOR_EACH_IN_BUCKET (usage, hmap_node, hash_ofp_port(ofp_port),
+                             &ofproto->ofport_usage) {
+        if (usage->ofp_port == ofp_port) {
+            return usage->last_used;
+        }
+    }
+    return 0;
+}
+
+void
+ofport_set_usage(struct ofproto *ofproto, ofp_port_t ofp_port,
+                 long long int last_used)
+{
+    struct ofport_usage *usage;
+    HMAP_FOR_EACH_IN_BUCKET (usage, hmap_node, hash_ofp_port(ofp_port),
+                             &ofproto->ofport_usage) {
+        if (usage->ofp_port == ofp_port) {
+            usage->last_used = last_used;
+            return;
+        }
+    }
+    ovs_assert(last_used == LLONG_MAX);
+
+    usage = xmalloc(sizeof *usage);
+    usage->ofp_port = ofp_port;
+    usage->last_used = last_used;
+    hmap_insert(&ofproto->ofport_usage, &usage->hmap_node,
+                hash_ofp_port(ofp_port));
+}
+
 int
 ofproto_port_get_stats(const struct ofport *port, struct netdev_stats *stats)
 {
-- 
1.7.9.5




More information about the dev mailing list