[ovs-dev] [PATCH 13/16] ofproto: Additional simplifications.

Ben Pfaff blp at nicira.com
Fri Jun 6 05:02:07 UTC 2014


This commit finishes the removal of asynchronous flow table operations
begun in the previous commit, by removing ofoperation and ofopgroup
entirely and all of the code that depended on them.  Following this commit,
all the internal documentation and comments should again be consistent and
correct.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/connmgr.c          |   4 +
 ofproto/ofproto-provider.h | 124 ++------
 ofproto/ofproto.c          | 690 ++++++++++-----------------------------------
 3 files changed, 170 insertions(+), 648 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b53b9f0..7d313c7 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -2074,6 +2074,10 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
     enum nx_flow_monitor_flags update;
     struct ofconn *ofconn;
 
+    if (rule_is_hidden(rule)) {
+        return;
+    }
+
     switch (event) {
     case NXFME_ADDED:
         update = NXFMF_ADD;
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index d61fc7c..8da2822 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -110,10 +110,6 @@ struct ofproto {
     /* OpenFlow connections. */
     struct connmgr *connmgr;
 
-    /* 'meaningful only within ofproto.c, one of the enum ofproto_state
-     * constants defined there. */
-    int state;
-
     /* Delayed rule executions.
      *
      * We delay calls to ->ofproto_class->rule_execute() past releasing
@@ -454,8 +450,6 @@ void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout,
                                   uint16_t hard_timeout)
     OVS_EXCLUDED(ofproto_mutex);
 
-void ofoperation_complete(struct ofoperation *, enum ofperr);
-
 /* A group within a "struct ofproto".
  *
  * With few exceptions, ofproto implementations may look at these fields but
@@ -712,18 +706,8 @@ struct ofproto_class {
      * Destruction
      * ===========
      *
-     * If 'ofproto' has any pending asynchronous operations, ->destruct()
-     * must complete all of them by calling ofoperation_complete().
-     *
      * ->destruct() must also destroy all remaining rules in the ofproto's
-     * tables, by passing each remaining rule to ofproto_rule_delete(), and
-     * then complete each of those deletions in turn by calling
-     * ofoperation_complete().
-     *
-     * (Thus, there is a multi-step process for any rule currently being
-     * inserted or modified at the beginning of destruction: first
-     * ofoperation_complete() that operation, then ofproto_rule_delete() the
-     * rule, then ofoperation_complete() the deletion operation.)
+     * tables, by passing each remaining rule to ofproto_rule_delete().
      *
      * The client will destroy the flow tables themselves after ->destruct()
      * returns.
@@ -742,9 +726,6 @@ struct ofproto_class {
      *   - Call ofproto_rule_expire() for each OpenFlow flow that has reached
      *     its hard_timeout or idle_timeout, to expire the flow.
      *
-     *     (But rules that are part of a pending operation, e.g. rules for
-     *     which ->pending is true, may not expire.)
-     *
      * Returns 0 if successful, otherwise a positive errno value. */
     int (*run)(struct ofproto *ofproto);
 
@@ -1089,8 +1070,8 @@ struct ofproto_class {
      * cycle described above under "Life Cycle".
      *
      * After a rule is successfully constructed, it is then inserted.  If
-     * insertion completes successfully, then before it is later destructed, it
-     * is deleted.
+     * insertion is successful, then before it is later destructed, it is
+     * deleted.
      *
      * You can think of a rule as having the following extra steps inserted
      * between "Life Cycle" steps 4 and 5:
@@ -1098,9 +1079,10 @@ struct ofproto_class {
      *   4.1. The client inserts the rule into the flow table, making it
      *        visible in flow table lookups.
      *
-     *   4.2. The client calls "rule_insert".  Immediately or eventually, the
-     *        implementation calls ofoperation_complete() to indicate that the
-     *        insertion completed.  If the operation failed, skip to step 5.
+     *   4.2. The client calls "rule_insert" to insert the flow.  The
+     *        implementation attempts to install the flow in the underlying
+     *        hardware and returns an error code indicate success or failure.
+     *        On failure, go to step 5.
      *
      *   4.3. The rule is now installed in the flow table.  Eventually it will
      *        be deleted.
@@ -1108,60 +1090,9 @@ struct ofproto_class {
      *   4.4. The client removes the rule from the flow table.  It is no longer
      *        visible in flow table lookups.
      *
-     *   4.5. The client calls "rule_delete".  Immediately or eventually, the
-     *        implementation calls ofoperation_complete() to indicate that the
-     *        deletion completed.  Deletion is not allowed to fail, so it must
-     *        be successful.
-     *
-     *
-     * Asynchronous Operation Support
-     * ==============================
-     *
-     * The "insert" and "delete" life-cycle operations on rules can operate
-     * asynchronously, meaning that ->rule_insert() and ->rule_delete() only
-     * need to initiate their respective operations and do not need to wait for
-     * them to complete before they return.  ->rule_modify_actions() also
-     * operates asynchronously.
-     *
-     * An ofproto implementation reports the success or failure of an
-     * asynchronous operation on a rule using the rule's 'pending' member,
-     * which points to a opaque "struct ofoperation" that represents the
-     * ongoing operation.  When the operation completes, the ofproto
-     * implementation calls ofoperation_complete(), passing the ofoperation and
-     * an error indication.
-     *
-     * Only the following contexts may call ofoperation_complete():
-     *
-     *   - The function called to initiate the operation, e.g. ->rule_insert()
-     *     or ->rule_delete().  This is the best choice if the operation
-     *     completes quickly.
-     *
-     *   - The implementation's ->run() function.
-     *
-     *   - The implementation's ->destruct() function.
-     *
-     * The ofproto base code updates the flow table optimistically, assuming
-     * that the operation will probably succeed:
-     *
-     *   - ofproto adds the rule in the flow table before calling
-     *     ->rule_insert().  If adding a rule in the flow table fails, ofproto
-     *     removes the new rule in the call to ofoperation_complete().
-     *
-     *   - ofproto updates the rule's actions and other properties before
-     *     calling ->rule_modify_actions().  Modifying a rule is not allowed to
-     *     fail (but ->rule_premodify_actions() can prevent the modification
-     *     attempt in advance).
-     *
-     *   - ofproto removes the rule before calling ->rule_delete().  Removing a
-     *     rule is not allowed to fail.  It must always succeed.
-     *
-     * The ofproto base code serializes operations: if any operation is in
-     * progress on a given rule, ofproto postpones initiating any new operation
-     * on that rule until the pending operation completes.  Therefore, every
-     * operation must eventually complete through a call to
-     * ofoperation_complete() to avoid delaying new operations indefinitely
-     * (including any OpenFlow request that affects the rule in question, even
-     * just to query its statistics).
+     *   4.5. The client calls "rule_delete".  The implementation uninstalls
+     *        the flow from the underlying hardware.  Deletion is not allowed
+     *        to fail.
      *
      *
      * Construction
@@ -1195,16 +1126,8 @@ struct ofproto_class {
      *
      * Following successful construction, the ofproto base case inserts 'rule'
      * into its flow table, then it calls ->rule_insert().  ->rule_insert()
-     * should set in motion adding the new rule to the datapath flow table.  It
-     * must act as follows:
-     *
-     *   - If it completes insertion, either by succeeding or failing, it must
-     *     call ofoperation_complete()
-     *
-     *   - If insertion is only partially complete, then it must return without
-     *     calling ofoperation_complete().  Later, when the insertion is
-     *     complete, the ->run() or ->destruct() function must call
-     *     ofoperation_complete() to report success or failure.
+     * 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
@@ -1215,15 +1138,8 @@ struct ofproto_class {
      * ========
      *
      * The ofproto base code removes 'rule' from its flow table before it calls
-     * ->rule_delete().  ->rule_delete() should set in motion removing 'rule'
-     * from the datapath flow table.  It must act as follows:
-     *
-     *   - If it completes deletion, it must call ofoperation_complete().
-     *
-     *   - If deletion is only partially complete, then it must return without
-     *     calling ofoperation_complete().  Later, when the deletion is
-     *     complete, the ->run() or ->destruct() function must call
-     *     ofoperation_complete().
+     * ->rule_delete().  ->rule_delete() must remove 'rule' from the datapath
+     * flow table and return only after this has completed successfully.
      *
      * Rule deletion must not fail.
      *
@@ -1281,10 +1197,11 @@ struct ofproto_class {
         /* OVS_REQUIRES(ofproto_mutex) */;
 
     /* When ->rule_modify_actions() is called, the caller has already replaced
-     * the OpenFlow actions in 'rule' by a new set.  (The original actions are
-     * in rule->pending->actions.)
+     * 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() should set the following in motion:
+     * ->rule_modify_actions() must:
      *
      *   - Update the datapath flow table with the new actions.
      *
@@ -1292,11 +1209,6 @@ struct ofproto_class {
      *     associated with the rule to zero, so that rule_get_stats() will not
      *     longer count those packets or bytes.
      *
-     * If the operation synchronously completes, ->rule_modify_actions() may
-     * call ofoperation_complete() before it returns.  Otherwise, ->run()
-     * should call ofoperation_complete() later, after the operation does
-     * complete.
-     *
      * Rule modification must not fail.
      *
      * ->rule_modify_actions() should not modify any base members of struct
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 0587aff..61daeda 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -69,92 +69,20 @@ COVERAGE_DEFINE(ofproto_recv_openflow);
 COVERAGE_DEFINE(ofproto_reinit_ports);
 COVERAGE_DEFINE(ofproto_update_port);
 
-struct flow_mod_requester;
-
 /* Default fields to use for prefix tries in each flow table, unless something
  * else is configured. */
 const enum mf_field_id default_prefix_fields[2] =
     { MFF_IPV4_DST, MFF_IPV4_SRC };
 
-enum ofproto_state {
-    S_OPENFLOW,                 /* Processing OpenFlow commands. */
-    S_EVICT,                    /* Evicting flows from over-limit tables. */
-    S_FLUSH,                    /* Deleting all flow table rules. */
-};
-
-enum ofoperation_type {
-    OFOPERATION_ADD,
-    OFOPERATION_DELETE,
-    OFOPERATION_MODIFY,
-    OFOPERATION_REPLACE
-};
-
-/* A single OpenFlow request can execute any number of operations.  The
- * ofopgroup maintain OpenFlow state common to all of the operations, e.g. the
- * ofconn to which an error reply should be sent if necessary.
- *
- * ofproto initiates some operations internally.  These operations are still
- * assigned to groups but will not have an associated ofconn. */
-struct ofopgroup {
-    struct ofproto *ofproto;    /* Owning ofproto. */
-    struct list ops;            /* List of "struct ofoperation"s. */
-
-    /* Data needed to send OpenFlow reply on failure or to send a buffered
-     * packet on success.
-     *
-     * If list_is_empty(ofconn_node) then this ofopgroup never had an
-     * associated ofconn or its ofconn's connection dropped after it initiated
-     * the operation.  In the latter case 'ofconn' is a wild pointer that
-     * refers to freed memory, so the 'ofconn' member must be used only if
-     * !list_is_empty(ofconn_node).
-     */
-    struct list ofconn_node;    /* In ofconn's list of pending opgroups. */
-    struct ofconn *ofconn;      /* ofconn for reply (but see note above). */
-    struct ofp_header *request; /* Original request (truncated at 64 bytes). */
-    uint32_t buffer_id;         /* Buffer id from original request. */
-};
-
-static struct ofopgroup *ofopgroup_create_unattached(struct ofproto *);
-static struct ofopgroup *ofopgroup_create(struct ofproto *,
-                                          uint32_t buffer_id,
-                                          const struct flow_mod_requester *);
-static void ofopgroup_submit(struct ofopgroup *);
-static void ofopgroup_complete(struct ofopgroup *);
-
-/* A single flow table operation. */
-struct ofoperation {
-    struct ofopgroup *group;    /* Owning group. */
-    struct list group_node;     /* In ofopgroup's "ops" list. */
-    struct hmap_node hmap_node; /* In ofproto's "deletions" hmap. */
-    struct rule *rule;          /* Rule being operated upon. */
-    enum ofoperation_type type; /* Type of operation. */
-
-    /* OFOPERATION_MODIFY, OFOPERATION_REPLACE: The old actions, if the actions
-     * are changing. */
-    const struct rule_actions *actions;
-
-    /* OFOPERATION_DELETE. */
-    enum ofp_flow_removed_reason reason; /* Reason flow was removed. */
-
-    ovs_be64 flow_cookie;               /* Rule's old flow cookie. */
-    uint16_t idle_timeout;              /* Rule's old idle timeout. */
-    uint16_t hard_timeout;              /* Rule's old hard timeout. */
-    enum ofputil_flow_mod_flags flags;  /* Rule's old flags. */
-    enum ofperr error;                  /* 0 if no error. */
-};
-
-static struct ofoperation *ofoperation_create(struct ofopgroup *,
-                                              struct rule *,
-                                              enum ofoperation_type,
-                                              enum ofp_flow_removed_reason);
-static void ofoperation_destroy(struct ofoperation *);
-
 /* oftable. */
 static void oftable_init(struct oftable *);
 static void oftable_destroy(struct oftable *);
 
 static void oftable_set_name(struct oftable *, const char *name);
 
+static enum ofperr evict_rules_from_table(struct oftable *,
+                                          unsigned int extra_space)
+    OVS_REQUIRES(ofproto_mutex);
 static void oftable_disable_eviction(struct oftable *);
 static void oftable_enable_eviction(struct oftable *,
                                     const struct mf_subfield *fields,
@@ -187,7 +115,6 @@ struct eviction_group {
 };
 
 static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep);
-static void ofproto_evict(struct ofproto *) OVS_EXCLUDED(ofproto_mutex);
 static uint32_t rule_eviction_priority(struct ofproto *ofproto, struct rule *);
 static void eviction_group_add_rule(struct rule *);
 static void eviction_group_remove_rule(struct rule *);
@@ -271,8 +198,7 @@ struct ofport_usage {
 
 /* rule. */
 static void ofproto_rule_send_removed(struct rule *, uint8_t reason);
-static bool rule_is_modifiable(const struct rule *rule,
-                               enum ofputil_flow_mod_flags flag);
+static bool rule_is_readonly(const struct rule *);
 
 /* The source of a flow_mod request, in the code that processes flow_mods.
  *
@@ -282,7 +208,7 @@ static bool rule_is_modifiable(const struct rule *rule,
  * meaningful and thus supplied as NULL. */
 struct flow_mod_requester {
     struct ofconn *ofconn;      /* Connection on which flow_mod arrived. */
-    const struct ofp_header *request; /* The flow_mod request. */
+    ovs_be32 xid;               /* OpenFlow xid of flow_mod request. */
 };
 
 /* OpenFlow. */
@@ -292,9 +218,14 @@ static enum ofperr add_flow(struct ofproto *, struct ofputil_flow_mod *,
 static enum ofperr modify_flows__(struct ofproto *, struct ofputil_flow_mod *,
                                   const struct rule_collection *,
                                   const struct flow_mod_requester *);
-static void delete_flow__(struct rule *, struct ofopgroup *,
-                          enum ofp_flow_removed_reason)
+static void delete_flow__(struct rule *, enum ofp_flow_removed_reason,
+                          const struct flow_mod_requester *)
+    OVS_REQUIRES(ofproto_mutex);
+
+static enum ofperr send_buffered_packet(struct ofconn *, uint32_t buffer_id,
+                                        struct rule *)
     OVS_REQUIRES(ofproto_mutex);
+
 static bool ofproto_group_exists__(const struct ofproto *ofproto,
                                    uint32_t group_id)
     OVS_REQ_RDLOCK(ofproto->groups_rwlock);
@@ -1205,26 +1136,13 @@ ofproto_configure_table(struct ofproto *ofproto, int table_id,
 
     table->max_flows = s->max_flows;
     fat_rwlock_wrlock(&table->cls.rwlock);
-    if (classifier_count(&table->cls) > table->max_flows
-        && table->eviction_fields) {
-        /* 'table' contains more flows than allowed.  We might not be able to
-         * evict them right away because of the asynchronous nature of flow
-         * table changes.  Schedule eviction for later. */
-        switch (ofproto->state) {
-        case S_OPENFLOW:
-            ofproto->state = S_EVICT;
-            break;
-        case S_EVICT:
-        case S_FLUSH:
-            /* We're already deleting flows, nothing more to do. */
-            break;
-        }
-    }
-
     classifier_set_prefix_fields(&table->cls,
                                  s->prefix_fields, s->n_prefix_fields);
-
     fat_rwlock_unlock(&table->cls.rwlock);
+
+    ovs_mutex_lock(&ofproto_mutex);
+    evict_rules_from_table(table, 0);
+    ovs_mutex_unlock(&ofproto_mutex);
 }
 
 bool
@@ -1243,11 +1161,10 @@ static void
 ofproto_rule_delete__(struct rule *rule, uint8_t reason)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct ofopgroup *group;
+    struct ofproto *ofproto = rule->ofproto;
 
-    group = ofopgroup_create_unattached(rule->ofproto);
-    delete_flow__(rule, group, reason);
-    ofopgroup_submit(group);
+    delete_flow__(rule, reason, NULL);
+    ofmonitor_flush(ofproto->connmgr);
 }
 
 /* Deletes 'rule' from 'ofproto'.
@@ -1263,17 +1180,9 @@ void
 ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule)
     OVS_EXCLUDED(ofproto_mutex)
 {
-    struct ofopgroup *group;
-    struct ofoperation *op;
-
     ovs_mutex_lock(&ofproto_mutex);
-    group = ofopgroup_create_unattached(ofproto);
-    op = ofoperation_create(group, rule, OFOPERATION_DELETE, OFPRR_DELETE);
     oftable_remove_rule__(ofproto, rule);
     ofproto->ofproto_class->rule_delete(rule);
-    ofoperation_complete(op, 0);
-    ofopgroup_submit(group);
-
     ovs_mutex_unlock(&ofproto_mutex);
 }
 
@@ -1527,27 +1436,7 @@ ofproto_run(struct ofproto *p)
         p->change_seq = new_seq;
     }
 
-    switch (p->state) {
-    case S_OPENFLOW:
-        connmgr_run(p->connmgr, handle_openflow);
-        break;
-
-    case S_EVICT:
-        connmgr_run(p->connmgr, NULL);
-        ofproto_evict(p);
-        p->state = S_OPENFLOW;
-        break;
-
-    case S_FLUSH:
-        connmgr_run(p->connmgr, NULL);
-        ofproto_flush__(p);
-        connmgr_flushed(p->connmgr);
-        p->state = S_OPENFLOW;
-        break;
-
-    default:
-        OVS_NOT_REACHED();
-    }
+    connmgr_run(p->connmgr, handle_openflow);
 
     return error;
 }
@@ -1560,18 +1449,7 @@ ofproto_wait(struct ofproto *p)
         p->ofproto_class->port_poll_wait(p);
     }
     seq_wait(connectivity_seq_get(), p->change_seq);
-
-    switch (p->state) {
-    case S_OPENFLOW:
-        connmgr_wait(p->connmgr);
-        break;
-
-    case S_EVICT:
-    case S_FLUSH:
-        connmgr_wait(p->connmgr);
-        poll_immediate_wake();
-        break;
-    }
+    connmgr_wait(p->connmgr);
 }
 
 bool
@@ -1975,23 +1853,24 @@ ofproto_delete_flow(struct ofproto *ofproto,
     rule = rule_from_cls_rule(classifier_find_match_exactly(cls, target,
                                                             priority));
     fat_rwlock_unlock(&cls->rwlock);
-    if (rule) {
-        /* Execute a full flow mod.  We can't optimize this at all because we
-         * didn't take enough locks above to ensure that the flow table didn't
-         * already change beneath us.  */
-        simple_flow_mod(ofproto, target, priority, NULL, 0,
-                        OFPFC_DELETE_STRICT);
+    if (!rule) {
+        return;
     }
+
+    /* Execute a flow mod.  We can't optimize this at all because we didn't
+     * take enough locks above to ensure that the flow table didn't already
+     * change beneath us. */
+    simple_flow_mod(ofproto, target, priority, NULL, 0, OFPFC_DELETE_STRICT);
 }
 
-/* Starts the process of deleting all of the flows from all of ofproto's flow
- * tables and then reintroducing the flows required by in-band control and
- * fail-open.  The process will complete in a later call to ofproto_run(). */
+/* Delete all of the flows from all of ofproto's flow tables, then reintroduce
+ * the flows required by in-band control and fail-open.  */
 void
 ofproto_flush_flows(struct ofproto *ofproto)
 {
     COVERAGE_INC(ofproto_flush);
-    ofproto->state = S_FLUSH;
+    ofproto_flush__(ofproto);
+    connmgr_flushed(ofproto->connmgr);
 }
 
 static void
@@ -2705,23 +2584,10 @@ destroy_rule_executes(struct ofproto *ofproto)
 }
 
 static bool
-oftable_is_modifiable(const struct oftable *table,
-                      enum ofputil_flow_mod_flags flags)
-{
-    if (flags & OFPUTIL_FF_NO_READONLY) {
-        return true;
-    }
-
-    return !(table->flags & OFTABLE_READONLY);
-}
-
-static bool
-rule_is_modifiable(const struct rule *rule, enum ofputil_flow_mod_flags flags)
+rule_is_readonly(const struct rule *rule)
 {
-    const struct oftable *rule_table;
-
-    rule_table = &rule->ofproto->tables[rule->table_id];
-    return oftable_is_modifiable(rule_table, flags);
+    const struct oftable *table = &rule->ofproto->tables[rule->table_id];
+    return (table->flags & OFTABLE_READONLY) != 0;
 }
 
 static enum ofperr
@@ -3174,22 +3040,6 @@ cookies_remove(struct ofproto *ofproto, struct rule *rule)
 }
 
 static void
-ofproto_rule_change_cookie(struct ofproto *ofproto, struct rule *rule,
-                           ovs_be64 new_cookie)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    if (new_cookie != rule->flow_cookie) {
-        cookies_remove(ofproto, rule);
-
-        ovs_mutex_lock(&rule->mutex);
-        rule->flow_cookie = new_cookie;
-        ovs_mutex_unlock(&rule->mutex);
-
-        cookies_insert(ofproto, rule);
-    }
-}
-
-static void
 calc_duration(long long int start, long long int now,
               uint32_t *sec, uint32_t *nsec)
 {
@@ -3393,7 +3243,7 @@ collect_rule(struct rule *rule, const struct rule_criteria *c,
         && !((rule->flow_cookie ^ c->cookie) & c->cookie_mask)
         && (!rule_is_hidden(rule) || c->include_hidden)) {
         /* Rule matches all the criteria... */
-        if (rule_is_modifiable(rule, 0) || c->include_readonly) {
+        if (!rule_is_readonly(rule) || c->include_readonly) {
             /* ...add it. */
             rule_collection_add(rules, rule);
         } else {
@@ -3868,8 +3718,7 @@ should_evict_a_rule(struct oftable *table, unsigned int extra_space)
 }
 
 static enum ofperr
-evict_rules_from_table(struct ofproto *ofproto, struct oftable *table,
-                       unsigned int extra_space)
+evict_rules_from_table(struct oftable *table, unsigned int extra_space)
     OVS_REQUIRES(ofproto_mutex)
 {
     while (should_evict_a_rule(table, extra_space)) {
@@ -3878,9 +3727,7 @@ evict_rules_from_table(struct ofproto *ofproto, struct oftable *table,
         if (!choose_rule_to_evict(table, &rule)) {
             return OFPERR_OFPFMFC_TABLE_FULL;
         } else {
-            struct ofopgroup *group = ofopgroup_create_unattached(ofproto);
-            delete_flow__(rule, group, OFPRR_EVICTION);
-            ofopgroup_submit(group);
+            ofproto_rule_delete__(rule, OFPRR_EVICTION);
         }
     }
 
@@ -3905,8 +3752,6 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     OVS_REQUIRES(ofproto_mutex)
 {
     const struct rule_actions *actions;
-    struct ofopgroup *group;
-    struct ofoperation *op;
     struct oftable *table;
     struct cls_rule cr;
     struct rule *rule;
@@ -3938,8 +3783,8 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     }
 
     table = &ofproto->tables[table_id];
-
-    if (!oftable_is_modifiable(table, fm->flags)) {
+    if (table->flags & OFTABLE_READONLY
+        && !(fm->flags & OFPUTIL_FF_NO_READONLY)) {
         return OFPERR_OFPBRC_EPERM;
     }
 
@@ -3958,20 +3803,17 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls, &cr));
     fat_rwlock_unlock(&table->cls.rwlock);
     if (rule) {
+        struct rule_collection rules;
+
         cls_rule_destroy(&cr);
-        if (!rule_is_modifiable(rule, fm->flags)) {
-            return OFPERR_OFPBRC_EPERM;
-        } else {
-            struct rule_collection rules;
 
-            rule_collection_init(&rules);
-            rule_collection_add(&rules, rule);
-            fm->modify_cookie = true;
-            error = modify_flows__(ofproto, fm, &rules, req);
-            rule_collection_destroy(&rules);
+        rule_collection_init(&rules);
+        rule_collection_add(&rules, rule);
+        fm->modify_cookie = true;
+        error = modify_flows__(ofproto, fm, &rules, req);
+        rule_collection_destroy(&rules);
 
-            return error;
-        }
+        return error;
     }
 
     /* Check for overlap, if requested. */
@@ -3989,7 +3831,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     }
 
     /* If necessary, evict an existing rule to clear out space. */
-    error = evict_rules_from_table(ofproto, table, 1);
+    error = evict_rules_from_table(table, 1);
     if (error) {
         cls_rule_destroy(&cr);
         return error;
@@ -4048,13 +3890,29 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     classifier_insert(&table->cls, CONST_CAST(struct cls_rule *, &rule->cr));
     fat_rwlock_unlock(&table->cls.rwlock);
 
-    group = ofopgroup_create(ofproto, fm->buffer_id, req);
-    op = ofoperation_create(group, rule, OFOPERATION_ADD, 0);
     error = ofproto->ofproto_class->rule_insert(rule);
-    ofoperation_complete(op, error);
-    ofopgroup_submit(group);
+    if (error) {
+        oftable_remove_rule(rule);
+        ofproto_rule_unref(rule);
+        return error;
+    }
 
-    return error;
+    if (minimask_get_vid_mask(&rule->cr.match.mask) == VLAN_VID_MASK) {
+        if (ofproto->vlan_bitmap) {
+            uint16_t vid = miniflow_get_vid(&rule->cr.match.flow);
+            if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) {
+                bitmap_set1(ofproto->vlan_bitmap, vid);
+                ofproto->vlans_changed = true;
+            }
+        } else {
+            ofproto->vlans_changed = true;
+        }
+    }
+
+    ofmonitor_report(ofproto->connmgr, rule, NXFME_ADDED, 0,
+                     req ? req->ofconn : NULL, req ? req->xid : 0);
+
+    return req ? send_buffered_packet(req->ofconn, fm->buffer_id, rule) : 0;
 }
 
 /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
@@ -4072,8 +3930,7 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
                const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
-    enum ofoperation_type type;
-    struct ofopgroup *group;
+    enum nx_flow_update_event event;
     enum ofperr error;
     size_t i;
 
@@ -4089,35 +3946,47 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
         }
     }
 
-    type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY;
-    group = ofopgroup_create(ofproto, fm->buffer_id, req);
+    event = fm->command == OFPFC_ADD ? NXFME_ADDED : NXFME_MODIFIED;
     for (i = 0; i < rules->n; i++) {
         struct rule *rule = rules->rules[i];
-        const struct rule_actions *actions;
-        struct ofoperation *op;
-        bool actions_changed;
-        bool reset_counters;
 
-        /* FIXME: Implement OFPFUTIL_FF_RESET_COUNTS */
+        /*  '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);
 
-        actions = rule_get_actions(rule);
-        actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
-                                         actions->ofpacts,
-                                         actions->ofpacts_len);
+        bool reset_counters = (fm->flags & OFPUTIL_FF_RESET_COUNTS) != 0;
 
-        op = ofoperation_create(group, rule, type, 0);
+        long long int now = time_msec();
+
+        /* FIXME: Implement OFPFUTIL_FF_RESET_COUNTS */
 
-        if (fm->modify_cookie && fm->new_cookie != OVS_BE64_MAX) {
-            ofproto_rule_change_cookie(ofproto, rule, fm->new_cookie);
+        if (change_cookie) {
+            cookies_remove(ofproto, rule);
         }
-        if (type == OFOPERATION_REPLACE) {
-            ovs_mutex_lock(&rule->mutex);
+
+        ovs_mutex_lock(&rule->mutex);
+        if (fm->command == OFPFC_ADD) {
             rule->idle_timeout = fm->idle_timeout;
             rule->hard_timeout = fm->hard_timeout;
-            ovs_mutex_unlock(&rule->mutex);
-
             rule->flags = fm->flags & OFPUTIL_FF_STATE;
+            rule->created = now;
+        }
+        if (change_cookie) {
+            rule->flow_cookie = fm->new_cookie;
+        }
+        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) {
                 if (!rule->eviction_group) {
                     eviction_group_add_rule(rule);
@@ -4127,21 +3996,27 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
             }
         }
 
-        reset_counters = (fm->flags & OFPUTIL_FF_RESET_COUNTS) != 0;
-        if (actions_changed || reset_counters) {
-            const struct rule_actions *new_actions;
-
-            op->actions = rule_get_actions(rule);
-            new_actions = rule_actions_create(ofproto,
-                                              fm->ofpacts, fm->ofpacts_len);
-
-            ovsrcu_set(&rule->actions, new_actions);
+        if (change_actions) {
+            ovsrcu_set(&rule->actions, rule_actions_create(ofproto,
+                                                           fm->ofpacts,
+                                                           fm->ofpacts_len));
+            rule_actions_destroy(actions);
+        }
 
+        if (change_actions || reset_counters) {
             ofproto->ofproto_class->rule_modify_actions(rule, reset_counters);
         }
-        ofoperation_complete(op, 0);
+
+        if (event != NXFME_MODIFIED || change_actions || change_cookie) {
+            ofmonitor_report(ofproto->connmgr, rule, event, 0,
+                             req ? req->ofconn : NULL, req ? req->xid : 0);
+        }
+    }
+
+    if (fm->buffer_id != UINT32_MAX && req) {
+        error = send_buffered_packet(req->ofconn, fm->buffer_id,
+                                     rules->rules[0]);
     }
-    ofopgroup_submit(group);
 
     return error;
 }
@@ -4223,15 +4098,16 @@ modify_flow_strict(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 /* OFPFC_DELETE implementation. */
 
 static void
-delete_flow__(struct rule *rule, struct ofopgroup *group,
-              enum ofp_flow_removed_reason reason)
+delete_flow__(struct rule *rule, enum ofp_flow_removed_reason reason,
+              const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct ofproto *ofproto = rule->ofproto;
 
     ofproto_rule_send_removed(rule, reason);
 
-    ofoperation_create(group, rule, OFOPERATION_DELETE, reason);
+    ofmonitor_report(ofproto->connmgr, rule, NXFME_DELETED, reason,
+                     req ? req->ofconn : NULL, req ? req->xid : 0);
     oftable_remove_rule(rule);
     ofproto->ofproto_class->rule_delete(rule);
 }
@@ -4246,14 +4122,12 @@ delete_flows__(struct ofproto *ofproto,
                const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct ofopgroup *group;
     size_t i;
 
-    group = ofopgroup_create(ofproto, UINT32_MAX, req);
     for (i = 0; i < rules->n; i++) {
-        delete_flow__(rules->rules[i], group, reason);
+        delete_flow__(rules->rules[i], reason, req);
     }
-    ofopgroup_submit(group);
+    ofmonitor_flush(ofproto->connmgr);
 
     return 0;
 }
@@ -4318,7 +4192,8 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason)
     struct ofputil_flow_removed fr;
     long long int used;
 
-    if (rule_is_hidden(rule) || !(rule->flags & OFPUTIL_FF_SEND_FLOW_REM)) {
+    if (rule_is_hidden(rule) ||
+        !(rule->flags & OFPUTIL_FF_SEND_FLOW_REM)) {
         return;
     }
 
@@ -4343,18 +4218,12 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason)
  * OFPRR_HARD_TIMEOUT or OFPRR_IDLE_TIMEOUT), and then removes 'rule' from its
  * ofproto.
  *
- * 'rule' must not have a pending operation (that is, 'rule->pending' must be
- * NULL).
- *
  * ofproto implementation ->run() functions should use this function to expire
  * OpenFlow flows. */
 void
 ofproto_rule_expire(struct rule *rule, uint8_t reason)
     OVS_REQUIRES(ofproto_mutex)
 {
-    ovs_assert(reason == OFPRR_HARD_TIMEOUT || reason == OFPRR_IDLE_TIMEOUT
-               || reason == OFPRR_DELETE || reason == OFPRR_GROUP_DELETE);
-
     ofproto_rule_delete__(rule, reason);
 }
 
@@ -4421,7 +4290,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
         struct flow_mod_requester req;
 
         req.ofconn = ofconn;
-        req.request = oh;
+        req.xid = oh->xid;
         error = handle_flow_mod__(ofproto, &fm, &req);
     }
     if (error) {
@@ -5968,282 +5837,37 @@ handle_openflow(struct ofconn *ofconn, const struct ofpbuf *ofp_msg)
 
 /* Asynchronous operations. */
 
-/* Creates and returns a new ofopgroup that is not associated with any
- * OpenFlow connection.
- *
- * The caller should add operations to the returned group with
- * ofoperation_create() and then submit it with ofopgroup_submit(). */
-static struct ofopgroup *
-ofopgroup_create_unattached(struct ofproto *ofproto)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct ofopgroup *group = xzalloc(sizeof *group);
-    group->ofproto = ofproto;
-    list_init(&group->ops);
-    list_init(&group->ofconn_node);
-    return group;
-}
-
-/* Creates and returns a new ofopgroup for 'ofproto'.
- *
- * If 'ofconn' is NULL, the new ofopgroup is not associated with any OpenFlow
- * connection.  The 'request' and 'buffer_id' arguments are ignored.
- *
- * If 'ofconn' is nonnull, then the new ofopgroup is associated with 'ofconn'.
- * If the ofopgroup eventually fails, then the error reply will include
- * 'request'.  If the ofopgroup eventually succeeds, then the packet with
- * buffer id 'buffer_id' on 'ofconn' will be sent by 'ofconn''s ofproto.
- *
- * The caller should add operations to the returned group with
- * ofoperation_create() and then submit it with ofopgroup_submit(). */
-static struct ofopgroup *
-ofopgroup_create(struct ofproto *ofproto, uint32_t buffer_id,
-                 const struct flow_mod_requester *req)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct ofopgroup *group = ofopgroup_create_unattached(ofproto);
-    if (req) {
-        size_t request_len = ntohs(req->request->length);
-
-        ovs_assert(ofconn_get_ofproto(req->ofconn) == ofproto);
-
-        group->ofconn = req->ofconn;
-        group->request = xmemdup(req->request, MIN(request_len, 64));
-        group->buffer_id = buffer_id;
-    }
-    return group;
-}
-
-/* Submits 'group' for processing.
- *
- * If 'group' contains no operations (e.g. none were ever added, or all of the
- * ones that were added completed synchronously), then it is destroyed
- * immediately.  Otherwise it is added to the ofproto's list of pending
- * groups. */
-static void
-ofopgroup_submit(struct ofopgroup *group)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    ofopgroup_complete(group);
-}
-
-static void
-ofopgroup_complete(struct ofopgroup *group)
+static enum ofperr
+send_buffered_packet(struct ofconn *ofconn, uint32_t buffer_id,
+                     struct rule *rule)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct ofproto *ofproto = group->ofproto;
-
-    struct ofconn *abbrev_ofconn;
-    ovs_be32 abbrev_xid;
-
-    struct ofoperation *op, *next_op;
-    int error;
-
-    error = 0;
-    LIST_FOR_EACH (op, group_node, &group->ops) {
-        if (op->error) {
-            error = op->error;
-            break;
-        }
-    }
-
-    if (!error && group->ofconn && group->buffer_id != UINT32_MAX) {
-        LIST_FOR_EACH (op, group_node, &group->ops) {
-            if (op->type != OFOPERATION_DELETE) {
-                struct ofpbuf *packet;
-                ofp_port_t in_port;
-
-                error = ofconn_pktbuf_retrieve(group->ofconn, group->buffer_id,
-                                               &packet, &in_port);
-                if (packet) {
-                    struct rule_execute *re;
-
-                    ovs_assert(!error);
-
-                    ofproto_rule_ref(op->rule);
-
-                    re = xmalloc(sizeof *re);
-                    re->rule = op->rule;
-                    re->in_port = in_port;
-                    re->packet = packet;
-
-                    if (!guarded_list_push_back(&ofproto->rule_executes,
-                                                &re->list_node, 1024)) {
-                        ofproto_rule_unref(op->rule);
-                        ofpbuf_delete(re->packet);
-                        free(re);
-                    }
-                }
-                break;
-            }
-        }
-    }
-
-    if (!error && !list_is_empty(&group->ofconn_node)) {
-        abbrev_ofconn = group->ofconn;
-        abbrev_xid = group->request->xid;
-    } else {
-        abbrev_ofconn = NULL;
-        abbrev_xid = htonl(0);
-    }
-    LIST_FOR_EACH_SAFE (op, next_op, group_node, &group->ops) {
-        struct rule *rule = op->rule;
-
-        /* We generally want to report the change to active OpenFlow flow
-           monitors (e.g. NXST_FLOW_MONITOR).  There are three exceptions:
-
-              - The operation failed.
-
-              - The affected rule is not visible to controllers.
-
-              - The operation's only effect was to update rule->modified. */
-        if (!(op->error
-              || rule_is_hidden(rule)
-              || (op->type == OFOPERATION_MODIFY
-                  && !op->actions
-                  && rule->flow_cookie == op->flow_cookie))) {
-            enum nx_flow_update_event event_type;
+    enum ofperr error = 0;
+    if (ofconn && buffer_id != UINT32_MAX) {
+        struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
+        struct ofpbuf *packet;
+        ofp_port_t in_port;
 
-            switch (op->type) {
-            case OFOPERATION_ADD:
-            case OFOPERATION_REPLACE:
-                event_type = NXFME_ADDED;
-                break;
+        error = ofconn_pktbuf_retrieve(ofconn, buffer_id, &packet, &in_port);
+        if (packet) {
+            struct rule_execute *re;
 
-            case OFOPERATION_DELETE:
-                event_type = NXFME_DELETED;
-                break;
+            ofproto_rule_ref(rule);
 
-            case OFOPERATION_MODIFY:
-                event_type = NXFME_MODIFIED;
-                break;
+            re = xmalloc(sizeof *re);
+            re->rule = rule;
+            re->in_port = in_port;
+            re->packet = packet;
 
-            default:
-                OVS_NOT_REACHED();
-            }
-
-            ofmonitor_report(ofproto->connmgr, rule, event_type,
-                             op->reason, abbrev_ofconn, abbrev_xid);
-        }
-
-        ovs_assert(!op->error || op->type == OFOPERATION_ADD);
-        switch (op->type) {
-        case OFOPERATION_ADD:
-            if (!op->error) {
-                uint16_t vid_mask;
-
-                vid_mask = minimask_get_vid_mask(&rule->cr.match.mask);
-                if (vid_mask == VLAN_VID_MASK) {
-                    if (ofproto->vlan_bitmap) {
-                        uint16_t vid = miniflow_get_vid(&rule->cr.match.flow);
-                        if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) {
-                            bitmap_set1(ofproto->vlan_bitmap, vid);
-                            ofproto->vlans_changed = true;
-                        }
-                    } else {
-                        ofproto->vlans_changed = true;
-                    }
-                }
-            } else {
-                oftable_remove_rule(rule);
+            if (!guarded_list_push_back(&ofproto->rule_executes,
+                                        &re->list_node, 1024)) {
                 ofproto_rule_unref(rule);
+                ofpbuf_delete(re->packet);
+                free(re);
             }
-            break;
-
-        case OFOPERATION_DELETE:
-            ofproto_rule_unref(rule);
-            op->rule = NULL;
-            break;
-
-        case OFOPERATION_MODIFY:
-        case OFOPERATION_REPLACE: {
-            long long now = time_msec();
-
-            ovs_mutex_lock(&rule->mutex);
-            rule->modified = now;
-            if (op->type == OFOPERATION_REPLACE) {
-                rule->created = now;
-            }
-            ovs_mutex_unlock(&rule->mutex);
-            break;
         }
-
-        default:
-            OVS_NOT_REACHED();
-        }
-
-        ofoperation_destroy(op);
-    }
-
-    ofmonitor_flush(ofproto->connmgr);
-
-    if (error) {
-        ofconn_send_error(group->ofconn, group->request, error);
     }
-    free(group->request);
-    free(group);
-}
-
-/* Initiates a new operation on 'rule', of the specified 'type', within
- * 'group'.  Prior to calling, 'rule' must not have any pending operation.
- *
- * For a 'type' of OFOPERATION_DELETE, 'reason' should specify the reason that
- * the flow is being deleted.  For other 'type's, 'reason' is ignored (use 0).
- *
- * Returns the newly created ofoperation (which is also available as
- * rule->pending). */
-static struct ofoperation *
-ofoperation_create(struct ofopgroup *group, struct rule *rule,
-                   enum ofoperation_type type,
-                   enum ofp_flow_removed_reason reason)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct ofoperation *op;
-
-    op = xzalloc(sizeof *op);
-    op->group = group;
-    list_push_back(&group->ops, &op->group_node);
-    op->rule = rule;
-    op->type = type;
-    op->reason = reason;
-    op->flow_cookie = rule->flow_cookie;
-    ovs_mutex_lock(&rule->mutex);
-    op->idle_timeout = rule->idle_timeout;
-    op->hard_timeout = rule->hard_timeout;
-    ovs_mutex_unlock(&rule->mutex);
-    op->flags = rule->flags;
-
-    return op;
-}
-
-static void
-ofoperation_destroy(struct ofoperation *op)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    rule_actions_destroy(op->actions);
-    free(op);
-}
-
-/* Indicates that 'op' completed with status 'error', which is either 0 to
- * indicate success or an OpenFlow error code on failure.
- *
- * If 'error' is 0, indicating success, the operation will be committed
- * permanently to the flow table.
- *
- * Flow modifications and deletions must always succeed.  Flow additions may
- * fail, indicated by nonzero 'error'.  If an "add flow" operation fails, this
- * function removes the new rule.  The caller must have uninitialized any
- * derived state in the new rule, as in step 5 of in the "Life Cycle" in
- * ofproto/ofproto-provider.h.  ofoperation_complete() performs steps 6 and and
- * 7 for the new rule, calling its ->rule_dealloc() function.
- *
- * Please see the large comment in ofproto/ofproto-provider.h titled
- * "Asynchronous Operation Support" for more information. */
-void
-ofoperation_complete(struct ofoperation *op, enum ofperr error)
-{
-    ovs_assert(!error || op->type == OFOPERATION_ADD);
-    op->error = error;
+    return error;
 }
 
 static uint64_t
@@ -6315,24 +5939,6 @@ choose_rule_to_evict(struct oftable *table, struct rule **rulep)
 
     return false;
 }
-
-/* Searches 'ofproto' for tables that have more flows than their configured
- * maximum and that have flow eviction enabled, and evicts as many flows as
- * necessary and currently feasible from them.
- *
- * This triggers only when an OpenFlow table has N flows in it and then the
- * client configures a maximum number of flows less than N. */
-static void
-ofproto_evict(struct ofproto *ofproto)
-{
-    struct oftable *table;
-
-    ovs_mutex_lock(&ofproto_mutex);
-    OFPROTO_FOR_EACH_TABLE (table, ofproto) {
-        evict_rules_from_table(ofproto, table, 0);
-    }
-    ovs_mutex_unlock(&ofproto_mutex);
-}
 
 /* Eviction groups. */
 
-- 
1.9.1




More information about the dev mailing list