From 628a3e52f4fc27956f98a17b8abe340292382141 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 12 Sep 2013 20:45:15 -0700 Subject: [PATCH] ofproto: Add global locking around flow table changes. This makes 'ofproto_mutex' protect the flow table well enough that threads other than the main one can realistically modify flows. I need to look at the interface between ofproto and connmgr: I think that there might need to be some locking there too. Signed-off-by: Ben Pfaff Acked-by: Ethan Jackson --- lib/classifier.h | 5 +- ofproto/connmgr.c | 3 + ofproto/connmgr.h | 3 +- ofproto/fail-open.c | 3 +- ofproto/ofproto-dpif.c | 68 +++++--- ofproto/ofproto-provider.h | 222 ++++++++++++++++++------ ofproto/ofproto.c | 343 +++++++++++++++++++++++++++---------- 7 files changed, 481 insertions(+), 166 deletions(-) diff --git a/lib/classifier.h b/lib/classifier.h index ead087b9e..a795b4a18 100644 --- a/lib/classifier.h +++ b/lib/classifier.h @@ -46,12 +46,15 @@ extern "C" { #endif +/* Needed only for the lock annotation in struct classifier. */ +extern struct ovs_mutex ofproto_mutex; + /* A flow classifier. */ struct classifier { int n_rules; /* Total number of rules. */ struct hmap tables; /* Contains "struct cls_table"s. */ struct list tables_priority; /* Tables in descending priority order */ - struct ovs_rwlock rwlock; + struct ovs_rwlock rwlock OVS_ACQ_AFTER(ofproto_mutex); }; /* A set of rules that all have the same fields wildcarded. */ diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 9ab7a3f7b..20484b5de 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -271,6 +271,7 @@ void connmgr_run(struct connmgr *mgr, bool (*handle_openflow)(struct ofconn *, const struct ofpbuf *ofp_msg)) + OVS_EXCLUDED(ofproto_mutex) { struct ofconn *ofconn, *next_ofconn; struct ofservice *ofservice; @@ -1658,6 +1659,7 @@ connmgr_has_in_band(struct connmgr *mgr) * In-band control has more sophisticated code that manages flows itself. */ void connmgr_flushed(struct connmgr *mgr) + OVS_EXCLUDED(ofproto_mutex) { if (mgr->fail_open) { fail_open_flushed(mgr->fail_open); @@ -1824,6 +1826,7 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, enum nx_flow_update_event event, enum ofp_flow_removed_reason reason, const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid) + OVS_REQUIRES(ofproto_mutex) { enum nx_flow_monitor_flags update; struct ofconn *ofconn; diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index b57e7411d..b31e855a1 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -184,7 +184,8 @@ void ofmonitor_destroy(struct ofmonitor *); void ofmonitor_report(struct connmgr *, struct rule *, enum nx_flow_update_event, enum ofp_flow_removed_reason, - const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid); + const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid) + OVS_REQUIRES(ofproto_mutex); void ofmonitor_flush(struct connmgr *); diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c index 2c0a8f33d..4900c0507 100644 --- a/ofproto/fail-open.c +++ b/ofproto/fail-open.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -211,6 +211,7 @@ fail_open_wait(struct fail_open *fo) void fail_open_flushed(struct fail_open *fo) + OVS_EXCLUDED(ofproto_mutex) { int disconn_secs = connmgr_failure_duration(fo->connmgr); bool open = disconn_secs >= trigger_duration(fo); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d0b950660..975097d4c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1420,12 +1420,12 @@ destruct(struct ofproto *ofproto_) OFPROTO_FOR_EACH_TABLE (table, &ofproto->up) { struct cls_cursor cursor; - ovs_rwlock_wrlock(&table->cls.rwlock); + ovs_rwlock_rdlock(&table->cls.rwlock); cls_cursor_init(&cursor, &table->cls, NULL); + ovs_rwlock_unlock(&table->cls.rwlock); CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, up.cr, &cursor) { ofproto_rule_delete(&ofproto->up, &table->cls, &rule->up); } - ovs_rwlock_unlock(&table->cls.rwlock); } guarded_list_pop_all(&ofproto->flow_mods, &flow_mods); @@ -3588,7 +3588,7 @@ handle_upcalls(struct dpif_backer *backer) static int subfacet_max_idle(const struct dpif_backer *); static void update_stats(struct dpif_backer *); -static void rule_expire(struct rule_dpif *); +static void rule_expire(struct rule_dpif *) OVS_REQUIRES(ofproto_mutex); static void expire_subfacets(struct dpif_backer *, int dp_max_idle); /* This function is called periodically by run(). Its job is to collect @@ -3908,30 +3908,31 @@ expire_subfacets(struct dpif_backer *backer, int dp_max_idle) * then delete it entirely. */ static void rule_expire(struct rule_dpif *rule) + OVS_REQUIRES(ofproto_mutex) { uint16_t idle_timeout, hard_timeout; - long long int now; - uint8_t reason; + long long int now = time_msec(); + int reason; ovs_assert(!rule->up.pending); + /* Has 'rule' expired? */ ovs_mutex_lock(&rule->up.mutex); hard_timeout = rule->up.hard_timeout; idle_timeout = rule->up.idle_timeout; - ovs_mutex_unlock(&rule->up.mutex); - - /* Has 'rule' expired? */ - now = time_msec(); if (hard_timeout && now > rule->up.modified + hard_timeout * 1000) { reason = OFPRR_HARD_TIMEOUT; } else if (idle_timeout && now > rule->up.used + idle_timeout * 1000) { reason = OFPRR_IDLE_TIMEOUT; } else { - return; + reason = -1; } + ovs_mutex_unlock(&rule->up.mutex); - COVERAGE_INC(ofproto_dpif_expired); - ofproto_rule_expire(&rule->up, reason); + if (reason >= 0) { + COVERAGE_INC(ofproto_dpif_expired); + ofproto_rule_expire(&rule->up, reason); + } } /* Facets. */ @@ -4123,17 +4124,21 @@ facet_is_controller_flow(struct facet *facet) if (facet) { struct ofproto_dpif *ofproto = facet->ofproto; const struct ofpact *ofpacts; + struct rule_actions *actions; struct rule_dpif *rule; size_t ofpacts_len; bool is_controller; rule_dpif_lookup(ofproto, &facet->flow, NULL, &rule); - ofpacts_len = rule->up.actions->ofpacts_len; - ofpacts = rule->up.actions->ofpacts; + actions = rule_dpif_get_actions(rule); + rule_dpif_unref(rule); + + ofpacts_len = actions->ofpacts_len; + ofpacts = actions->ofpacts; is_controller = ofpacts_len > 0 && ofpacts->type == OFPACT_CONTROLLER && ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len); - rule_dpif_unref(rule); + rule_actions_unref(actions); return is_controller; } @@ -4355,7 +4360,10 @@ facet_revalidate(struct facet *facet) facet->xout.nf_output_iface = xout.nf_output_iface; facet->xout.mirrors = xout.mirrors; facet->nf_flow.output_iface = facet->xout.nf_output_iface; + + ovs_mutex_lock(&new_rule->up.mutex); facet->used = MAX(facet->used, new_rule->up.created); + ovs_mutex_unlock(&new_rule->up.mutex); xlate_out_uninit(&xout); rule_dpif_unref(new_rule); @@ -4475,6 +4483,7 @@ rule_dpif_fail_open(const struct rule_dpif *rule) ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule) + OVS_REQUIRES(rule->up.mutex) { return rule->up.flow_cookie; } @@ -4492,14 +4501,7 @@ rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout, struct rule_actions * rule_dpif_get_actions(const struct rule_dpif *rule) { - struct rule_actions *actions; - - ovs_mutex_lock(&rule->up.mutex); - actions = rule->up.actions; - rule_actions_ref(actions); - ovs_mutex_unlock(&rule->up.mutex); - - return actions; + return rule_get_actions(&rule->up); } /* Subfacets. */ @@ -4846,6 +4848,7 @@ rule_dpif_unref(struct rule_dpif *rule) static void complete_operation(struct rule_dpif *rule) + OVS_REQUIRES(ofproto_mutex) { struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto); @@ -4886,6 +4889,7 @@ rule_construct(struct rule *rule_) static void rule_insert(struct rule *rule_) + OVS_REQUIRES(ofproto_mutex) { struct rule_dpif *rule = rule_dpif_cast(rule_); complete_operation(rule); @@ -4893,6 +4897,7 @@ rule_insert(struct rule *rule_) static void rule_delete(struct rule *rule_) + OVS_REQUIRES(ofproto_mutex) { struct rule_dpif *rule = rule_dpif_cast(rule_); complete_operation(rule); @@ -4957,6 +4962,7 @@ rule_execute(struct rule *rule, const struct flow *flow, static void rule_modify_actions(struct rule *rule_, bool reset_counters) + OVS_REQUIRES(ofproto_mutex) { struct rule_dpif *rule = rule_dpif_cast(rule_); @@ -5271,22 +5277,32 @@ struct trace_ctx { static void trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule) { + struct rule_actions *actions; + ovs_be64 cookie; + ds_put_char_multiple(result, '\t', level); if (!rule) { ds_put_cstr(result, "No match\n"); return; } + ovs_mutex_lock(&rule->up.mutex); + cookie = rule->up.flow_cookie; + ovs_mutex_unlock(&rule->up.mutex); + ds_put_format(result, "Rule: table=%"PRIu8" cookie=%#"PRIx64" ", - rule ? rule->up.table_id : 0, ntohll(rule->up.flow_cookie)); + rule ? rule->up.table_id : 0, ntohll(cookie)); cls_rule_format(&rule->up.cr, result); ds_put_char(result, '\n'); + actions = rule_dpif_get_actions(rule); + ds_put_char_multiple(result, '\t', level); ds_put_cstr(result, "OpenFlow "); - ofpacts_format(rule->up.actions->ofpacts, rule->up.actions->ofpacts_len, - result); + ofpacts_format(actions->ofpacts, actions->ofpacts_len, result); ds_put_char(result, '\n'); + + rule_actions_unref(actions); } static void diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 2c1329a7b..bfd3e8dca 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -17,7 +17,21 @@ #ifndef OFPROTO_OFPROTO_PROVIDER_H #define OFPROTO_OFPROTO_PROVIDER_H 1 -/* Definitions for use within ofproto. */ +/* Definitions for use within ofproto. + * + * + * Thread-safety + * ============= + * + * Lots of ofproto data structures are only accessed from a single thread. + * Those data structures are generally not thread-safe. + * + * The ofproto-dpif ofproto implementation accesses the flow table from + * multiple threads, including modifying the flow table from multiple threads + * via the "learn" action, so the flow table and various structures that index + * it have been made thread-safe. Refer to comments on individual data + * structures for details. + */ #include "cfm.h" #include "classifier.h" @@ -93,11 +107,21 @@ struct ofproto { /* OpenFlow connections. */ struct connmgr *connmgr; - /* Flow table operation tracking. */ - int state; /* Internal state. */ - struct list pending; /* List of "struct ofopgroup"s. */ - unsigned int n_pending; /* list_size(&pending). */ - struct hmap deletions; /* All OFOPERATION_DELETE "ofoperation"s. */ + /* Flow table operation tracking. + * + * 'state' is meaningful only within ofproto.c, one of the enum + * ofproto_state constants defined there. + * + * 'pending' is the list of "struct ofopgroup"s currently pending. + * + * 'n_pending' is the number of elements in 'pending'. + * + * 'deletions' contains pending ofoperations of type OFOPERATION_DELETE, + * indexed on its rule's flow.*/ + int state; + struct list pending OVS_GUARDED_BY(ofproto_mutex); + unsigned int n_pending OVS_GUARDED_BY(ofproto_mutex); + struct hmap deletions OVS_GUARDED_BY(ofproto_mutex); /* Delayed rule executions. * @@ -105,7 +129,7 @@ struct ofproto { * ofproto_mutex during a flow_mod, because otherwise a "learn" action * triggered by the executing the packet would try to recursively modify * the flow table and reacquire the global lock. */ - struct guarded_list rule_executes; + struct guarded_list rule_executes; /* Contains "struct rule_execute"s. */ /* Flow table operation logging. */ int n_add, n_delete, n_modify; /* Number of unreported ops of each kind. */ @@ -175,7 +199,29 @@ enum oftable_flags { OFTABLE_READONLY = 1 << 1 /* Don't allow OpenFlow to change this table. */ }; -/* A flow table within a "struct ofproto". */ +/* A flow table within a "struct ofproto". + * + * + * Thread-safety + * ============= + * + * A cls->rwlock read-lock holder prevents rules from being added or deleted. + * + * Adding or removing rules requires holding ofproto_mutex AND the cls->rwlock + * write-lock. + * + * cls->rwlock should be held only briefly. For extended access to a rule, + * increment its ref_count with ofproto_rule_ref(). A rule will not be freed + * until its ref_count reaches zero. + * + * Modifying a rule requires the rule's own mutex. Holding cls->rwlock (for + * read or write) does not allow the holder to modify the rule. + * + * Freeing a rule requires ofproto_mutex and the cls->rwlock write-lock. After + * removing the rule from the classifier, release a ref_count from the rule + * ('cls''s reference to the rule). + * + * Refer to the thread-safety notes on struct rule for more information.*/ struct oftable { enum oftable_flags flags; struct classifier cls; /* Contains "struct rule"s. */ @@ -219,7 +265,59 @@ struct oftable { /* An OpenFlow flow within a "struct ofproto". * * With few exceptions, ofproto implementations may look at these fields but - * should not modify them. */ + * should not modify them. + * + * + * Thread-safety + * ============= + * + * Except near the beginning or ending of its lifespan, rule 'rule' belongs to + * the classifier rule->ofproto->tables[rule->table_id].cls. The text below + * calls this classifier 'cls'. + * + * Motivation + * ---------- + * + * The thread safety rules described here for "struct rule" are motivated by + * two goals: + * + * - Prevent threads that read members of "struct rule" from reading bad + * data due to changes by some thread concurrently modifying those + * members. + * + * - Prevent two threads making changes to members of a given "struct rule" + * from interfering with each other. + * + * + * Rules + * ----- + * + * A rule 'rule' may be accessed without a risk of being freed by code that + * holds a read-lock or write-lock on 'cls->rwlock' or that owns a reference to + * 'rule->ref_count' (or both). Code that needs to hold onto a rule for a + * while should take 'cls->rwlock', find the rule it needs, increment + * 'rule->ref_count' with ofproto_rule_ref(), and drop 'cls->rwlock'. + * + * 'rule->ref_count' protects 'rule' from being freed. It doesn't protect the + * rule from being deleted from 'cls' (that's 'cls->rwlock') and it doesn't + * protect members of 'rule' from modification (that's 'rule->rwlock'). + * + * 'rule->mutex' protects the members of 'rule' from modification. It doesn't + * protect the rule from being deleted from 'cls' (that's 'cls->rwlock') and it + * doesn't prevent the rule from being freed (that's 'rule->ref_count'). + * + * Regarding thread safety, the members of a rule fall into the following + * categories: + * + * - Immutable. These members are marked 'const'. + * + * - Members that may be safely read or written only by code holding + * ofproto_mutex. These are marked OVS_GUARDED_BY(ofproto_mutex). + * + * - Members that may be safely read only by code holding ofproto_mutex or + * 'rule->mutex', and safely written only by coding holding ofproto_mutex + * AND 'rule->mutex'. These are marked OVS_GUARDED. + */ struct rule { /* Where this rule resides in an OpenFlow switch. * @@ -228,48 +326,58 @@ struct rule { const struct cls_rule cr; /* In owning ofproto's classifier. */ const uint8_t table_id; /* Index in ofproto's 'tables' array. */ + /* Protects members marked OVS_GUARDED. + * Readers only need to hold this mutex. + * Writers must hold both this mutex AND ofproto_mutex. */ + struct ovs_mutex mutex OVS_ACQ_AFTER(ofproto_mutex); + + /* Number of references. + * The classifier owns one reference. + * Any thread trying to keep a rule from being freed should hold its own + * reference. */ atomic_uint ref_count; - struct ofoperation *pending; /* Operation now in progress, if nonnull. */ + /* Operation now in progress, if nonnull. */ + struct ofoperation *pending OVS_GUARDED_BY(ofproto_mutex); - ovs_be64 flow_cookie; /* Controller-issued identifier. Guarded by - mutex. */ + /* A "flow cookie" is the OpenFlow name for a 64-bit value associated with + * a flow.. */ + ovs_be64 flow_cookie OVS_GUARDED; struct hindex_node cookie_node OVS_GUARDED_BY(ofproto_mutex); - long long int created; /* Creation time. */ - long long int modified; /* Time of last modification. */ - long long int used; /* Last use; time created if never used. */ + /* Times. */ + long long int created OVS_GUARDED; /* Creation time. */ + long long int modified OVS_GUARDED; /* Time of last modification. */ + long long int used OVS_GUARDED; /* Last use; time created if never used. */ bool send_flow_removed; /* Send a flow removed message? */ + /* Timeouts. */ uint16_t hard_timeout OVS_GUARDED; /* In seconds from ->modified. */ uint16_t idle_timeout OVS_GUARDED; /* In seconds from ->used. */ - /* Eviction groups. */ - struct heap_node evg_node; /* In eviction_group's "rules" heap. */ - struct eviction_group *eviction_group; /* NULL if not in any group. */ - - /* The mutex is used to protect those elements in struct rule which are - * accessed by multiple threads. The main ofproto code is guaranteed not - * to change any of the elements "Guarded by mutex" without holding the - * lock. - * - * While maintaining a pointer to struct rule, threads are required to hold - * a readlock on the classifier that holds the rule or increment the rule's - * ref_count. + /* Eviction groups (see comment on struct eviction_group for explanation) . * - * A rule will not be evicted unless its classifier's write lock is - * held. */ - struct ovs_mutex mutex; + * 'eviction_group' is this rule's eviction group, or NULL if it is not in + * any eviction group. When 'eviction_group' is nonnull, 'evg_node' is in + * the ->eviction_group->rules hmap. */ + struct eviction_group *eviction_group OVS_GUARDED_BY(ofproto_mutex); + struct heap_node evg_node OVS_GUARDED_BY(ofproto_mutex); - /* Guarded by mutex. */ - struct rule_actions *actions; + /* OpenFlow actions. See struct rule_actions for more thread-safety + * notes. */ + struct rule_actions *actions OVS_GUARDED; - struct list meter_list_node; /* In owning meter's 'rules' list. */ + /* In owning meter's 'rules' list. An empty list if there is no meter. */ + struct list meter_list_node OVS_GUARDED_BY(ofproto_mutex); - /* Flow monitors. */ - enum nx_flow_monitor_flags monitor_flags; - uint64_t add_seqno; /* Sequence number when added. */ - uint64_t modify_seqno; /* Sequence number when changed. */ + /* Flow monitors (e.g. for NXST_FLOW_MONITOR, related to struct ofmonitor). + * + * 'add_seqno' is the sequence number when this rule was created. + * 'modify_seqno' is the sequence number when this rule was last modified. + * See 'monitor_seqno' in connmgr.c for more information. */ + enum nx_flow_monitor_flags monitor_flags OVS_GUARDED_BY(ofproto_mutex); + uint64_t add_seqno OVS_GUARDED_BY(ofproto_mutex); + uint64_t modify_seqno OVS_GUARDED_BY(ofproto_mutex); /* Optimisation for flow expiry. In ofproto's 'expirable' list if this * rule is expirable, otherwise empty. */ @@ -279,6 +387,11 @@ struct rule { void ofproto_rule_ref(struct rule *); void ofproto_rule_unref(struct rule *); +struct rule_actions *rule_get_actions(const struct rule *rule) + OVS_EXCLUDED(rule->mutex); +struct rule_actions *rule_get_actions__(const struct rule *rule) + OVS_REQUIRES(rule->mutex); + /* A set of actions within a "struct rule". * * @@ -314,6 +427,8 @@ struct rule_collection { void rule_collection_init(struct rule_collection *); void rule_collection_add(struct rule_collection *, struct rule *); +void rule_collection_ref(struct rule_collection *) OVS_REQUIRES(ofproto_mutex); +void rule_collection_unref(struct rule_collection *); void rule_collection_destroy(struct rule_collection *); /* Threshold at which to begin flow table eviction. Only affects the @@ -334,16 +449,19 @@ rule_from_cls_rule(const struct cls_rule *cls_rule) return cls_rule ? CONTAINER_OF(cls_rule, struct rule, cr) : NULL; } -void ofproto_rule_expire(struct rule *rule, uint8_t reason); +void ofproto_rule_expire(struct rule *rule, uint8_t reason) + OVS_REQUIRES(ofproto_mutex); void ofproto_rule_delete(struct ofproto *, struct classifier *cls, - struct rule *) OVS_REQ_WRLOCK(cls->rwlock); + struct rule *) + OVS_EXCLUDED(ofproto_mutex); void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout, uint16_t hard_timeout) - OVS_EXCLUDED(ofproto_mutex, rule->mutex); + OVS_EXCLUDED(ofproto_mutex); void ofoperation_complete(struct ofoperation *, enum ofperr); -bool ofoperation_has_out_port(const struct ofoperation *, ofp_port_t out_port); +bool ofoperation_has_out_port(const struct ofoperation *, ofp_port_t out_port) + OVS_REQUIRES(ofproto_mutex); /* ofproto class structure, to be defined by each ofproto implementation. * @@ -1101,9 +1219,10 @@ struct ofproto_class { * * Rule destruction must not fail. */ struct rule *(*rule_alloc)(void); - enum ofperr (*rule_construct)(struct rule *rule); - void (*rule_insert)(struct rule *rule); - void (*rule_delete)(struct rule *rule); + enum ofperr (*rule_construct)(struct rule *rule) + /* OVS_REQUIRES(ofproto_mutex) */; + void (*rule_insert)(struct rule *rule) /* OVS_REQUIRES(ofproto_mutex) */; + void (*rule_delete)(struct rule *rule) /* OVS_REQUIRES(ofproto_mutex) */; void (*rule_destruct)(struct rule *rule); void (*rule_dealloc)(struct rule *rule); @@ -1112,7 +1231,8 @@ struct ofproto_class { * in '*byte_count'. UINT64_MAX indicates that the packet count or byte * count is unknown. */ void (*rule_get_stats)(struct rule *rule, uint64_t *packet_count, - uint64_t *byte_count); + uint64_t *byte_count) + /* OVS_EXCLUDED(ofproto_mutex) */; /* Applies the actions in 'rule' to 'packet'. (This implements sending * buffered packets for OpenFlow OFPT_FLOW_MOD commands.) @@ -1158,7 +1278,8 @@ struct ofproto_class { * * ->rule_modify_actions() should not modify any base members of struct * rule. */ - void (*rule_modify_actions)(struct rule *rule, bool reset_counters); + void (*rule_modify_actions)(struct rule *rule, bool reset_counters) + /* OVS_REQUIRES(ofproto_mutex) */; /* Changes the OpenFlow IP fragment handling policy to 'frag_handling', * which takes one of the following values, with the corresponding @@ -1523,12 +1644,15 @@ int ofproto_class_unregister(const struct ofproto_class *); enum { OFPROTO_POSTPONE = 1 << 16 }; BUILD_ASSERT_DECL(OFPROTO_POSTPONE < OFPERR_OFS); -int ofproto_flow_mod(struct ofproto *, struct ofputil_flow_mod *); +int ofproto_flow_mod(struct ofproto *, struct ofputil_flow_mod *) + OVS_EXCLUDED(ofproto_mutex); void ofproto_add_flow(struct ofproto *, const struct match *, unsigned int priority, - const struct ofpact *ofpacts, size_t ofpacts_len); + const struct ofpact *ofpacts, size_t ofpacts_len) + OVS_EXCLUDED(ofproto_mutex); bool ofproto_delete_flow(struct ofproto *, - const struct match *, unsigned int priority); + const struct match *, unsigned int priority) + OVS_EXCLUDED(ofproto_mutex); void ofproto_flush_flows(struct ofproto *); #endif /* ofproto/ofproto-provider.h */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 2d9696c3e..284d874f2 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -153,10 +153,10 @@ static void oftable_enable_eviction(struct oftable *, const struct mf_subfield *fields, size_t n_fields); -static void oftable_remove_rule(struct rule *rule) OVS_RELEASES(rule->mutex); +static void oftable_remove_rule(struct rule *rule) OVS_REQUIRES(ofproto_mutex); static void oftable_remove_rule__(struct ofproto *ofproto, struct classifier *cls, struct rule *rule) - OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->mutex); + OVS_REQUIRES(ofproto_mutex); static void oftable_insert_rule(struct rule *); /* A set of rules within a single OpenFlow table (oftable) that have the same @@ -181,9 +181,8 @@ struct eviction_group { struct heap rules; /* Contains "struct rule"s. */ }; -static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep) - OVS_TRY_WRLOCK(true, (*rulep)->mutex); -static void ofproto_evict(struct ofproto *); +static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep); +static void ofproto_evict(struct ofproto *) OVS_EXCLUDED(ofproto_mutex); static uint32_t rule_eviction_priority(struct rule *); static void eviction_group_add_rule(struct rule *); static void eviction_group_remove_rule(struct rule *); @@ -218,7 +217,10 @@ static void rule_criteria_init(struct rule_criteria *, uint8_t table_id, ofp_port_t out_port); static void rule_criteria_destroy(struct rule_criteria *); -/* A packet that needs to be passed to rule_execute(). */ +/* A packet that needs to be passed to rule_execute(). + * + * (We can't do this immediately from ofopgroup_complete() because that holds + * ofproto_mutex, which rule_execute() needs released.) */ struct rule_execute { struct list list_node; /* In struct ofproto's "rule_executes" list. */ struct rule *rule; /* Owns a reference to the rule. */ @@ -226,11 +228,11 @@ struct rule_execute { struct ofpbuf *packet; /* Owns the packet. */ }; -static void run_rule_executes(struct ofproto *); +static void run_rule_executes(struct ofproto *) OVS_EXCLUDED(ofproto_mutex); static void destroy_rule_executes(struct ofproto *); /* ofport. */ -static void ofport_destroy__(struct ofport *); +static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex); static void ofport_destroy(struct ofport *); static void update_port(struct ofproto *, const char *devname); @@ -252,11 +254,12 @@ static enum ofperr modify_flows__(struct ofproto *, struct ofconn *, const struct rule_collection *); static void delete_flow__(struct rule *rule, struct ofopgroup *, enum ofp_flow_removed_reason) - OVS_RELEASES(rule->mutex); + OVS_REQUIRES(ofproto_mutex); static bool handle_openflow(struct ofconn *, const struct ofpbuf *); static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *, struct ofputil_flow_mod *, - const struct ofp_header *); + const struct ofp_header *) + OVS_EXCLUDED(ofproto_mutex); static void calc_duration(long long int start, long long int now, uint32_t *sec, uint32_t *nsec); @@ -275,7 +278,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; +/* Global lock that protects all flow table operations. */ +struct ovs_mutex ofproto_mutex = OVS_MUTEX_INITIALIZER; unsigned flow_eviction_threshold = OFPROTO_FLOW_EVICTION_THRESHOLD_DEFAULT; unsigned n_handler_threads; @@ -307,8 +311,6 @@ 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. */ @@ -1122,6 +1124,21 @@ ofproto_get_snoops(const struct ofproto *ofproto, struct sset *snoops) connmgr_get_snoops(ofproto->connmgr, snoops); } +static void +ofproto_rule_delete__(struct ofproto *ofproto, struct classifier *cls, + struct rule *rule) + OVS_REQUIRES(ofproto_mutex) +{ + struct ofopgroup *group; + + ovs_assert(!rule->pending); + ovs_assert(cls == &ofproto->tables[rule->table_id].cls); + + group = ofopgroup_create_unattached(ofproto); + delete_flow__(rule, group, OFPRR_DELETE); + ofopgroup_submit(group); +} + /* Deletes 'rule' from 'cls' within 'ofproto'. * * Within an ofproto implementation, this function allows an ofproto @@ -1129,8 +1146,6 @@ ofproto_get_snoops(const struct ofproto *ofproto, struct sset *snoops) * function is called. This function is not suitable for use elsewhere in an * ofproto implementation. * - * This function is also used internally in ofproto.c. - * * This function implements steps 4.4 and 4.5 in the section titled "Rule Life * Cycle" in ofproto-provider.h. @@ -1139,23 +1154,26 @@ ofproto_get_snoops(const struct ofproto *ofproto, struct sset *snoops) void ofproto_rule_delete(struct ofproto *ofproto, struct classifier *cls, struct rule *rule) - OVS_REQ_WRLOCK(cls->rwlock) + OVS_EXCLUDED(ofproto_mutex) { struct ofopgroup *group; + ovs_mutex_lock(&ofproto_mutex); ovs_assert(!rule->pending); ovs_assert(cls == &ofproto->tables[rule->table_id].cls); group = ofopgroup_create_unattached(ofproto); ofoperation_create(group, rule, OFOPERATION_DELETE, OFPRR_DELETE); - ovs_mutex_lock(&rule->mutex); oftable_remove_rule__(ofproto, cls, rule); ofproto->ofproto_class->rule_delete(rule); ofopgroup_submit(group); + + ovs_mutex_unlock(&ofproto_mutex); } static void ofproto_flush__(struct ofproto *ofproto) + OVS_EXCLUDED(ofproto_mutex) { struct oftable *table; @@ -1163,6 +1181,7 @@ ofproto_flush__(struct ofproto *ofproto) ofproto->ofproto_class->flush(ofproto); } + ovs_mutex_lock(&ofproto_mutex); OFPROTO_FOR_EACH_TABLE (table, ofproto) { struct rule *rule, *next_rule; struct cls_cursor cursor; @@ -1171,24 +1190,25 @@ ofproto_flush__(struct ofproto *ofproto) continue; } - ovs_rwlock_wrlock(&table->cls.rwlock); + ovs_rwlock_rdlock(&table->cls.rwlock); cls_cursor_init(&cursor, &table->cls, NULL); + ovs_rwlock_unlock(&table->cls.rwlock); CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cr, &cursor) { if (!rule->pending) { - ofproto_rule_delete(ofproto, &table->cls, rule); + ofproto_rule_delete__(ofproto, &table->cls, rule); } } - ovs_rwlock_unlock(&table->cls.rwlock); } + ovs_mutex_unlock(&ofproto_mutex); } static void ofproto_destroy__(struct ofproto *ofproto) + OVS_EXCLUDED(ofproto_mutex) { struct oftable *table; ovs_assert(list_is_empty(&ofproto->pending)); - ovs_assert(!ofproto->n_pending); destroy_rule_executes(ofproto); guarded_list_destroy(&ofproto->rule_executes); @@ -1222,6 +1242,7 @@ ofproto_destroy__(struct ofproto *ofproto) void ofproto_destroy(struct ofproto *p) + OVS_EXCLUDED(ofproto_mutex) { struct ofport *ofport, *next_ofport; @@ -1320,8 +1341,15 @@ ofproto_type_wait(const char *datapath_type) static bool any_pending_ops(const struct ofproto *p) + OVS_EXCLUDED(ofproto_mutex) { - return !list_is_empty(&p->pending); + bool b; + + ovs_mutex_lock(&ofproto_mutex); + b = !list_is_empty(&p->pending); + ovs_mutex_unlock(&ofproto_mutex); + + return b; } int @@ -1355,6 +1383,7 @@ ofproto_run(struct ofproto *p) continue; } + ovs_mutex_lock(&ofproto_mutex); HEAP_FOR_EACH (evg, size_node, &table->eviction_groups_by_size) { heap_rebuild(&evg->rules); } @@ -1368,6 +1397,7 @@ ofproto_run(struct ofproto *p) } } ovs_rwlock_unlock(&table->cls.rwlock); + ovs_mutex_unlock(&ofproto_mutex); } } @@ -1529,8 +1559,11 @@ ofproto_get_memory_usage(const struct ofproto *ofproto, struct simap *usage) unsigned int n_rules; simap_increase(usage, "ports", hmap_count(&ofproto->ports)); + + ovs_mutex_lock(&ofproto_mutex); simap_increase(usage, "ops", ofproto->n_pending + hmap_count(&ofproto->deletions)); + ovs_mutex_unlock(&ofproto_mutex); n_rules = 0; OFPROTO_FOR_EACH_TABLE (table, ofproto) { @@ -1768,6 +1801,7 @@ simple_flow_mod(struct ofproto *ofproto, fm.flags = 0; fm.ofpacts = CONST_CAST(struct ofpact *, ofpacts); fm.ofpacts_len = ofpacts_len; + return handle_flow_mod__(ofproto, NULL, &fm, NULL); } @@ -1786,6 +1820,7 @@ void ofproto_add_flow(struct ofproto *ofproto, const struct match *match, unsigned int priority, const struct ofpact *ofpacts, size_t ofpacts_len) + OVS_EXCLUDED(ofproto_mutex) { const struct rule *rule; bool must_add; @@ -1823,6 +1858,7 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match, * This is a helper function for in-band control and fail-open. */ int ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm) + OVS_EXCLUDED(ofproto_mutex) { return handle_flow_mod__(ofproto, NULL, fm, NULL); } @@ -1834,6 +1870,7 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm) bool ofproto_delete_flow(struct ofproto *ofproto, const struct match *target, unsigned int priority) + OVS_EXCLUDED(ofproto_mutex) { struct classifier *cls = &ofproto->tables[0].cls; struct rule *rule; @@ -2370,8 +2407,30 @@ ofproto_rule_unref(struct rule *rule) } } +struct rule_actions * +rule_get_actions(const struct rule *rule) + OVS_EXCLUDED(rule->mutex) +{ + struct rule_actions *actions; + + ovs_mutex_lock(&rule->mutex); + actions = rule_get_actions__(rule); + ovs_mutex_unlock(&rule->mutex); + + return actions; +} + +struct rule_actions * +rule_get_actions__(const struct rule *rule) + OVS_REQUIRES(rule->mutex) +{ + rule_actions_ref(rule->actions); + return rule->actions; +} + static void ofproto_rule_destroy__(struct rule *rule) + OVS_NO_THREAD_SAFETY_ANALYSIS { cls_rule_destroy(CONST_CAST(struct cls_rule *, &rule->cr)); rule_actions_unref(rule->actions); @@ -2427,6 +2486,7 @@ rule_actions_unref(struct rule_actions *actions) * that outputs to 'port' (output to OFPP_FLOOD and OFPP_ALL doesn't count). */ static bool ofproto_rule_has_out_port(const struct rule *rule, ofp_port_t port) + OVS_REQUIRES(ofproto_mutex) { return (port == OFPP_ANY || ofpacts_output_to_port(rule->actions->ofpacts, @@ -2437,6 +2497,7 @@ ofproto_rule_has_out_port(const struct rule *rule, ofp_port_t port) * OFPAT_ENQUEUE action that outputs to 'out_port'. */ bool ofoperation_has_out_port(const struct ofoperation *op, ofp_port_t out_port) + OVS_REQUIRES(ofproto_mutex) { if (ofproto_rule_has_out_port(op->rule, out_port)) { return true; @@ -2468,6 +2529,7 @@ rule_execute_destroy(struct rule_execute *e) * by passing them to the ofproto provider. */ static void run_rule_executes(struct ofproto *ofproto) + OVS_EXCLUDED(ofproto_mutex) { struct rule_execute *e, *next; struct list executes; @@ -2977,9 +3039,9 @@ cookies_remove(struct ofproto *ofproto, struct rule *rule) static void ofproto_rule_change_cookie(struct ofproto *ofproto, struct rule *rule, ovs_be64 new_cookie) + OVS_REQUIRES(ofproto_mutex) { if (new_cookie != rule->flow_cookie) { - ovs_mutex_lock(&ofproto_mutex); cookies_remove(ofproto, rule); ovs_mutex_lock(&rule->mutex); @@ -2987,7 +3049,6 @@ ofproto_rule_change_cookie(struct ofproto *ofproto, struct rule *rule, ovs_mutex_unlock(&rule->mutex); cookies_insert(ofproto, rule); - ovs_mutex_unlock(&ofproto_mutex); } } @@ -3121,6 +3182,27 @@ rule_collection_add(struct rule_collection *rules, struct rule *rule) rules->rules[rules->n++] = rule; } +void +rule_collection_ref(struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) +{ + size_t i; + + for (i = 0; i < rules->n; i++) { + ofproto_rule_ref(rules->rules[i]); + } +} + +void +rule_collection_unref(struct rule_collection *rules) +{ + size_t i; + + for (i = 0; i < rules->n; i++) { + ofproto_rule_unref(rules->rules[i]); + } +} + void rule_collection_destroy(struct rule_collection *rules) { @@ -3132,6 +3214,7 @@ rule_collection_destroy(struct rule_collection *rules) static enum ofperr collect_rule(struct rule *rule, const struct rule_criteria *c, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { if (ofproto_rule_is_hidden(rule)) { return 0; @@ -3159,6 +3242,7 @@ static enum ofperr collect_rules_loose(struct ofproto *ofproto, const struct rule_criteria *criteria, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { struct oftable *table; enum ofperr error; @@ -3173,7 +3257,6 @@ collect_rules_loose(struct ofproto *ofproto, if (criteria->cookie_mask == htonll(UINT64_MAX)) { struct rule *rule; - ovs_mutex_lock(&ofproto_mutex); HINDEX_FOR_EACH_WITH_HASH (rule, cookie_node, hash_cookie(criteria->cookie), &ofproto->cookies) { @@ -3184,7 +3267,6 @@ collect_rules_loose(struct ofproto *ofproto, } } } - ovs_mutex_unlock(&ofproto_mutex); } else { FOR_EACH_MATCHING_TABLE (table, criteria->table_id, ofproto) { struct cls_cursor cursor; @@ -3221,6 +3303,7 @@ static enum ofperr collect_rules_strict(struct ofproto *ofproto, const struct rule_criteria *criteria, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { struct oftable *table; int error; @@ -3235,7 +3318,6 @@ collect_rules_strict(struct ofproto *ofproto, if (criteria->cookie_mask == htonll(UINT64_MAX)) { struct rule *rule; - ovs_mutex_lock(&ofproto_mutex); HINDEX_FOR_EACH_WITH_HASH (rule, cookie_node, hash_cookie(criteria->cookie), &ofproto->cookies) { @@ -3246,7 +3328,6 @@ collect_rules_strict(struct ofproto *ofproto, } } } - ovs_mutex_unlock(&ofproto_mutex); } else { FOR_EACH_MATCHING_TABLE (table, criteria->table_id, ofproto) { struct rule *rule; @@ -3284,6 +3365,7 @@ age_secs(long long int age_ms) static enum ofperr handle_flow_stats_request(struct ofconn *ofconn, const struct ofp_header *request) + OVS_EXCLUDED(ofproto_mutex) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); struct ofputil_flow_stats_request fsr; @@ -3300,8 +3382,15 @@ handle_flow_stats_request(struct ofconn *ofconn, rule_criteria_init(&criteria, fsr.table_id, &fsr.match, 0, fsr.cookie, fsr.cookie_mask, fsr.out_port); + + ovs_mutex_lock(&ofproto_mutex); error = collect_rules_loose(ofproto, &criteria, &rules); rule_criteria_destroy(&criteria); + if (!error) { + rule_collection_ref(&rules); + } + ovs_mutex_unlock(&ofproto_mutex); + if (error) { return error; } @@ -3311,32 +3400,44 @@ handle_flow_stats_request(struct ofconn *ofconn, struct rule *rule = rules.rules[i]; long long int now = time_msec(); struct ofputil_flow_stats fs; - - minimatch_expand(&rule->cr.match, &fs.match); - fs.priority = rule->cr.priority; - fs.cookie = rule->flow_cookie; - fs.table_id = rule->table_id; - calc_duration(rule->created, now, &fs.duration_sec, &fs.duration_nsec); - fs.idle_age = age_secs(now - rule->used); - fs.hard_age = age_secs(now - rule->modified); - ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count, - &fs.byte_count); - fs.ofpacts = rule->actions->ofpacts; - fs.ofpacts_len = rule->actions->ofpacts_len; + long long int created, used, modified; + struct rule_actions *actions; + bool send_flow_removed; ovs_mutex_lock(&rule->mutex); + fs.cookie = rule->flow_cookie; fs.idle_timeout = rule->idle_timeout; fs.hard_timeout = rule->hard_timeout; + created = rule->created; + used = rule->used; + modified = rule->modified; + actions = rule_get_actions__(rule); + send_flow_removed = rule->send_flow_removed; ovs_mutex_unlock(&rule->mutex); + minimatch_expand(&rule->cr.match, &fs.match); + fs.table_id = rule->table_id; + calc_duration(created, now, &fs.duration_sec, &fs.duration_nsec); + fs.priority = rule->cr.priority; + fs.idle_age = age_secs(now - used); + fs.hard_age = age_secs(now - modified); + ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count, + &fs.byte_count); + fs.ofpacts = actions->ofpacts; + fs.ofpacts_len = actions->ofpacts_len; + fs.flags = 0; - if (rule->send_flow_removed) { + if (send_flow_removed) { fs.flags |= OFPUTIL_FF_SEND_FLOW_REM; /* FIXME: Implement OFPUTIL_FF_NO_PKT_COUNTS and OFPUTIL_FF_NO_BYT_COUNTS. */ } ofputil_append_flow_stats_reply(&fs, &replies); + + rule_actions_unref(actions); } + + rule_collection_unref(&rules); rule_collection_destroy(&rules); ofconn_send_replies(ofconn, &replies); @@ -3348,23 +3449,32 @@ static void flow_stats_ds(struct rule *rule, struct ds *results) { uint64_t packet_count, byte_count; + struct rule_actions *actions; + long long int created; rule->ofproto->ofproto_class->rule_get_stats(rule, &packet_count, &byte_count); + ovs_mutex_lock(&rule->mutex); + actions = rule_get_actions__(rule); + created = rule->created; + ovs_mutex_unlock(&rule->mutex); + if (rule->table_id != 0) { ds_put_format(results, "table_id=%"PRIu8", ", rule->table_id); } - ds_put_format(results, "duration=%llds, ", - (time_msec() - rule->created) / 1000); + ds_put_format(results, "duration=%llds, ", (time_msec() - created) / 1000); ds_put_format(results, "priority=%u, ", rule->cr.priority); ds_put_format(results, "n_packets=%"PRIu64", ", packet_count); ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count); cls_rule_format(&rule->cr, results); ds_put_char(results, ','); - ofpacts_format(rule->actions->ofpacts, rule->actions->ofpacts_len, - results); + + ofpacts_format(actions->ofpacts, actions->ofpacts_len, results); + ds_put_cstr(results, "\n"); + + rule_actions_unref(actions); } /* Adds a pretty-printed description of all flows to 'results', including @@ -3415,6 +3525,7 @@ ofproto_port_get_cfm_status(const struct ofproto *ofproto, ofp_port_t ofp_port, static enum ofperr handle_aggregate_stats_request(struct ofconn *ofconn, const struct ofp_header *oh) + OVS_EXCLUDED(ofproto_mutex) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); struct ofputil_flow_stats_request request; @@ -3434,8 +3545,15 @@ handle_aggregate_stats_request(struct ofconn *ofconn, rule_criteria_init(&criteria, request.table_id, &request.match, 0, request.cookie, request.cookie_mask, request.out_port); + + ovs_mutex_lock(&ofproto_mutex); error = collect_rules_loose(ofproto, &criteria, &rules); rule_criteria_destroy(&criteria); + if (!error) { + rule_collection_ref(&rules); + } + ovs_mutex_unlock(&ofproto_mutex); + if (error) { return error; } @@ -3471,6 +3589,7 @@ handle_aggregate_stats_request(struct ofconn *ofconn, stats.byte_count = UINT64_MAX; } + rule_collection_unref(&rules); rule_collection_destroy(&rules); reply = ofputil_encode_aggregate_stats_reply(&stats, oh); @@ -3581,6 +3700,7 @@ static bool is_flow_deletion_pending(const struct ofproto *ofproto, const struct cls_rule *cls_rule, uint8_t table_id) + OVS_REQUIRES(ofproto_mutex) { if (!hmap_is_empty(&ofproto->deletions)) { struct ofoperation *op; @@ -3599,19 +3719,16 @@ is_flow_deletion_pending(const struct ofproto *ofproto, static bool should_evict_a_rule(struct oftable *table, unsigned int extra_space) + OVS_REQUIRES(ofproto_mutex) + OVS_NO_THREAD_SAFETY_ANALYSIS { - size_t count; - - ovs_rwlock_rdlock(&table->cls.rwlock); - count = classifier_count(&table->cls); - ovs_rwlock_unlock(&table->cls.rwlock); - - return count + extra_space > table->max_flows; + return classifier_count(&table->cls) + extra_space > table->max_flows; } static enum ofperr evict_rules_from_table(struct ofproto *ofproto, struct oftable *table, unsigned int extra_space) + OVS_REQUIRES(ofproto_mutex) { while (should_evict_a_rule(table, extra_space)) { struct rule *rule; @@ -3619,14 +3736,11 @@ evict_rules_from_table(struct ofproto *ofproto, struct oftable *table, if (!choose_rule_to_evict(table, &rule)) { return OFPERR_OFPFMFC_TABLE_FULL; } else if (rule->pending) { - ovs_mutex_unlock(&rule->mutex); return OFPROTO_POSTPONE; } else { struct ofopgroup *group = ofopgroup_create_unattached(ofproto); - ofoperation_create(group, rule, - OFOPERATION_DELETE, OFPRR_EVICTION); - oftable_remove_rule(rule); - ofproto->ofproto_class->rule_delete(rule); + delete_flow__(rule, group, OFPRR_EVICTION); + ofopgroup_submit(group); } } @@ -3648,6 +3762,7 @@ evict_rules_from_table(struct ofproto *ofproto, struct oftable *table, static enum ofperr add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct ofputil_flow_mod *fm, const struct ofp_header *request) + OVS_REQUIRES(ofproto_mutex) { struct oftable *table; struct ofopgroup *group; @@ -3810,6 +3925,7 @@ static enum ofperr modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, struct ofputil_flow_mod *fm, const struct ofp_header *request, const struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { enum ofoperation_type type; struct ofopgroup *group; @@ -3893,6 +4009,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, static enum ofperr modify_flows_add(struct ofproto *ofproto, struct ofconn *ofconn, struct ofputil_flow_mod *fm, const struct ofp_header *request) + OVS_REQUIRES(ofproto_mutex) { if (fm->cookie_mask != htonll(0) || fm->new_cookie == htonll(UINT64_MAX)) { return 0; @@ -3909,6 +4026,7 @@ static enum ofperr modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn, struct ofputil_flow_mod *fm, const struct ofp_header *request) + OVS_REQUIRES(ofproto_mutex) { struct rule_criteria criteria; struct rule_collection rules; @@ -3939,6 +4057,7 @@ static enum ofperr modify_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn, struct ofputil_flow_mod *fm, const struct ofp_header *request) + OVS_REQUIRES(ofproto_mutex) { struct rule_criteria criteria; struct rule_collection rules; @@ -3967,6 +4086,7 @@ modify_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn, static void delete_flow__(struct rule *rule, struct ofopgroup *group, enum ofp_flow_removed_reason reason) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = rule->ofproto; @@ -3985,15 +4105,14 @@ delete_flows__(struct ofproto *ofproto, struct ofconn *ofconn, const struct ofp_header *request, const struct rule_collection *rules, enum ofp_flow_removed_reason reason) + OVS_REQUIRES(ofproto_mutex) { struct ofopgroup *group; size_t i; group = ofopgroup_create(ofproto, ofconn, request, UINT32_MAX); for (i = 0; i < rules->n; i++) { - struct rule *rule = rules->rules[i]; - ovs_mutex_lock(&rule->mutex); - delete_flow__(rule, group, reason); + delete_flow__(rules->rules[i], group, reason); } ofopgroup_submit(group); @@ -4005,6 +4124,7 @@ static enum ofperr delete_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn, const struct ofputil_flow_mod *fm, const struct ofp_header *request) + OVS_REQUIRES(ofproto_mutex) { struct rule_criteria criteria; struct rule_collection rules; @@ -4029,6 +4149,7 @@ static enum ofperr delete_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn, const struct ofputil_flow_mod *fm, const struct ofp_header *request) + OVS_REQUIRES(ofproto_mutex) { struct rule_criteria criteria; struct rule_collection rules; @@ -4049,6 +4170,7 @@ delete_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn, static void ofproto_rule_send_removed(struct rule *rule, uint8_t reason) + OVS_REQUIRES(ofproto_mutex) { struct ofputil_flow_removed fr; @@ -4084,16 +4206,16 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason) * OpenFlow flows. */ void ofproto_rule_expire(struct rule *rule, uint8_t reason) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = rule->ofproto; struct classifier *cls = &ofproto->tables[rule->table_id].cls; - ovs_assert(reason == OFPRR_HARD_TIMEOUT || reason == OFPRR_IDLE_TIMEOUT); - ofproto_rule_send_removed(rule, reason); + ovs_assert(reason == OFPRR_HARD_TIMEOUT || reason == OFPRR_IDLE_TIMEOUT + || reason == OFPRR_DELETE); - ovs_rwlock_wrlock(&cls->rwlock); - ofproto_rule_delete(ofproto, cls, rule); - ovs_rwlock_unlock(&cls->rwlock); + ofproto_rule_send_removed(rule, reason); + ofproto_rule_delete__(ofproto, cls, rule); } /* Reduces '*timeout' to no more than 'max'. A value of zero in either case @@ -4134,6 +4256,7 @@ ofproto_rule_reduce_timeouts(struct rule *rule, static enum ofperr handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh) + OVS_EXCLUDED(ofproto_mutex) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); struct ofputil_flow_mod fm; @@ -4192,9 +4315,11 @@ exit: static enum ofperr handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn, struct ofputil_flow_mod *fm, const struct ofp_header *oh) + OVS_EXCLUDED(ofproto_mutex) { enum ofperr error; + ovs_mutex_lock(&ofproto_mutex); if (ofproto->n_pending < 50) { switch (fm->command) { case OFPFC_ADD: @@ -4230,6 +4355,7 @@ handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn, ovs_assert(!list_is_empty(&ofproto->pending)); error = OFPROTO_POSTPONE; } + ovs_mutex_unlock(&ofproto_mutex); run_rule_executes(ofproto); return error; @@ -4388,6 +4514,7 @@ static void ofproto_compose_flow_refresh_update(const struct rule *rule, enum nx_flow_monitor_flags flags, struct list *msgs) + OVS_REQUIRES(ofproto_mutex) { struct ofoperation *op = rule->pending; const struct rule_actions *actions; @@ -4449,6 +4576,7 @@ ofproto_compose_flow_refresh_update(const struct rule *rule, void ofmonitor_compose_refresh_updates(struct rule_collection *rules, struct list *msgs) + OVS_REQUIRES(ofproto_mutex) { size_t i; @@ -4465,6 +4593,7 @@ static void ofproto_collect_ofmonitor_refresh_rule(const struct ofmonitor *m, struct rule *rule, uint64_t seqno, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { enum nx_flow_monitor_flags update; @@ -4504,6 +4633,7 @@ static void ofproto_collect_ofmonitor_refresh_rules(const struct ofmonitor *m, uint64_t seqno, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { const struct ofproto *ofproto = ofconn_get_ofproto(m->ofconn); const struct ofoperation *op; @@ -4540,6 +4670,7 @@ ofproto_collect_ofmonitor_refresh_rules(const struct ofmonitor *m, static void ofproto_collect_ofmonitor_initial_rules(struct ofmonitor *m, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { if (m->flags & NXFMF_INITIAL) { ofproto_collect_ofmonitor_refresh_rules(m, 0, rules); @@ -4549,12 +4680,14 @@ ofproto_collect_ofmonitor_initial_rules(struct ofmonitor *m, void ofmonitor_collect_resume_rules(struct ofmonitor *m, uint64_t seqno, struct rule_collection *rules) + OVS_REQUIRES(ofproto_mutex) { ofproto_collect_ofmonitor_refresh_rules(m, seqno, rules); } static enum ofperr handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh) + OVS_EXCLUDED(ofproto_mutex) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); struct ofmonitor **monitors; @@ -4569,6 +4702,8 @@ handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh) ofpbuf_use_const(&b, oh, ntohs(oh->length)); monitors = NULL; n_monitors = allocated_monitors = 0; + + ovs_mutex_lock(&ofproto_mutex); for (;;) { struct ofputil_flow_monitor_request request; struct ofmonitor *m; @@ -4607,15 +4742,18 @@ handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh) ofpmp_init(&replies, oh); ofmonitor_compose_refresh_updates(&rules, &replies); + ovs_mutex_unlock(&ofproto_mutex); + rule_collection_destroy(&rules); ofconn_send_replies(ofconn, &replies); - free(monitors); return 0; error: + ovs_mutex_unlock(&ofproto_mutex); + for (i = 0; i < n_monitors; i++) { ofmonitor_destroy(monitors[i]); } @@ -4625,18 +4763,25 @@ error: static enum ofperr handle_flow_monitor_cancel(struct ofconn *ofconn, const struct ofp_header *oh) + OVS_EXCLUDED(ofproto_mutex) { struct ofmonitor *m; + enum ofperr error; uint32_t id; id = ofputil_decode_flow_monitor_cancel(oh); + + ovs_mutex_lock(&ofproto_mutex); m = ofmonitor_lookup(ofconn, id); - if (!m) { - return OFPERR_NXBRC_FM_BAD_ID; + if (m) { + ofmonitor_destroy(m); + error = 0; + } else { + error = OFPERR_NXBRC_FM_BAD_ID; } + ovs_mutex_unlock(&ofproto_mutex); - ofmonitor_destroy(m); - return 0; + return error; } /* Meters implementation. @@ -4707,6 +4852,7 @@ meter_create(const struct ofputil_meter_config *config, static void meter_delete(struct ofproto *ofproto, uint32_t first, uint32_t last) + OVS_REQUIRES(ofproto_mutex) { uint32_t mid; for (mid = first; mid <= last; ++mid) { @@ -4764,6 +4910,7 @@ handle_modify_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm) static enum ofperr handle_delete_meter(struct ofconn *ofconn, const struct ofp_header *oh, struct ofputil_meter_mod *mm) + OVS_EXCLUDED(ofproto_mutex) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); uint32_t meter_id = mm->meter.meter_id; @@ -4784,6 +4931,7 @@ handle_delete_meter(struct ofconn *ofconn, const struct ofp_header *oh, /* First delete the rules that use this meter. If any of those rules are * currently being modified, postpone the whole operation until later. */ rule_collection_init(&rules); + ovs_mutex_lock(&ofproto_mutex); for (meter_id = first; meter_id <= last; ++meter_id) { struct meter *meter = ofproto->meters[meter_id]; if (meter && !list_is_empty(&meter->rules)) { @@ -4806,6 +4954,7 @@ handle_delete_meter(struct ofconn *ofconn, const struct ofp_header *oh, meter_delete(ofproto, first, last); exit: + ovs_mutex_unlock(&ofproto_mutex); rule_collection_destroy(&rules); return error; @@ -4961,6 +5110,7 @@ handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request, static enum ofperr handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg) + OVS_EXCLUDED(ofproto_mutex) { const struct ofp_header *oh = msg->data; enum ofptype type; @@ -5106,6 +5256,7 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg) static bool handle_openflow(struct ofconn *ofconn, const struct ofpbuf *ofp_msg) + OVS_EXCLUDED(ofproto_mutex) { int error = handle_openflow__(ofconn, ofp_msg); if (error && error != OFPROTO_POSTPONE) { @@ -5124,6 +5275,7 @@ handle_openflow(struct ofconn *ofconn, const struct ofpbuf *ofp_msg) * ofoperation_create() and then submit it with ofopgroup_submit(). */ static struct ofopgroup * ofopgroup_create_unattached(struct ofproto *ofproto) + OVS_REQUIRES(ofproto_mutex) { struct ofopgroup *group = xzalloc(sizeof *group); group->ofproto = ofproto; @@ -5148,6 +5300,7 @@ ofopgroup_create_unattached(struct ofproto *ofproto) static struct ofopgroup * ofopgroup_create(struct ofproto *ofproto, struct ofconn *ofconn, const struct ofp_header *request, uint32_t buffer_id) + OVS_REQUIRES(ofproto_mutex) { struct ofopgroup *group = ofopgroup_create_unattached(ofproto); if (ofconn) { @@ -5171,6 +5324,7 @@ ofopgroup_create(struct ofproto *ofproto, struct ofconn *ofconn, * groups. */ static void ofopgroup_submit(struct ofopgroup *group) + OVS_REQUIRES(ofproto_mutex) { if (!group->n_running) { ofopgroup_complete(group); @@ -5182,6 +5336,7 @@ ofopgroup_submit(struct ofopgroup *group) static void ofopgroup_complete(struct ofopgroup *group) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = group->ofproto; @@ -5302,7 +5457,6 @@ ofopgroup_complete(struct ofopgroup *group) } } } else { - ovs_mutex_lock(&rule->mutex); oftable_remove_rule(rule); ofproto_rule_unref(rule); } @@ -5381,6 +5535,7 @@ static struct ofoperation * ofoperation_create(struct ofopgroup *group, struct rule *rule, enum ofoperation_type type, enum ofp_flow_removed_reason reason) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = group->ofproto; struct ofoperation *op; @@ -5412,6 +5567,7 @@ ofoperation_create(struct ofopgroup *group, struct rule *rule, static void ofoperation_destroy(struct ofoperation *op) + OVS_REQUIRES(ofproto_mutex) { struct ofopgroup *group = op->group; @@ -5453,13 +5609,19 @@ ofoperation_complete(struct ofoperation *op, enum ofperr error) { struct ofopgroup *group = op->group; - ovs_assert(op->rule->pending == op); ovs_assert(group->n_running > 0); ovs_assert(!error || op->type != OFOPERATION_DELETE); op->error = error; if (!--group->n_running && !list_is_empty(&group->ofproto_node)) { + /* This function can be called from ->rule_construct(), in which case + * ofproto_mutex is held, or it can be called from ->run(), in which + * case ofproto_mutex is not held. But only in the latter case can we + * arrive here, so we can safely take ofproto_mutex now. */ + ovs_mutex_lock(&ofproto_mutex); + ovs_assert(op->rule->pending == op); ofopgroup_complete(group); + ovs_mutex_unlock(&ofproto_mutex); } } @@ -5500,6 +5662,7 @@ pick_fallback_dpid(void) * or with no timeouts are not evictable.) */ static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep) + OVS_REQUIRES(ofproto_mutex) { struct eviction_group *evg; @@ -5524,10 +5687,8 @@ choose_rule_to_evict(struct oftable *table, struct rule **rulep) struct rule *rule; HEAP_FOR_EACH (rule, evg_node, &evg->rules) { - if (!ovs_mutex_trylock(&rule->mutex)) { - *rulep = rule; - return true; - } + *rulep = rule; + return true; } } @@ -5545,9 +5706,11 @@ ofproto_evict(struct ofproto *ofproto) { struct oftable *table; + ovs_mutex_lock(&ofproto_mutex); OFPROTO_FOR_EACH_TABLE (table, ofproto) { evict_rules_from_table(ofproto, table, 0); } + ovs_mutex_unlock(&ofproto_mutex); } /* Eviction groups. */ @@ -5566,6 +5729,7 @@ eviction_group_priority(size_t n_rules) * adds or removes rules in 'evg'. */ static void eviction_group_resized(struct oftable *table, struct eviction_group *evg) + OVS_REQUIRES(ofproto_mutex) { heap_change(&table->eviction_groups_by_size, &evg->size_node, eviction_group_priority(heap_count(&evg->rules))); @@ -5581,6 +5745,7 @@ eviction_group_resized(struct oftable *table, struct eviction_group *evg) * - Frees 'evg'. */ static void eviction_group_destroy(struct oftable *table, struct eviction_group *evg) + OVS_REQUIRES(ofproto_mutex) { while (!heap_is_empty(&evg->rules)) { struct rule *rule; @@ -5597,6 +5762,7 @@ eviction_group_destroy(struct oftable *table, struct eviction_group *evg) /* Removes 'rule' from its eviction group, if any. */ static void eviction_group_remove_rule(struct rule *rule) + OVS_REQUIRES(ofproto_mutex) { if (rule->eviction_group) { struct oftable *table = &rule->ofproto->tables[rule->table_id]; @@ -5616,6 +5782,7 @@ eviction_group_remove_rule(struct rule *rule) * returns the hash value. */ static uint32_t eviction_group_hash_rule(struct rule *rule) + OVS_REQUIRES(ofproto_mutex) { struct oftable *table = &rule->ofproto->tables[rule->table_id]; const struct mf_subfield *sf; @@ -5653,6 +5820,7 @@ eviction_group_hash_rule(struct rule *rule) * if necessary. */ static struct eviction_group * eviction_group_find(struct oftable *table, uint32_t id) + OVS_REQUIRES(ofproto_mutex) { struct eviction_group *evg; @@ -5674,6 +5842,7 @@ eviction_group_find(struct oftable *table, uint32_t id) * for eviction. */ static uint32_t rule_eviction_priority(struct rule *rule) + OVS_REQUIRES(ofproto_mutex) { long long int hard_expiration; long long int idle_expiration; @@ -5713,6 +5882,7 @@ rule_eviction_priority(struct rule *rule) * The caller must ensure that 'rule' is not already in an eviction group. */ static void eviction_group_add_rule(struct rule *rule) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = rule->ofproto; struct oftable *table = &ofproto->tables[rule->table_id]; @@ -5785,6 +5955,7 @@ oftable_set_name(struct oftable *table, const char *name) * This function configures the former policy on 'table'. */ static void oftable_disable_eviction(struct oftable *table) + OVS_REQUIRES(ofproto_mutex) { if (table->eviction_fields) { struct eviction_group *evg, *next; @@ -5811,6 +5982,7 @@ oftable_disable_eviction(struct oftable *table) static void oftable_enable_eviction(struct oftable *table, const struct mf_subfield *fields, size_t n_fields) + OVS_REQUIRES(ofproto_mutex) { struct cls_cursor cursor; struct rule *rule; @@ -5845,42 +6017,39 @@ oftable_enable_eviction(struct oftable *table, static void oftable_remove_rule__(struct ofproto *ofproto, struct classifier *cls, struct rule *rule) - OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->mutex) + OVS_REQUIRES(ofproto_mutex) { + ovs_rwlock_wrlock(&cls->rwlock); classifier_remove(cls, CONST_CAST(struct cls_rule *, &rule->cr)); + ovs_rwlock_unlock(&cls->rwlock); - ovs_mutex_lock(&ofproto_mutex); cookies_remove(ofproto, rule); - ovs_mutex_unlock(&ofproto_mutex); eviction_group_remove_rule(rule); - ovs_mutex_lock(&ofproto_mutex); if (!list_is_empty(&rule->expirable)) { list_remove(&rule->expirable); } - 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); } - ovs_mutex_unlock(&rule->mutex); } static void oftable_remove_rule(struct rule *rule) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = rule->ofproto; struct oftable *table = &ofproto->tables[rule->table_id]; - ovs_rwlock_wrlock(&table->cls.rwlock); oftable_remove_rule__(ofproto, &table->cls, rule); - ovs_rwlock_unlock(&table->cls.rwlock); } /* Inserts 'rule' into its oftable, which must not already contain any rule for * the same cls_rule. */ static void oftable_insert_rule(struct rule *rule) + OVS_REQUIRES(ofproto_mutex) { struct ofproto *ofproto = rule->ofproto; struct oftable *table = &ofproto->tables[rule->table_id]; @@ -5891,14 +6060,10 @@ oftable_insert_rule(struct rule *rule) ovs_mutex_unlock(&rule->mutex); if (may_expire) { - ovs_mutex_lock(&ofproto_mutex); list_insert(&ofproto->expirable, &rule->expirable); - ovs_mutex_unlock(&ofproto_mutex); } - ovs_mutex_lock(&ofproto_mutex); cookies_insert(ofproto, rule); - ovs_mutex_unlock(&ofproto_mutex); if (rule->actions->meter_id) { struct meter *meter = ofproto->meters[rule->actions->meter_id]; @@ -5975,6 +6140,7 @@ ofproto_get_vlan_usage(struct ofproto *ofproto, unsigned long int *vlan_bitmap) OFPROTO_FOR_EACH_TABLE (oftable, ofproto) { const struct cls_table *table; + ovs_rwlock_rdlock(&oftable->cls.rwlock); HMAP_FOR_EACH (table, hmap_node, &oftable->cls.tables) { if (minimask_get_vid_mask(&table->mask) == VLAN_VID_MASK) { const struct cls_rule *rule; @@ -5986,6 +6152,7 @@ ofproto_get_vlan_usage(struct ofproto *ofproto, unsigned long int *vlan_bitmap) } } } + ovs_rwlock_unlock(&oftable->cls.rwlock); } } -- 2.20.1