secchan: Avoid dynamic allocation in xlate_actions().

The ofproto code currently does malloc()/realloc()/free() whenever it needs
to translate OpenFlow actions into datapath actions.  This is more or less
OK as-is, but the next commits will start keeping the datapath actions as
part of the rule.  That will require either wasting memory (because we
e.g. double the size of the malloc()'d buffer each realloc()) or making
a new malloc()'d copy of already malloc()'d memory.  Both solutions seem
wasteful.

So this commit instead prepares by keeping the ODP actions on-stack while
accumulating them, with a fixed maximum size that is the same maximum
used by the kernel datapath anyhow.

diff --git a/include/openflow/openflow.h b/include/openflow/openflow.h
index 5d48f7911563353c1051bf2fe530747a80b3edc5..92a05a2f7b5973b3d76865b7b561bf5d36a15915 100644 (file)
--- a/include/openflow/openflow.h
+++ b/include/openflow/openflow.h
@@ -616,7 +616,8 @@ enum ofp_bad_action_code {
     OFPBAC_BAD_VENDOR,         /* Unknown vendor id specified. */
     OFPBAC_BAD_VENDOR_TYPE,    /* Unknown action type for vendor id. */
     OFPBAC_BAD_OUT_PORT,       /* Problem validating output action. */
-    OFPBAC_BAD_ARGUMENT        /* Bad action argument. */
+    OFPBAC_BAD_ARGUMENT,       /* Bad action argument. */
+    OFPBAC_TOO_MANY            /* Can't handle this many actions. */
 };
 
 /* ofp_error_msg 'code' values for OFPET_FLOW_MOD_FAILED.  'data' contains 

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 4fd4e0a7a535ab151de3dc4790a3eae36c319c77..63b3da1639463f34c5748f53385db03dcea9a4f3 100644 (file)
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -42,30 +42,16 @@
 #include "timeval.h"
 #include "util.h"
 
-void
-odp_actions_init(struct odp_actions *actions)
-{
-    actions->actions = NULL;
-    actions->n_actions = 0;
-    actions->allocated_actions = 0;
-}
-
-void
-odp_actions_free(struct odp_actions *actions)
-{
-    free(actions->actions);
-}
-
 union odp_action *
 odp_actions_add(struct odp_actions *actions, uint16_t type)
 {
     union odp_action *a;
-    if (actions->n_actions >= actions->allocated_actions) {
-        actions->actions = x2nrealloc(actions->actions,
-                                      &actions->allocated_actions,
-                                      sizeof *actions->actions);
+    if (actions->n_actions < MAX_ODP_ACTIONS) {
+        a = &actions->actions[actions->n_actions++];
+    } else {
+        actions->n_actions = MAX_ODP_ACTIONS + 1;
+        a = &actions->actions[MAX_ODP_ACTIONS - 1];
     }
-    a = &actions->actions[actions->n_actions++];
     memset(a, 0, sizeof *a);
     a->type = type;
     return a;

diff --git a/lib/odp-util.h b/lib/odp-util.h
index 8604fbc9cba22daa48f45bf85a16fb63e5da7b23..16b85cd271f53eaf8e36654d035bb9316bafa426 100644 (file)
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -34,20 +34,35 @@
 #ifndef ODP_UTIL_H
 #define ODP_UTIL_H 1
 
+#include <stdbool.h>
 #include <stdint.h>
 #include "openflow/datapath-protocol.h"
 #include "openflow/openflow.h"
 
 struct ds;
 
+/* The kernel datapaths limits actions to those that fit in a single page of
+ * memory, so there is no point in allocating more than that.  */
+enum { MAX_ODP_ACTIONS = 4096 / sizeof(union odp_action) };
+
 struct odp_actions {
-    union odp_action *actions;
-    size_t n_actions, allocated_actions;
+    size_t n_actions;
+    union odp_action actions[MAX_ODP_ACTIONS];
 };
 
-void odp_actions_init(struct odp_actions *);
-void odp_actions_free(struct odp_actions *);
-union odp_action *odp_actions_add(struct odp_actions *, uint16_t type);
+static inline void
+odp_actions_init(struct odp_actions *actions)
+{
+    actions->n_actions = 0;
+}
+
+union odp_action *odp_actions_add(struct odp_actions *actions, uint16_t type);
+
+static inline bool
+odp_actions_overflow(const struct odp_actions *actions)
+{
+    return actions->n_actions > MAX_ODP_ACTIONS;
+}
 
 static inline uint16_t
 ofp_port_to_odp_port(uint16_t ofp_port)

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 8c62115bcdfdaf0d6026ca9b6a2d68e9d0a8cf0d..682071e0f6d51621c32e0e7d606b640c400046d5 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -90,10 +90,10 @@ struct ofport {
 static void ofport_free(struct ofport *);
 static void hton_ofp_phy_port(struct ofp_phy_port *);
 
-static void xlate_actions(const union ofp_action *in, size_t n_in,
-                          const flow_t *flow, struct ofproto *ofproto,
-                          bool revalidating,
-                          struct odp_actions *out, tag_type *tags);
+static int xlate_actions(const union ofp_action *in, size_t n_in,
+                         const flow_t *flow, struct ofproto *ofproto,
+                         bool revalidating,
+                         struct odp_actions *out, tag_type *tags);
 
 #define UNKNOWN_SUPER ((struct rule *)-1)
 struct rule {
@@ -878,11 +878,17 @@ ofproto_send_packet(struct ofproto *p, const flow_t *flow,
     struct odp_actions odp_actions;
     int error;
 
-    xlate_actions(actions, n_actions, flow, p, false, &odp_actions, NULL);
-    error = dpif_execute(&p->dpif, flow->in_port, odp_actions.actions,
-                         odp_actions.n_actions, packet);
-    odp_actions_free(&odp_actions);
-    return error;
+    error = xlate_actions(actions, n_actions, flow, p, false, &odp_actions,
+                          NULL);
+    if (error) {
+        return error;
+    }
+
+    /* XXX Should we translate the dpif_execute() errno value into an OpenFlow
+     * error code? */
+    dpif_execute(&p->dpif, flow->in_port, odp_actions.actions,
+                 odp_actions.n_actions, packet);
+    return 0;
 }
 
 void
@@ -930,7 +936,6 @@ ofproto_add_flow(struct ofproto *p,
         odp_flow.actions = odp_actions.actions;
         odp_flow.n_actions = odp_actions.n_actions;
         dpif_flow_add(&p->dpif, &odp_flow);
-        odp_actions_free(&odp_actions);
     } else {
         assert(!packet);
     }
@@ -1701,7 +1706,7 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
     }
 }
 
-static void
+static int
 xlate_actions(const union ofp_action *in, size_t n_in,
               const flow_t *flow, struct ofproto *ofproto, bool revalidating,
               struct odp_actions *out, tag_type *tags)
@@ -1716,6 +1721,11 @@ xlate_actions(const union ofp_action *in, size_t n_in,
     ctx.out = out;
     ctx.tags = tags ? tags : &no_tags;
     do_xlate_actions(in, n_in, &ctx);
+    if (odp_actions_overflow(out)) {
+        odp_actions_init(out);
+        return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_TOO_MANY);
+    }
+    return 0;
 }
 
 static int
