ofproto-dpif: Lock rules to prevent eviction.
authorEthan Jackson <ethan@nicira.com>
Wed, 17 Jul 2013 23:14:02 +0000 (16:14 -0700)
committerEthan Jackson <ethan@nicira.com>
Sun, 11 Aug 2013 17:36:18 +0000 (10:36 -0700)
This patch uses a read-write lock to prevent rules from being evicted
while they're used by child threads.  It also changes the prototypes
of the various rule lookup functions so that the thread safety
analysis can be used to ensure that the locking is handled properly.

Signed-off-by: Ethan Jackson <ethan@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
ofproto/ofproto-dpif-xlate.c
ofproto/ofproto-dpif.c
ofproto/ofproto-dpif.h
ofproto/ofproto-provider.h
ofproto/ofproto.c

index 30697ac..8081547 100644 (file)
@@ -1660,34 +1660,6 @@ compose_output_action(struct xlate_ctx *ctx, ofp_port_t ofp_port)
     compose_output_action__(ctx, ofp_port, true);
 }
 
-/* Common rule processing in one place to avoid duplicating code. */
-static struct rule_dpif *
-ctx_rule_hooks(struct xlate_ctx *ctx, struct rule_dpif *rule,
-               bool may_packet_in)
-{
-    if (ctx->xin->resubmit_hook) {
-        ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse);
-    }
-    if (rule == NULL && may_packet_in) {
-        struct xport *xport;
-
-        /* XXX
-         * check if table configuration flags
-         * OFPTC_TABLE_MISS_CONTROLLER, default.
-         * OFPTC_TABLE_MISS_CONTINUE,
-         * OFPTC_TABLE_MISS_DROP
-         * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? */
-        xport = get_ofp_port(ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
-        rule = choose_miss_rule(xport ? xport->config : 0,
-                                ctx->xbridge->miss_rule,
-                                ctx->xbridge->no_packet_in_rule);
-    }
-    if (rule && ctx->xin->resubmit_stats) {
-        rule_credit_stats(rule, ctx->xin->resubmit_stats);
-    }
-    return rule;
-}
-
 static void
 xlate_table_action(struct xlate_ctx *ctx,
                    ofp_port_t in_port, uint8_t table_id, bool may_packet_in)
@@ -1701,15 +1673,39 @@ xlate_table_action(struct xlate_ctx *ctx,
 
         /* Look up a flow with 'in_port' as the input port. */
         ctx->xin->flow.in_port.ofp_port = in_port;
-        rule = rule_dpif_lookup_in_table(ctx->xbridge->ofproto,
-                                         &ctx->xin->flow, &ctx->xout->wc,
-                                         table_id);
+        rule_dpif_lookup_in_table(ctx->xbridge->ofproto, &ctx->xin->flow,
+                                  &ctx->xout->wc, table_id, &rule);
 
         /* Restore the original input port.  Otherwise OFPP_NORMAL and
          * OFPP_IN_PORT will have surprising behavior. */
         ctx->xin->flow.in_port.ofp_port = old_in_port;
 
-        rule = ctx_rule_hooks(ctx, rule, may_packet_in);
+        if (ctx->xin->resubmit_hook) {
+            ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse);
+        }
+
+        if (rule == NULL && may_packet_in) {
+            struct xport *xport;
+
+            /* Makes clang's thread safety analysis happy. */
+            rule_release(rule);
+
+            /* XXX
+             * check if table configuration flags
+             * OFPTC_TABLE_MISS_CONTROLLER, default.
+             * OFPTC_TABLE_MISS_CONTINUE,
+             * OFPTC_TABLE_MISS_DROP
+             * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? */
+            xport = get_ofp_port(ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
+            rule = choose_miss_rule(xport ? xport->config : 0,
+                                    ctx->xbridge->miss_rule,
+                                    ctx->xbridge->no_packet_in_rule);
+            ovs_rwlock_rdlock(&rule->up.evict);
+        }
+
+        if (rule && ctx->xin->resubmit_stats) {
+            rule_credit_stats(rule, ctx->xin->resubmit_stats);
+        }
 
         if (rule) {
             struct rule_dpif *old_rule = ctx->rule;
@@ -1720,6 +1716,7 @@ xlate_table_action(struct xlate_ctx *ctx,
             ctx->rule = old_rule;
             ctx->recurse--;
         }
+        rule_release(rule);
 
         ctx->table_id = old_table_id;
     } else {
@@ -2198,15 +2195,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 {
     struct flow_wildcards *wc = &ctx->xout->wc;
     struct flow *flow = &ctx->xin->flow;
-    bool was_evictable = true;
     const struct ofpact *a;
 
-    if (ctx->rule) {
-        /* Don't let the rule we're working on get evicted underneath us. */
-        was_evictable = ctx->rule->up.evictable;
-        ctx->rule->up.evictable = false;
-    }
-
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
         struct ofpact_controller *controller;
         const struct ofpact_metadata *metadata;
@@ -2352,20 +2342,20 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_SET_MPLS_TTL:
             if (compose_set_mpls_ttl_action(ctx,
                                             ofpact_get_SET_MPLS_TTL(a)->ttl)) {
-                goto out;
+                return;
             }
             break;
 
         case OFPACT_DEC_MPLS_TTL:
             if (compose_dec_mpls_ttl_action(ctx)) {
-                goto out;
+                return;
             }
             break;
 
         case OFPACT_DEC_TTL:
             wc->masks.nw_ttl = 0xff;
             if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
-                goto out;
+                return;
             }
             break;
 
@@ -2432,11 +2422,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
         }
     }
