[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