[ovs-dev] [PATCH v4 07/12] Use classifier versioning.

Jarno Rajahalme jrajahalme at nicira.com
Wed Jun 10 00:24:14 UTC 2015


Each rule is now added or deleted in a specific tables version.  Flow
tables are versioned with a monotonically increasing 64-bit integer,
where positive values are valid version numbers.

Rule modifications are implemented as an insertion of a new rule and a
deletion of the old rule, both taking place in the same tables
version.  Since concurrent lookups may use different versions, both
the old and new rule must be available for lookups at the same time.

The ofproto provider interface is changed to accomodate the above.  As
rule's actions need not be modified any more, we no longer need
'rule_premodify_actions', nor 'rule_modify_actions'.  'rule_insert'
now takes a pointer to the old rule and adds a flag that tells whether
the old stats should be forwarded to the new rule or not (this
replaces the 'reset_counters' flag of the now removed
'rule_modify_actions').

Versioning all flow table changes has the side effect of making
learned flows visible for future lookups only.  I.e., the upcall that
executes the learn action, will not see the newly learned action in
it's classifier lookups.  Only upcalls that start executing after the
new flow was added will match on it.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 NEWS                       |   21 +-
 lib/classifier.c           |   14 +-
 lib/classifier.h           |    2 +
 ofproto/bundles.h          |    5 +-
 ofproto/ofproto-dpif.c     |   87 ++--
 ofproto/ofproto-provider.h |   59 +--
 ofproto/ofproto.c          |  950 ++++++++++++++++++++++++--------------------
 tests/ofproto.at           |   42 +-
 tests/ovs-ofctl.at         |   40 +-
 utilities/ovs-ofctl.8.in   |   19 +-
 utilities/ovs-ofctl.c      |    4 +-
 11 files changed, 675 insertions(+), 568 deletions(-)

diff --git a/NEWS b/NEWS
index 5bea237..f171cfc 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,10 @@
 Post-v2.3.0
 ---------------------
+   - Flow table modifications are now atomic, meaning that each packet
+     now sees a coherent version of the OpenFlow pipeline.  For
+     example, if a controller removes all flows, no packet sees an
+     intermediate version of the OpenFlow pipeline where only some of
+     the flows have been deleted.
    - Added support for SFQ, FQ_CoDel and CoDel qdiscs.
    - Add bash command-line completion support for ovs-vsctl Please check
      utilities/ovs-command-compgen.INSTALL.md for how to use.
@@ -28,10 +33,10 @@ Post-v2.3.0
      release.  See ovs-vswitchd(8) for details.
    - OpenFlow:
      * OpenFlow 1.4 bundles are now supported, but for flow mod
-       messages only. 'atomic' bundles are not yet supported, and
-       'ordered' bundles are trivially supported, as all bundled
-       messages are executed in the order they were added to the
-       bundle regardless of the presence of the 'ordered' flag.
+       messages only.  Both 'atomic' and 'ordered' bundle flags are
+       trivially supported, as all bundled messages are executed in
+       the order they were added and all flow table modifications are
+       now atomic to the datapath.
      * IPv6 flow label and neighbor discovery fields are now modifiable.
      * OpenFlow 1.5 extended registers are now supported.
      * The OpenFlow 1.5 actset_output field is now supported.
@@ -49,15 +54,15 @@ Post-v2.3.0
    - ovs-ofctl has a new '--bundle' option that makes the flow mod commands
      ('add-flow', 'add-flows', 'mod-flows', 'del-flows', and 'replace-flows')
      use an OpenFlow 1.4 bundle to operate the modifications as a single
-     transaction.  If any of the flow mods in a transaction fail, none of
-     them are executed.
+     atomic transaction.  If any of the flow mods in a transaction fail, none
+     of them are executed.  All flow mods in a bundle appear to datapath
+     lookups simultaneously.
    - ovs-ofctl 'add-flow' and 'add-flows' commands now accept arbitrary flow
      mods as an input by allowing the flow specification to start with an
      explicit 'add', 'modify', 'modify_strict', 'delete', or 'delete_strict'
      keyword.  A missing keyword is treated as 'add', so this is fully
      backwards compatible.  With the new '--bundle' option all the flow mods
-     are executed as a single transaction using the new OpenFlow 1.4 bundles
-     support.
+     are executed as a single atomic transaction using an OpenFlow 1.4 bundle.
    - ovs-pki: Changed message digest algorithm from MD5 to SHA-1 because
      MD5 is no longer secure and some operating systems have started to disable
      it in OpenSSL.
diff --git a/lib/classifier.c b/lib/classifier.c
index feebd80..be92151 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -202,14 +202,24 @@ cls_rule_init_from_minimatch(struct cls_rule *rule,
     minimatch_clone(CONST_CAST(struct minimatch *, &rule->match), match);
 }
 
+/* Initializes 'dst' as a copy of 'src', but with 'version'.
+ *
+ * The caller must eventually destroy 'dst' with cls_rule_destroy(). */
+void
+cls_rule_clone_in_version(struct cls_rule *dst, const struct cls_rule *src,
+                          long long version)
+{
+    cls_rule_init__(dst, src->priority, version);
+    minimatch_clone(CONST_CAST(struct minimatch *, &dst->match), &src->match);
+}
+
 /* Initializes 'dst' as a copy of 'src'.
  *
  * The caller must eventually destroy 'dst' with cls_rule_destroy(). */
 void
 cls_rule_clone(struct cls_rule *dst, const struct cls_rule *src)
 {
-    cls_rule_init__(dst, src->priority, src->version);
-    minimatch_clone(CONST_CAST(struct minimatch *, &dst->match), &src->match);
+    cls_rule_clone_in_version(dst, src, src->version);
 }
 
 /* Initializes 'dst' with the data in 'src', destroying 'src'.
diff --git a/lib/classifier.h b/lib/classifier.h
index cb0030a..ef57446 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -367,6 +367,8 @@ void cls_rule_init(struct cls_rule *, const struct match *, int priority,
 void cls_rule_init_from_minimatch(struct cls_rule *, const struct minimatch *,
                                   int priority, long long version);
 void cls_rule_clone(struct cls_rule *, const struct cls_rule *);
+void cls_rule_clone_in_version(struct cls_rule *, const struct cls_rule *,
+        long long version);
 void cls_rule_move(struct cls_rule *dst, struct cls_rule *src);
 void cls_rule_destroy(struct cls_rule *);
 
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index 778cba2..0c7daf2 100644
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -40,9 +40,8 @@ struct ofp_bundle_entry {
     };
 
     /* Used during commit. */
-    struct rule_collection rules;   /* Affected rules. */
-    struct rule *rule;
-    bool modify;
+    struct rule_collection old_rules;   /* Affected rules. */
+    struct rule_collection new_rules;   /* Affected rules. */
 
     /* OpenFlow header and some of the message contents for error reporting. */
     struct ofp_header ofp_msg[DIV_ROUND_UP(64, sizeof(struct ofp_header))];
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index cc5d9d4..7a903fa 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -89,6 +89,11 @@ struct rule_dpif {
     struct ovs_mutex stats_mutex;
     struct dpif_flow_stats stats OVS_GUARDED;
 
+   /* In non-NULL, will point to a new rule (for which a reference is held) to
+    * which all the stats updates should be forwarded. This exists only
+    * transitionally when flows are replaced. */
+    struct rule_dpif *new_rule OVS_GUARDED;
+
     /* If non-zero then the recirculation id that has
      * been allocated for use with this rule.
      * The recirculation id and associated internal flow should
@@ -3668,9 +3673,13 @@ rule_dpif_credit_stats(struct rule_dpif *rule,
                        const struct dpif_flow_stats *stats)
 {
     ovs_mutex_lock(&rule->stats_mutex);
-    rule->stats.n_packets += stats->n_packets;
-    rule->stats.n_bytes += stats->n_bytes;
-    rule->stats.used = MAX(rule->stats.used, stats->used);
+    if (OVS_UNLIKELY(rule->new_rule)) {
+        rule_dpif_credit_stats(rule->new_rule, stats);
+    } else {
+        rule->stats.n_packets += stats->n_packets;
+        rule->stats.n_bytes += stats->n_bytes;
+        rule->stats.used = MAX(rule->stats.used, stats->used);
+    }
     ovs_mutex_unlock(&rule->stats_mutex);
 }
 
@@ -3722,11 +3731,12 @@ rule_set_recirc_id(struct rule *rule_, uint32_t id)
 }
 
 long long
-ofproto_dpif_get_tables_version(struct ofproto_dpif *ofproto)
+ofproto_dpif_get_tables_version(struct ofproto_dpif *ofproto OVS_UNUSED)
 {
     long long version;
 
     atomic_read_relaxed(&ofproto->tables_version, &version);
+
     return version;
 }
 
@@ -3756,12 +3766,12 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, long long version,
     return rule;
 }
 
-/* Look up 'flow' in 'ofproto''s classifier starting from table '*table_id'.
- * Returns the rule that was found, which may be one of the special rules
- * according to packet miss hadling.  If 'may_packet_in' is false, returning of
- * the miss_rule (which issues packet ins for the controller) is avoided.
- * Updates 'wc', if nonnull, to reflect the fields that were used during the
- * lookup.
+/* Look up 'flow' in 'ofproto''s classifier version 'version', starting from
+ * table '*table_id'.  Returns the rule that was found, which may be one of the
+ * special rules according to packet miss hadling.  If 'may_packet_in' is
+ * false, returning of the miss_rule (which issues packet ins for the
+ * controller) is avoided.  Updates 'wc', if nonnull, to reflect the fields
+ * that were used during the lookup.
  *
  * If 'honor_table_miss' is true, the first lookup occurs in '*table_id', but
  * if none is found then the table miss configuration for that table is
@@ -3927,17 +3937,35 @@ rule_construct(struct rule *rule_)
     rule->stats.n_bytes = 0;
     rule->stats.used = rule->up.modified;
     rule->recirc_id = 0;
+    rule->new_rule = NULL;
 
     return 0;
 }
 
-static enum ofperr
-rule_insert(struct rule *rule_)
+static void
+rule_insert(struct rule *rule_, struct rule *old_rule_, bool forward_stats)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct rule_dpif *rule = rule_dpif_cast(rule_);
+
+    if (old_rule_ && forward_stats) {
+        struct rule_dpif *old_rule = rule_dpif_cast(old_rule_);
+
+        ovs_assert(!old_rule->new_rule);
+
+        /* Take a reference to the new rule, and refer all stats updates from
+         * the old rule to the new rule. */
+        rule_dpif_ref(rule);
+
+        ovs_mutex_lock(&old_rule->stats_mutex);
+        ovs_mutex_lock(&rule->stats_mutex);
+        old_rule->new_rule = rule;       /* Forward future stats. */
+        rule->stats = old_rule->stats;   /* Transfer stats to the new rule. */
+        ovs_mutex_unlock(&rule->stats_mutex);
+        ovs_mutex_unlock(&old_rule->stats_mutex);
+    }
+
     complete_operation(rule);