-
-out:
-    if (ctx->rule) {
-        ctx->rule->up.evictable = was_evictable;
-    }
 }
 
 void
index 13faad6..6e8c6e9 100644 (file)
@@ -82,10 +82,6 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
 struct flow_miss;
 struct facet;
 
-static struct rule_dpif *rule_dpif_lookup(struct ofproto_dpif *,
-                                          const struct flow *,
-                                          struct flow_wildcards *wc);
-
 static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes);
 
 struct ofbundle {
@@ -1387,9 +1383,12 @@ add_internal_flow(struct ofproto_dpif *ofproto, int id,
         return error;
     }
 
-    *rulep = rule_dpif_lookup_in_table(ofproto, &fm.match.flow, NULL,
-                                       TBL_INTERNAL);
-    ovs_assert(*rulep != NULL);
+    if (rule_dpif_lookup_in_table(ofproto, &fm.match.flow, NULL, TBL_INTERNAL,
+                                  rulep)) {
+        ovs_rwlock_unlock(&(*rulep)->up.evict);
+    } else {
+        NOT_REACHED();
+    }
 
     return 0;
 }
@@ -3506,9 +3505,10 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
             struct rule_dpif *rule;
             struct xlate_in xin;
 
-            rule = rule_dpif_lookup(facet->ofproto, &facet->flow, NULL);
+            rule_dpif_lookup(facet->ofproto, &facet->flow, NULL, &rule);
             xlate_in_init(&xin, facet->ofproto, &miss->flow, rule, 0, packet);
             xlate_actions_for_side_effects(&xin);
