Remove MAX_ACTIONS (which was 16) as a limit on the number of actions in a flow table...

Now the number of actions is limited, as a practical matter, to the size
of the buffer that the kernel provides for Netlink dumps, which is usually
4096 bytes.  A flow statistics entry must fit in a single one of these
buffers.  Actions are 8 bytes each, so this is about 500 actions
considering overhead.

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 444b5e96aa01e73bebad5356664643b09cc61dbe..839dbe637f86d72e5d5bb7eac526505356e83b11 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -838,36 +838,6 @@ dp_send_error_msg(struct datapath *dp, const struct sender *sender,
        return send_openflow_skb(skb, sender);
 }
 
-static int
-fill_flow_stats(struct ofp_flow_stats *ofs, struct sw_flow *flow,
-               int table_idx)
-{
-       int length = sizeof *ofs + sizeof *ofs->actions * flow->n_actions;
-       ofs->length          = htons(length);
-       ofs->table_id        = table_idx;
-       ofs->pad             = 0;
-       ofs->match.wildcards = htons(flow->key.wildcards);
-       ofs->match.in_port   = flow->key.in_port;
-       memcpy(ofs->match.dl_src, flow->key.dl_src, ETH_ALEN);
-       memcpy(ofs->match.dl_dst, flow->key.dl_dst, ETH_ALEN);
-       ofs->match.dl_vlan   = flow->key.dl_vlan;
-       ofs->match.dl_type   = flow->key.dl_type;
-       ofs->match.nw_src    = flow->key.nw_src;
-       ofs->match.nw_dst    = flow->key.nw_dst;
-       ofs->match.nw_proto  = flow->key.nw_proto;
-       memset(ofs->match.pad, 0, sizeof ofs->match.pad);
-       ofs->match.tp_src    = flow->key.tp_src;
-       ofs->match.tp_dst    = flow->key.tp_dst;
-       ofs->duration        = htonl((jiffies - flow->init_time) / HZ);
-       ofs->packet_count    = cpu_to_be64(flow->packet_count);
-       ofs->byte_count      = cpu_to_be64(flow->byte_count);
-       ofs->priority        = htons(flow->priority);
-       ofs->max_idle        = htons(flow->max_idle);
-       memcpy(ofs->actions, flow->actions,
-              sizeof *ofs->actions * flow->n_actions);
-       return length;
-}
-
 /* Generic Netlink interface.
  *
  * See netlink(7) for an introduction to netlink.  See
@@ -1109,9 +1079,6 @@ struct flow_stats_state {
        int bytes_used, bytes_allocated;
 };
 
-#define MAX_FLOW_STATS_SIZE (sizeof(struct ofp_flow_stats) \
-                            + MAX_ACTIONS * sizeof(struct ofp_action))
-
 static int flow_stats_init(struct datapath *dp, const void *body, int body_len,
                           void **state)
 {
@@ -1129,9 +1096,40 @@ static int flow_stats_init(struct datapath *dp, const void *body, int body_len,
 static int flow_stats_dump_callback(struct sw_flow *flow, void *private)
 {
        struct flow_stats_state *s = private;
-       s->bytes_used += fill_flow_stats(s->body + s->bytes_used, flow,
-                                        s->table_idx);
-       return s->bytes_used + MAX_FLOW_STATS_SIZE > s->bytes_allocated;
+       struct ofp_flow_stats *ofs;
+       int actions_length;
+       int length;
+
+       actions_length = sizeof *ofs->actions * flow->n_actions;
+       length = sizeof *ofs + sizeof *ofs->actions * flow->n_actions;
+       if (length + s->bytes_used > s->bytes_allocated)
+               return 1;
+
+       ofs = s->body + s->bytes_used;
+       ofs->length          = htons(length);
+       ofs->table_id        = s->table_idx;
+       ofs->pad             = 0;
+       ofs->match.wildcards = htons(flow->key.wildcards);
+       ofs->match.in_port   = flow->key.in_port;
+       memcpy(ofs->match.dl_src, flow->key.dl_src, ETH_ALEN);
+       memcpy(ofs->match.dl_dst, flow->key.dl_dst, ETH_ALEN);
+       ofs->match.dl_vlan   = flow->key.dl_vlan;
+       ofs->match.dl_type   = flow->key.dl_type;
+       ofs->match.nw_src    = flow->key.nw_src;
+       ofs->match.nw_dst    = flow->key.nw_dst;
+       ofs->match.nw_proto  = flow->key.nw_proto;
+       memset(ofs->match.pad, 0, sizeof ofs->match.pad);
+       ofs->match.tp_src    = flow->key.tp_src;
+       ofs->match.tp_dst    = flow->key.tp_dst;
+       ofs->duration        = htonl((jiffies - flow->init_time) / HZ);
+       ofs->packet_count    = cpu_to_be64(flow->packet_count);
+       ofs->byte_count      = cpu_to_be64(flow->byte_count);
+       ofs->priority        = htons(flow->priority);
+       ofs->max_idle        = htons(flow->max_idle);
+       memcpy(ofs->actions, flow->actions, actions_length);
+
+       s->bytes_used += length;
+       return 0;
 }
 
 static int flow_stats_dump(struct datapath *dp, void *state,
@@ -1139,11 +1137,10 @@ static int flow_stats_dump(struct datapath *dp, void *state,
 {
        struct flow_stats_state *s = state;
        struct sw_flow_key match_key;
+       int error = 0;
 
        s->bytes_used = 0;
        s->bytes_allocated = *body_len;
-       if (s->bytes_allocated < MAX_FLOW_STATS_SIZE)
-               return -ENOMEM;
        s->body = body;
 
        flow_extract_match(&match_key, &s->rq->match);
@@ -1152,15 +1149,22 @@ static int flow_stats_dump(struct datapath *dp, void *state,
        {
                struct sw_table *table = dp->chain->tables[s->table_idx];
 
-               if (table->iterate(table, &match_key, &s->position,
-                                  flow_stats_dump_callback, s))
+               error = table->iterate(table, &match_key, &s->position,
+                                      flow_stats_dump_callback, s);
+               if (error)
                        break;
 
                s->table_idx++;
                memset(&s->position, 0, sizeof s->position);
        }
        *body_len = s->bytes_used;
-       return s->bytes_used + MAX_FLOW_STATS_SIZE > s->bytes_allocated;
+
+       /* If error is 0, we're done.
+        * Otherwise, if some bytes were used, there are more flows to come.
+        * Otherwise, we were not able to fit even a single flow in the body,
+        * which indicates that we have a single flow with too many actions to
+        * fit.  We won't ever make any progress at that rate, so give up. */
+       return !error ? 0 : s->bytes_used ? 1 : -ENOMEM;
 }
 
 static void flow_stats_done(void *state)

