ofproto: Ditch SLOW_IN_BAND slow path reason.

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@nicira.com>

diff --git a/lib/odp-util.c b/lib/odp-util.c
index aa50333..4ff1f96 100644 (file)
--- 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_BFD:
         return "bfd";
     case SLOW_CONTROLLER:

diff --git a/lib/odp-util.h b/lib/odp-util.h
index a2f2b14..0892cf7 100644 (file)
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -178,12 +178,8 @@ 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. */
-    SLOW_BFD = 1 << 4,          /* BFD packets need per-packet processing. */
-
-    /* Mutually exclusive with SLOW_BFD, SLOW_CFM, SLOW_LACP, SLOW_STP.
-     * Could possibly appear with SLOW_IN_BAND. */
-    SLOW_CONTROLLER = 1 << 5,   /* Packets must go to OpenFlow controller. */
+    SLOW_BFD = 1 << 3,          /* BFD packets need per-packet processing. */
+    SLOW_CONTROLLER = 1 << 4,   /* Packets must go to OpenFlow controller. */
 };
 
 #endif /* odp-util.h */

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b8cdfa5..1a1ab6c 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 3aac752..55d030f 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -423,8 +423,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);
@@ -3735,13 +3734,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);
@@ -3751,19 +3756,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);
         }
     }
 
@@ -5011,11 +5007,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;
@@ -5440,22 +5431,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;
@@ -5464,12 +5455,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'
@@ -7168,16 +7157,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);
@@ -8317,15 +8301,6 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
                 case SLOW_BFD:
                     ds_put_cstr(ds, "\n\t- Consists of BFD 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.");