+            rule_release(rule);
         }
 
         if (facet->xout.odp_actions.size) {
@@ -3605,7 +3605,7 @@ handle_flow_miss(struct flow_miss *miss, struct flow_miss_op *ops,
         struct xlate_in xin;
 
         flow_wildcards_init_catchall(&wc);
-        rule = rule_dpif_lookup(ofproto, &miss->flow, &wc);
+        rule_dpif_lookup(ofproto, &miss->flow, &wc, &rule);
         rule_credit_stats(rule, stats);
 
         xlate_in_init(&xin, ofproto, &miss->flow, rule, stats->tcp_flags,
@@ -3623,10 +3623,12 @@ handle_flow_miss(struct flow_miss *miss, struct flow_miss_op *ops,
         if (miss->key_fitness == ODP_FIT_TOO_LITTLE
             || !flow_miss_should_make_facet(miss, &xout.wc)) {
             handle_flow_miss_without_facet(rule, &xout, miss, ops, n_ops);
+            rule_release(rule);
             return;
         }
 
         facet = facet_create(miss, rule, &xout, stats);
+        rule_release(rule);
         stats = NULL;
     }
     handle_flow_miss_with_facet(miss, facet, now, stats, ops, n_ops);
@@ -4342,10 +4344,12 @@ rule_expire(struct rule_dpif *rule)
         return;
     }
 
-    COVERAGE_INC(ofproto_dpif_expired);
+    if (!ovs_rwlock_trywrlock(&rule->up.evict)) {
+        COVERAGE_INC(ofproto_dpif_expired);
 
-    /* Get rid of the rule. */
-    ofproto_rule_expire(&rule->up, reason);
+        /* Get rid of the rule. */
+        ofproto_rule_expire(&rule->up, reason);
+    }
 }
 \f
 /* Facets. */
@@ -4542,16 +4546,19 @@ facet_is_controller_flow(struct facet *facet)
 {
     if (facet) {
         struct ofproto_dpif *ofproto = facet->ofproto;
-        const struct rule_dpif *rule = rule_dpif_lookup(ofproto, &facet->flow,
-                                                        NULL);
-        const struct ofpact *ofpacts = rule->up.ofpacts;
-        size_t ofpacts_len = rule->up.ofpacts_len;
-
-        if (ofpacts_len > 0 &&
-            ofpacts->type == OFPACT_CONTROLLER &&
-            ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len)) {
-            return true;
-        }
+        const struct ofpact *ofpacts;
+        struct rule_dpif *rule;
+        size_t ofpacts_len;
+        bool is_controller;
+
+        rule_dpif_lookup(ofproto, &facet->flow, NULL, &rule);
+        ofpacts_len = rule->up.ofpacts_len;
+        ofpacts = rule->up.ofpacts;
+        is_controller = ofpacts_len > 0
+            && ofpacts->type == OFPACT_CONTROLLER
+            && ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len);
+        rule_release(rule);
+        return is_controller;
     }
     return false;
 }
@@ -4641,9 +4648,10 @@ facet_check_consistency(struct facet *facet)
     bool ok, fail_open;
 
     /* Check the datapath actions for consistency. */
-    rule = rule_dpif_lookup(facet->ofproto, &facet->flow, NULL);
+    rule_dpif_lookup(facet->ofproto, &facet->flow, NULL, &rule);
     xlate_in_init(&xin, facet->ofproto, &facet->flow, rule, 0, NULL);
     xlate_actions(&xin, &xout);
+    rule_release(rule);
 
     fail_open = rule->up.cr.priority == FAIL_OPEN_PRIORITY;
     ok = ofpbuf_equal(&facet->xout.odp_actions, &xout.odp_actions)
@@ -4724,7 +4732,7 @@ facet_revalidate(struct facet *facet)
     }
 
     flow_wildcards_init_catchall(&wc);
-    new_rule = rule_dpif_lookup(ofproto, &facet->flow, &wc);
+    rule_dpif_lookup(ofproto, &facet->flow, &wc, &new_rule);
 
     /* Calculate new datapath actions.
      *
@@ -4747,6 +4755,7 @@ facet_revalidate(struct facet *facet)
         || memcmp(&facet->xout.wc, &xout.wc, sizeof xout.wc)) {
         facet_remove(facet);
         xlate_out_uninit(&xout);
+        rule_release(new_rule);
         return false;
     }
 
@@ -4779,6 +4788,7 @@ facet_revalidate(struct facet *facet)
     facet->fail_open = new_rule->up.cr.priority == FAIL_OPEN_PRIORITY;
 
     xlate_out_uninit(&xout);
+    rule_release(new_rule);
     return true;
 }
 
@@ -4821,7 +4831,7 @@ facet_push_stats(struct facet *facet, bool may_learn)
             netdev_vport_inc_rx(in_port->up.netdev, &stats);
         }
 
-        rule = rule_dpif_lookup(ofproto, &facet->flow, NULL);
+        rule_dpif_lookup(ofproto, &facet->flow, NULL, &rule);
         rule_credit_stats(rule, &stats);
         netflow_flow_update_time(ofproto->netflow, &facet->nf_flow,
                                  facet->used);
@@ -4834,6 +4844,7 @@ facet_push_stats(struct facet *facet, bool may_learn)
         xin.resubmit_stats = &stats;
         xin.may_learn = may_learn;
         xlate_actions_for_side_effects(&xin);
+        rule_release(rule);
     }
 }
 
@@ -5134,16 +5145,14 @@ subfacet_update_stats(struct subfacet *subfacet,
 
 /* Lookup 'flow' in 'ofproto''s classifier.  If 'wc' is non-null, sets
  * the fields that were relevant as part of the lookup. */
