ofproto: Add a fixed bundle idle timeout of 10 seconds.

Timing out idle bundles frees memory that would effectively be leaked if a long standing OpenFlow connection would fail to commit or discard a bundle. OpenFlow specification mandates the timeout to be at least one second, if the switch implements such a timeout. This patch makes the bundle idle timeout to be 10 seconds. We do not limit the number of messages in a bundle, so it does not make sense to limit the number of bundles either, especially now that idle bundles are timed out. Signed-off-by: Jarno Rajahalme <[email protected]> Acked-by: Ben Pfaff <[email protected]>
water0615 · Sep 13, 2016 · 51bb26f · 51bb26f
1 parent 68030e1
commit 51bb26f
Show file tree

Hide file tree

Showing 7 changed files with 150 additions and 20 deletions.
diff --git a/NEWS b/NEWS
@@ -1,6 +1,8 @@
 Post-v2.6.0
 ---------------------
-
+   - OpenFlow:
+     * Bundles now expire after 10 seconds since the last time the
+       bundle was either opened, modified, or closed.
 
 v2.6.0 - xx xxx xxxx
 ---------------------

diff --git a/ofproto/bundles.c b/ofproto/bundles.c
@@ -1,7 +1,7 @@
 /*
  * Copyright (c) 2013, 2014 Alexandru Copot <[email protected]>, with support from IXIA.
  * Copyright (c) 2013, 2014 Daniel Baluta <[email protected]>
- * Copyright (c) 2014, 2015 Nicira, Inc.
+ * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -41,18 +41,23 @@
 VLOG_DEFINE_THIS_MODULE(bundles);
 
 static struct ofp_bundle *
-ofp_bundle_create(uint32_t id, uint16_t flags)
+ofp_bundle_create(uint32_t id, uint16_t flags, const struct ofp_header *oh)
 {
     struct ofp_bundle *bundle;
 
     bundle = xmalloc(sizeof(*bundle));
 
+    bundle->used = time_msec();
     bundle->id = id;
     bundle->flags = flags;
     bundle->state = BS_OPEN;
 
     ovs_list_init(&bundle->msg_list);
 
+    /* Max 64 bytes for error reporting. */
+    memcpy(bundle->ofp_msg_data, oh,
+           MIN(ntohs(oh->length), sizeof bundle->ofp_msg_data));
+
     return bundle;
 }
 
@@ -75,7 +80,8 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle,
 }
 
 enum ofperr
-ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t flags)
+ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t flags,
+                const struct ofp_header *oh)
 {
     struct ofp_bundle *bundle;
     enum ofperr error;
@@ -89,7 +95,7 @@ ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t flags)
         return OFPERR_OFPBFC_BAD_ID;
     }
 
-    bundle = ofp_bundle_create(id, flags);
+    bundle = ofp_bundle_create(id, flags, oh);
     error = ofconn_insert_bundle(ofconn, bundle);
     if (error) {
         free(bundle);
@@ -119,6 +125,7 @@ ofp_bundle_close(struct ofconn *ofconn, uint32_t id, uint16_t flags)
         return OFPERR_OFPBFC_BAD_FLAGS;
     }
 
+    bundle->used = time_msec();
     bundle->state = BS_CLOSED;
     return 0;
 }
