secchan: Eliminate UNKNOWN_SUPER.

When a super-rule is destroyed, secchan must reassess each of its subrules.
Each subrule might now have no super-rule (which we suspect is the common
case) or it might have a new super-rule.

Until now, secchan has "optimized" this reassessment by initially assigning
each of the deleted super-rule's subrules a super-rule of UNKNOWN_SUPER,
which is not a valid rule at all.  It did this in the hope that the
subrule would get deleted before we need to know what its super-rule is.
However, this has repeatedly led to bugs, since it's not always obvious
what code will need to find a rule's super-rule.

This commit fixes the problem by removing the "optimization" (in quotes
because there is no evidence that it was a useful optimization in
practice).

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 9191ab6e61e10f9f0d115fc09bd53b8fe8d3d8fc..50b126aa2474d1b11ac043e59fdab3b49f9fedc4 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -97,7 +97,6 @@ static int xlate_actions(const union ofp_action *in, size_t n_in,
                          struct odp_actions *out, tag_type *tags,
                          bool *may_setup_flow);
 
-#define UNKNOWN_SUPER ((struct rule *)-1)
 struct rule {
     struct cls_rule cr;
 
@@ -111,17 +110,14 @@ struct rule {
     uint8_t ip_tos;             /* Last-seen IP type-of-service. */
     tag_type tags;              /* Tags (set only by hooks). */
 
-    /* A subrule with a known super-rule has a non-null 'super' other than
-     * UNKNOWN_SUPER, in which case 'list' is an element of the super-rule's
-     * list of subrules.
+    /* If 'super' is non-NULL, this rule is a subrule, that is, it is an
+     * exact-match rule (having cr.wc.wildcards of 0) generated from the
+     * wildcard rule 'super'.  In this case, 'list' is an element of the
+     * super-rule's list.
      *
-     * A subrule with an unknown super-rule (which is an optimization that
-     * might well be better removed) has 'super' set to UNKNOWN_SUPER, in
-     * which case 'list' is unused.
-     *
-     * A super-rule has 'super' of NULL, in which case 'list' is the head of a
-     * list of sub-rules.  (A super-rule with no wildcards will never have any
-     * sub-rules.) */
+     * If 'super' is NULL, this rule is a super-rule, and 'list' is the head of
+     * a list of subrules.  A super-rule with no wildcards (where
+     * cr.wc.wildcards is 0) will never have any subrules. */
     struct rule *super;
     struct list list;
 
@@ -165,7 +161,7 @@ static struct rule *rule_create(struct rule *super, const union ofp_action *,
                                 size_t n_actions, uint16_t idle_timeout,
                                 uint16_t hard_timeout);
 static void rule_free(struct rule *);
-static void rule_destroy(struct rule *);
+static void rule_destroy(struct ofproto *, struct rule *);
 static struct rule *rule_from_cls_rule(const struct cls_rule *);
 static bool rule_make_actions(struct ofproto *, struct rule *,
                               const struct ofpbuf *packet);
@@ -968,7 +964,7 @@ ofproto_add_flow(struct ofproto *p,
 
     displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr));
     if (displaced_rule) {
-        rule_destroy(displaced_rule);
+        rule_destroy(p, displaced_rule);
     }
 
     if (!wildcards) {
@@ -1002,7 +998,7 @@ ofproto_delete_flow(struct ofproto *ofproto, const flow_t *flow,
             ofproto->need_revalidate = true;
         }
         classifier_remove(&ofproto->cls, &rule->cr);
-        rule_destroy(rule);
+        rule_destroy(ofproto, rule);
     }
 }
 
@@ -1371,15 +1367,23 @@ rule_free(struct rule *rule)
     free(rule);
 }
 
+/* Destroys 'rule'.  If 'rule' is a subrule, also removes it from its
+ * super-rule's list of subrules.  If 'rule' is a super-rule, also iterates
+ * through all of its subrules and revalidates them, destroying any that no
+ * longer has a super-rule (which is probably all of them).
+ *
+ * Before calling this function, the caller must make have removed 'rule' from
+ * the classifier.  If 'rule' is an exact-match rule, the caller is also
+ * responsible for ensuring that it has been uninstalled from the datapath. */
 static void