-static struct rule_dpif *
+void
 rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow,
-                 struct flow_wildcards *wc)
+                 struct flow_wildcards *wc, struct rule_dpif **rule)
 {
     struct ofport_dpif *port;
-    struct rule_dpif *rule;
 
-    rule = rule_dpif_lookup_in_table(ofproto, flow, wc, 0);
-    if (rule) {
-        return rule;
+    if (rule_dpif_lookup_in_table(ofproto, flow, wc, 0, rule)) {
+        return;
     }
     port = get_ofp_port(ofproto, flow->in_port.ofp_port);
     if (!port) {
@@ -5151,21 +5160,23 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow,
                      flow->in_port.ofp_port);
     }
 
-    return choose_miss_rule(port ? port->up.pp.config : 0, ofproto->miss_rule,
-                            ofproto->no_packet_in_rule);
+    *rule = choose_miss_rule(port ? port->up.pp.config : 0, ofproto->miss_rule,
+                             ofproto->no_packet_in_rule);
+    ovs_rwlock_rdlock(&(*rule)->up.evict);
 }
 
-struct rule_dpif *
+bool
 rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
                           const struct flow *flow, struct flow_wildcards *wc,
-                          uint8_t table_id)
+                          uint8_t table_id, struct rule_dpif **rule)
 {
     struct cls_rule *cls_rule;
     struct classifier *cls;
     bool frag;
 
+    *rule = NULL;
     if (table_id >= N_TABLES) {
-        return NULL;
+        return false;
     }
 
     if (wc) {
@@ -5174,26 +5185,32 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
     }
 
     cls = &ofproto->up.tables[table_id].cls;
+    ovs_rwlock_rdlock(&cls->rwlock);
     frag = (flow->nw_frag & FLOW_NW_FRAG_ANY) != 0;
     if (frag && ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
         /* We must pretend that transport ports are unavailable. */
         struct flow ofpc_normal_flow = *flow;
         ofpc_normal_flow.tp_src = htons(0);
         ofpc_normal_flow.tp_dst = htons(0);
-        ovs_rwlock_rdlock(&cls->rwlock);
         cls_rule = classifier_lookup(cls, &ofpc_normal_flow, wc);
-        ovs_rwlock_unlock(&cls->rwlock);
     } else if (frag && ofproto->up.frag_handling == OFPC_FRAG_DROP) {
         cls_rule = &ofproto->drop_frags_rule->up.cr;
         if (wc) {
             flow_wildcards_init_exact(wc);
         }
     } else {
-        ovs_rwlock_rdlock(&cls->rwlock);
         cls_rule = classifier_lookup(cls, flow, wc);
-        ovs_rwlock_unlock(&cls->rwlock);
     }
-    return rule_dpif_cast(rule_from_cls_rule(cls_rule));
+
+    *rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
+    if (*rule && ovs_rwlock_tryrdlock(&(*rule)->up.evict)) {
+        /* The rule is in the process of being removed.  Best we can do is
+         * pretend it isn't there. */
+        *rule = NULL;
+    }
+    ovs_rwlock_unlock(&cls->rwlock);
+
+    return *rule != NULL;
 }
 
 /* Given a port configuration (specified as zero if there's no port), chooses
@@ -5206,6 +5223,14 @@ choose_miss_rule(enum ofputil_port_config config, struct rule_dpif *miss_rule,
     return config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule;
 }
 
+void
+rule_release(struct rule_dpif *rule)
+{
+    if (rule) {
+        ovs_rwlock_unlock(&rule->up.evict);
+    }
+}
+
 static void
 complete_operation(struct rule_dpif *rule)
 {
@@ -5825,7 +5850,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
     flow_format(ds, flow);
     ds_put_char(ds, '\n');
 
-    rule = rule_dpif_lookup(ofproto, flow, NULL);
+    rule_dpif_lookup(ofproto, flow, NULL, &rule);
 
     trace_format_rule(ds, 0, rule);
     if (rule == ofproto->miss_rule) {
@@ -5895,6 +5920,8 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
 
         xlate_out_uninit(&trace.xout);
     }
+
+    rule_release(rule);
 }
 
 static void
index a74146b..30ba566 100644 (file)
@@ -55,10 +55,16 @@ static inline struct rule_dpif *rule_dpif_cast(const struct rule *rule)
     return rule ? CONTAINER_OF(rule, struct rule_dpif, up) : NULL;
 }
 
-struct rule_dpif *rule_dpif_lookup_in_table(struct ofproto_dpif *,
-                                            const struct flow *,
-                                            struct flow_wildcards *,
-                                            uint8_t table_id);
+void rule_dpif_lookup(struct ofproto_dpif *, const struct flow *,
+                      struct flow_wildcards *, struct rule_dpif **rule)
+    OVS_ACQ_RDLOCK((*rule)->up.evict);
+
+bool rule_dpif_lookup_in_table(struct ofproto_dpif *, const struct flow *,
+                               struct flow_wildcards *, uint8_t table_id,
+                               struct rule_dpif **rule)
+    OVS_ACQ_RDLOCK((*rule)->up.evict);
+
+void rule_release(struct rule_dpif *rule) OVS_RELEASES(rule->up.evict);
 
 void rule_credit_stats(struct rule_dpif *, const struct dpif_flow_stats *);
 
index f081482..aa262bc 100644 (file)
@@ -230,10 +230,18 @@ struct rule {
     uint16_t idle_timeout OVS_GUARDED; /* In seconds from ->used. */
 
     /* Eviction groups. */