@@ -1748,11 +1758,14 @@ handle_packet_out(struct ofproto *p, struct ofconn *ofconn,
     }
 
     flow_extract(&payload, ofp_port_to_odp_port(ntohs(opo->in_port)), &flow);
-    xlate_actions((const union ofp_action *) opo->actions, n_actions, &flow,
-                  p, false, &actions, NULL);
+    error = xlate_actions((const union ofp_action *) opo->actions, n_actions,
+                          &flow, p, false, &actions, NULL);
+    if (error) {
+        return error;
+    }
+
     dpif_execute(&p->dpif, flow.in_port, actions.actions, actions.n_actions,
                  &payload);
-    odp_actions_free(&actions);
     ofpbuf_delete(buffer);
 
     return 0;
@@ -2223,14 +2236,17 @@ send_buffered(struct ofproto *p, struct ofconn *ofconn, uint32_t buffer_id,
     }
 
     flow_extract(packet, in_port, &flow);
-    xlate_actions(rule->actions, rule->n_actions, &flow, p, false,
-                  &actions, NULL);
+    error = xlate_actions(rule->actions, rule->n_actions, &flow, p, false,
+                          &actions, NULL);
+    if (error) {
+        return error;
+    }
+
     error = dpif_execute(&p->dpif, in_port,
                          actions.actions, actions.n_actions, packet);
     if (!error) {
         *byte_count = packet->size;
     }
-    odp_actions_free(&actions);
     ofpbuf_delete(packet);
 
     return 0;
@@ -2241,7 +2257,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
          struct ofp_flow_mod *ofm, size_t n_actions)
 {
     struct rule *rule, *displaced_rule;
-    int buffer_error = 0;
+    int error = 0;
 
     rule = xmalloc(sizeof *rule);
     cls_rule_from_match(&rule->cr, &ofm->match, ntohs(ofm->priority));
@@ -2260,8 +2276,8 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
 
     if (ofm->buffer_id != htonl(UINT32_MAX)) {
         int byte_count = 0;
-        buffer_error = send_buffered(p, ofconn, ntohl(ofm->buffer_id),
-                                     rule, &byte_count);
+        error = send_buffered(p, ofconn, ntohl(ofm->buffer_id),
+                              rule, &byte_count);
         rule->byte_count += byte_count;
         rule->packet_count += byte_count > 0;
     }
@@ -2289,9 +2305,14 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
     } else {
         struct odp_flow odp_flow;
         struct odp_actions actions;
+        int retval;
 
-        xlate_actions((const union ofp_action *) ofm->actions, n_actions,
-                      &rule->cr.flow, p, false, &actions, NULL);
+        retval = xlate_actions((const union ofp_action *) ofm->actions,
+                               n_actions, &rule->cr.flow, p, false, &actions,
+                               NULL);
+        if (retval) {
+            error = retval;
+        }
 
         odp_flow.key = rule->cr.flow;
         odp_flow.actions = actions.actions;
@@ -2304,9 +2325,8 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
             }
             rule_destroy(displaced_rule);
         }
-        odp_actions_free(&actions);
     }
-    return buffer_error;
+    return error;
 }
 
 static int
@@ -2336,13 +2356,13 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
             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);
-            odp_actions_free(&actions);
 
             update_stats(rule, &odp_flow.stats);
         }
@@ -2706,7 +2726,6 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
                 rule_make_actions(p, old_sr, false, &actions);
                 dpif_execute(&p->dpif, msg->port,
                              actions.actions, actions.n_actions, &payload);
-                odp_actions_free(&actions);
                 ofpbuf_delete(packet);
                 return;
             } else {
@@ -2734,7 +2753,6 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
     /* Execute subrule on packet. */
     dpif_execute(&p->dpif, msg->port, actions.actions, actions.n_actions,
                  &payload);
-    odp_actions_free(&actions);
     ofpbuf_delete(packet);
 }
 \f
@@ -2782,7 +2800,6 @@ 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);
-    odp_actions_free(&actions);
     return true;
 }