secchan: Clean up and simplify handle_odp_msg().

diff --git a/lib/classifier.c b/lib/classifier.c
index 2551662255ba24c9a41d659bffff361b10fe6dee..7ef4119fd4667fb9b941c7cd4ce99bee65b5f343 100644 (file)
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -233,6 +233,18 @@ classifier_insert(struct classifier *cls, struct cls_rule *rule)
     return old;
 }
 
+/* Inserts 'rule' into 'cls'.  Transfers ownership of 'rule' to 'cls'.
+ *
+ * 'rule' must be an exact-match rule (rule->wc.wildcards must be 0) and 'cls'
+ * must not contain any rule with an identical key. */
+void
+classifier_insert_exact(struct classifier *cls, struct cls_rule *rule)
+{
+    hmap_insert(&cls->exact_table, &rule->node.hmap,
+                flow_hash(&rule->flow, 0));
+    cls->n_rules++;
+}
+
 /* Removes 'rule' from 'cls'.  It is caller's responsibility to free 'rule', if
  * this is desirable. */
 void

diff --git a/lib/classifier.h b/lib/classifier.h
index dd2fb52502da23e30743c2b53a0a6a60df21dae5..4f547f8095437efb86fd6269b54ff5c4cbb25dfa 100644 (file)
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -138,6 +138,7 @@ bool classifier_is_empty(const struct classifier *);
 int classifier_count(const struct classifier *);
 int classifier_count_exact(const struct classifier *);
 struct cls_rule *classifier_insert(struct classifier *, struct cls_rule *);
+void classifier_insert_exact(struct classifier *, struct cls_rule *);
 void classifier_remove(struct classifier *, struct cls_rule *);
 struct cls_rule *classifier_lookup(const struct classifier *, const flow_t *);
 struct cls_rule *classifier_lookup_wild(const struct classifier *,

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 12b7bc12a85cc8cefc6400c66c5fbc7d28400d6c..6ea28c7f293f1fbfe89a95957d1a3d2edf2abbad 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -1430,6 +1430,20 @@ rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet)
     }
 }
 
+static struct rule *
+rule_create_subrule(struct ofproto *ofproto, struct rule *rule,
+                    const flow_t *flow)
+{
+    struct rule *subrule = rule_create(rule, NULL, 0,
+                                       rule->idle_timeout, rule->hard_timeout);
+    cls_rule_from_flow(&subrule->cr, flow, 0,
+                       (rule->cr.priority <= UINT16_MAX ? UINT16_MAX
+                        : rule->cr.priority));
+    classifier_insert_exact(&ofproto->cls, &subrule->cr);
+
+    return subrule;
+}
+
 static void
 rule_remove(struct ofproto *ofproto, struct rule *rule)
 {
@@ -2797,7 +2811,7 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
 {
     struct odp_msg *msg = packet->data;
     uint16_t in_port = odp_port_to_ofp_port(msg->port);
-    struct rule *rule, *subrule;
+    struct rule *rule;
     struct ofpbuf payload;
     flow_t flow;
 
@@ -2831,56 +2845,33 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
     }
 
     if (rule->cr.wc.wildcards) {
-        struct rule *old_sr;
-
-        subrule = rule_create(rule, NULL, 0,
-                              rule->idle_timeout, rule->hard_timeout);
-        cls_rule_from_flow(&subrule->cr, &flow, 0,
-                           (rule->cr.priority <= UINT16_MAX ? UINT16_MAX
-                            : rule->cr.priority));
-
-        /* Insert 'subrule' into the classifier and compose actions for it. */
-        old_sr = rule_from_cls_rule(classifier_insert(&p->cls, &subrule->cr));
-        if (old_sr) {
-            if (!old_sr->super) {
-                /* 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(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(p, old_sr);
-                rule_make_actions(p, subrule, packet);
-            }
-        } else {
-            rule_make_actions(p, subrule, packet);
-        }
-
-        /* Install flow entry into datapath. */
-        rule_install(p, subrule, NULL);
+        rule = rule_create_subrule(p, rule, &flow);
+        rule_make_actions(p, rule, packet);
     } else {
-        subrule = rule;
         if (!rule->may_install) {
             /* The rule is not installable, that is, we need to process every
-             * packet, so do that here. */
-            rule_make_actions(p, subrule, packet);
-            rule_install(p, subrule, NULL);
+             * packet, so process the current packet and set its actions into
+             * 'subrule'. */
+            rule_make_actions(p, rule, packet);
         } else {
-            /* A flow got dropped due to a hash collision, or the packet was
-             * buffered before we processed another packet from the same
-             * flow. */
+            /* XXX revalidate rule if it needs it */
         }
     }
 
-    /* Execute subrule on packet. */
-    if (!dpif_execute(&p->dpif, msg->port, subrule->odp_actions,
-                      subrule->n_odp_actions, &payload)) {
-        subrule->packet_count++;
-        subrule->byte_count += payload.size;
+    /* Invariant: 'rule' is an exact-match rule. */
+
+    /* XXX should forcibly install the rule here, since it's possible that it
+     * got deleted externally (e.g. "dpctl dp-del-flows") and unless we
+     * re-install it here we'll end up processing every packet by hand.  But
+     * the datapath doesn't provide a good interface for that. */
+    rule_install(p, rule, NULL);
+
+    /* Execute rule on packet. */
+    if (!dpif_execute(&p->dpif, msg->port, rule->odp_actions,
+                      rule->n_odp_actions, &payload)) {
+        /* XXX use flow_extract_stats(). */
+        rule->packet_count++;
+        rule->byte_count += payload.size;
     }
     ofpbuf_delete(packet);
 }

diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index b281134e8e403546f08005c817b392b64372b5b8..a47e83806069220b0c4467d9d5ce5088e9fff3d0 100644 (file)
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -520,7 +520,11 @@ test_single_rule(void)
         tcls_init(&tcls);
 
         tcls_rule = tcls_insert(&tcls, rule);
-        assert(!classifier_insert(&cls, &rule->cls_rule));
+        if (wc_fields) {
+            assert(!classifier_insert(&cls, &rule->cls_rule));
+        } else {
+            classifier_insert_exact(&cls, &rule->cls_rule);
+        }
         check_tables(&cls, 1, 1, 1);
         compare_classifiers(&cls, &tcls);