-    bool evictable;              /* If false, prevents eviction. */
     struct heap_node evg_node;   /* In eviction_group's "rules" heap. */
     struct eviction_group *eviction_group; /* NULL if not in any group. */
 
+    /* The evict lock is used to prevent rules from being evicted while child
+     * threads are using them to xlate flows.  A read lock means the rule is
+     * currently being used.  A write lock means the rule is in the process of
+     * being evicted and should be considered gone.  A rule will not be evicted
+     * unless both its own and its classifiers write locks are held.
+     * Therefore, while holding a classifier readlock, one can be assured that
+     * even write locked rules are safe. */
+    struct ovs_rwlock evict;
+
     struct ofpact *ofpacts;      /* Sequence of "struct ofpacts". */
     unsigned int ofpacts_len;    /* Size of 'ofpacts', in bytes. */
 
@@ -265,7 +273,8 @@ rule_from_cls_rule(const struct cls_rule *cls_rule)
 }
 
 void ofproto_rule_update_used(struct rule *, long long int used);
-void ofproto_rule_expire(struct rule *, uint8_t reason);
+void ofproto_rule_expire(struct rule *rule, uint8_t reason)
+    OVS_RELEASES(rule->evict);
 void ofproto_rule_destroy(struct ofproto *, struct classifier *cls,
                           struct rule *) OVS_REQ_WRLOCK(cls->rwlock);
 
index 3cdc72c..c7fc09b 100644 (file)
@@ -152,10 +152,10 @@ static void oftable_enable_eviction(struct oftable *,
                                     const struct mf_subfield *fields,
                                     size_t n_fields);
 
-static void oftable_remove_rule(struct rule *);
+static void oftable_remove_rule(struct rule *rule) OVS_RELEASES(rule->evict);
 static void oftable_remove_rule__(struct ofproto *ofproto,
                                   struct classifier *cls, struct rule *rule)