-    return 0;
 }
 
 static void
@@ -3950,10 +3978,15 @@ rule_delete(struct rule *rule_)
 
 static void
 rule_destruct(struct rule *rule_)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     struct rule_dpif *rule = rule_dpif_cast(rule_);
 
     ovs_mutex_destroy(&rule->stats_mutex);
+    /* Release reference to the new rule, if any. */
+    if (rule->new_rule) {
+        rule_dpif_unref(rule->new_rule);
+    }
     if (rule->recirc_id) {
         recirc_free_id(rule->recirc_id);
     }
@@ -3966,9 +3999,13 @@ rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes,
     struct rule_dpif *rule = rule_dpif_cast(rule_);
 
     ovs_mutex_lock(&rule->stats_mutex);
-    *packets = rule->stats.n_packets;
-    *bytes = rule->stats.n_bytes;
-    *used = rule->stats.used;
+    if (OVS_UNLIKELY(rule->new_rule)) {
+        rule_get_stats(&rule->new_rule->up, packets, bytes, used);
+    } else {
+        *packets = rule->stats.n_packets;
+        *bytes = rule->stats.n_bytes;
+        *used = rule->stats.used;
+    }
     ovs_mutex_unlock(&rule->stats_mutex);
 }
 
@@ -3990,22 +4027,6 @@ rule_execute(struct rule *rule, const struct flow *flow,
     return 0;
 }
 
-static void
-rule_modify_actions(struct rule *rule_, bool reset_counters)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct rule_dpif *rule = rule_dpif_cast(rule_);
-
-    if (reset_counters) {
-        ovs_mutex_lock(&rule->stats_mutex);
-        rule->stats.n_packets = 0;
-        rule->stats.n_bytes = 0;
-        ovs_mutex_unlock(&rule->stats_mutex);
-    }
-
-    complete_operation(rule);
-}
-
 static struct group_dpif *group_dpif_cast(const struct ofgroup *group)
 {
     return group ? CONTAINER_OF(group, struct group_dpif, up) : NULL;
@@ -5550,8 +5571,6 @@ const struct ofproto_class ofproto_dpif_class = {
     rule_dealloc,
     rule_get_stats,
     rule_execute,
-    NULL,                       /* rule_premodify_actions */
-    rule_modify_actions,
     set_frag_handling,
     packet_out,
     set_netflow,
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 46ffe7f..07229c5 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -323,6 +323,8 @@ struct rule {
     struct ofproto *const ofproto; /* The ofproto that contains this rule. */
     const struct cls_rule cr;      /* In owning ofproto's classifier. */
     const uint8_t table_id;        /* Index in ofproto's 'tables' array. */
+    bool removed;                  /* Rule has been removed from the ofproto
+                                    * data structures. */
 
     /* Protects members marked OVS_GUARDED.
      * Readers only need to hold this mutex.
@@ -361,7 +363,7 @@ struct rule {
 
     /* OpenFlow actions.  See struct rule_actions for more thread-safety
      * notes. */
-    OVSRCU_TYPE(const struct rule_actions *) actions;
+    const struct rule_actions * const actions;
 
     /* In owning meter's 'rules' list.  An empty list if there is no meter. */
     struct ovs_list meter_list_node OVS_GUARDED_BY(ofproto_mutex);
@@ -405,7 +407,7 @@ static inline bool rule_is_hidden(const struct rule *);
  * code that holds 'rule->mutex' (where 'rule' is the rule for which
  * 'rule->actions == actions') or during the RCU active period.
  *
- * All members are immutable: they do not change during the struct's
+ * All members are immutable: they do not change during the rule's
  * lifetime. */
 struct rule_actions {
     /* Flags.
@@ -1124,7 +1126,7 @@ struct ofproto_class {
      * OpenFlow error code), the ofproto base code will uninitialize and
      * deallocate 'rule'.  See "Rule Life Cycle" above for more details.
      *
-     * ->rule_construct() may also:
+     * ->rule_construct() must also:
      *
      *   - Validate that the datapath supports the matching rule in 'rule->cr'
      *     datapath.  For example, if the rule's table does not support
@@ -1133,8 +1135,9 @@ struct ofproto_class {
      *
      *   - Validate that the datapath can correctly implement 'rule->ofpacts'.
      *
-     * Some implementations might need to defer these tasks to ->rule_insert(),
-     * which is also acceptable.
+     * After a successful construction the rest of the rule life cycle calls
+     * may not fail, so ->rule_construct() must also make sure that the rule
+     * can be inserted in to the datapath.
      *
      *
      * Insertion
@@ -1143,11 +1146,10 @@ struct ofproto_class {
      * Following successful construction, the ofproto base case inserts 'rule'
      * into its flow table, then it calls ->rule_insert().  ->rule_insert()
      * must add the new rule to the datapath flow table and return only after
-     * this is complete (whether it succeeds or fails).
-     *
-     * If ->rule_insert() fails, the ofproto base code will remove 'rule' from
-     * the flow table, destruct, uninitialize, and deallocate 'rule'.  See
-     * "Rule Life Cycle" above for more details.
+     * this is complete.  The 'new_rule' may be a duplicate of an 'old_rule'.
+     * In this case the 'old_rule' is non-null, and the implementation should
+     * forward rule statistics from the 'old_rule' to the 'new_rule' if
+     * 'forward_stats' is 'true'.  This may not fail.
      *
      *
      * Deletion
@@ -1169,7 +1171,8 @@ struct ofproto_class {
     struct rule *(*rule_alloc)(void);
     enum ofperr (*rule_construct)(struct rule *rule)
         /* OVS_REQUIRES(ofproto_mutex) */;
-    enum ofperr (*rule_insert)(struct rule *rule)
+    void (*rule_insert)(struct rule *rule, struct rule *old_rule,
+                        bool forward_stats)
         /* OVS_REQUIRES(ofproto_mutex) */;
     void (*rule_delete)(struct rule *rule) /* OVS_REQUIRES(ofproto_mutex) */;
     void (*rule_destruct)(struct rule *rule);
@@ -1202,36 +1205,6 @@ struct ofproto_class {
     enum ofperr (*rule_execute)(struct rule *rule, const struct flow *flow,
                                 struct dp_packet *packet);
 
-    /* If the datapath can properly implement changing 'rule''s actions to the
-     * 'ofpacts_len' bytes in 'ofpacts', returns 0.  Otherwise, returns an enum
-     * ofperr indicating why the new actions wouldn't work.
-     *
-     * May be a null pointer if any set of actions is acceptable. */
-    enum ofperr (*rule_premodify_actions)(const struct rule *rule,
-                                          const struct ofpact *ofpacts,
-                                          size_t ofpacts_len)
-        /* OVS_REQUIRES(ofproto_mutex) */;
-
-    /* When ->rule_modify_actions() is called, the caller has already replaced
-     * the OpenFlow actions in 'rule' by a new set.  (If
-     * ->rule_premodify_actions is nonnull, then it was previously called to
-     * verify that the new set of actions is acceptable.)
-     *
-     * ->rule_modify_actions() must:
-     *
-     *   - Update the datapath flow table with the new actions.
-     *
-     *   - Only if 'reset_counters' is true, reset any packet or byte counters
-     *     associated with the rule to zero, so that rule_get_stats() will not
-     *     longer count those packets or bytes.
-     *
-     * Rule modification must not fail.
-     *
-     * ->rule_modify_actions() should not modify any base members of struct
-     * rule. */
-    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
      * meanings:
@@ -1785,7 +1758,7 @@ void ofproto_flush_flows(struct ofproto *);
 static inline const struct rule_actions *
 rule_get_actions(const struct rule *rule)
 {
-    return ovsrcu_get(const struct rule_actions *, &rule->actions);
+    return rule->actions;
 }
 
 /* Returns true if 'rule' is an OpenFlow 1.3 "table-miss" rule, false
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 716fbfa..35e1ed2 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -89,8 +89,6 @@ 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_REQUIRES(ofproto_mutex);
-
 /* A set of rules within a single OpenFlow table (oftable) that have the same
  * values for the oftable's eviction_fields.  A rule to be evicted, when one is
  * needed, is taken from the eviction group that contains the greatest number
@@ -254,15 +252,29 @@ struct flow_mod_requester {
 };
 
 /* OpenFlow. */
-static enum ofperr modify_flow_check__(struct ofproto *,
+static enum ofperr replace_rule_create(struct ofproto *,
                                        struct ofputil_flow_mod *,
-                                       const struct rule *)
+                                       struct cls_rule *cr, uint8_t table_id,
+                                       struct rule *old_rule,
+                                       struct rule **new_rule)
+    OVS_REQUIRES(ofproto_mutex);
+
+static void replace_rule_start(struct ofproto *,
+                               struct rule *old_rule,
+                               struct rule *new_rule,
+                               struct cls_conjunction *, size_t n_conjs)
     OVS_REQUIRES(ofproto_mutex);
-static void modify_flow__(struct ofproto *, struct ofputil_flow_mod *,
-                          const struct flow_mod_requester *, struct rule *,
-                          struct ovs_list *dead_cookies)
+
+static void replace_rule_revert(struct ofproto *,
+                                struct rule *old_rule, struct rule *new_rule)
+    OVS_REQUIRES(ofproto_mutex);
+
+static void replace_rule_finish(struct ofproto *, struct ofputil_flow_mod *,
+                                const struct flow_mod_requester *,
+                                struct rule *old_rule, struct rule *new_rule,
+                                struct ovs_list *dead_cookies)
     OVS_REQUIRES(ofproto_mutex);
-static void delete_flows__(const struct rule_collection *,
+static void delete_flows__(struct rule_collection *,
                            enum ofp_flow_removed_reason,
                            const struct flow_mod_requester *)
     OVS_REQUIRES(ofproto_mutex);
@@ -474,6 +486,14 @@ ofproto_enumerate_names(const char *type, struct sset *names)
     return class ? class->enumerate_names(type, names) : EAFNOSUPPORT;
 }
 
+static void
+ofproto_bump_tables_version(struct ofproto *ofproto)
+{
+    ++ofproto->tables_version;
+    ofproto->ofproto_class->set_tables_version(ofproto,
+                                               ofproto->tables_version);
+}
+
 int
 ofproto_create(const char *datapath_name, const char *datapath_type,
                struct ofproto **ofprotop)
@@ -579,8 +599,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
                               * sizeof(struct meter *));
 
     /* Set the initial tables version. */
-    ofproto->ofproto_class->set_tables_version(ofproto,
-                                               ofproto->tables_version);
+    ofproto_bump_tables_version(ofproto);
 
     *ofprotop = ofproto;
     return 0;
@@ -1447,9 +1466,19 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule)
      * switch is being deleted and any OpenFlow channels have been or soon will
      * be killed. */
     ovs_mutex_lock(&ofproto_mutex);
-    oftable_remove_rule(rule);
-    ofproto->ofproto_class->rule_delete(rule);
-    ofproto_rule_unref(rule);
+
+    if (!rule->removed) {
+        /* Make sure there is no postponed removal of the rule. */
+        ovs_assert(cls_rule_visible_in_version(&rule->cr, CLS_MAX_VERSION));
+
+        if (!classifier_remove(&rule->ofproto->tables[rule->table_id].cls,
+                               &rule->cr)) {
+            OVS_NOT_REACHED();
+        }
+        ofproto_rule_remove__(rule->ofproto, rule);
+        ofproto->ofproto_class->rule_delete(rule);
+        ofproto_rule_unref(rule);
+    }
     ovs_mutex_unlock(&ofproto_mutex);
 }
 
@@ -1485,7 +1514,6 @@ ofproto_flush__(struct ofproto *ofproto)
             rule_collection_add(&rules, rule);
         }
         delete_flows__(&rules, OFPRR_DELETE, NULL);
-        rule_collection_destroy(&rules);
     }
     /* XXX: Concurrent handler threads may insert new learned flows based on
      * learn actions of the now deleted flows right after we release
@@ -1537,6 +1565,16 @@ ofproto_destroy__(struct ofproto *ofproto)
     ofproto->ofproto_class->dealloc(ofproto);
 }
 
+/* Destroying rules is doubly deferred, must have 'ofproto' around for them.
+ * - 1st we defer the removal of the rules from the classifier
+ * - 2nd we defer the actual destruction of the rules. */
+static void
+ofproto_destroy_defer__(struct ofproto *ofproto)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    ovsrcu_postpone(ofproto_destroy__, ofproto);
+}
+
 void
 ofproto_destroy(struct ofproto *p)
     OVS_EXCLUDED(ofproto_mutex)
