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 <ja...@ovn.org>
---
v3: New patch for v3.
NEWS | 4 +++-
ofproto/bundles.c | 21 +++++++++++++------
ofproto/bundles.h | 23 +++++++++++++++-----
ofproto/connmgr.c | 32 +++++++++++++++++++++++++---
ofproto/ofproto.c | 10 ++++-----
tests/ofproto.at | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 133 insertions(+), 20 deletions(-)
diff --git a/NEWS b/NEWS
index 8c78b36..9e8034a 100644
--- 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
index 353a3a7..e0b4b7d 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -1,7 +1,7 @@
/*
* Copyright (c) 2013, 2014 Alexandru Copot <alex.miha...@gmail.com>, with
support from IXIA.
* Copyright (c) 2013, 2014 Daniel Baluta <dbal...@ixiacom.com>
- * 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
index 802de77..3069224 100644
--- 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
index 51e676a..6014899 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -131,6 +131,12 @@ struct ofconn {
/* Active bundles. Contains "struct ofp_bundle"s. */
struct hmap bundles;
+ long long int next_bundle_expiry_check;
+};
+
+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 +1177,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 +1199,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 +1240,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 +1385,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
index f4f4fd6..cfc4d41 100644
--- 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
index 6901565..24867c7 100644
--- 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
--
2.1.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
