From 3ef905e11a2448dbd9f7a493e6ac1071f827b875 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 2 May 2008 11:37:04 -0700 Subject: [PATCH] Remove MAX_ACTIONS (which was 16) as a limit on the number of actions in a flow table entry. 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. --- datapath/datapath.c | 86 +++++++++++++++++++++-------------------- datapath/flow.h | 3 -- datapath/forward.c | 8 +--- datapath/table-hash.c | 16 ++++++-- datapath/table-linear.c | 2 +- datapath/table.h | 5 ++- switch/datapath.c | 8 +--- switch/switch-flow.h | 3 -- 8 files changed, 63 insertions(+), 68 deletions(-) diff --git a/datapath/datapath.c b/datapath/datapath.c index 444b5e96..839dbe63 100644 --- 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 a3abc848..8534342d 100644 --- 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 6553c947..929703fb 100644 --- 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; iactions[i]; diff --git a/datapath/table-hash.c b/datapath/table-hash.c index 04b73013..9c8b532f 100644 --- 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 1df1d831..c965b497 100644 --- 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 b0681b6a..12543352 100644 --- 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 bef86fbd..a5e54cd6 100644 --- 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; iactions[i]; diff --git a/switch/switch-flow.h b/switch/switch-flow.h index 8de25bef..d5e85b41 100644 --- 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; -- 2.30.2