@@ -1573,7 +1611,7 @@ ofproto_destroy(struct ofproto *p)
     connmgr_destroy(p->connmgr);
 
     /* Destroying rules is deferred, must have 'ofproto' around for them. */
-    ovsrcu_postpone(ofproto_destroy__, p);
+    ovsrcu_postpone(ofproto_destroy_defer__, p);
 }
 
 /* Destroys the datapath with the respective 'name' and 'type'.  With the Linux
@@ -2135,9 +2173,9 @@ ofproto_delete_flow(struct ofproto *ofproto,
 
     /* First do a cheap check whether the rule we're looking for has already
      * been deleted.  If so, then we're done. */
-    rule = rule_from_cls_rule(classifier_find_match_exactly(cls, target,
-                                                            priority,
-                                                            CLS_MAX_VERSION));
+    rule = rule_from_cls_rule(classifier_find_match_exactly(
+                                  cls, target, priority,
+                                  CLS_MAX_VERSION));
     if (!rule) {
         return;
     }
@@ -2721,62 +2759,6 @@ ofproto_rule_destroy__(struct rule *rule)
     rule->ofproto->ofproto_class->rule_dealloc(rule);
 }
 
-/* Create a new rule based on attributes in 'fm', match in 'cr', and
- * 'table_id'.  Note that the rule is NOT inserted into a any data structures
- * yet.  Takes ownership of 'cr'. */
-static enum ofperr
-ofproto_rule_create(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                    struct cls_rule *cr, uint8_t table_id,
-                    struct rule **rulep)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct rule *rule;
-    enum ofperr error;
-
-    /* Allocate new rule. */
-    rule = ofproto->ofproto_class->rule_alloc();
-    if (!rule) {
-        cls_rule_destroy(cr);
-        VLOG_WARN_RL(&rl, "%s: failed to allocate a rule.", ofproto->name);
-        return OFPERR_OFPFMFC_UNKNOWN;
-    }
-
-    /* Initialize base state. */
-    *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
-    cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr);
-    ovs_refcount_init(&rule->ref_count);
-    rule->flow_cookie = fm->new_cookie;
-    rule->created = rule->modified = time_msec();
-
-    ovs_mutex_init(&rule->mutex);
-    ovs_mutex_lock(&rule->mutex);
-    rule->idle_timeout = fm->idle_timeout;
-    rule->hard_timeout = fm->hard_timeout;
-    rule->importance = fm->importance;
-    ovs_mutex_unlock(&rule->mutex);
-
-    *CONST_CAST(uint8_t *, &rule->table_id) = table_id;
-    rule->flags = fm->flags & OFPUTIL_FF_STATE;
-    ovsrcu_set_hidden(&rule->actions,
-                      rule_actions_create(fm->ofpacts, fm->ofpacts_len));
-    list_init(&rule->meter_list_node);
-    rule->eviction_group = NULL;
-    list_init(&rule->expirable);
-    rule->monitor_flags = 0;
-    rule->add_seqno = 0;
-    rule->modify_seqno = 0;
-
-    /* Construct rule, initializing derived state. */
-    error = ofproto->ofproto_class->rule_construct(rule);
-    if (error) {
-        ofproto_rule_destroy__(rule);
-        return error;
-    }
-
-    *rulep = rule;
-    return 0;
-}
-
 static void
 rule_destroy_cb(struct rule *rule)
 {
@@ -2815,6 +2797,70 @@ ofproto_rule_unref(struct rule *rule)
     }
 }
 
+static void
+remove_rule_rcu__(struct rule *rule)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct ofproto *ofproto = rule->ofproto;
+    struct oftable *table = &ofproto->tables[rule->table_id];
+
+    ovs_assert(!cls_rule_visible_in_version(&rule->cr, CLS_MAX_VERSION));
+    if (!classifier_remove(&table->cls, &rule->cr)) {
+        OVS_NOT_REACHED();
+    }
+    ofproto->ofproto_class->rule_delete(rule);
+    ofproto_rule_unref(rule);
+}
+
+static void
+remove_rule_rcu(struct rule *rule)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    ovs_mutex_lock(&ofproto_mutex);
+    remove_rule_rcu__(rule);
+    ovs_mutex_unlock(&ofproto_mutex);
+}
+
+/* Removes and deletes rules from a NULL-terminated array of rule pointers. */
+static void
+remove_rules_rcu(struct rule **rules)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    struct rule **orig_rules = rules;
+
+    if (*rules) {
+        struct ofproto *ofproto = rules[0]->ofproto;
+        unsigned long tables[BITMAP_N_LONGS(256)];
+        struct rule *rule;
+        size_t table_id;
+
+        memset(tables, 0, sizeof tables);
+
+        ovs_mutex_lock(&ofproto_mutex);
+        while ((rule = *rules++)) {
+            /* Defer once for each new table.  This defers the subtable cleanup
+             * until later, so that when removing large number of flows the
+             * operation is faster. */
+            if (!bitmap_is_set(tables, rule->table_id)) {
+                struct classifier *cls = &ofproto->tables[rule->table_id].cls;
+
+                bitmap_set1(tables, rule->table_id);
+                classifier_defer(cls);
+            }
+            remove_rule_rcu__(rule);
+        }
+
+        BITMAP_FOR_EACH_1(table_id, 256, tables) {
+            struct classifier *cls = &ofproto->tables[table_id].cls;
+
+            classifier_publish(cls);
+        }
+        ovs_mutex_unlock(&ofproto_mutex);
+    }
+
+    free(orig_rules);
+}
+
 void
 ofproto_group_ref(struct ofgroup *group)
 {
@@ -3046,13 +3092,13 @@ learned_cookies_flush(struct ofproto *ofproto, struct ovs_list *dead_cookies)
         struct match match;
 
         match_init_catchall(&match);
-        rule_criteria_init(&criteria, c->table_id, &match, 0, CLS_MAX_VERSION,
+        rule_criteria_init(&criteria, c->table_id, &match, 0,
+                           CLS_MAX_VERSION,
                            c->cookie, OVS_BE64_MAX, OFPP_ANY, OFPG_ANY);
         rule_criteria_require_rw(&criteria, false);
         collect_rules_loose(ofproto, &criteria, &rules);
-        delete_flows__(&rules, OFPRR_DELETE, NULL);
         rule_criteria_destroy(&criteria);
-        rule_collection_destroy(&rules);
+        delete_flows__(&rules, OFPRR_DELETE, NULL);
 
         free(c);
     }
@@ -3778,6 +3824,28 @@ rule_collection_unref(struct rule_collection *rules)
     }
 }
 
