Looks good to me. This certainly makes it easier to get reproducible
performance results.
Acked-by: Joe Stringer <joestrin...@nicira.com>
On 27 January 2014 16:50, Ethan Jackson <et...@nicira.com> 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
>
> -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
