Oops I intended that review for the previous patch. This patch is
fine too though.
Acked-by: Ethan Jackson <et...@nicira.com>
On Wed, Sep 11, 2013 at 7:12 PM, Ethan Jackson <et...@nicira.com> wrote:
> FWIW I think this is the right choice given the options. May be worth
> adding a comment in the actual code as to why we can't just execute
> the rule and have to queue it instead.
>
> Acked-by: Ethan Jackson <et...@nicira.com>
>
>
> On Tue, Sep 10, 2013 at 10:27 PM, Ben Pfaff <b...@nicira.com> wrote:
>> This is the first step toward making a global lock that protects everything
>> needed for updating the flow table. This commit starts out by merging one
>> lock into the new one, and later commits will continue that process.
>>
>> The mutex is initially a recursive one, because I wasn't sure that there
>> were no nested acquisitions at this stage, but a later commit will change
>> it to a regular error-checking mutex once it's settled down a bit.
>>
>> Signed-off-by: Ben Pfaff <b...@nicira.com>
>> ---
>> ofproto/ofproto-dpif.c | 4 ++--
>> ofproto/ofproto-provider.h | 17 ++++++++---------
>> ofproto/ofproto.c | 23 ++++++++++++++---------
>> 3 files changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index eab5533..2e2f571 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -3636,12 +3636,12 @@ expire(struct dpif_backer *backer)
>>
>> /* Expire OpenFlow flows whose idle_timeout or hard_timeout
>> * has passed. */
>> - ovs_mutex_lock(&ofproto->up.expirable_mutex);
>> + ovs_mutex_lock(&ofproto_mutex);
>> LIST_FOR_EACH_SAFE (rule, next_rule, expirable,
>> &ofproto->up.expirable) {
>> rule_expire(rule_dpif_cast(rule));
>> }
>> - ovs_mutex_unlock(&ofproto->up.expirable_mutex);
>> + ovs_mutex_unlock(&ofproto_mutex);
>>
>> /* All outstanding data in existing flows has been accounted, so
>> it's a
>> * good time to do bond rebalancing. */
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index d23ff9c..edc9abc 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -40,6 +40,8 @@ struct ofputil_flow_mod;
>> struct bfd_cfg;
>> struct meter;
>>
>> +extern struct ovs_mutex ofproto_mutex;
>> +
>> /* An OpenFlow switch.
>> *
>> * With few exceptions, ofproto implementations may look at these fields but
>> @@ -77,11 +79,8 @@ struct ofproto {
>>
>> struct hindex cookies; /* Rules indexed on their cookie values. */
>>
>> - /* Optimisation for flow expiry.
>> - * These flows should all be present in tables. */
>> - struct ovs_mutex expirable_mutex;
>> - struct list expirable OVS_GUARDED; /* Expirable 'struct rule"s in all
>> - tables. */
>> + /* List of expirable flows, in all flow tables. */
>> + struct list expirable OVS_GUARDED_BY(ofproto_mutex);
>>
>> /* Meter table.
>> * OpenFlow meters start at 1. To avoid confusion we leave the first
>> @@ -270,9 +269,9 @@ struct rule {
>> uint64_t add_seqno; /* Sequence number when added. */
>> uint64_t modify_seqno; /* Sequence number when changed. */
>>
>> - /* Optimisation for flow expiry. */
>> - struct list expirable; /* In ofproto's 'expirable' list if this
>> rule
>> - * is expirable, otherwise empty. */
>> + /* Optimisation for flow expiry. In ofproto's 'expirable' list if this
>> + * rule is expirable, otherwise empty. */
>> + struct list expirable OVS_GUARDED_BY(ofproto_mutex);
>> };
>>
>> void ofproto_rule_ref(struct rule *);
>> @@ -325,7 +324,7 @@ void ofproto_rule_delete(struct ofproto *, struct
>> classifier *cls,
>> struct rule *) OVS_REQ_WRLOCK(cls->rwlock);
>> void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout,
>> uint16_t hard_timeout)
>> - OVS_EXCLUDED(rule->ofproto->expirable_mutex, rule->timeout_mutex);
>> + OVS_EXCLUDED(ofproto_mutex, rule->timeout_mutex);
>>
>> bool ofproto_rule_has_out_port(const struct rule *, ofp_port_t out_port);
>>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 10c803c..9e33684 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -245,6 +245,8 @@ static const struct ofproto_class **ofproto_classes;
>> static size_t n_ofproto_classes;
>> static size_t allocated_ofproto_classes;
>>
>> +struct ovs_mutex ofproto_mutex;
>> +
>> unsigned flow_eviction_threshold = OFPROTO_FLOW_EVICTION_THRESHOLD_DEFAULT;
>> unsigned n_handler_threads;
>> enum ofproto_flow_miss_model flow_miss_model = OFPROTO_HANDLE_MISS_AUTO;
>> @@ -275,6 +277,8 @@ ofproto_init(const struct shash *iface_hints)
>> struct shash_node *node;
>> size_t i;
>>
>> + ovs_mutex_init_recursive(&ofproto_mutex);
>> +
>> ofproto_class_register(&ofproto_dpif_class);
>>
>> /* Make a local copy, since we don't own 'iface_hints' elements. */
>> @@ -430,6 +434,7 @@ ofproto_create(const char *datapath_name, const char
>> *datapath_type,
>> }
>>
>> /* Initialize. */
>> + ovs_mutex_lock(&ofproto_mutex);
>> memset(ofproto, 0, sizeof *ofproto);
>> ofproto->ofproto_class = class;
>> ofproto->name = xstrdup(datapath_name);
>> @@ -454,7 +459,6 @@ ofproto_create(const char *datapath_name, const char
>> *datapath_type,
>> ofproto->n_tables = 0;
>> hindex_init(&ofproto->cookies);
>> list_init(&ofproto->expirable);
>> - ovs_mutex_init_recursive(&ofproto->expirable_mutex);
>> ofproto->connmgr = connmgr_create(ofproto, datapath_name,
>> datapath_name);
>> ofproto->state = S_OPENFLOW;
>> list_init(&ofproto->pending);
>> @@ -470,6 +474,7 @@ ofproto_create(const char *datapath_name, const char
>> *datapath_type,
>> ofproto->min_mtu = INT_MAX;
>> ovs_rwlock_init(&ofproto->groups_rwlock);
>> hmap_init(&ofproto->groups);
>> + ovs_mutex_unlock(&ofproto_mutex);
>>
>> error = ofproto->ofproto_class->construct(ofproto);
>> if (error) {
>> @@ -1194,7 +1199,7 @@ ofproto_destroy__(struct ofproto *ofproto)
>>
>> free(ofproto->vlan_bitmap);
>>
>> - ovs_mutex_destroy(&ofproto->expirable_mutex);
>> + ovs_mutex_destroy(&ofproto_mutex);
>> ofproto->ofproto_class->dealloc(ofproto);
>> }
>>
>> @@ -3991,17 +3996,17 @@ reduce_timeout(uint16_t max, uint16_t *timeout)
>> void
>> ofproto_rule_reduce_timeouts(struct rule *rule,
>> uint16_t idle_timeout, uint16_t hard_timeout)
>> - OVS_EXCLUDED(rule->ofproto->expirable_mutex, rule->timeout_mutex)
>> + OVS_EXCLUDED(ofproto_mutex, rule->timeout_mutex)
>> {
>> if (!idle_timeout && !hard_timeout) {
>> return;
>> }
>>
>> - ovs_mutex_lock(&rule->ofproto->expirable_mutex);
>> + ovs_mutex_lock(&ofproto_mutex);
>> if (list_is_empty(&rule->expirable)) {
>> list_insert(&rule->ofproto->expirable, &rule->expirable);
>> }
>> - ovs_mutex_unlock(&rule->ofproto->expirable_mutex);
>> + ovs_mutex_unlock(&ofproto_mutex);
>>
>> ovs_mutex_lock(&rule->timeout_mutex);
>> reduce_timeout(idle_timeout, &rule->idle_timeout);
>> @@ -6202,11 +6207,11 @@ oftable_remove_rule__(struct ofproto *ofproto,
>> struct classifier *cls,
>> classifier_remove(cls, &rule->cr);
>> cookies_remove(ofproto, rule);
>> eviction_group_remove_rule(rule);
>> - ovs_mutex_lock(&ofproto->expirable_mutex);
>> + ovs_mutex_lock(&ofproto_mutex);
>> if (!list_is_empty(&rule->expirable)) {
>> list_remove(&rule->expirable);
>> }
>> - ovs_mutex_unlock(&ofproto->expirable_mutex);
>> + ovs_mutex_unlock(&ofproto_mutex);
>> if (!list_is_empty(&rule->meter_list_node)) {
>> list_remove(&rule->meter_list_node);
>> list_init(&rule->meter_list_node);
>> @@ -6239,9 +6244,9 @@ oftable_insert_rule(struct rule *rule)
>> ovs_mutex_unlock(&rule->timeout_mutex);
>>
>> if (may_expire) {
>> - ovs_mutex_lock(&ofproto->expirable_mutex);
>> + ovs_mutex_lock(&ofproto_mutex);
>> list_insert(&ofproto->expirable, &rule->expirable);
>> - ovs_mutex_unlock(&ofproto->expirable_mutex);
>> + ovs_mutex_unlock(&ofproto_mutex);
>> }
>> cookies_insert(ofproto, rule);
>>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