@@ -141,7 +148,8 @@ ofp_bundle_discard(struct ofconn *ofconn, uint32_t id)
 
 enum ofperr
 ofp_bundle_add_message(struct ofconn *ofconn, uint32_t id, uint16_t flags,
-                       struct ofp_bundle_entry *bmsg)
+                       struct ofp_bundle_entry *bmsg,
+                       const struct ofp_header *oh)
 {
     struct ofp_bundle *bundle;
 
@@ -150,7 +158,7 @@ ofp_bundle_add_message(struct ofconn *ofconn, uint32_t id, uint16_t flags,
     if (!bundle) {
         enum ofperr error;
 
-        bundle = ofp_bundle_create(id, flags);
+        bundle = ofp_bundle_create(id, flags, oh);
         error = ofconn_insert_bundle(ofconn, bundle);
         if (error) {
             free(bundle);
@@ -164,6 +172,7 @@ ofp_bundle_add_message(struct ofconn *ofconn, uint32_t id, uint16_t flags,
         return OFPERR_OFPBFC_BAD_FLAGS;
     }
 
+    bundle->used = time_msec();
     ovs_list_push_back(&bundle->msg_list, &bmsg->node);
     return 0;
 }
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
@@ -42,33 +42,45 @@ struct ofp_bundle_entry {
     };
 
     /* OpenFlow header and some of the message contents for error reporting. */
-    struct ofp_header ofp_msg[DIV_ROUND_UP(64, sizeof(struct ofp_header))];
+    union {
+        struct ofp_header ofp_msg;
+        uint8_t ofp_msg_data[64];
+    };
 };
 
-enum bundle_state {
+enum OVS_PACKED_ENUM bundle_state {
     BS_OPEN,
     BS_CLOSED
 };
 
 struct ofp_bundle {
     struct hmap_node  node;      /* In struct ofconn's "bundles" hmap. */
+    long long int     used;      /* Last time bundle was used. */
     uint32_t          id;
     uint16_t          flags;
     enum bundle_state state;
 
     /* List of 'struct bundle_message's */
     struct ovs_list   msg_list;
+
+    /* OpenFlow header and some of the message contents for error reporting. */
+    union {
+        struct ofp_header ofp_msg;
+        uint8_t ofp_msg_data[64];
+    };
 };
 
 static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc(
     enum ofptype type, const struct ofp_header *oh);
 static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *);
 
-enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags);
+enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags,
+                            const struct ofp_header *);
 enum ofperr ofp_bundle_close(struct ofconn *, uint32_t id, uint16_t flags);
 enum ofperr ofp_bundle_discard(struct ofconn *, uint32_t id);
 enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t id,
-                                   uint16_t flags, struct ofp_bundle_entry *);
+                                   uint16_t flags, struct ofp_bundle_entry *,
+                                   const struct ofp_header *);
 
 void ofp_bundle_remove__(struct ofconn *, struct ofp_bundle *, bool success);
 
@@ -80,7 +92,8 @@ ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh)
     entry->type = type;
 
     /* Max 64 bytes for error reporting. */
-    memcpy(entry->ofp_msg, oh, MIN(ntohs(oh->length), sizeof entry->ofp_msg));
+    memcpy(entry->ofp_msg_data, oh,
+           MIN(ntohs(oh->length), sizeof entry->ofp_msg_data));
 
     return entry;
 }

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
@@ -131,6 +131,14 @@ struct ofconn {
 
     /* Active bundles. Contains "struct ofp_bundle"s. */
     struct hmap bundles;
+    long long int next_bundle_expiry_check;
+};
+
+/* vswitchd/ovs-vswitchd.8.in documents the value of BUNDLE_IDLE_LIFETIME in
+ * seconds.  That documentation must be kept in sync with the value below. */
+enum {
+    BUNDLE_EXPIRY_INTERVAL = 1000,  /* Check bundle expiry every 1 sec. */
+    BUNDLE_IDLE_TIMEOUT = 10000,    /* Expire idle bundles after 10 seconds. */
 };
 
 static struct ofconn *ofconn_create(struct connmgr *, struct rconn *,
@@ -1171,8 +1179,6 @@ ofconn_get_bundle(struct ofconn *ofconn, uint32_t id)
 enum ofperr
 ofconn_insert_bundle(struct ofconn *ofconn, struct ofp_bundle *bundle)
 {
-    /* XXX: Check the limit of open bundles */
-
     hmap_insert(&ofconn->bundles, &bundle->node, bundle_hash(bundle->id));
 
     return 0;
@@ -1195,6 +1201,20 @@ bundle_remove_all(struct ofconn *ofconn)
         ofp_bundle_remove__(ofconn, b, false);
     }
 }
