Re: [ovs-dev] [PATCH] ofproto: Allow bundle idle timeout to be configured.

2018-04-19 Thread Flavio Leitner 
On Fri, Apr 13, 2018 at 10:48:09AM -0700, Ben Pfaff wrote:
> On Fri, Apr 13, 2018 at 01:45:30PM -0300, Flavio Leitner wrote:
> > In some cases 10 seconds might be too much time and in
> > other cases it might be too little.
> > 
> > The OpenFlow spec mandates that it should wait at least one
> > second, so enforce that as the minimum acceptable value.
> > 
> > Signed-off-by: Flavio Leitner 
> 
> Thanks for the patch.
> 
> I don't think that this will set the idle timeout back to the default if
> the setting is removed.  It's better if there's that behavior.

Agreed.

> What do you think of this incremental?  I have not tested it but it is
> meant to behave that way.  Also it avoids integer overflow.

I sent out a v2 including your fix and another test case:
https://mail.openvswitch.org/pipermail/ovs-dev/2018-April/346319.html

Thanks Ben,
fbl

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto: Allow bundle idle timeout to be configured.

2018-04-13 Thread Ben Pfaff 
On Fri, Apr 13, 2018 at 01:45:30PM -0300, Flavio Leitner wrote:
> In some cases 10 seconds might be too much time and in
> other cases it might be too little.
> 
> The OpenFlow spec mandates that it should wait at least one
> second, so enforce that as the minimum acceptable value.
> 
> Signed-off-by: Flavio Leitner 

Thanks for the patch.

I don't think that this will set the idle timeout back to the default if
the setting is removed.  It's better if there's that behavior.

What do you think of this incremental?  I have not tested it but it is
meant to behave that way.  Also it avoids integer overflow.

Thanks,

Ben.

--8<--cut here-->8--

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 677deef948ea..f78b4c5ff411 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -37,6 +37,7 @@
 #include "openvswitch/poll-loop.h"
 #include "openvswitch/rconn.h"
 #include "openvswitch/shash.h"
+#include "sat-math.h"
 #include "simap.h"
 #include "stream.h"
 #include "timeval.h"
@@ -142,7 +143,7 @@ struct ofconn {
 #define BUNDLE_IDLE_TIMEOUT_DEFAULT 1   /* Expire idle bundles after
  * 10 seconds. */
 
-static int bundle_idle_timeout = BUNDLE_IDLE_TIMEOUT_DEFAULT;
+static unsigned int bundle_idle_timeout = BUNDLE_IDLE_TIMEOUT_DEFAULT;
 
 static struct ofconn *ofconn_create(struct connmgr *, struct rconn *,
 enum ofconn_type, bool enable_async_msgs)
@@ -471,12 +472,16 @@ ofconn_get_ofproto(const struct ofconn *ofconn)
 return ofconn->connmgr->ofproto;
 }
 
+/* Sets the bundle idle timeout to 'timeout' seconds, interpreting 0 as
+ * requesting the default timeout.
+ *
+ * The OpenFlow spec mandates the timeout to be at least one second; . */
 void