-rule_destroy(struct rule *rule)
+rule_destroy(struct ofproto *ofproto, struct rule *rule)
 {
     if (!rule->super) {
-        struct rule *subrule;
-        LIST_FOR_EACH (subrule, struct rule, list, &rule->list) {
-            subrule->super = UNKNOWN_SUPER;
+        struct rule *subrule, *next;
+        LIST_FOR_EACH_SAFE (subrule, next, struct rule, list, &rule->list) {
+            revalidate_rule(ofproto, subrule);
         }
-    } else if (rule->super != UNKNOWN_SUPER) {
+    } else {
         list_remove(&rule->list);
     }
     rule_free(rule);
@@ -1412,7 +1416,6 @@ rule_make_actions(struct ofproto *p, struct rule *rule,
     struct odp_actions a;
     size_t actions_len;
 
-    assert(rule->super != UNKNOWN_SUPER);
     assert(!rule->cr.wc.wildcards);
 
     super = rule->super ? rule->super : rule;
@@ -1478,8 +1481,7 @@ rule_uninstall(struct ofproto *p, struct rule *rule)
         odp_flow.actions = NULL;
         odp_flow.n_actions = 0;
         if (!dpif_flow_del(&p->dpif, &odp_flow)) {
-            update_stats(rule->super && rule->super != UNKNOWN_SUPER
-                         ? rule->super : rule, &odp_flow.stats);
+            update_stats(rule->super ? rule->super : rule, &odp_flow.stats);
         }
         rule->installed = false;
 
@@ -1706,7 +1708,7 @@ lookup_valid_rule(struct ofproto *ofproto, const flow_t *flow)
      * revalidation.  If it is not valid, don't return it. */
     if (rule
         && rule->super
-        && (ofproto->need_revalidate || rule->super == UNKNOWN_SUPER)
+        && ofproto->need_revalidate
         && !revalidate_rule(ofproto, rule)) {
         return NULL;
     }
@@ -2453,9 +2455,10 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
              * the subrule that got actions from 'displaced_rule'.  We could
              * add more data to track such situations, but we have no evidence
              * that the optimization would be worthwhile performance-wise
-             * anyhow.
+             * anyhow.  So we just let rule_destroy() check each subrule and
+             * transfer or destroy them as necessary.
              */
-            rule_destroy(displaced_rule);
+            rule_destroy(p, displaced_rule);
         }
 
         /* We might need to change the rules for arbitrary subrules. */
@@ -2466,11 +2469,10 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
         rule_make_actions(p, rule, packet);
         rule_install(p, rule, &stats);
         if (displaced_rule) {
-            if (displaced_rule->super &&
-                displaced_rule->super != UNKNOWN_SUPER) {
+            if (displaced_rule->super) {
                 update_stats(displaced_rule->super, &stats);
             }
-            rule_destroy(displaced_rule);
+            rule_destroy(p, displaced_rule);
         }
     }
     ofpbuf_delete(packet);
@@ -2493,7 +2495,7 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
             rule_uninstall(p, rule);
         }
         classifier_remove(&p->cls, &rule->cr);
-        rule_destroy(rule);
+        rule_destroy(p, rule);
     } else {
         free(rule->actions);
         rule->actions = xmemdup(ofm->actions,
@@ -2574,6 +2576,7 @@ modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm,
     cbdata.command = command;
 
     cls_rule_from_match(&target, &ofm->match, 0);
+
     classifier_for_each_match(&p->cls, &target, CLS_INC_ALL,
                               modify_flows_cb, &cbdata);
     return 0;
@@ -2834,14 +2837,14 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
                 /* We just replaced a real rule with a subrule.  We shouldn't
                  * have done that.  Put old_sr back and destroy the subrule. */
                 cls_rule_replace(&p->cls, &subrule->cr, &old_sr->cr);
-                rule_destroy(subrule);
+                rule_destroy(p, subrule);
                 subrule = old_sr;
             } else {
                 /* XXX this seems wrong: why are we accounting the old rule's
                  * traffic to the new one? */
                 subrule->packet_count += old_sr->packet_count;
                 subrule->byte_count += old_sr->byte_count;
-                rule_destroy(old_sr);
+                rule_destroy(p, old_sr);
                 rule_make_actions(p, subrule, packet);
             }
         } else {
@@ -2897,12 +2900,10 @@ revalidate_rule(struct ofproto *p, struct rule *rule)
         if (!super) {
             rule_uninstall(p, rule);
             classifier_remove(&p->cls, &rule->cr);
-            rule_destroy(rule);
+            rule_destroy(p, rule);
             return false;
         } else if (super != rule->super) {
-            if (rule->super != UNKNOWN_SUPER) {
-                list_remove(&rule->list);
-            }
+            list_remove(&rule->list);
             list_push_back(&super->list, &rule->list);
             rule->super = super;
             rule->hard_timeout = super->hard_timeout;
@@ -2970,7 +2971,7 @@ uninstall_idle_flow(struct ofproto *ofproto, struct rule *rule)
     rule_uninstall(ofproto, rule);
     if (rule->super) {
         classifier_remove(&ofproto->cls, &rule->cr);
-        rule_destroy(rule);
+        rule_destroy(ofproto, rule);
     }
 }
 
@@ -3014,7 +3015,7 @@ expire_rule(struct cls_rule *cls_rule, void *p_)
                                 struct rule, list, &rule->list) {
                 rule_uninstall(p, subrule);
                 classifier_remove(&p->cls, &subrule->cr);
-                rule_destroy(subrule);
+                rule_destroy(p, subrule);
             }
         } else {
             rule_uninstall(p, rule);
@@ -3025,7 +3026,7 @@ expire_rule(struct cls_rule *cls_rule, void *p_)
                   (now >= hard_expire
                    ? OFPER_HARD_TIMEOUT : OFPER_IDLE_TIMEOUT));
     classifier_remove(&p->cls, &rule->cr);
-    rule_destroy(rule);
+    rule_destroy(p, rule);
 }
 
 static void