[ovs-dev] polished async OFPT_FLOW_MOD (was: Re: [async 6/6] ofproto: Implement asynchronous OFPT_FLOW_MOD commands.)

Ben Pfaff blp at nicira.com
Tue Jun 7 23:20:42 UTC 2011


Here's the set of changes I folded in to the previously posted patch
to make destruction work, for final review.  It also fixes some bugs I
found in testing.  I'll push the full revised version to the "next"
branch in a little while.

--8<--------------------------cut here-------------------------->8--

diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index 30385b2..14cfa04 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -493,6 +493,7 @@ in_band_destroy(struct in_band *ib)
 
         HMAP_FOR_EACH_SAFE (rule, next, cls_rule.hmap_node, &ib->rules) {
             hmap_remove(&ib->rules, &rule->cls_rule.hmap_node);
+            free(rule);
         }
         hmap_destroy(&ib->rules);
         in_band_set_remotes(ib, NULL, 0);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 66e1528..9e804f1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -470,11 +470,32 @@ construct(struct ofproto *ofproto_)
 }
 
 static void
+complete_operations(struct ofproto_dpif *ofproto)
+{
+    struct dpif_completion *c, *next;
+
+    LIST_FOR_EACH_SAFE (c, next, list_node, &ofproto->completions) {
+        ofoperation_complete(c->op, 0);
+        list_remove(&c->list_node);
+        free(c);
+    }
+}
+
+static void
 destruct(struct ofproto *ofproto_)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+    struct rule_dpif *rule, *next_rule;
+    struct cls_cursor cursor;
     int i;
 
+    complete_operations(ofproto);
+
+    cls_cursor_init(&cursor, &ofproto->up.tables[0], NULL);
+    CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, up.cr, &cursor) {
+        ofproto_rule_destroy(&rule->up);
+    }
+
     for (i = 0; i < MAX_MIRRORS; i++) {
         mirror_destroy(ofproto->mirrors[i]);
     }
@@ -498,15 +519,8 @@ run(struct ofproto *ofproto_)
     int i;
 
     if (!clogged) {
-        struct dpif_completion *c, *next;
-
-        LIST_FOR_EACH_SAFE (c, next, list_node, &ofproto->completions) {
-            ofoperation_complete(c->op, 0);
-            list_remove(&c->list_node);
-            free(c);
-        }
+        complete_operations(ofproto);
     }