diff --git a/datapath/flow.h b/datapath/flow.h
index a3abc848914719925901bf999e37ce8478ac2d9e..8534342d9c0c2e5b4a6b51d97c87d9a60f381f02 100644 (file)
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -46,9 +46,6 @@ static inline void check_key_align(void)
        BUILD_BUG_ON(sizeof(struct sw_flow_key) != 36); 
 }
 
-/* Maximum number of actions in a single flow entry. */
-#define MAX_ACTIONS 16
-
 /* Locking:
  *
  * - Readers must take rcu_read_lock and hold it the entire time that the flow

diff --git a/datapath/forward.c b/datapath/forward.c
index 6553c947b7d348196f6d850ff80d3fa5d49b2543..929703fbbc4a48aea06e8f6914a9fbe2d9b0cab6 100644 (file)
--- a/datapath/forward.c
+++ b/datapath/forward.c
@@ -351,16 +351,10 @@ add_flow(struct sw_chain *chain, const struct ofp_flow_mod *ofm)
        struct sw_flow *flow;
 
 
-       /* Check number of actions. */
-       n_acts = (ntohs(ofm->header.length) - sizeof *ofm) / sizeof *ofm->actions;
-       if (n_acts > MAX_ACTIONS) {
-               error = -E2BIG;
-               goto error;
-       }
-
        /* To prevent loops, make sure there's no action to send to the
         * OFP_TABLE virtual port.
         */