+/* Returns a NULL-terminated array of rule pointers,
+ * destroys 'rules'. */
+static struct rule **
+rule_collection_detach(struct rule_collection *rules)
+{
+    struct rule **rule_array;
+
+    rule_collection_add(rules, NULL);
+
+    if (rules->rules == rules->stub) {
+        size_t size = rules->n * sizeof *rules->rules;
+
+        rules->rules = xmalloc(size);
+        memcpy(rules->rules, rules->stub, size);
+    }
+
+    rule_array = rules->rules;
+    rule_collection_init(rules);
+
+    return rule_array;
+}
+
 void
 rule_collection_destroy(struct rule_collection *rules)
 {
@@ -3789,6 +3857,20 @@ rule_collection_destroy(struct rule_collection *rules)
     rule_collection_init(rules);
 }
 
+/* Schedules postponed removal of rules, destroys 'rules'. */
+static void
+rule_collection_remove_postponed(struct rule_collection *rules)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    if (rules->n > 0) {
+        if (rules->n == 1) {
+            ovsrcu_postpone(remove_rule_rcu, rules->rules[0]);
+        } else {
+            ovsrcu_postpone(remove_rules_rcu, rule_collection_detach(rules));
+        }
+    }
+}
+
 /* Checks whether 'rule' matches 'c' and, if so, adds it to 'rules'.  This
  * function verifies most of the criteria in 'c' itself, but the caller must
  * check 'c->cr' itself.
@@ -3959,7 +4041,8 @@ handle_flow_stats_request(struct ofconn *ofconn,
         return error;
     }
 
-    rule_criteria_init(&criteria, fsr.table_id, &fsr.match, 0, CLS_MAX_VERSION,
+    rule_criteria_init(&criteria, fsr.table_id, &fsr.match, 0,
+                       CLS_MAX_VERSION,
                        fsr.cookie, fsr.cookie_mask, fsr.out_port,
                        fsr.out_group);
 
@@ -4124,8 +4207,9 @@ handle_aggregate_stats_request(struct ofconn *ofconn,
     }
 
     rule_criteria_init(&criteria, request.table_id, &request.match, 0,
-                       CLS_MAX_VERSION, request.cookie, request.cookie_mask,
-                       request.out_port, request.out_group);
+                       CLS_MAX_VERSION, request.cookie,
+                       request.cookie_mask, request.out_port,
+                       request.out_group);
 
     ovs_mutex_lock(&ofproto_mutex);
     error = collect_rules_loose(ofproto, &criteria, &rules);
@@ -4301,7 +4385,6 @@ evict_rules_from_table(struct oftable *table, unsigned int extra_space)
         }
     }
     delete_flows__(&rules, OFPRR_EVICTION, NULL);
-    rule_collection_destroy(&rules);
 
     return error;
 }
@@ -4344,16 +4427,6 @@ get_conjunctions(const struct ofputil_flow_mod *fm,
     *n_conjsp = n_conjs;
 }
 
-static void
-set_conjunctions(struct rule *rule, const struct cls_conjunction *conjs,
-                 size_t n_conjs)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct cls_rule *cr = CONST_CAST(struct cls_rule *, &rule->cr);
-
-    cls_rule_set_conjunctions(cr, conjs, n_conjs);
-}
-
 /* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT
  * in which no matching flow already exists in the flow table.
  *
@@ -4367,14 +4440,16 @@ set_conjunctions(struct rule *rule, const struct cls_conjunction *conjs,
  * The caller retains ownership of 'fm->ofpacts'. */
 static enum ofperr
 add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-               struct rule **rulep, bool *modify)
+               struct rule **old_rule, struct rule **new_rule)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct oftable *table;
     struct cls_rule cr;
     struct rule *rule;
     uint8_t table_id;
-    enum ofperr error = 0;
+    struct cls_conjunction *conjs;
+    size_t n_conjs;
+    enum ofperr error;
 
     if (!check_table_id(ofproto, fm->table_id)) {
         error = OFPERR_OFPBRC_BAD_TABLE_ID;
@@ -4413,26 +4488,13 @@ add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
         return OFPERR_OFPBRC_EPERM;
     }
 
-    cls_rule_init(&cr, &fm->match, fm->priority, CLS_MIN_VERSION);
+    cls_rule_init(&cr, &fm->match, fm->priority, ofproto->tables_version + 1);
 
     /* Check for the existence of an identical rule.
      * This will not return rules earlier marked for removal. */
     rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls, &cr));
-    if (rule) {
-        /* Transform "add" into "modify" of an existing identical flow. */
-        cls_rule_destroy(&cr);
-
-        fm->modify_cookie = true;
-        error = modify_flow_check__(ofproto, fm, rule);
-        if (error) {
-            return error;
-        }
-
-        *modify = true;
-    } else {   /* New rule. */
-        struct cls_conjunction *conjs;
-        size_t n_conjs;
-
+    *old_rule = rule;
+    if (!rule) {
         /* Check for overlap, if requested. */
         if (fm->flags & OFPUTIL_FF_CHECK_OVERLAP
             && classifier_rule_overlaps(&table->cls, &cr)) {
@@ -4446,82 +4508,52 @@ add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
             cls_rule_destroy(&cr);
             return error;
         }
+    } else {
+        fm->modify_cookie = true;
+    }
 
-        /* Allocate new rule. */
-        error = ofproto_rule_create(ofproto, fm, &cr, table - ofproto->tables,
-                                    &rule);
-        if (error) {
-            return error;
-        }
-
-        /* Insert flow to the classifier, so that later flow_mods may relate
-         * to it.  This is reversible, in case later errors require this to
-         * be reverted. */
-        ofproto_rule_insert__(ofproto, rule);
-        /* Make the new rule invisible for classifier lookups. */
-        classifier_defer(&table->cls);
-        get_conjunctions(fm, &conjs, &n_conjs);
-        classifier_insert(&table->cls, &rule->cr, conjs, n_conjs);
-        free(conjs);
-
-        error = ofproto->ofproto_class->rule_insert(rule);
-        if (error) {
-            oftable_remove_rule(rule);
-            ofproto_rule_unref(rule);
-            return error;
-        }
-
-        *modify = false;
+    /* Allocate new rule. */
+    error = replace_rule_create(ofproto, fm, &cr, table - ofproto->tables,
+                                rule, new_rule);
+    if (error) {
+        return error;
     }
 
-    *rulep = rule;
+    get_conjunctions(fm, &conjs, &n_conjs);
+    replace_rule_start(ofproto, rule, *new_rule, conjs, n_conjs);
+    free(conjs);
+
     return 0;
 }
 
 /* Revert the effects of add_flow_start().
- * 'new_rule' must be passed in as NULL, if no new rule was allocated and
- * inserted to the classifier.
- * Note: evictions cannot be reverted. */
+ * XXX: evictions cannot be reverted. */
 static void
-add_flow_revert(struct ofproto *ofproto, struct rule *new_rule)
+add_flow_revert(struct ofproto *ofproto, struct rule *old_rule,
+                struct rule *new_rule)
     OVS_REQUIRES(ofproto_mutex)
 {
-    /* Old rule was not changed yet, only need to revert a new rule. */
-    if (new_rule) {
-        struct oftable *table = &ofproto->tables[new_rule->table_id];
-
-        if (!classifier_remove(&table->cls, &new_rule->cr)) {
-            OVS_NOT_REACHED();
-        }
-        classifier_publish(&table->cls);
-
-        ofproto_rule_remove__(ofproto, new_rule);
-        ofproto->ofproto_class->rule_delete(new_rule);
-        ofproto_rule_unref(new_rule);
-    }
+    replace_rule_revert(ofproto, old_rule, new_rule);
 }
 
+/* To be called after version bump. */
 static void
 add_flow_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
                 const struct flow_mod_requester *req,
