[ovs-dev] [xlate 1.11 5/9] ofproto: Ditch SLOW_IN_BAND slow path reason.
Ethan Jackson
ethan at nicira.com
Wed May 29 21:24:37 UTC 2013
Before this patch, when in band control was enabled, every DHCP
packet had to be sent to userspace to calculate it's actions.
Those DHCP packets intended for the local port would have a special
action added to ensure they actually make it there. This
unnecessarily complicates the code, so this patch takes a slightly
different approach. When in-band is enabled, *all* DHCP packets
must be sent to the local port. This guarantees that
xlate_actions() returns the same result every time for a given
flow.
Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
lib/odp-util.c | 2 --
lib/odp-util.h | 6 +---
ofproto/connmgr.c | 15 +++------
ofproto/connmgr.h | 10 +++---
ofproto/in-band.c | 31 ------------------
ofproto/in-band.h | 2 --
ofproto/ofproto-dpif.c | 81 +++++++++++++++++-------------------------------
7 files changed, 37 insertions(+), 110 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 42f4828..1fba981 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -181,8 +181,6 @@ slow_path_reason_to_string(uint32_t data)
return "lacp";
case SLOW_STP:
return "stp";
- case SLOW_IN_BAND:
- return "in_band";
case SLOW_CONTROLLER:
return "controller";
default:
diff --git a/lib/odp-util.h b/lib/odp-util.h
index ea9bf3a..d48c1a3 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -178,11 +178,7 @@ enum slow_path_reason {
SLOW_CFM = 1 << 0, /* CFM packets need per-packet processing. */
SLOW_LACP = 1 << 1, /* LACP packets need per-packet processing. */
SLOW_STP = 1 << 2, /* STP packets need per-packet processing. */
- SLOW_IN_BAND = 1 << 3, /* In-band control needs every packet. */
-
- /* Mutually exclusive with SLOW_CFM, SLOW_LACP, SLOW_STP.
- * Could possibly appear with SLOW_IN_BAND. */
- SLOW_CONTROLLER = 1 << 4, /* Packets must go to OpenFlow controller. */
+ SLOW_CONTROLLER = 1 << 3, /* Packets must go to OpenFlow controller. */
};
#endif /* odp-util.h */
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b8cdfa5..1a1ab6c 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1643,17 +1643,10 @@ any_extras_changed(const struct connmgr *mgr,
/* In-band implementation. */
bool
-connmgr_msg_in_hook(struct connmgr *mgr, const struct flow *flow,
- const struct ofpbuf *packet)
-{
- return mgr->in_band && in_band_msg_in_hook(mgr->in_band, flow, packet);
-}
-
-bool
-connmgr_may_set_up_flow(struct connmgr *mgr, const struct flow *flow,
- uint32_t local_odp_port,
- const struct nlattr *odp_actions,
- size_t actions_len)
+connmgr_must_output_local(struct connmgr *mgr, const struct flow *flow,
+ uint32_t local_odp_port,
+ const struct nlattr *odp_actions,
+ size_t actions_len)
{
return !mgr->in_band || in_band_rule_check(flow, local_odp_port,
odp_actions, actions_len);
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 48b8119..506f9c7 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -156,12 +156,10 @@ void connmgr_set_extra_in_band_remotes(struct connmgr *,
void connmgr_set_in_band_queue(struct connmgr *, int queue_id);
/* In-band implementation. */
-bool connmgr_msg_in_hook(struct connmgr *, const struct flow *,
- const struct ofpbuf *packet);
-bool connmgr_may_set_up_flow(struct connmgr *, const struct flow *,
- uint32_t local_odp_port,
- const struct nlattr *odp_actions,
- size_t actions_len);
+bool connmgr_must_output_local(struct connmgr *, const struct flow *,
+ uint32_t local_odp_port,
+ const struct nlattr *odp_actions,
+ size_t actions_len);
/* Fail-open and in-band implementation. */
void connmgr_flushed(struct connmgr *);
diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index 1a08fcc..ba6fc54 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -222,37 +222,6 @@ refresh_local(struct in_band *ib)
return true;
}
-/* Returns true if 'packet' should be sent to the local port regardless
- * of the flow table. */
-bool
-in_band_msg_in_hook(struct in_band *in_band, const struct flow *flow,
- const struct ofpbuf *packet)
-{
- /* Regardless of how the flow table is configured, we want to be
- * able to see replies to our DHCP requests. */
- if (flow->dl_type == htons(ETH_TYPE_IP)
- && flow->nw_proto == IPPROTO_UDP
- && flow->tp_src == htons(DHCP_SERVER_PORT)
- && flow->tp_dst == htons(DHCP_CLIENT_PORT)
- && packet->l7) {
- struct dhcp_header *dhcp;
-
- dhcp = ofpbuf_at(packet, (char *)packet->l7 - (char *)packet->data,
- sizeof *dhcp);
- if (!dhcp) {
- return false;
- }
-
- refresh_local(in_band);
- if (!eth_addr_is_zero(in_band->local_mac)
- && eth_addr_equals(dhcp->chaddr, in_band->local_mac)) {
- return true;
- }
- }
-
- return false;
-}
-
/* Returns true if the rule that would match 'flow' with 'actions' is
* allowed to be set up in the datapath. */
bool
diff --git a/ofproto/in-band.h b/ofproto/in-band.h
index 71de6ff..4f52e00 100644
--- a/ofproto/in-band.h
+++ b/ofproto/in-band.h
@@ -39,8 +39,6 @@ void in_band_set_remotes(struct in_band *,
bool in_band_run(struct in_band *);
void in_band_wait(struct in_band *);
-bool in_band_msg_in_hook(struct in_band *, const struct flow *,
- const struct ofpbuf *packet);
bool in_band_rule_check(const struct flow *, uint32_t local_odp_port,
const struct nlattr *odp_actions, size_t actions_len);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6c0aa2d..ee0954b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -415,8 +415,7 @@ static void subfacet_update_time(struct subfacet *, long long int used);
static void subfacet_update_stats(struct subfacet *,
const struct dpif_flow_stats *);
static void subfacet_make_actions(struct subfacet *,
- const struct ofpbuf *packet,
- struct ofpbuf *odp_actions);
+ const struct ofpbuf *packet);
static int subfacet_install(struct subfacet *,
const struct nlattr *actions, size_t actions_len,
struct dpif_flow_stats *, enum slow_path_reason);
@@ -3689,13 +3688,19 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
LIST_FOR_EACH (packet, list_node, &miss->packets) {
struct flow_miss_op *op = &ops[*n_ops];
struct dpif_flow_stats stats;
- struct ofpbuf odp_actions;
handle_flow_miss_common(facet->rule, packet, &miss->flow);
- ofpbuf_use_stub(&odp_actions, op->stub, sizeof op->stub);
- if (!subfacet->actions || subfacet->slow) {
- subfacet_make_actions(subfacet, packet, &odp_actions);
+ if (!subfacet->actions) {
+ subfacet_make_actions(subfacet, packet);
+ } else if (subfacet->slow) {
+ struct action_xlate_ctx ctx;
+
+ action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
+ &subfacet->initial_vals, facet->rule, 0,
+ packet);
+ xlate_actions_for_side_effects(&ctx, facet->rule->up.ofpacts,
+ facet->rule->up.ofpacts_len);
}
dpif_flow_stats_extract(&facet->flow, packet, now, &stats);
@@ -3705,19 +3710,10 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
struct dpif_execute *execute = &op->dpif_op.u.execute;
init_flow_miss_execute_op(miss, packet, op);
- if (!subfacet->slow) {
- execute->actions = subfacet->actions;
- execute->actions_len = subfacet->actions_len;
- ofpbuf_uninit(&odp_actions);
- } else {
- execute->actions = odp_actions.data;
- execute->actions_len = odp_actions.size;
- op->garbage = ofpbuf_get_uninit_pointer(&odp_actions);
- }
+ execute->actions = subfacet->actions;
+ execute->actions_len = subfacet->actions_len;
(*n_ops)++;
- } else {
- ofpbuf_uninit(&odp_actions);
}
}
@@ -4982,11 +4978,6 @@ facet_check_consistency(struct facet *facet)
}
want_path = subfacet_want_path(subfacet->slow);
- if (want_path == SF_SLOW_PATH && subfacet->path == SF_SLOW_PATH) {
- /* The actions for slow-path flows may legitimately vary from one
- * packet to the next. We're done. */
- continue;
- }
if (!subfacet_should_install(subfacet, subfacet->slow, &odp_actions)) {
continue;
@@ -5410,22 +5401,22 @@ subfacet_destroy_batch(struct ofproto_dpif *ofproto,
}
}
-/* Composes the datapath actions for 'subfacet' based on its rule's actions.
- * Translates the actions into 'odp_actions', which the caller must have
- * initialized and is responsible for uninitializing. */
+/* Composes the datapath actions for 'subfacet' based on its rule's actions. */
static void
-subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet,
- struct ofpbuf *odp_actions)
+subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet)
{
struct facet *facet = subfacet->facet;
struct rule_dpif *rule = facet->rule;
struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
struct action_xlate_ctx ctx;
+ struct ofpbuf odp_actions;
+ uint64_t stub[1024 / 8];
+ ofpbuf_use_stub(&odp_actions, stub, sizeof stub);
action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
&subfacet->initial_vals, rule, 0, packet);
- xlate_actions(&ctx, rule->up.ofpacts, rule->up.ofpacts_len, odp_actions);
+ xlate_actions(&ctx, rule->up.ofpacts, rule->up.ofpacts_len, &odp_actions);
facet->tags = ctx.tags;
facet->has_learn = ctx.has_learn;
facet->has_normal = ctx.has_normal;
@@ -5434,12 +5425,10 @@ subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet,
facet->mirrors = ctx.mirrors;
subfacet->slow = ctx.slow;
- if (subfacet->actions_len != odp_actions->size
- || memcmp(subfacet->actions, odp_actions->data, odp_actions->size)) {
- free(subfacet->actions);
- subfacet->actions_len = odp_actions->size;
- subfacet->actions = xmemdup(odp_actions->data, odp_actions->size);
- }
+
+ ovs_assert(!subfacet->actions);
+ subfacet->actions_len = odp_actions.size;
+ subfacet->actions = ofpbuf_steal_data(&odp_actions);
}
/* Updates 'subfacet''s datapath flow, setting its actions to 'actions_len'
@@ -7179,16 +7168,11 @@ xlate_actions(struct action_xlate_ctx *ctx,
}
local_odp_port = ofp_port_to_odp_port(ctx->ofproto, OFPP_LOCAL);
- if (!connmgr_may_set_up_flow(ctx->ofproto->up.connmgr, &ctx->flow,
- local_odp_port,
- ctx->odp_actions->data,
- ctx->odp_actions->size)) {
- ctx->slow |= SLOW_IN_BAND;
- if (ctx->packet
- && connmgr_msg_in_hook(ctx->ofproto->up.connmgr, &ctx->flow,
- ctx->packet)) {
- compose_output_action(ctx, OFPP_LOCAL);
- }
+ if (!connmgr_must_output_local(ctx->ofproto->up.connmgr, &ctx->flow,
+ local_odp_port,
+ ctx->odp_actions->data,
+ ctx->odp_actions->size)) {
+ compose_output_action(ctx, OFPP_LOCAL);
}
if (ctx->ofproto->has_mirrors) {
add_mirror_actions(ctx, &orig_flow);
@@ -8327,15 +8311,6 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
case SLOW_STP:
ds_put_cstr(ds, "\n\t- Consists of STP packets.");
break;
- case SLOW_IN_BAND:
- ds_put_cstr(ds, "\n\t- Needs in-band special case "
- "processing.");
- if (!packet) {
- ds_put_cstr(ds, "\n\t (The datapath actions are "
- "incomplete--for complete actions, "
- "please supply a packet.)");
- }
- break;
case SLOW_CONTROLLER:
ds_put_cstr(ds, "\n\t- Sends \"packet-in\" messages "
"to the OpenFlow controller.");
--
1.7.9.5
More information about the dev
mailing list