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
