ofproto-dpif-upcall: Hardcode max_idle to 1500ms.

Before this patch, OVS tried to guess an optimal max idle time for datapath flows based on the number of datapath flows relative to the limit. This caused instability because the limit was based on the dump duration which was affected by the max idle time. This patch chooses instead to hardcode the max idle time to 1.5s except in extreme case where the datapath flow limit is exceeded. 1.5s was chosen to ensure pings occurring at once per second stay cached in the datapath. Signed-off-by: Ethan Jackson <[email protected]> Acked-by: Joe Stringer <[email protected]>
kalyazin · Jan 29, 2014 · 0a8763f · 0a8763f
1 parent 08d74a9
commit 0a8763f
Show file tree

Hide file tree

Showing 2 changed files with 6 additions and 29 deletions.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
@@ -41,6 +41,7 @@
 #define MAX_QUEUE_LENGTH 512
 #define FLOW_MISS_MAX_BATCH 50
 #define REVALIDATE_MAX_BATCH 50
+#define MAX_IDLE 1500
 
 VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
 
@@ -125,7 +126,6 @@ struct udpif {
     unsigned int avg_n_flows;
 
     /* Following fields are accessed and modified by different threads. */
-    atomic_llong max_idle;             /* Maximum datapath flow idle time. */
     atomic_uint flow_limit;            /* Datapath flow hard limit. */
 
     /* n_flows_mutex prevents multiple threads updating these concurrently. */
@@ -259,7 +259,6 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif)
 
     udpif->dpif = dpif;
     udpif->backer = backer;
-    atomic_init(&udpif->max_idle, 5000);
     atomic_init(&udpif->flow_limit, MIN(ofproto_flow_limit, 10000));
     udpif->secret = random_uint32();
     udpif->reval_seq = seq_create();
@@ -283,7 +282,6 @@ udpif_destroy(struct udpif *udpif)
     latch_destroy(&udpif->exit_latch);
     seq_destroy(udpif->reval_seq);
     seq_destroy(udpif->dump_seq);
-    atomic_destroy(&udpif->max_idle);
     atomic_destroy(&udpif->flow_limit);
     atomic_destroy(&udpif->n_flows);
     atomic_destroy(&udpif->n_flows_timestamp);
@@ -533,7 +531,6 @@ udpif_flow_dumper(void *arg)
         struct dpif_flow_dump dump;
         size_t key_len, mask_len;
         unsigned int flow_limit;
-        long long int max_idle;
         bool need_revalidate;
         uint64_t reval_seq;
         size_t n_flows, i;
@@ -546,18 +543,6 @@ udpif_flow_dumper(void *arg)
         udpif->max_n_flows = MAX(n_flows, udpif->max_n_flows);
         udpif->avg_n_flows = (udpif->avg_n_flows + n_flows) / 2;
 
-        atomic_read(&udpif->flow_limit, &flow_limit);
-        if (n_flows < flow_limit / 8) {
-            max_idle = 5000;
-        } else if (n_flows < flow_limit / 4) {
-            max_idle = 2000;
-        } else if (n_flows < flow_limit / 2) {
-            max_idle = 1000;
-        } else {
-            max_idle = 500;
-        }
-        atomic_store(&udpif->max_idle, max_idle);
-
         start_time = time_msec();
         dpif_flow_dump_start(&dump, udpif->dpif);
         while (dpif_flow_dump_next(&dump, &key, &key_len, &mask, &mask_len,
@@ -617,6 +602,7 @@ udpif_flow_dumper(void *arg)
 
         duration = MAX(time_msec() - start_time, 1);
         udpif->dump_duration = duration;
+        atomic_read(&udpif->flow_limit, &flow_limit);
         if (duration > 2000) {
             flow_limit /= duration / 1000;
         } else if (duration > 1300) {
@@ -633,7 +619,7 @@ udpif_flow_dumper(void *arg)
                       duration);
         }
 
-        poll_timer_wait_until(start_time + MIN(max_idle, 500));
+        poll_timer_wait_until(start_time + MIN(MAX_IDLE, 500));
         seq_wait(udpif->reval_seq, udpif->last_reval_seq);
         latch_wait(&udpif->exit_latch);
         poll_block();
@@ -1386,12 +1372,12 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps)
     long long int max_idle;
     bool must_del;
 
-    atomic_read(&udpif->max_idle, &max_idle);
     atomic_read(&udpif->flow_limit, &flow_limit);
 
     n_flows = udpif_get_n_flows(udpif);
 
     must_del = false;
+    max_idle = MAX_IDLE;
     if (n_flows > flow_limit) {
         must_del = n_flows > 2 * flow_limit;
         max_idle = 100;
@@ -1526,17 +1512,14 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
 
     LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
         unsigned int flow_limit;
-        long long int max_idle;
         size_t i;
 
         atomic_read(&udpif->flow_limit, &flow_limit);
-        atomic_read(&udpif->max_idle, &max_idle);
 
         ds_put_format(&ds, "%s:\n", dpif_name(udpif->dpif));
         ds_put_format(&ds, "\tflows         : (current %"PRIu64")"
             " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif),
             udpif->avg_n_flows, udpif->max_n_flows, flow_limit);
-        ds_put_format(&ds, "\tmax idle      : %lldms\n", max_idle);
         ds_put_format(&ds, "\tdump duration : %lldms\n", udpif->dump_duration);
 
         ds_put_char(&ds, '\n');

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
@@ -2442,20 +2442,14 @@ AT_CHECK([ovs-ofctl add-flow br1 actions=LOCAL,output:1,output:3])
 
 for i in $(seq 1 10); do
     ovs-appctl netdev-dummy/receive br0 'in_port(100),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
-    if [[ $i -eq 1 ]]; then
-        sleep 1
-    fi
 done
 
 for i in $(seq 1 5); do
     ovs-appctl netdev-dummy/receive br1 'in_port(101),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
-    if [[ $i -eq 1 ]]; then
-        sleep 1
-    fi
 done
 
-AT_CHECK([ovs-appctl time/warp 1000 && ovs-appctl time/warp 1000], [0], [warped
-warped
+AT_CHECK([ovs-appctl time/warp 500], [0],
+[warped
 ])
 
 AT_CHECK([ovs-appctl dpif/show], [0], [dnl