-    OVS_REQ_WRLOCK(cls->rwlock);
+    OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->evict);
 static struct rule *oftable_replace_rule(struct rule *);
 static void oftable_substitute_rule(struct rule *old, struct rule *new);
 
@@ -181,7 +181,8 @@ struct eviction_group {
     struct heap rules;          /* Contains "struct rule"s. */
 };
 
-static struct rule *choose_rule_to_evict(struct oftable *);
+static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep)
+    OVS_TRY_WRLOCK(true, (*rulep)->evict);
 static void ofproto_evict(struct ofproto *);
 static uint32_t rule_eviction_priority(struct rule *);
 
@@ -202,8 +203,9 @@ static bool rule_is_modifiable(const struct rule *);
 static enum ofperr add_flow(struct ofproto *, struct ofconn *,
                             struct ofputil_flow_mod *,
                             const struct ofp_header *);
-static void delete_flow__(struct rule *, struct ofopgroup *,
-                          enum ofp_flow_removed_reason);
+static void delete_flow__(struct rule *rule, struct ofopgroup *,
+                          enum ofp_flow_removed_reason)
+    OVS_RELEASES(rule->evict);
 static bool handle_openflow(struct ofconn *, const struct ofpbuf *);
 static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
                                      struct ofputil_flow_mod *,
@@ -1077,6 +1079,7 @@ ofproto_flush__(struct ofproto *ofproto)
             if (!rule->pending) {
                 ofoperation_create(group, rule, OFOPERATION_DELETE,
                                    OFPRR_DELETE);
+                ovs_rwlock_wrlock(&rule->evict);
                 oftable_remove_rule__(ofproto, &table->cls, rule);
                 ofproto->ofproto_class->rule_destruct(rule);
             }
@@ -1678,6 +1681,7 @@ ofproto_delete_flow(struct ofproto *ofproto,
         /* Initiate deletion -> success. */
         struct ofopgroup *group = ofopgroup_create_unattached(ofproto);
         ofoperation_create(group, rule, OFOPERATION_DELETE, OFPRR_DELETE);
+        ovs_rwlock_wrlock(&rule->evict);
         oftable_remove_rule(rule);
         ofproto->ofproto_class->rule_destruct(rule);
         ofopgroup_submit(group);
@@ -2181,6 +2185,7 @@ ofproto_rule_destroy__(struct rule *rule)
         cls_rule_destroy(&rule->cr);
         free(rule->ofpacts);
         ovs_mutex_destroy(&rule->timeout_mutex);
+        ovs_rwlock_destroy(&rule->evict);
         rule->ofproto->ofproto_class->rule_dealloc(rule);
     }
 }
@@ -2198,7 +2203,11 @@ ofproto_rule_destroy(struct ofproto *ofproto, struct classifier *cls,
                      struct rule *rule) OVS_REQ_WRLOCK(cls->rwlock)
 {
     ovs_assert(!rule->pending);
-    oftable_remove_rule__(ofproto, cls, rule);
+    if (!ovs_rwlock_trywrlock(&rule->evict)) {
+        oftable_remove_rule__(ofproto, cls, rule);
+    } else {
+        NOT_REACHED();
+    }
     ofproto_rule_destroy__(rule);
 }
 
@@ -3423,12 +3432,12 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
     rule->ofpacts_len = fm->ofpacts_len;
     rule->meter_id = find_meter(rule->ofpacts, rule->ofpacts_len);
     list_init(&rule->meter_list_node);
-    rule->evictable = true;
     rule->eviction_group = NULL;
     list_init(&rule->expirable);
     rule->monitor_flags = 0;
     rule->add_seqno = 0;
     rule->modify_seqno = 0;
+    ovs_rwlock_init(&rule->evict);
 
     /* Insert new rule. */
     victim = oftable_replace_rule(rule);
