ofproto: Add global locking around flow table changes.
authorBen Pfaff <blp@nicira.com>
Fri, 13 Sep 2013 03:45:15 +0000 (20:45 -0700)
committerBen Pfaff <blp@nicira.com>
Fri, 13 Sep 2013 04:35:32 +0000 (21:35 -0700)
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 <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
lib/classifier.h
ofproto/connmgr.c
ofproto/connmgr.h
ofproto/fail-open.c
ofproto/ofproto-dpif.c
ofproto/ofproto-provider.h
ofproto/ofproto.c

index ead087b..a795b4a 100644 (file)
 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. */
index 9ab7a3f..20484b5 100644 (file)
@@ -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;
index b57e741..b31e855 100644 (file)
@@ -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 *);
 
 
index 2c0a8f3..4900c05 100644 (file)
@@ -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);
index d0b9506..975097d 100644 (file)
@@ -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);
+    }
 }
 \f
 /* 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);
 }
 \f
 /* 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
index 2c1329a..bfd3e8d 100644 (file)
 #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 */
index 2d9696c..284d874 100644 (file)
@@ -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,
 \f
 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);
     }
 }
 \f
@@ -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);
 }
 \f
 /* 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);
     }
 }