-                struct rule *rule, bool modify)
+                struct rule *old_rule, struct rule *new_rule)
     OVS_REQUIRES(ofproto_mutex)
 {
-    if (modify) {
-        struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
-
-        modify_flow__(ofproto, fm, req, rule, &dead_cookies);
-        learned_cookies_flush(ofproto, &dead_cookies);
-    } else {
-        struct oftable *table = &ofproto->tables[rule->table_id];
-
-        classifier_publish(&table->cls);
+    struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
 
-        learned_cookies_inc(ofproto, rule_get_actions(rule));
+    replace_rule_finish(ofproto, fm, req, old_rule, new_rule, &dead_cookies);
+    learned_cookies_flush(ofproto, &dead_cookies);
 
-        if (minimask_get_vid_mask(&rule->cr.match.mask) == VLAN_VID_MASK) {
+    if (old_rule) {
+        ovsrcu_postpone(remove_rule_rcu, old_rule);
+    } else {
+        if (minimask_get_vid_mask(&new_rule->cr.match.mask) == VLAN_VID_MASK) {
             if (ofproto->vlan_bitmap) {
-                uint16_t vid = miniflow_get_vid(&rule->cr.match.flow);
+                uint16_t vid = miniflow_get_vid(&new_rule->cr.match.flow);
 
                 if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) {
                     bitmap_set1(ofproto->vlan_bitmap, vid);
@@ -4532,208 +4564,238 @@ add_flow_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
             }
         }
 
-        ofmonitor_report(ofproto->connmgr, rule, NXFME_ADDED, 0,
+        ofmonitor_report(ofproto->connmgr, new_rule, NXFME_ADDED, 0,
                          req ? req->ofconn : NULL,
                          req ? req->request->xid : 0, NULL);
     }
 
-    send_buffered_packet(req, fm->buffer_id, rule);
+    send_buffered_packet(req, fm->buffer_id, new_rule);
 }
 
 /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
 
-/* Checks if the 'rule' can be modified to match 'fm'.
- *
- * Returns 0 on success, otherwise an OpenFlow error code. */
-static enum ofperr
-modify_flow_check__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                    const struct rule *rule)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    if (ofproto->ofproto_class->rule_premodify_actions) {
-        return ofproto->ofproto_class->rule_premodify_actions(
-            rule, fm->ofpacts, fm->ofpacts_len);
-    }
-    return 0;
-}
-
-/* Checks if the rules listed in 'rules' can have their actions changed to
- * match those in 'fm'.
- *
- * Returns 0 on success, otherwise an OpenFlow error code. */
+/* Create a new rule based on attributes in 'fm', match in 'cr', 'table_id',
+ * and 'old_rule'.  Note that the rule is NOT inserted into a any data
+ * structures yet.  Takes ownership of 'cr'. */
 static enum ofperr
-modify_flows_check__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                     const struct rule_collection *rules)
-    OVS_REQUIRES(ofproto_mutex)
+replace_rule_create(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
+                    struct cls_rule *cr, uint8_t table_id,
+                    struct rule *old_rule, struct rule **new_rule)
 {
+    struct rule *rule;
     enum ofperr error;
-    size_t i;
 
-    if (ofproto->ofproto_class->rule_premodify_actions) {
-        for (i = 0; i < rules->n; i++) {
-            error = modify_flow_check__(ofproto, fm, rules->rules[i]);
-            if (error) {
-                return error;
-            }
-        }
+    /* Allocate new rule. */
+    rule = ofproto->ofproto_class->rule_alloc();
+    if (!rule) {
+        cls_rule_destroy(cr);
+        VLOG_WARN_RL(&rl, "%s: failed to allocate a rule.", ofproto->name);
+        return OFPERR_OFPFMFC_UNKNOWN;
     }
 
-    return 0;
-}
-
-/* Modifies the 'rule', changing it to match 'fm'. */
-static void
-modify_flow__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-              const struct flow_mod_requester *req, struct rule *rule,
-              struct ovs_list *dead_cookies)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    enum nx_flow_update_event event = fm->command == OFPFC_ADD
-        ? NXFME_ADDED : NXFME_MODIFIED;
-
-    /*  'fm' says that  */
-    bool change_cookie = (fm->modify_cookie
-                          && fm->new_cookie != OVS_BE64_MAX
-                          && fm->new_cookie != rule->flow_cookie);
-
-    const struct rule_actions *actions = rule_get_actions(rule);
-    bool change_actions = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
-                                         actions->ofpacts,
-                                         actions->ofpacts_len);
-
-    bool reset_counters = (fm->flags & OFPUTIL_FF_RESET_COUNTS) != 0;
+    /* Initialize base state. */
+    *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
+    cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr);
+    ovs_refcount_init(&rule->ref_count);
+    rule->flow_cookie = fm->new_cookie;
+    rule->created = rule->modified = time_msec();
 
-    long long int now = time_msec();
+    ovs_mutex_init(&rule->mutex);
+    ovs_mutex_lock(&rule->mutex);
+    rule->idle_timeout = fm->idle_timeout;
+    rule->hard_timeout = fm->hard_timeout;
+    rule->importance = fm->importance;
 
-    if (change_cookie) {
-        cookies_remove(ofproto, rule);
-    }
+    *CONST_CAST(uint8_t *, &rule->table_id) = table_id;
+    rule->flags = fm->flags & OFPUTIL_FF_STATE;
+    *CONST_CAST(const struct rule_actions **, &rule->actions)
+        = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
+    list_init(&rule->meter_list_node);
+    rule->eviction_group = NULL;
+    list_init(&rule->expirable);
+    rule->monitor_flags = 0;
+    rule->add_seqno = 0;
+    rule->modify_seqno = 0;
 
-    ovs_mutex_lock(&rule->mutex);
-    if (fm->command == OFPFC_ADD) {
-        rule->idle_timeout = fm->idle_timeout;
-        rule->hard_timeout = fm->hard_timeout;
-        rule->importance = fm->importance;
-        rule->flags = fm->flags & OFPUTIL_FF_STATE;
-        rule->created = now;
-    }
-    if (change_cookie) {
-        rule->flow_cookie = fm->new_cookie;
+    /* Copy values from old rule for modify semantics. */
+    if (old_rule) {
+        /*  'fm' says that  */
+        bool change_cookie = (fm->modify_cookie
+                              && fm->new_cookie != OVS_BE64_MAX
+                              && fm->new_cookie != old_rule->flow_cookie);
+
+        ovs_mutex_lock(&old_rule->mutex);
+        if (fm->command != OFPFC_ADD) {
+            rule->idle_timeout = old_rule->idle_timeout;
+            rule->hard_timeout = old_rule->hard_timeout;
+            rule->importance = old_rule->importance;
+            rule->flags = old_rule->flags;
+            rule->created = old_rule->created;
+        }
+        if (!change_cookie) {
+            rule->flow_cookie = old_rule->flow_cookie;
+        }
+        ovs_mutex_unlock(&old_rule->mutex);
     }
-    rule->modified = now;
     ovs_mutex_unlock(&rule->mutex);
 
-    if (change_cookie) {
-        cookies_insert(ofproto, rule);
-    }
-    if (fm->command == OFPFC_ADD) {
-        if (fm->idle_timeout || fm->hard_timeout || fm->importance) {
-            if (!rule->eviction_group) {
-                eviction_group_add_rule(rule);
-            }
-        } else {
-            eviction_group_remove_rule(rule);
-        }
+    /* Construct rule, initializing derived state. */
+    error = ofproto->ofproto_class->rule_construct(rule);
+    if (error) {
+        ofproto_rule_destroy__(rule);
+        return error;
     }
 
-    if (change_actions) {
-        /* We have to change the actions.  The rule's conjunctive match set
-         * is a function of its actions, so we need to update that too.  The
-         * conjunctive match set is used in the lookup process to figure
-         * which (if any) collection of conjunctive sets the packet matches
-         * with.  However, a rule with conjunction actions is never to be
-         * returned as a classifier lookup result.  To make sure a rule with
-         * conjunction actions is not returned as a lookup result, we update
-         * them in a carefully chosen order:
-         *
-         * - If we're adding a conjunctive match set where there wasn't one
-         *   before, we have to make the conjunctive match set available to
-         *   lookups before the rule's actions are changed, as otherwise
-         *   rule with a conjunction action could be returned as a lookup
-         *   result.
-         *
-         * - To clear some nonempty conjunctive set, we set the rule's
-         *   actions first, so that a lookup can't return a rule with
-         *   conjunction actions.
-         *
-         * - Otherwise, order doesn't matter for changing one nonempty
-         *   conjunctive match set to some other nonempty set, since the
-         *   rule's actions are not seen by the classifier, and hence don't
-         *   matter either before or after the change. */
-        struct cls_conjunction *conjs;
-        size_t n_conjs;
-        get_conjunctions(fm, &conjs, &n_conjs);
+    rule->removed = true;   /* Not yet in ofproto data structures. */
 
-        if (n_conjs) {
-            set_conjunctions(rule, conjs, n_conjs);
-        }
-        ovsrcu_set(&rule->actions, rule_actions_create(fm->ofpacts,
-                                                           fm->ofpacts_len));
-        if (!conjs) {
-            set_conjunctions(rule, conjs, n_conjs);
-        }
+    *new_rule = rule;
+    return 0;
+}
 
-        free(conjs);
-    }
+static void
+replace_rule_start(struct ofproto *ofproto,
+                   struct rule *old_rule, struct rule *new_rule,
+                   struct cls_conjunction *conjs, size_t n_conjs)
+{
+    struct oftable *table = &ofproto->tables[new_rule->table_id];
 
-    if (change_actions || reset_counters) {
-        ofproto->ofproto_class->rule_modify_actions(rule, reset_counters);
+    if (old_rule) {
+        /* Mark the old rule for removal in the next version. */
+        cls_rule_make_invisible_in_version(&old_rule->cr,
+                                           ofproto->tables_version + 1,
+                                           ofproto->tables_version);
     }
+    /* Insert flow to the classifier, so that later flow_mods may relate
+     * to it.  This is reversible, in case later errors require this to
+     * be reverted. */
+    ofproto_rule_insert__(ofproto, new_rule);
+    /* Make the new rule visible for classifier lookups only from the next
+     * version. */
+    classifier_insert(&table->cls, &new_rule->cr, conjs, n_conjs);
+}
 
-    if (event != NXFME_MODIFIED || change_actions || change_cookie) {
-        ofmonitor_report(ofproto->connmgr, rule, event, 0,
-                         req ? req->ofconn : NULL, req ? req->request->xid : 0,
-                         change_actions ? actions : NULL);
+static void replace_rule_revert(struct ofproto *ofproto,
+                                struct rule *old_rule, struct rule *new_rule)
+{
+    struct oftable *table = &ofproto->tables[new_rule->table_id];
+
+    /* Restore the original visibility of the old rule. */
+    if (old_rule) {
+        cls_rule_restore_visibility(&old_rule->cr);
     }
 
-    if (change_actions) {
-        learned_cookies_inc(ofproto, rule_get_actions(rule));
-        learned_cookies_dec(ofproto, actions, dead_cookies);
-        rule_actions_destroy(actions);
+    /* Remove the new rule immediately.  It was never visible to lookups. */
+    if (!classifier_remove(&table->cls, &new_rule->cr)) {
+        OVS_NOT_REACHED();
     }
+    ofproto_rule_remove__(ofproto, new_rule);
+    /* The rule was not inserted to the ofproto provider, so we can
+     * release it without deleting it from the ofproto provider. */
+    ofproto_rule_unref(new_rule);
 }
 
-/* Modifies the rules listed in 'rules', changing their actions to match those
- * in 'fm'.
- *
- * 'req' is used to retrieve the packet buffer specified in fm->buffer_id,
- * if any. */
+/* Adds the 'new_rule', replacing the 'old_rule'. */
 static void
-modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-               const struct flow_mod_requester *req,
-               const struct rule_collection *rules)
+replace_rule_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
+                    const struct flow_mod_requester *req,
+                    struct rule *old_rule, struct rule *new_rule,
+                    struct ovs_list *dead_cookies)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
-    size_t i;
+    bool forward_stats = !(fm->flags & OFPUTIL_FF_RESET_COUNTS);
 
-    for (i = 0; i < rules->n; i++) {
-        modify_flow__(ofproto, fm, req, rules->rules[i], &dead_cookies);
+    /* Insert the new flow to the ofproto provider.  A non-NULL 'old_rule' is a
+     * duplicate rule the 'new_rule' is replacing.  The provider should link
+     * the stats from the old rule to the new one if 'forward_stats' is
+     * 'true'.  The 'old_rule' will be deleted right after this call. */
+    ofproto->ofproto_class->rule_insert(new_rule, old_rule, forward_stats);
+    learned_cookies_inc(ofproto, rule_get_actions(new_rule));
+
+    if (old_rule) {
+        const struct rule_actions *old_actions = rule_get_actions(old_rule);
+
+        enum nx_flow_update_event event = fm->command == OFPFC_ADD
+            ? NXFME_ADDED : NXFME_MODIFIED;
+
+        /*  'fm' says that  */
+        bool change_cookie = (fm->modify_cookie
+                              && fm->new_cookie != OVS_BE64_MAX
+                              && fm->new_cookie != old_rule->flow_cookie);
+
+        bool change_actions = !ofpacts_equal(fm->ofpacts,
+                                             fm->ofpacts_len,
+                                             old_actions->ofpacts,
+                                             old_actions->ofpacts_len);
+
+        /* Remove the old rule from data structures.  Removal from the
+         * classifier and the deletion of the rule is RCU postponed by the
+         * caller. */
+        ofproto_rule_remove__(ofproto, old_rule);
+        learned_cookies_dec(ofproto, old_actions, dead_cookies);
+
+        if (event != NXFME_MODIFIED || change_actions || change_cookie) {
+            ofmonitor_report(ofproto->connmgr, new_rule, event, 0,
+                             req ? req->ofconn : NULL,
+                             req ? req->request->xid : 0,
+                             change_actions ? old_actions : NULL);
+        }
     }
-    learned_cookies_flush(ofproto, &dead_cookies);
 }
 
 static enum ofperr
 modify_flows_start__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                     struct rule_collection *rules)
+                     struct rule_collection *old_rules,
+                     struct rule_collection *new_rules)
     OVS_REQUIRES(ofproto_mutex)
 {
     enum ofperr error;
 
-    if (rules->n > 0) {
-        error = modify_flows_check__(ofproto, fm, rules);
+    rule_collection_init(new_rules);
+
+    if (old_rules->n > 0) {
+        struct cls_conjunction *conjs;
+        size_t n_conjs;
+        size_t i;
+
+        /* Create a new 'modified' rule for each old rule. */
+        for (i = 0; i < old_rules->n; i++) {
+            struct rule *old_rule = old_rules->rules[i];
+            struct rule *new_rule;
+            struct cls_rule cr;
+
+            cls_rule_clone_in_version(&cr, &old_rule->cr,
+                                      ofproto->tables_version + 1);
+            error = replace_rule_create(ofproto, fm, &cr, old_rule->table_id,
+                                        old_rule, &new_rule);
+            if (!error) {
+                rule_collection_add(new_rules, new_rule);
+            } else {
+                rule_collection_unref(new_rules);
+                rule_collection_destroy(new_rules);
+                return error;
+            }
+        }
+        ovs_assert(new_rules->n == old_rules->n);
+
+        get_conjunctions(fm, &conjs, &n_conjs);
+        for (i = 0; i < old_rules->n; i++) {
+            replace_rule_start(ofproto, old_rules->rules[i],
+                               new_rules->rules[i], conjs, n_conjs);
+        }
+        free(conjs);
     } else if (!(fm->cookie_mask != htonll(0)
                  || fm->new_cookie == OVS_BE64_MAX)) {
-        bool modify;
-
-        error = add_flow_start(ofproto, fm, &rules->rules[0], &modify);
+        /* No match, add a new flow. */
+        error = add_flow_start(ofproto, fm, &old_rules->rules[0],
+                               &new_rules->rules[0]);
         if (!error) {
-            ovs_assert(!modify);
+            ovs_assert(!old_rules->rules[0]);
         }
+        new_rules->n = 1;
     } else {
-        rules->rules[0] = NULL;
         error = 0;
     }
+
     return error;
 }
 
@@ -4744,132 +4806,162 @@ modify_flows_start__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
  * if any. */
 static enum ofperr
 modify_flows_start_loose(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                         struct rule_collection *rules)
+                         struct rule_collection *old_rules,
+                         struct rule_collection *new_rules)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct rule_criteria criteria;
     enum ofperr error;
 