-
     dpif_run(ofproto->dpif);
 
     for (i = 0; i < 50; i++) {
@@ -2608,10 +2622,9 @@ rule_construct(struct rule *rule_)
     rule->used = rule->up.created;
     rule->packet_count = 0;
     rule->byte_count = 0;
-    list_init(&rule->facets);
 
     victim = rule_dpif_cast(ofoperation_get_victim(rule->up.pending));
-    if (victim) {
+    if (victim && !list_is_empty(&victim->facets)) {
         struct facet *facet;
 
         rule->facets = victim->facets;
@@ -2619,6 +2632,9 @@ rule_construct(struct rule *rule_)
         LIST_FOR_EACH (facet, list_node, &rule->facets) {
             facet->rule = rule;
         }
+    } else {
+        /* Must avoid list_moved() in this case. */
+        list_init(&rule->facets);
     }
 
     complete_operation(rule);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 12d16e1..ff5cb01 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -675,6 +675,8 @@ ofproto_destroy__(struct ofproto *ofproto)
 {
     size_t i;
 
+    assert(list_is_empty(&ofproto->pending));
+
     connmgr_destroy(ofproto->connmgr);
 
     hmap_remove(&all_ofprotos, &ofproto->hmap_node);
@@ -689,6 +691,7 @@ ofproto_destroy__(struct ofproto *ofproto)
     shash_destroy(&ofproto->port_by_name);
 
     for (i = 0; i < ofproto->n_tables; i++) {
+        assert(classifier_is_empty(&ofproto->tables[i]));
         classifier_destroy(&ofproto->tables[i]);
     }
     free(ofproto->tables);
@@ -1372,6 +1375,22 @@ ofproto_rule_destroy__(struct rule *rule)
     rule->ofproto->ofproto_class->rule_dealloc(rule);
 }
 
+/* This function allows an ofproto implementation to destroy any rules that
+ * remain when its ->destruct() function is called.  The caller must have
+ * already uninitialized any derived members of 'rule' (step 5 described in the
+ * large comment in ofproto/private.h titled "Life Cycle").  This function
+ * implements steps 6 and 7.
+ *
+ * This function should only be called from an ofproto implementation's
+ * ->destruct() function.  It is not suitable elsewhere. */
+void
+ofproto_rule_destroy(struct rule *rule)
+{
+    assert(!rule->pending);
+    classifier_remove(&rule->ofproto->tables[rule->table_id], &rule->cr);
+    ofproto_rule_destroy__(rule);
+}
+
 /* Returns true if 'rule' has an OpenFlow OFPAT_OUTPUT or OFPAT_ENQUEUE action
  * that outputs to 'out_port' (output to OFPP_FLOOD and OFPP_ALL doesn't
  * count). */
@@ -2186,7 +2205,9 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct flow_mod *fm,
     if (victim && victim->pending) {
         error = OFPROTO_POSTPONE;
     } else {
-        group = ofopgroup_create_for_ofconn(ofconn, request, fm->buffer_id);
+        group = (ofconn
+                 ? ofopgroup_create_for_ofconn(ofconn, request, fm->buffer_id)
+                 : ofopgroup_create(ofproto));
         ofoperation_create(group, rule, OFOPERATION_ADD);
         rule->pending->victim = victim;
 
@@ -2796,7 +2817,8 @@ ofoperation_complete(struct ofoperation *op, int error)
 
         error = ofconn_pktbuf_retrieve(group->ofconn, group->buffer_id,
                                        &packet, &in_port);
-        if (!error) {
+        if (packet) {
+            assert(!error);
             error = rule_execute(rule, in_port, packet);
         }
     }
diff --git a/ofproto/private.h b/ofproto/private.h
index 3e419b2..f4caa6c 100644
--- a/ofproto/private.h
+++ b/ofproto/private.h
@@ -248,6 +248,10 @@ struct ofproto_class {
 
     /* Life-cycle functions for an "ofproto" (see "Life Cycle" above).
      *
+     *
+     * Construction
+     * ============
+     *
      * ->construct() should not modify most base members of the ofproto.  In
      * particular, the client will initialize the ofproto's 'ports' member
      * after construction is complete.
@@ -266,7 +270,19 @@ struct ofproto_class {
      * allowed to fail with an error.
      *
      * ->construct() returns 0 if successful, otherwise a positive errno
-     * value. */
+     * value.
+     *
+     *
+     * Destruction
+     * ===========
+     *
+     * ->destruct() must do at least the following:
+     *
+     *   - If 'ofproto' has any pending asynchronous operations, ->destruct()
+     *     must complete all of them by calling ofoperation_complete().
+     *
+     *   - If 'ofproto' has any rules left in any of its flow tables, ->
+     */
     struct ofproto *(*alloc)(void);
     int (*construct)(struct ofproto *ofproto);
     void (*destruct)(struct ofproto *ofproto);
@@ -563,8 +579,9 @@ struct ofproto_class {
      *     e.g. ->rule_construct() or ->rule_destruct().  This is the best
      *     choice if the operation completes quickly.
      *
-     *   - The implementation's ->run() function.  This is the only choice for
-     *     an operation that is truly asynchronous.
+     *   - 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:
@@ -648,8 +665,9 @@ struct ofproto_class {
      *     case, it will not.  ->rule_dealloc() will be called in either case.
      *
      *   - If the operation is only partially complete, then it must return 0.
-     *     Later, when the operation is complete, the ->run() function must
-     *     call ofoperation_complete() to report success or failure.
+     *     Later, when the operation is complete, the ->run() or ->destruct()
+     *     function must call ofoperation_complete() to report success or
+     *     failure.
      *
      * ->rule_construct() should not modify any base members of struct rule.
      *
@@ -661,8 +679,8 @@ struct ofproto_class {
      * from 'rule->ofproto''s flow table.  ->rule_destruct() should set in
      * motion removing 'rule' from the datapath flow table.  If removal
      * completes synchronously, it should call ofoperation_complete().
-     * Otherwise, the ->run() function must later call ofoperation_complete()
-     * after the operation completes.
+     * Otherwise, the ->run() or ->destruct() function must later call
+     * ofoperation_complete() after the operation completes.
      *
      * Rule destruction must not fail. */
     struct rule *(*rule_alloc)(void);



More information about the dev mailing list