From: Ben Pfaff <blp@nicira.com>
Date: Tue, 17 Mar 2009 17:24:28 +0000 (-0700)
Subject: secchan: Track datapath actions in userspace, to avoid system calls.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5e52b645eaa51265b7fb4ed99699790f67a69f7a;p=openvswitch

secchan: Track datapath actions in userspace, to avoid system calls.

Until now, secchan had no way to determine when datapath actions actually
changed, and so it had to be conservative and update all flows' actions,
or almost all, in some circumstances.

With this commit, secchan keeps track of flows' actions in allocated memory
and only updates them in the datapath when they actually change.

As part of the change, this factors out common code into new functions
rule_install() and rule_uninstall(), which should make secchan more
maintainable.
---

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index bc765e5f..d27cc3bf 100644
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -123,9 +123,19 @@ struct rule {
     struct rule *super;
     struct list list;
 
-    /* A subrule has no actions (it uses the super-rule's actions). */
+    /* OpenFlow actions.
+     *
+     * A subrule has no actions (it uses the super-rule's actions). */
     int n_actions;
     union ofp_action *actions;
+
+    /* Datapath actions.
+     *
+     * A super-rule with wildcard fields never has ODP actions (since the
+     * datapath only supports exact-match flows). */
+    bool installed;             /* Installed in datapath? */
+    int n_odp_actions;
+    union odp_action *odp_actions;
 };
 
 static struct rule *rule_create(struct rule *super, const union ofp_action *,
@@ -134,8 +144,12 @@ static struct rule *rule_create(struct rule *super, const union ofp_action *,
 static void rule_free(struct rule *);
 static void rule_destroy(struct rule *);
 static struct rule *rule_from_cls_rule(const struct cls_rule *);
-static void rule_make_actions(struct ofproto *, struct rule *,
-                              bool revalidating, struct odp_actions *);
+static bool rule_make_actions(struct ofproto *, struct rule *,
+                              bool revalidating);
+static void rule_install(struct ofproto *, struct rule *,
+                         struct odp_flow_stats *);
+static void rule_uninstall(struct ofproto *, struct rule *,
+                           struct odp_flow_stats *);
 
 struct ofconn {
     struct list node;
@@ -901,8 +915,6 @@ ofproto_add_flow(struct ofproto *p,
                  const struct ofpbuf *packet, int idle_timeout)
 {
     struct rule *rule, *displaced_rule;
-    struct odp_actions odp_actions;
-    struct odp_flow odp_flow;
 
     rule = rule_create(NULL, actions, n_actions,
                        idle_timeout >= 0 ? idle_timeout : 5 /* XXX */, 0);
@@ -915,19 +927,14 @@ ofproto_add_flow(struct ofproto *p,
     }
 
     if (!wildcards) {
-        rule_make_actions(p, rule, false, &odp_actions);
-        if (packet) {
-            if (!ofproto_send_packet(p, flow, actions, n_actions, packet)) {
-                rule->byte_count = packet->size;
-                rule->packet_count++;
-            }
+        rule_make_actions(p, rule, false);
+        if (packet
+            && !dpif_execute(&p->dpif, flow->in_port,
+                             rule->odp_actions, rule->n_odp_actions, packet)) {
+            rule->byte_count = packet->size;
+            rule->packet_count++;
         }
-
-        memset(&odp_flow.stats, 0, sizeof odp_flow.stats);
-        odp_flow.key = *flow;
-        odp_flow.actions = odp_actions.actions;
-        odp_flow.n_actions = odp_actions.n_actions;
-        dpif_flow_add(&p->dpif, &odp_flow);
+        rule_install(p, rule, NULL);
     } else {
         assert(!packet);
     }
@@ -1338,16 +1345,71 @@ rule_has_out_port(const struct rule *rule, uint16_t out_port)
     return false;
 }
 
-/* The caller must ensure that rule->super != UNKNOWN_SUPER. */
-static void
-rule_make_actions(struct ofproto *p, struct rule *rule, bool revalidating,
-                  struct odp_actions *actions)
+/* Returns true if the actions changed, false otherwise. */
+static bool
+rule_make_actions(struct ofproto *p, struct rule *rule, bool revalidating)
 {
-    const struct rule *super = rule->super ? rule->super : rule;
+    const struct rule *super;
+    struct odp_actions a;
+    size_t actions_len;
+
+    assert(rule->super != UNKNOWN_SUPER);
     assert(!rule->cr.wc.wildcards);
+
+    super = rule->super ? rule->super : rule;
     rule->tags = 0;
     xlate_actions(super->actions, super->n_actions, &rule->cr.flow, p,
-                  revalidating, actions, &rule->tags);
+                  revalidating, &a, &rule->tags);
+
+    actions_len = a.n_actions * sizeof *a.actions;
+    if (rule->n_odp_actions != a.n_actions
+        || memcmp(rule->odp_actions, a.actions, actions_len)) {
+        free(rule->odp_actions);
+        rule->n_odp_actions = a.n_actions;
+        rule->odp_actions = xmemdup(a.actions, actions_len);
+        return true;
+    } else {
+        return false;
+    }
+}
+
+static void
+rule_install(struct ofproto *p, struct rule *rule,
+             struct odp_flow_stats *stats)
+{
+    struct odp_flow odp_flow;
+
+    assert(!rule->cr.wc.wildcards);
+
+    odp_flow.key = rule->cr.flow;
+    odp_flow.actions = rule->odp_actions;
+    odp_flow.n_actions = rule->n_odp_actions;
+    if (!dpif_flow_add(&p->dpif, &odp_flow)) {
+        rule->installed = true;
+        if (stats) {
+            *stats = odp_flow.stats;
+        }
+    }
+}
+
+static void
+rule_uninstall(struct ofproto *p, struct rule *rule,
+               struct odp_flow_stats *stats)
+{
+    assert(!rule->cr.wc.wildcards);
+    if (stats) {
+        memset(stats, 0, sizeof *stats);
+    }
+    if (rule->installed) {
+        struct odp_flow odp_flow;
+        odp_flow.key = rule->cr.flow;
+        odp_flow.actions = NULL;
+        odp_flow.n_actions = 0;
+        if (!dpif_flow_del(&p->dpif, &odp_flow) && stats) {
+            *stats = odp_flow.stats;
+        }
+        rule->installed = false;
+    }
 }
 
 static void
@@ -2307,25 +2369,14 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
         /* We might need to change the rules for arbitrary subrules. */
         p->need_revalidate = true;
     } else {
-        struct odp_flow odp_flow;
-        struct odp_actions actions;
-        int retval;
-
-        retval = xlate_actions((const union ofp_action *) ofm->actions,
-                               n_actions, &rule->cr.flow, p, false, &actions,
-                               NULL);
-        if (retval) {
-            error = retval;
-        }
+        struct odp_flow_stats stats;
 
-        odp_flow.key = rule->cr.flow;
-        odp_flow.actions = actions.actions;
-        odp_flow.n_actions = actions.n_actions;
-        dpif_flow_add(&p->dpif, &odp_flow);
+        rule_make_actions(p, rule, false);
+        rule_install(p, rule, &stats);
         if (displaced_rule) {
             if (displaced_rule->super &&
                 displaced_rule->super != UNKNOWN_SUPER) {
-                update_stats(displaced_rule->super, &odp_flow.stats);
+                update_stats(displaced_rule->super, &stats);
             }
             rule_destroy(displaced_rule);
         }
@@ -2347,33 +2398,21 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
     }
     if (command == OFPFC_DELETE) {
         if (!rule->cr.wc.wildcards) {
-            struct odp_flow odp_flow;
-            flow_from_match(&odp_flow.key, NULL, &ofm->match);
-            odp_flow.actions = NULL;
-            odp_flow.n_actions = 0;
-            dpif_flow_del(&p->dpif, &odp_flow);
+            rule_uninstall(p, rule, NULL);
         }
         classifier_remove(&p->cls, &rule->cr);
         rule_destroy(rule);
     } else {
-        if (!rule->cr.wc.wildcards) {
-            struct odp_flow odp_flow;
-            struct odp_actions actions;
-
-            /* XXX We should check the return value here. */
-            xlate_actions((const union ofp_action *) ofm->actions, n_actions,
-                          &rule->cr.flow, p, false, &actions, NULL);
-            odp_flow.key = rule->cr.flow;
-            odp_flow.actions = actions.actions;
-            odp_flow.n_actions = actions.n_actions;
-            dpif_flow_add(&p->dpif, &odp_flow);
-
-            update_stats(rule, &odp_flow.stats);
-        }
         free(rule->actions);
         rule->actions = xmemdup(ofm->actions,
                                 n_actions * sizeof *rule->actions);
         rule->n_actions = n_actions;
+        if (!rule->cr.wc.wildcards) {
+            if (rule_make_actions(p, rule, false)) {
+                dpif_flow_set_actions(&p->dpif, &rule->cr.flow,
+                                      rule->odp_actions, rule->n_odp_actions);
+            }
+        }
     }
 
     return 0;
@@ -2649,7 +2688,6 @@ 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 odp_actions actions;
     struct ofpbuf payload;
     flow_t flow;
 
@@ -2704,7 +2742,6 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
 
     if (rule->cr.wc.wildcards) {
         struct rule *old_sr;
-        struct odp_flow odp_flow;
 
         subrule = rule_create(rule, NULL, 0,
                               rule->idle_timeout, rule->hard_timeout);
@@ -2718,9 +2755,13 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
                 rule_destroy(subrule);
 
                 /* Execute old_sr on packet. */
-                rule_make_actions(p, old_sr, false, &actions);
-                dpif_execute(&p->dpif, msg->port,
-                             actions.actions, actions.n_actions, &payload);
+                rule_install(p, old_sr, NULL);
+                if (!dpif_execute(&p->dpif, msg->port,
+                                  old_sr->odp_actions, old_sr->n_odp_actions,
+                                  &payload)) {
+                    old_sr->packet_count++;
+                    old_sr->byte_count += payload.size;
+                }
                 ofpbuf_delete(packet);
                 return;
             } else {
@@ -2731,21 +2772,17 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
         }
 
         /* Install flow entry into datapath. */
-        rule_make_actions(p, subrule, false, &actions);
-        odp_flow.key = flow;
-        odp_flow.actions = actions.actions;
-        odp_flow.n_actions = actions.n_actions;
-        dpif_flow_add(&p->dpif, &odp_flow);
+        rule_make_actions(p, subrule, false);
+        rule_install(p, subrule, NULL);
     } else {
         /* A flow got dropped due to a hash collision, or the packet was
          * buffered before we processed another packet from the same flow. */
         subrule = rule;
-        rule_make_actions(p, subrule, false, &actions);
     }
 
     /* Execute subrule on packet. */
-    dpif_execute(&p->dpif, msg->port, actions.actions, actions.n_actions,
-                 &payload);
+    dpif_execute(&p->dpif, msg->port, subrule->odp_actions,
+                 subrule->n_odp_actions, &payload);
     ofpbuf_delete(packet);
 }
 