-    rule_criteria_init(&criteria, fm->table_id, &fm->match, 0, CLS_MAX_VERSION,
-                       fm->cookie, fm->cookie_mask, OFPP_ANY, OFPG11_ANY);
+    rule_criteria_init(&criteria, fm->table_id, &fm->match, 0,
+                       CLS_MAX_VERSION, fm->cookie, fm->cookie_mask,
+                       OFPP_ANY, OFPG11_ANY);
     rule_criteria_require_rw(&criteria,
                              (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
-    error = collect_rules_loose(ofproto, &criteria, rules);
+    error = collect_rules_loose(ofproto, &criteria, old_rules);
     rule_criteria_destroy(&criteria);
 
     if (!error) {
-        error = modify_flows_start__(ofproto, fm, rules);
+        error = modify_flows_start__(ofproto, fm, old_rules, new_rules);
     }
 
     if (error) {
-        rule_collection_destroy(rules);
+        rule_collection_destroy(old_rules);
     }
     return error;
 }
 
 static void
-modify_flows_revert(struct ofproto *ofproto, struct rule_collection *rules)
+modify_flows_revert(struct ofproto *ofproto, struct rule_collection *old_rules,
+                    struct rule_collection *new_rules)
     OVS_REQUIRES(ofproto_mutex)
 {
-    /* Old rules were not changed yet, only need to revert a new rule. */
-    if (rules->n == 0 && rules->rules[0] != NULL) {
-        add_flow_revert(ofproto, rules->rules[0]);
+    /* Old rules were not changed yet, only need to revert new rules. */
+    if (old_rules->n == 0 && new_rules->n == 1) {
+        add_flow_revert(ofproto, new_rules->rules[0], NULL);
+    } else if (old_rules->n > 0) {
+        for (size_t i = 0; i < old_rules->n; i++) {
+            replace_rule_revert(ofproto, old_rules->rules[i],
+                                new_rules->rules[i]);
+        }
+        rule_collection_destroy(new_rules);
+        rule_collection_destroy(old_rules);
     }
-    rule_collection_destroy(rules);
 }
 
 static void
 modify_flows_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
                     const struct flow_mod_requester *req,
-                    struct rule_collection *rules)
+                    struct rule_collection *old_rules,
+                    struct rule_collection *new_rules)
     OVS_REQUIRES(ofproto_mutex)
 {
-    if (rules->n > 0) {
-        modify_flows__(ofproto, fm, req, rules);
-        send_buffered_packet(req, fm->buffer_id, rules->rules[0]);
-    } else if (rules->rules[0] != NULL) {
-        add_flow_finish(ofproto, fm, req, rules->rules[0], false);
+    if (old_rules->n == 0 && new_rules->n == 1) {
+        add_flow_finish(ofproto, fm, req, old_rules->rules[0],
+                        new_rules->rules[0]);
+    } else if (old_rules->n > 0) {
+        struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
+
+        ovs_assert(new_rules->n == old_rules->n);
+
+        for (size_t i = 0; i < old_rules->n; i++) {
+            replace_rule_finish(ofproto, fm, req, old_rules->rules[i],
+                                new_rules->rules[i], &dead_cookies);
+        }
+        learned_cookies_flush(ofproto, &dead_cookies);
+        rule_collection_remove_postponed(old_rules);
+
+        send_buffered_packet(req, fm->buffer_id, new_rules->rules[0]);
+        rule_collection_destroy(new_rules);
     }
-    rule_collection_destroy(rules);
 }
 
 /* Implements OFPFC_MODIFY_STRICT.  Returns 0 on success or an OpenFlow error
  * code on failure. */
 static enum ofperr
 modify_flow_start_strict(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                         struct rule_collection *rules)
+                         struct rule_collection *old_rules,
+                         struct rule_collection *new_rules)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct rule_criteria criteria;
     enum ofperr error;
 
     rule_criteria_init(&criteria, fm->table_id, &fm->match, fm->priority,
-                       CLS_MAX_VERSION, fm->cookie, fm->cookie_mask, OFPP_ANY,
-                       OFPG11_ANY);
+                       CLS_MAX_VERSION, fm->cookie, fm->cookie_mask,
+                       OFPP_ANY, OFPG11_ANY);
     rule_criteria_require_rw(&criteria,
                              (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
-    error = collect_rules_strict(ofproto, &criteria, rules);
+    error = collect_rules_strict(ofproto, &criteria, old_rules);
     rule_criteria_destroy(&criteria);
 
     if (!error) {
         /* collect_rules_strict() can return max 1 rule. */
-        error = modify_flows_start__(ofproto, fm, rules);
+        error = modify_flows_start__(ofproto, fm, old_rules, new_rules);
     }
 
     if (error) {
-        rule_collection_destroy(rules);
+        rule_collection_destroy(old_rules);
     }
     return error;
 }
 
 /* OFPFC_DELETE implementation. */
 
-/* Deletes the rules listed in 'rules'. */
 static void
-delete_flows__(const struct rule_collection *rules,
-               enum ofp_flow_removed_reason reason,
-               const struct flow_mod_requester *req)
+delete_flows_start__(struct ofproto *ofproto,
+                     const struct rule_collection *rules)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    for (size_t i = 0; i < rules->n; i++) {
+        cls_rule_make_invisible_in_version(&rules->rules[i]->cr,
+                                           ofproto->tables_version + 1,
+                                           ofproto->tables_version);
+    }
+}
+
+static void
+delete_flows_finish__(struct ofproto *ofproto,
+                      struct rule_collection *rules,
+                      enum ofp_flow_removed_reason reason,
+                      const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
     if (rules->n) {
         struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
-        struct ofproto *ofproto = rules->rules[0]->ofproto;
-        struct rule *rule, *next;
-        uint8_t prev_table = UINT8_MAX;
-        size_t i;
 
-        for (i = 0, next = rules->rules[0];
-             rule = next, next = (++i < rules->n) ? rules->rules[i] : NULL,
-                 rule; prev_table = rule->table_id) {
-            struct classifier *cls = &ofproto->tables[rule->table_id].cls;
-            uint8_t next_table = next ? next->table_id : UINT8_MAX;
+        for (size_t i = 0; i < rules->n; i++) {
+            struct rule *rule = rules->rules[i];
 
             ofproto_rule_send_removed(rule, reason);
-
             ofmonitor_report(ofproto->connmgr, rule, NXFME_DELETED, reason,
                              req ? req->ofconn : NULL,
                              req ? req->request->xid : 0, NULL);
-
-            /* Defer once for each new table. */
-            if (rule->table_id != prev_table) {
-                classifier_defer(cls);
-            }
-            if (!classifier_remove(cls, &rule->cr)) {
-                OVS_NOT_REACHED();
-            }
-            if (next_table != rule->table_id) {
-                classifier_publish(cls);
-            }
             ofproto_rule_remove__(ofproto, rule);
-
-            ofproto->ofproto_class->rule_delete(rule);
-
             learned_cookies_dec(ofproto, rule_get_actions(rule),
                                 &dead_cookies);
-
-            ofproto_rule_unref(rule);
         }
+        rule_collection_remove_postponed(rules);
+
         learned_cookies_flush(ofproto, &dead_cookies);
+    }
+}
+
+/* Deletes the rules listed in 'rules'.
+ * The deleted rules will become invisible to the lookups in the next version.
+ * Destroys 'rules'. */
+static void
+delete_flows__(struct rule_collection *rules,
+               enum ofp_flow_removed_reason reason,
+               const struct flow_mod_requester *req)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    if (rules->n) {
+        struct ofproto *ofproto = rules->rules[0]->ofproto;
+
+        delete_flows_start__(ofproto, rules);
+        ofproto_bump_tables_version(ofproto);
+        delete_flows_finish__(ofproto, rules, reason, req);
         ofmonitor_flush(ofproto->connmgr);
     }
 }