+
+static void
+bundle_remove_expired(struct ofconn *ofconn, long long int now)
+{
+    struct ofp_bundle *b, *next;
+    long long int limit = now - BUNDLE_IDLE_TIMEOUT;
+
+    HMAP_FOR_EACH_SAFE (b, next, node, &ofconn->bundles) {
+        if (b->used <= limit) {
+            ofconn_send_error(ofconn, &b->ofp_msg, OFPERR_OFPBFC_TIMEOUT);
+            ofp_bundle_remove__(ofconn, b, false);
+        }
+    }
+}
 
 /* Private ofconn functions. */
 
@@ -1222,6 +1242,7 @@ ofconn_create(struct connmgr *mgr, struct rconn *rconn, enum ofconn_type type,
     ovs_list_init(&ofconn->updates);
 
     hmap_init(&ofconn->bundles);
+    ofconn->next_bundle_expiry_check = time_msec() + BUNDLE_EXPIRY_INTERVAL;
 
     ofconn_flush(ofconn);
 
@@ -1366,7 +1387,14 @@ ofconn_run(struct ofconn *ofconn,
         ofpbuf_delete(of_msg);
     }
 
-    if (time_msec() >= ofconn->next_op_report) {
+    long long int now = time_msec();
+
+    if (now >= ofconn->next_bundle_expiry_check) {
+        ofconn->next_bundle_expiry_check = now + BUNDLE_EXPIRY_INTERVAL;
+        bundle_remove_expired(ofconn, now);
+    }
+
+    if (now >= ofconn->next_op_report) {
         ofconn_log_flow_mods(ofconn);
     }
 

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
@@ -7277,7 +7277,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
         if (error) {
             /* Send error referring to the original message. */
             if (error) {
-                ofconn_send_error(ofconn, be->ofp_msg, error);
+                ofconn_send_error(ofconn, &be->ofp_msg, error);
                 error = OFPERR_OFPBFC_MSG_FAILED;
             }
 
@@ -7304,7 +7304,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                     port_mod_finish(ofconn, &be->opm.pm, be->opm.port);
                 } else {
                     struct openflow_mod_requester req = { ofconn,
-                                                          be->ofp_msg };
+                                                          &be->ofp_msg };
                     if (be->type == OFPTYPE_FLOW_MOD) {
                         /* Bump the lookup version to the one of the current
                          * message.  This makes all the changes in the bundle
@@ -7362,8 +7362,8 @@ handle_bundle_control(struct ofconn *ofconn, const struct ofp_header *oh)
     reply.bundle_id = bctrl.bundle_id;
 
     switch (bctrl.type) {
-        case OFPBCT_OPEN_REQUEST:
-        error = ofp_bundle_open(ofconn, bctrl.bundle_id, bctrl.flags);
+    case OFPBCT_OPEN_REQUEST:
+        error = ofp_bundle_open(ofconn, bctrl.bundle_id, bctrl.flags, oh);
         reply.type = OFPBCT_OPEN_REPLY;
         break;
     case OFPBCT_CLOSE_REQUEST:
@@ -7441,7 +7441,7 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
 
     if (!error) {
         error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags,
-                                       bmsg);
+                                       bmsg, oh);
     }
 
     if (error) {

diff --git a/tests/ofproto.at b/tests/ofproto.at
@@ -4933,6 +4933,69 @@ OVS_VSWITCHD_STOP
 AT_CLEANUP
 
 
+AT_SETUP([ofproto - bundle timeout (OpenFlow 1.4)])
+AT_KEYWORDS([monitor])
+OVS_VSWITCHD_START
+
+# Start a monitor, use the required protocol version
+ovs-ofctl -O OpenFlow14 monitor br0 --detach --no-chdir --pidfile >monitor.log 2>&1
+AT_CAPTURE_FILE([monitor.log])
+
+ovs-appctl time/stop
+
+# Send an OpenFlow14 message (05), OFPT_BUNDLE_CONTROL (21), length (10), xid (01)
+ovs-appctl -t ovs-ofctl ofctl/send "05 21 00 10 00 00 00 01 00 00 00 01 00 00 00 03"
+ovs-appctl time/warp 8000
+# Send a bundle flow mod, it should keep the bundle alive.
+ovs-appctl -t ovs-ofctl ofctl/send "05 22 00 a0 00 00 00 02 00 00 00 01 00 00 00 03 \
+05 0e 00 90 00 00 00 02 00 00 00 00 00 00 00 00 \
+00 00 00 00 00 00 00 00 01 00 00 00 00 00 ff ff \
+ff ff ff ff ff ff ff ff ff ff ff ff 00 00 00 00 \
+00 01 00 42 80 00 00 04 00 00 00 01 80 00 08 06 \
+50 54 00 00 00 06 80 00 06 06 50 54 00 00 00 05 \
+80 00 0a 02 08 06 80 00 0c 02 00 00 80 00 2a 02 \
+00 02 80 00 2c 04 c0 a8 00 02 80 00 2e 04 c0 a8 \
+00 01 00 00 00 00 00 00 00 04 00 18 00 00 00 00 \
+00 00 00 10 00 00 00 03 00 00 00 00 00 00 00 00 \
+"
+ovs-appctl time/warp 8000
+# Send a bundle close, it should keep the bundle alive.
+ovs-appctl -t ovs-ofctl ofctl/send "05 21 00 10 00 00 00 03 00 00 00 01 00 02 00 03"
+ovs-appctl time/warp 11000
+# Make sure that timeouts are processed after the expiry
+ovs-appctl time/warp 1000
+# Send a Commit, but too late.
+ovs-appctl -t ovs-ofctl ofctl/send "05 21 00 10 00 00 00 04 00 00 00 01 00 04 00 03"
+ovs-appctl -t ovs-ofctl ofctl/barrier
+OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
+
+AT_CHECK([ofctl_strip < monitor.log], [], [dnl
+send: OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=OPEN_REQUEST flags=atomic ordered
+OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=OPEN_REPLY flags=0
+send: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
+ bundle_id=0x1 flags=atomic ordered
+OFPT_FLOW_MOD (OF1.4): ADD table:1 priority=65535,arp,in_port=1,vlan_tci=0x0000/0x1fff,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,arp_spa=192.168.0.2,arp_tpa=192.168.0.1,arp_op=2 actions=output:3
+send: OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=CLOSE_REQUEST flags=atomic ordered
+OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=CLOSE_REPLY flags=0
+OFPT_ERROR (OF1.4): OFPBFC_TIMEOUT
+OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=OPEN_REQUEST flags=atomic ordered
+send: OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=COMMIT_REQUEST flags=atomic ordered
+OFPT_ERROR (OF1.4): OFPBFC_BAD_ID
+OFPT_BUNDLE_CONTROL (OF1.4):
+ bundle_id=0x1 type=COMMIT_REQUEST flags=atomic ordered
+OFPT_BARRIER_REPLY (OF1.4):
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
 AT_SETUP([ofproto - bundle open (OpenFlow 1.3)])
 AT_KEYWORDS([monitor])
 OVS_VSWITCHD_START

diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
@@ -287,6 +287,21 @@ of time before buffers may be reused.
 .PP
 Open vSwitch does not maintains any packet buffers.
 .
+.SS "Bundle lifetime"
+The OpenFlow specification, version 1.4, says:
+.
+.IP
+If the switch does not receive any OFPT_BUNDLE_CONTROL or
+OFPT_BUNDLE_ADD_MESSAGE message for an opened bundle_id for a switch
+defined time greater than 1s, it may send an ofp_error_msg with
+OFPET_BUNDLE_FAILED type and OFPBFC_TIMEOUT code.  If the switch does
+not receive any new message in a bundle apart from echo request and
+replies for a switch defined time greater than 1s, it may send an
+ofp_error_msg with OFPET_BUNDLE_FAILED type and OFPBFC_TIMEOUT code.
+.
+.PP
+Open vSwitch implements idle bundle lifetime of 10 seconds.
+.
 .SH "LIMITS"
 .
 .PP