-connmgr_set_bundle_idle_timeout(unsigned timeout) {
-/* OpenFlow spec mandates the timeout to be at least one second. */
-if (timeout > 0) {
-bundle_idle_timeout = timeout * 1000;
-}
+connmgr_set_bundle_idle_timeout(unsigned timeout)
+{
+bundle_idle_timeout = (timeout
+   ? sat_mul(timeout, 1000)
+   : BUNDLE_IDLE_TIMEOUT_DEFAULT);
 }
 
 /* OpenFlow configuration. */
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ofproto: Allow bundle idle timeout to be configured.

2018-04-13 Thread Flavio Leitner 
In some cases 10 seconds might be too much time and in
other cases it might be too little.

The OpenFlow spec mandates that it should wait at least one
second, so enforce that as the minimum acceptable value.

Signed-off-by: Flavio Leitner 
---
 ofproto/connmgr.c  | 19 ++
 ofproto/connmgr.h  |  2 ++
 ofproto/ofproto.c  |  7 ++
 ofproto/ofproto.h  |  1 +
 tests/ofproto.at   | 62 ++
 vswitchd/bridge.c  |  3 ++-
 vswitchd/ovs-vswitchd.8.in |  5 +++-
 vswitchd/vswitch.xml   | 12 +
 8 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 964b8c8d8..677deef94 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -138,10 +138,11 @@ struct ofconn {
 
 /* 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 = 1,/* Expire idle bundles after 10 seconds. */
-};
+#define BUNDLE_EXPIRY_INTERVAL 1000 /* Check bundle expiry every 1 sec. */
+#define BUNDLE_IDLE_TIMEOUT_DEFAULT 1   /* Expire idle bundles after
+ * 10 seconds. */
+
+static int bundle_idle_timeout = BUNDLE_IDLE_TIMEOUT_DEFAULT;
 
 static struct ofconn *ofconn_create(struct connmgr *, struct rconn *,
 enum ofconn_type, bool enable_async_msgs)
@@ -469,6 +470,14 @@ ofconn_get_ofproto(const struct ofconn *ofconn)
 {
 return ofconn->connmgr->ofproto;
 }
+
+void
+connmgr_set_bundle_idle_timeout(unsigned timeout) {
+/* OpenFlow spec mandates the timeout to be at least one second. */
+if (timeout > 0) {
+bundle_idle_timeout = timeout * 1000;
+}
+}
 
 /* OpenFlow configuration. */
 
@@ -1247,7 +1256,7 @@ 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;
+long long int limit = now - bundle_idle_timeout;
 
 HMAP_FOR_EACH_SAFE (b, next, node, >bundles) {
 if (b->used <= limit) {
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 2405fbd79..eb3be1668 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -86,6 +86,8 @@ void connmgr_get_memory_usage(const struct connmgr *, struct 
simap *usage);
 
 struct ofproto *ofconn_get_ofproto(const struct ofconn *);
 
+void connmgr_set_bundle_idle_timeout(unsigned timeout);
+
 void connmgr_retry(struct connmgr *);
 
 /* OpenFlow configuration. */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 36f4c0b16..4af7e6486 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -680,6 +680,13 @@ ofproto_set_in_band_queue(struct ofproto *ofproto, int 
queue_id)
 connmgr_set_in_band_queue(ofproto->connmgr, queue_id);
 }
 
+/* Sets the bundle max idle timeout */
+void
+ofproto_set_bundle_idle_timeout(unsigned timeout)
+{
+connmgr_set_bundle_idle_timeout(timeout);
+}
+
 /* Sets the number of flows at which eviction from the kernel flow table
  * will occur. */
 void
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 8c85bbf7f..3ca88baf0 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -312,6 +312,7 @@ void ofproto_reconnect_controllers(struct ofproto *);
 void ofproto_set_extra_in_band_remotes(struct ofproto *,
const struct sockaddr_in *, size_t n);
 void ofproto_set_in_band_queue(struct ofproto *, int queue_id);
+void ofproto_set_bundle_idle_timeout(unsigned timeout);
 void ofproto_set_flow_limit(unsigned limit);
 void ofproto_set_max_idle(unsigned max_idle);
 void ofproto_set_forward_bpdu(struct ofproto *, bool forward_bpdu);
diff --git a/tests/ofproto.at b/tests/ofproto.at
index c1beea7ae..f5a75dbcb 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -5456,6 +5456,68 @@ OFPT_BARRIER_REPLY (OF1.4):
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto - bundle custom timeout (OpenFlow 1.4)])
+AT_KEYWORDS([monitor])
+OVS_VSWITCHD_START([set Open_vSwitch . other_config:bundle-idle-timeout=4])
+
+# 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 2000
+# 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