@@ -2766,18 +2803,12 @@ static bool
 revalidate_rule(struct ofproto *p, struct rule *rule)
 {
     const flow_t *flow = &rule->cr.flow;
-    struct odp_actions actions;
 
     if (rule->super) {
         struct rule *super;
         super = rule_from_cls_rule(classifier_lookup_wild(&p->cls, flow));
         if (!super) {
-            struct odp_flow odp_flow;
-            memset(&odp_flow.stats, 0, sizeof odp_flow.stats);
-            odp_flow.key = rule->cr.flow;
-            odp_flow.actions = NULL;
-            odp_flow.n_actions = 0;
-            dpif_flow_del(&p->dpif, &odp_flow);
+            rule_uninstall(p, rule, NULL);
             classifier_remove(&p->cls, &rule->cr);
             rule_destroy(rule);
             return false;
@@ -2790,9 +2821,10 @@ revalidate_rule(struct ofproto *p, struct rule *rule)
         }
     }
 
-    rule_make_actions(p, rule, true, &actions);
-    dpif_flow_set_actions(&p->dpif, flow, actions.actions,
-                          actions.n_actions);
+    if (rule_make_actions(p, rule, true)) {
+        dpif_flow_set_actions(&p->dpif, flow, rule->odp_actions,
+                              rule->n_odp_actions);
+    }
     return true;
 }
 
@@ -2890,23 +2922,14 @@ expire_rule(struct cls_rule *cls_rule, void *p_)
             struct rule *subrule, *next_subrule;
             LIST_FOR_EACH_SAFE (subrule, next_subrule,
                                 struct rule, list, &rule->list) {
-                struct odp_flow odp_flow;
-                odp_flow.key = subrule->cr.flow;
-                odp_flow.actions = NULL;
-                odp_flow.n_actions = 0;
-                if (!dpif_flow_del(&p->dpif, &odp_flow)) {
-                    update_stats(rule, &odp_flow.stats);
-                }
+                struct odp_flow_stats stats;
+                rule_uninstall(p, subrule, &stats);
+                update_stats(rule, &stats);
                 classifier_remove(&p->cls, &subrule->cr);
                 rule_destroy(subrule);
             }
         } else {
-            struct odp_flow odp_flow;
-            memset(&odp_flow, 0, sizeof odp_flow);
-            odp_flow.key = rule->cr.flow;
-            odp_flow.actions = NULL;
-            odp_flow.n_actions = 0;
-            dpif_flow_del(&p->dpif, &odp_flow);
+            rule_uninstall(p, rule, NULL);
         }
     }