I think the use of the atomic variable in this patch is a bit odd.
ofproto_max_idle, is a regular old variable, even though it can be
written and read from different threads: the flow dumper and the main
thread. But when shared among the revalidators, then we create a
special atomic in the struct udpif.
At any rate, I think it's good enough to just read ofproto_max_idle
directly whenever we need it. If it changes, all the threads may not
get the update at the same time, but I don't think its a big deal.
What do you think?
Ethan
On Tue, Feb 11, 2014 at 2:19 PM, Joe Stringer <joestrin...@nicira.com> wrote:
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> ofproto/ofproto-dpif-upcall.c | 8 +++++---
> ofproto/ofproto-provider.h | 4 ++++
> ofproto/ofproto.c | 9 +++++++++
> ofproto/ofproto.h | 2 ++
> vswitchd/bridge.c | 2 ++
> vswitchd/vswitch.ovsschema | 8 ++++++--
> vswitchd/vswitch.xml | 21 +++++++++++++++++++++
> 7 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index e0a5aed..399da8a 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -41,7 +41,6 @@
> #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);
>
> @@ -127,6 +126,7 @@ struct udpif {
>
> /* Following fields are accessed and modified by different threads. */
> atomic_uint flow_limit; /* Datapath flow hard limit. */
> + atomic_llong max_idle; /* Max idle time for flows. */
>
> /* n_flows_mutex prevents multiple threads updating these concurrently.
> */
> atomic_uint64_t n_flows; /* Number of flows in the datapath. */
> @@ -263,6 +263,7 @@ udpif_create(struct dpif_backer *backer, struct dpif
> *dpif)
> udpif->dpif = dpif;
> udpif->backer = backer;
> atomic_init(&udpif->flow_limit, MIN(ofproto_flow_limit, 10000));
> + atomic_init(&udpif->max_idle, ofproto_max_idle);
> udpif->secret = random_uint32();
> udpif->reval_seq = seq_create();
> udpif->dump_seq = seq_create();
> @@ -622,7 +623,8 @@ udpif_flow_dumper(void *arg)
> duration);
> }
>
> - poll_timer_wait_until(start_time + MIN(MAX_IDLE, 500));
> + atomic_store(&udpif->max_idle, ofproto_max_idle);
> + poll_timer_wait_until(start_time + MIN(ofproto_max_idle, 500));
> seq_wait(udpif->reval_seq, udpif->last_reval_seq);
> latch_wait(&udpif->exit_latch);
> poll_block();
> @@ -1383,7 +1385,7 @@ revalidate_udumps(struct revalidator *revalidator,
> struct list *udumps)
> n_flows = udpif_get_n_flows(udpif);
>
> must_del = false;
> - max_idle = MAX_IDLE;
> + atomic_read(&udpif->max_idle, &max_idle);
> if (n_flows > flow_limit) {
> must_del = n_flows > 2 * flow_limit;
> max_idle = 100;
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 19d1551..4911788 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -459,6 +459,10 @@ void rule_collection_destroy(struct rule_collection *);
> * ofproto-dpif implementation. */
> extern unsigned ofproto_flow_limit;
>
> +/* Determines how long flows may be idle in the datapath before they are
> + * expired. */
> +extern long long int ofproto_max_idle;
> +
> /* Number of upcall handler and revalidator threads. Only affects the
> * ofproto-dpif implementation. */
> extern size_t n_handlers, n_revalidators;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index a9cf221..cd027cf 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -307,6 +307,7 @@ static size_t allocated_ofproto_classes;
> struct ovs_mutex ofproto_mutex = OVS_MUTEX_INITIALIZER;
>
> unsigned ofproto_flow_limit = OFPROTO_FLOW_LIMIT_DEFAULT;
> +long long int ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT;
> enum ofproto_flow_miss_model flow_miss_model = OFPROTO_HANDLE_MISS_AUTO;
>
> size_t n_handlers, n_revalidators;
> @@ -698,6 +699,14 @@ ofproto_set_flow_limit(unsigned limit)
> ofproto_flow_limit = limit;
> }
>
> +/* Sets the maximum idle time for flows in the datapath before they are
> + * evicted. */
> +void
> +ofproto_set_max_idle(long long int max_idle)
> +{
> + ofproto_max_idle = max_idle;
> +}
> +
> /* Sets the path for handling flow misses. */
> void
> ofproto_set_flow_miss_model(unsigned model)
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 0ac4454..a5d7824 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -214,6 +214,7 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
> )
>
> #define OFPROTO_FLOW_LIMIT_DEFAULT 200000
> +#define OFPROTO_MAX_IDLE_DEFAULT 1200
>
> /* How flow misses should be handled in ofproto-dpif */
> enum ofproto_flow_miss_model {
> @@ -243,6 +244,7 @@ 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_flow_limit(unsigned limit);
> +void ofproto_set_max_idle(long long int max_idle);
> void ofproto_set_flow_miss_model(unsigned model);
> void ofproto_set_forward_bpdu(struct ofproto *, bool forward_bpdu);
> void ofproto_set_mac_table_config(struct ofproto *, unsigned idle_time,
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index cde4bd0..455ba14 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -494,6 +494,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
>
> ofproto_set_flow_limit(smap_get_int(&ovs_cfg->other_config, "flow-limit",
> OFPROTO_FLOW_LIMIT_DEFAULT));
> + ofproto_set_max_idle(smap_get_int(&ovs_cfg->other_config, "max-idle",
> + OFPROTO_MAX_IDLE_DEFAULT));
>
> ofproto_set_threads(
> smap_get_int(&ovs_cfg->other_config, "n-handler-threads", 0),
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 9eb21ed..c98b561 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
> {"name": "Open_vSwitch",
> - "version": "7.4.0",
> - "cksum": "951746691 20389",
> + "version": "7.4.1",
> + "cksum": "3218236985 20562",
> "tables": {
> "Open_vSwitch": {
> "columns": {
> @@ -295,6 +295,10 @@
> "flow_limit": {
> "type": {"key": {"type": "integer", "minInteger": 0},
> "min": 0, "max": 1}},
> + "max_idle": {
> + "type": {"key": {"type": "integer", "minInteger": 500,
> + "maxInteger": 10000},
> + "min": 0, "max": 1}},
> "overflow_policy": {
> "type": {"key": {"type": "string",
> "enum": ["set", ["refuse", "evict"]]},
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index e915caf..6847f4c 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -136,6 +136,21 @@
> </p>
> </column>
>
> + <column name="other_config" key="max-idle"
> + type='{"type": "integer", "minInteger": 500,
> + "maxInteger": 10000}'>
> + <p>
> + The maximum idle time in milliseconds for flows to be cached in
> the
> + datapath. A lower value may improve flow setup performance, but
> + decrease the number of cached flows in the datapath. Conversely,
> a
> + higher value allows more flows to be maintained in the cache at
> the
> + expense of flow setup performance.
> + </p>
> + <p>
> + The default is 1200.
> + </p>
> + </column>
> +
> <column name="other_config" key="force-miss-model">
> <p>
> Specifies userspace behaviour for handling flow misses. This takes
> @@ -2500,6 +2515,12 @@
> performance reasons.
> </column>
>
> + <column name="max_idle">
> + Limits the idle time (in milliseconds) for the datapath flow cache.
> + Flows may be swapped out of the cache if they are idle for longer
> than
> + this value.
> + </column>
> +
> <column name="overflow_policy">
> <p>
> Controls the switch's behavior when an OpenFlow flow table
> modification
> --
> 1.7.9.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
