From b3cb2fbdf99cd9ad8f0bc44ff86cbc5b1ea81578 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 17 Mar 2009 10:26:47 -0700 Subject: [PATCH] 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. --- include/openflow/openflow.h | 3 +- lib/odp-util.c | 24 +++--------- lib/odp-util.h | 25 ++++++++++--- secchan/ofproto.c | 73 +++++++++++++++++++++++-------------- 4 files changed, 72 insertions(+), 53 deletions(-) diff --git a/include/openflow/openflow.h b/include/openflow/openflow.h index 5d48f791..92a05a2f 100644 --- 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 4fd4e0a7..63b3da16 100644 --- 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 8604fbc9..16b85cd2 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -34,20 +34,35 @@ #ifndef ODP_UTIL_H #define ODP_UTIL_H 1 +#include #include #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 8c62115b..682071e0 100644 --- 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); } @@ -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; } -- 2.30.2