[ovs-dev] [PATCH] upcall: Allow max_idle be configured through ovs-vsctl.
Jarno Rajahalme
jrajahalme at nicira.com
Fri Feb 14 19:17:24 UTC 2014
On Feb 13, 2014, at 5:18 PM, Ethan Jackson <ethan at nicira.com> wrote:
> 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?
>
Access to a shared variable that may change concurrently without any sort of synchronization should be atomic to protect against “tearing”, where the reader gets a mixture of the old and new versions of the variable. This could happen for a “long int” on a 32-bit platform (if, e.g., the 8 bytes span a cache line boundary). Atomic access to a variable for which this can not happen (e.g., aligned 32-bit int on a 32-bit CPU) compiles to normal memory access, so there is no overhead.
In practice it is safe to assume that for an aligned “int” or any pointer this will not be a problem, so why not make the ofproto_max_idle an int instead of long long int?
Jarno
> Ethan
>
> On Tue, Feb 11, 2014 at 2:19 PM, Joe Stringer <joestringer at nicira.com> wrote:
>> Signed-off-by: Joe Stringer <joestringer at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list