@@ -4893,37 +4985,34 @@ delete_flows_start_loose(struct ofproto *ofproto,
     rule_criteria_destroy(&criteria);
 
     if (!error) {
-        for (size_t i = 0; i < rules->n; i++) {
-            struct rule *rule = rules->rules[i];
-
-            cls_rule_make_invisible_in_version(CONST_CAST(struct cls_rule *,
-                                                          &rule->cr),
-                                               CLS_MIN_VERSION,
-                                               CLS_MIN_VERSION);
-        }
+        delete_flows_start__(ofproto, rules);
     }
 
     return error;
 }
 
 static void
-delete_flows_revert(struct rule_collection *rules)
+delete_flows_revert(struct ofproto *ofproto OVS_UNUSED,
+                    struct rule_collection *rules)
     OVS_REQUIRES(ofproto_mutex)
 {
     for (size_t i = 0; i < rules->n; i++) {
-        cls_rule_restore_visibility(&rules->rules[i]->cr);
+        struct rule *rule = rules->rules[i];
+
+        /* Restore the original visibility of the rule. */
+        cls_rule_restore_visibility(&rule->cr);
     }
     rule_collection_destroy(rules);
 }
 
 static void
-delete_flows_finish(const struct ofputil_flow_mod *fm,
+delete_flows_finish(struct ofproto *ofproto,
+                    const struct ofputil_flow_mod *fm,
                     const struct flow_mod_requester *req,
                     struct rule_collection *rules)
     OVS_REQUIRES(ofproto_mutex)
 {
-    delete_flows__(rules, fm->delete_reason, req);
-    rule_collection_destroy(rules);
+    delete_flows_finish__(ofproto, rules, fm->delete_reason, req);
 }
 
 /* Implements OFPFC_DELETE_STRICT. */
@@ -4945,14 +5034,7 @@ delete_flow_start_strict(struct ofproto *ofproto,
     rule_criteria_destroy(&criteria);
 
     if (!error) {
-        for (size_t i = 0; i < rules->n; i++) {
-            struct rule *rule = rules->rules[i];
-
-            cls_rule_make_invisible_in_version(CONST_CAST(struct cls_rule *,
-                                                          &rule->cr),
-                                               CLS_MIN_VERSION,
-                                               CLS_MIN_VERSION);
-        }
+        delete_flows_start__(ofproto, rules);
     }
 
     return error;
@@ -5094,6 +5176,7 @@ handle_flow_mod__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     ovs_mutex_lock(&ofproto_mutex);
     error = do_bundle_flow_mod_start(ofproto, fm, &be);
     if (!error) {
+        ofproto_bump_tables_version(ofproto);
         do_bundle_flow_mod_finish(ofproto, fm, req, &be);
     }
     ofmonitor_flush(ofproto->connmgr);
@@ -5665,7 +5748,6 @@ handle_delete_meter(struct ofconn *ofconn, struct ofputil_meter_mod *mm)
     meter_delete(ofproto, first, last);
 
     ovs_mutex_unlock(&ofproto_mutex);
-    rule_collection_destroy(&rules);
 
     return error;
 }
@@ -6464,19 +6546,19 @@ do_bundle_flow_mod_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 {
     switch (fm->command) {
     case OFPFC_ADD:
-        return add_flow_start(ofproto, fm, &be->rule, &be->modify);
-
+        return add_flow_start(ofproto, fm, &be->old_rules.stub[0],
+                              &be->new_rules.stub[0]);
     case OFPFC_MODIFY:
-        return modify_flows_start_loose(ofproto, fm, &be->rules);
-
+        return modify_flows_start_loose(ofproto, fm, &be->old_rules,
+                                        &be->new_rules);
     case OFPFC_MODIFY_STRICT:
-        return modify_flow_start_strict(ofproto, fm, &be->rules);
-
+        return modify_flow_start_strict(ofproto, fm, &be->old_rules,
+                                        &be->new_rules);
     case OFPFC_DELETE:
-        return delete_flows_start_loose(ofproto, fm, &be->rules);
+        return delete_flows_start_loose(ofproto, fm, &be->old_rules);
 
     case OFPFC_DELETE_STRICT:
-        return delete_flow_start_strict(ofproto, fm, &be->rules);
+        return delete_flow_start_strict(ofproto, fm, &be->old_rules);
     }
 
     return OFPERR_OFPFMFC_BAD_COMMAND;
@@ -6489,17 +6571,17 @@ do_bundle_flow_mod_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 {
     switch (fm->command) {
     case OFPFC_ADD:
-        add_flow_revert(ofproto, be->modify ? NULL : be->rule);
+        add_flow_revert(ofproto, be->old_rules.stub[0], be->new_rules.stub[0]);
         break;
 
     case OFPFC_MODIFY:
     case OFPFC_MODIFY_STRICT:
-        modify_flows_revert(ofproto, &be->rules);
+        modify_flows_revert(ofproto, &be->old_rules, &be->new_rules);
         break;
 
     case OFPFC_DELETE:
     case OFPFC_DELETE_STRICT:
-        delete_flows_revert(&be->rules);
+        delete_flows_revert(ofproto, &be->old_rules);
         break;
 
     default:
@@ -6515,17 +6597,18 @@ do_bundle_flow_mod_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 {
     switch (fm->command) {
     case OFPFC_ADD:
-        add_flow_finish(ofproto, fm, req, be->rule, be->modify);
+        add_flow_finish(ofproto, fm, req, be->old_rules.stub[0],
+                        be->new_rules.stub[0]);
         break;
 
     case OFPFC_MODIFY:
     case OFPFC_MODIFY_STRICT:
-        modify_flows_finish(ofproto, fm, req, &be->rules);
+        modify_flows_finish(ofproto, fm, req, &be->old_rules, &be->new_rules);
         break;
 
     case OFPFC_DELETE:
     case OFPFC_DELETE_STRICT:
-        delete_flows_finish(fm, req, &be->rules);
+        delete_flows_finish(ofproto, fm, req, &be->old_rules);
         break;
 
     default:
@@ -6533,6 +6616,25 @@ do_bundle_flow_mod_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     }
 }
 
+/* Commit phases (all while locking ofproto_mutex):
+ *
+ * 1. Begin: Gather resources and make changes visible in the next version.
+ *           - Mark affected rules for removal in the next version.
+ *           - Create new replacement rules, make visible in the next
+ *             version.
+ *           - Do not send any events or notifications.
+ *
+ * 2. Revert: Fail if any errors are found.  After this point no errors are
+ * possible.  No visible changes were made, so rollback is minimal (remove
+ * added invisible rules, restore visibility of rules marked for removal).
+ *
+ * 3. Bump the version visible to lookups.
+ *
+ * 4. Finish: Insert replacement rules to the ofproto provider. Remove replaced
+ * and deleted rules from ofproto data structures, and Schedule postponed
+ * removal of deleted rules from the classifier.  Send notifications, buffered
+ * packets, etc.
+ */
 static enum ofperr
 do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
 {
@@ -6551,6 +6653,8 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
     } else {
         error = 0;
         ovs_mutex_lock(&ofproto_mutex);
+
+        /* 1. Begin. */
         LIST_FOR_EACH (be, node, &bundle->msg_list) {
             if (be->type == OFPTYPE_PORT_MOD) {
                 /* Not supported yet. */
@@ -6571,14 +6675,21 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                 error = OFPERR_OFPBFC_MSG_FAILED;
             }
 
-            /* Revert all previous entires. */
+            /* 2. Revert.  Undo all the changes made above. */
             LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
                 if (be->type == OFPTYPE_FLOW_MOD) {
                     do_bundle_flow_mod_revert(ofproto, &be->fm, be);
                 }
             }
         } else {
-            /* Finish the changes. */
+            /* 3. Bump the version.  This makes all the changes in the bundle
+             * visible to the lookups at once.  For this to work an upcall must
+             * read the tables_version once at the beginning and keep using the
+             * same version number for the whole duration of the upcall
+             * processing. */
+            ofproto_bump_tables_version(ofproto);
+
+            /* 4. Finish. */
             LIST_FOR_EACH (be, node, &bundle->msg_list) {
                 if (be->type == OFPTYPE_FLOW_MOD) {
                     struct flow_mod_requester req = { ofconn, be->ofp_msg };
@@ -6615,10 +6726,6 @@ handle_bundle_control(struct ofconn *ofconn, const struct ofp_header *oh)
     if (error) {
         return error;
     }
-    /* Atomic updates not supported yet. */
-    if (bctrl.flags & OFPBF_ATOMIC) {
-        return OFPERR_OFPBFC_BAD_FLAGS;
-    }
     reply.flags = 0;
     reply.bundle_id = bctrl.bundle_id;
 
@@ -7326,6 +7433,8 @@ ofproto_rule_insert__(struct ofproto *ofproto, struct rule *rule)
 {
     const struct rule_actions *actions = rule_get_actions(rule);
 
+    ovs_assert(rule->removed);
+
     if (rule->hard_timeout || rule->idle_timeout) {
         list_insert(&ofproto->expirable, &rule->expirable);
     }
@@ -7334,14 +7443,16 @@ ofproto_rule_insert__(struct ofproto *ofproto, struct rule *rule)
     if (actions->has_meter) {
         meter_insert_rule(rule);
     }
+    rule->removed = false;
 }
 
-/* Removes 'rule' from the ofproto data structures AFTER caller has removed
- * it from the classifier. */
+/* Removes 'rule' from the ofproto data structures. */
 static void
 ofproto_rule_remove__(struct ofproto *ofproto, struct rule *rule)
     OVS_REQUIRES(ofproto_mutex)
 {
+    ovs_assert(!rule->removed);
+
     cookies_remove(ofproto, rule);
 
     eviction_group_remove_rule(rule);
@@ -7352,19 +7463,8 @@ ofproto_rule_remove__(struct ofproto *ofproto, struct rule *rule)
         list_remove(&rule->meter_list_node);
         list_init(&rule->meter_list_node);
     }
-}
-
-static void
-oftable_remove_rule(struct rule *rule)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct classifier *cls = &rule->ofproto->tables[rule->table_id].cls;
 
-    if (classifier_remove(cls, &rule->cr)) {
-        ofproto_rule_remove__(rule->ofproto, rule);
-    } else {
-        OVS_NOT_REACHED();
-    }
+    rule->removed = true;
 }
 
 /* unixctl commands. */
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 1fa5b2d..9c5f0bb 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -3526,38 +3526,38 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
  version bitmap: 0x01, 0x05
 vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=OPEN_REQUEST flags=ordered
+ bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REPLY flags=0
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:66:77:88:99:aa idle:50 actions=output:1
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:77:88:99:aa:bb idle:60 actions=output:2
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:88:99:aa:bb:cc idle:70 actions=output:3
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:66:77:88:99:aa idle:50 actions=output:4
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): DEL table:255 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:66:77:88:99:aa idle:50 actions=output:5
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:77:88:99:aa:bb idle:60 actions=output:6
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:88:99:aa:bb:cc idle:70 actions=output:7
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): DEL table:255 in_port=2,dl_src=00:88:99:aa:bb:cc actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=COMMIT_REQUEST flags=ordered
+ bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=COMMIT_REPLY flags=0
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
@@ -3578,17 +3578,17 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
  version bitmap: 0x01, 0x05
 vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=OPEN_REQUEST flags=ordered
+ bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REPLY flags=0
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): MOD actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): MOD_STRICT in_port=2,dl_src=00:77:88:99:aa:bb actions=output:7
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=COMMIT_REQUEST flags=ordered
+ bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=COMMIT_REPLY flags=0
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
@@ -3609,20 +3609,20 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
  version bitmap: 0x01, 0x05
 vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=OPEN_REQUEST flags=ordered
+ bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REPLY flags=0
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:77:88:99:aa:bb idle:60 actions=output:8
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:255 in_port=2,dl_src=00:66:77:88:99:aa actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:66:77:88:99:aa actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=COMMIT_REQUEST flags=ordered
+ bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=COMMIT_REPLY flags=0
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
@@ -3681,7 +3681,7 @@ OFPT_ERROR (OF1.4) (xid=0xb): OFPBRC_EPERM
 OFPT_FLOW_MOD (OF1.4) (xid=0xb): ADD table:254 actions=drop
 OFPT_ERROR (OF1.4) (xid=0xd): OFPBFC_MSG_FAILED
 OFPT_BUNDLE_CONTROL (OF1.4) (xid=0xd):
- bundle_id=0 type=COMMIT_REQUEST flags=ordered
+ bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 ovs-ofctl: talking to unix:br0.mgmt (Protocol error)
 ])
 
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index b7db9bb..6c48569 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -2843,35 +2843,35 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
  version bitmap: 0x01, 0x05
 vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=OPEN_REQUEST flags=ordered
+ bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REPLY flags=0
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=1 importance:1 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=2 importance:2 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=3 importance:3 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=4 importance:4 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=5 importance:5 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=6 importance:6 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=7 importance:7 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=8 importance:8 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=COMMIT_REQUEST flags=ordered
+ bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=COMMIT_REPLY flags=0
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
@@ -2882,23 +2882,23 @@ vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and ea
 vconn|DBG|unix: received: OFPST_FLOW request (OF1.4):
 vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4):
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=OPEN_REQUEST flags=ordered
+ bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REPLY flags=0
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=1 importance:11 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=3 importance:13 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=5 importance:15 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
- bundle_id=0 flags=ordered
+ bundle_id=0 flags=atomic ordered
 OFPT_FLOW_MOD (OF1.4): ADD dl_vlan=7 importance:17 actions=drop
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
- bundle_id=0 type=COMMIT_REQUEST flags=ordered
+ bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=COMMIT_REPLY flags=0
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
@@ -2908,14 +2908,14 @@ vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
 vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
 vconn|DBG|unix: received: OFPST_FLOW request (OF1.4):
 vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4):
- importance=11, dl_vlan=1 actions=drop
  importance=2, dl_vlan=2 actions=drop
- importance=13, dl_vlan=3 actions=drop
  importance=4, dl_vlan=4 actions=drop
- importance=15, dl_vlan=5 actions=drop
  importance=6, dl_vlan=6 actions=drop
- importance=17, dl_vlan=7 actions=drop
  importance=8, dl_vlan=8 actions=drop
+ importance=11, dl_vlan=1 actions=drop
+ importance=13, dl_vlan=3 actions=drop
+ importance=15, dl_vlan=5 actions=drop
+ importance=17, dl_vlan=7 actions=drop
 ])
 
 OVS_VSWITCHD_STOP
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 2c6a073..63c2ecc 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -298,8 +298,8 @@ These commands manage the flow table in an OpenFlow switch.  In each
 case, \fIflow\fR specifies a flow entry in the format described in
 \fBFlow Syntax\fR, below, \fIfile\fR is a text file that contains zero
 or more flows in the same syntax, one per line, and the optional
-\fB\-\-bundle\fR option operates the command as a single transation,
-see option \fB\-\-bundle\fR, below.
+\fB\-\-bundle\fR option operates the command as a single atomic
+transation, see option \fB\-\-bundle\fR, below.
 .
 .IP "[\fB\-\-bundle\fR] \fBadd\-flow \fIswitch flow\fR"
 .IQ "[\fB\-\-bundle\fR] \fBadd\-flow \fIswitch \fB\- < \fIfile\fR"
@@ -2397,13 +2397,16 @@ depending on its configuration.
 Uses strict matching when running flow modification commands.
 .
 .IP "\fB\-\-bundle\fR"
-Execute flow mods as an OpenFlow 1.4 bundle transaction.
+Execute flow mods as an OpenFlow 1.4 atomic bundle transaction.
 .RS
 .IP \(bu
 Within a bundle, all flow mods are processed in the order they appear
-and as a single transaction, meaning that if one of them fails, the
-whole transaction fails and none of the changes are made to the
-\fIswitch\fR's flow table.
+and as a single atomic transaction, meaning that if one of them fails,
+the whole transaction fails and none of the changes are made to the
+\fIswitch\fR's flow table, and that each given datapath packet
+traversing the OpenFlow tables sees the flow tables either as before
+the transaction, or after all the flow mods in the bundle have been
+successfully applied.
 .IP \(bu
 The beginning and the end of the flow table modification commands in a
 bundle are delimited with OpenFlow 1.4 bundle control messages, which
@@ -2416,10 +2419,6 @@ Bundles require OpenFlow 1.4 or higher.  An explicit \fB-O
 OpenFlow14\fR option is not needed, but you may need to enable
 OpenFlow 1.4 support for OVS by setting the OVSDB \fIprotocols\fR
 column in the \fIbridge\fR table.
-.IP \(bu
-Current implementation executes all bundles with the 'ordered' flag,
-so that the flow mods are always executed in the order specified.
-Atomic bundles are not yet supported.
 .RE
 .
 .so lib/ofp-version.man
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 72ec758..68c2201 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2337,7 +2337,7 @@ bundle_flow_mod__(const char *remote, struct ofputil_flow_mod *fms,
         free(CONST_CAST(struct ofpact *, fm->ofpacts));
     }
 
-    bundle_transact(vconn, &requests, OFPBF_ORDERED);
+    bundle_transact(vconn, &requests, OFPBF_ORDERED | OFPBF_ATOMIC);
     vconn_close(vconn);
 }
 
@@ -2716,7 +2716,7 @@ ofctl_replace_flows(struct ovs_cmdl_context *ctx)
         }
     }
     if (bundle) {
-        bundle_transact(vconn, &requests, OFPBF_ORDERED);
+        bundle_transact(vconn, &requests, OFPBF_ORDERED | OFPBF_ATOMIC);
     } else {
         transact_multiple_noreply(vconn, &requests);
     }
-- 
1.7.10.4




More information about the dev mailing list