@@ -3445,19 +3454,18 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
         n_rules = classifier_count(&table->cls);
         ovs_rwlock_unlock(&table->cls.rwlock);
         if (n_rules > table->max_flows) {
-            bool was_evictable;
-
-            was_evictable = rule->evictable;
-            rule->evictable = false;
-            evict = choose_rule_to_evict(table);
-            rule->evictable = was_evictable;
-
-            if (!evict) {
+            ovs_rwlock_rdlock(&rule->evict);
+            if (choose_rule_to_evict(table, &evict)) {
+                ovs_rwlock_unlock(&rule->evict);
+                ovs_rwlock_unlock(&evict->evict);
+                if (evict->pending) {
+                    error = OFPROTO_POSTPONE;
+                    goto exit;
+                }
+            } else {
+                ovs_rwlock_unlock(&rule->evict);
                 error = OFPERR_OFPFMFC_TABLE_FULL;
                 goto exit;
-            } else if (evict->pending) {
-                error = OFPROTO_POSTPONE;
-                goto exit;
             }
         } else {
             evict = NULL;
@@ -3472,6 +3480,13 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
             op->group->n_running--;
             ofoperation_destroy(rule->pending);
         } else if (evict) {
+            /* It would be better if we maintained the lock we took in
+             * choose_rule_to_evict() earlier, but that confuses the thread
+             * safety analysis, and this code is fragile enough that we really
+             * need it.  In the worst case, we'll have to block a little while
+             * before we perform the eviction, which doesn't seem like a big
+             * problem. */
+            ovs_rwlock_wrlock(&evict->evict);
             delete_flow__(evict, group, OFPRR_EVICTION);
         }
         ofopgroup_submit(group);
@@ -3642,6 +3657,7 @@ delete_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
 
     group = ofopgroup_create(ofproto, ofconn, request, UINT32_MAX);
     LIST_FOR_EACH_SAFE (rule, next, ofproto_node, rules) {
+        ovs_rwlock_wrlock(&rule->evict);
         delete_flow__(rule, group, reason);
     }
     ofopgroup_submit(group);
@@ -5065,17 +5081,18 @@ pick_fallback_dpid(void)
 \f
 /* Table overflow policy. */
 
-/* Chooses and returns a rule to evict from 'table'.  Returns NULL if the table
- * is not configured to evict rules or if the table contains no evictable
- * rules.  (Rules with 'evictable' set to false or with no timeouts are not
- * evictable.) */
-static struct rule *
-choose_rule_to_evict(struct oftable *table)
+/* Chooses and updates 'rulep' with a rule to evict from 'table'.  Sets 'rulep'
+ * to NULL if the table is not configured to evict rules or if the table
+ * contains no evictable rules.  (Rules with a readlock on their evict rwlock,
+ * or with no timeouts are not evictable.) */
+static bool
+choose_rule_to_evict(struct oftable *table, struct rule **rulep)
 {
     struct eviction_group *evg;
 
+    *rulep = NULL;
     if (!table->eviction_fields) {
-        return NULL;
+        return false;
     }
 
     /* In the common case, the outer and inner loops here will each be entered
@@ -5094,13 +5111,14 @@ choose_rule_to_evict(struct oftable *table)
         struct rule *rule;
 
         HEAP_FOR_EACH (rule, evg_node, &evg->rules) {
-            if (rule->evictable) {
-                return rule;
+            if (!ovs_rwlock_trywrlock(&rule->evict)) {
+                *rulep = rule;
+                return true;
             }
         }
     }
 
-    return NULL;
+    return false;
 }
 
 /* Searches 'ofproto' for tables that have more flows than their configured
@@ -5129,8 +5147,12 @@ ofproto_evict(struct ofproto *ofproto)
                 break;
             }
 
-            rule = choose_rule_to_evict(table);
-            if (!rule || rule->pending) {
+            if (!choose_rule_to_evict(table, &rule)) {
+                break;
+            }
+
+            if (rule->pending) {
+                ovs_rwlock_unlock(&rule->evict);
                 break;
             }
 
@@ -5518,6 +5540,7 @@ oftable_substitute_rule(struct rule *old, struct rule *new)
     if (new) {
         oftable_replace_rule(new);
     } else {
+        ovs_rwlock_wrlock(&old->evict);
         oftable_remove_rule(old);
     }
 }