+       n_acts = (ntohs(ofm->header.length) - sizeof *ofm) / sizeof *ofm->actions;
        for (i=0; i<n_acts; i++) {
                const struct ofp_action *a = &ofm->actions[i];
 

diff --git a/datapath/table-hash.c b/datapath/table-hash.c
index 04b73013f64d076d010e03d58e32d22eea0277af..9c8b532f217875a925feda9fad5452e373f732ac 100644 (file)
--- a/datapath/table-hash.c
+++ b/datapath/table-hash.c
@@ -159,9 +159,17 @@ static int table_hash_iterate(struct sw_table *swt,
                return 0;
 
        if (key->wildcards == 0) {
-               struct sw_flow *flow = table_hash_lookup(swt, key);
-               position->private[0] = -1;
-               return flow ? callback(flow, private) : 0;
+               struct sw_flow *flow;
+               int error;
+
+               flow = table_hash_lookup(swt, key);
+               if (!flow)
+                       return 0;
+
+               error = callback(flow, private);
+               if (!error)
+                       position->private[0] = -1;
+               return error;
        } else {
                int i;
 
@@ -170,7 +178,7 @@ static int table_hash_iterate(struct sw_table *swt,
                        if (flow && flow_matches(key, &flow->key)) {
                                int error = callback(flow, private);
                                if (error) {
-                                       position->private[0] = i + 1;
+                                       position->private[0] = i;
                                        return error;
                                }
                        }

diff --git a/datapath/table-linear.c b/datapath/table-linear.c
index 1df1d831de8d9d8665efe83e3456339ef0dd72de..c965b49724804b9a28b4c8f0f2004110107b2f3c 100644 (file)
--- a/datapath/table-linear.c
+++ b/datapath/table-linear.c
@@ -154,7 +154,7 @@ static int table_linear_iterate(struct sw_table *swt,
                if (flow->serial <= start && flow_matches(key, &flow->key)) {
                        int error = callback(flow, private);
                        if (error) {
-                               position->private[0] = ~(flow->serial - 1);
+                               position->private[0] = ~flow->serial;
                                return error;
                        }
                }

diff --git a/datapath/table.h b/datapath/table.h
index b0681b6a5ded24ed81df002e77d7c8d2ed62b1e3..12543352b80549eb0545a9c3372bb9dbfa1a3ad8 100644 (file)
--- a/datapath/table.h
+++ b/datapath/table.h
@@ -69,8 +69,9 @@ struct sw_table {
         * all-zero-bits to iterate from the beginning of the table.  If the
         * iteration terminates due to an error from the callback function,
         * 'position' is updated to a value that can be passed back to the
-        * iterator function to resume iteration later with the following
-        * flow. */
+        * iterator function to continue iteration later from the same position
+        * that caused the error (assuming that that flow entry has not been
+        * deleted in the meantime). */
        int (*iterate)(struct sw_table *table,
                       const struct sw_flow_key *key,
                       struct sw_table_position *position,

diff --git a/switch/datapath.c b/switch/datapath.c
index bef86fbd8dba4e4cbf921ba635991a60ecbe8773..a5e54cd65a2b93afe777f79110acf3bd5fdc6704 100644 (file)
--- a/switch/datapath.c
+++ b/switch/datapath.c
@@ -1048,16 +1048,10 @@ add_flow(struct datapath *dp, const struct ofp_flow_mod *ofm)
     struct sw_flow *flow;
 
 
-    /* Check number of actions. */
-    n_acts = (ntohs(ofm->header.length) - sizeof *ofm) / sizeof *ofm->actions;
-    if (n_acts > MAX_ACTIONS) {
-        error = -E2BIG;
-        goto error;
-    }
-
     /* To prevent loops, make sure there's no action to send to the
      * OFP_TABLE virtual port.
      */
+    n_acts = (ntohs(ofm->header.length) - sizeof *ofm) / sizeof *ofm->actions;
     for (i=0; i<n_acts; i++) {
         const struct ofp_action *a = &ofm->actions[i];
 

diff --git a/switch/switch-flow.h b/switch/switch-flow.h
index 8de25bef97f8b3dbed7c7eff2e0ebab39f2b917a..d5e85b41ee83a97f1f57373938d0e34bc9e773c0 100644 (file)
--- a/switch/switch-flow.h
+++ b/switch/switch-flow.h
@@ -46,9 +46,6 @@ struct sw_flow_key {
     uint32_t wildcards;         /* Wildcard fields (in host byte order). */
 };
 
-/* Maximum number of actions in a single flow entry. */
-#define MAX_ACTIONS 16
-
 struct sw_flow {
     struct sw_flow_key key;