I think you're right. I'll write up a patch.
Ethan
On Wed, Jan 29, 2014 at 4:39 PM, YAMAMOTO Takashi
<yamam...@valinux.co.jp> wrote:
>> 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 <et...@nicira.com>
>> ---
>> ofproto/ofproto-dpif-upcall.c | 25 ++++---------------------
>> tests/ofproto-dpif.at | 10 ++--------
>> 2 files changed, 6 insertions(+), 29 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 24919db..9f761b8 100644
>> --- 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 = time_msec() - start_time;
>> 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
>> index 23a1f14..1348790 100644
>> --- 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
>
> won't this re-introduce races workarounded by commit bf06c4fe?
>
> YAMAMOTO Takashi
>
>>
>> -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
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
