[ovs-dev,no-slow,6/6] ofproto-dpif: Don't slow-path controller actions with pause.

Message ID 1513895115-35879-6-git-send-email-jpettit@ovn.org
State Superseded
Headers show
Series
  • [ovs-dev,no-slow,1/6] ofproto-dpif: Add ability to look up an ofproto by UUID.
Related show

Commit Message

Justin Pettit Dec. 21, 2017, 10:25 p.m.
A previous patch removed slow-pathing for controller actions with the
exception of ones that specified "pause".  This commit removes that
restriction so that no controller actions are slow-pathed.

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 lib/odp-util.h                     |   2 +-
 ofproto/ofproto-dpif-upcall.c      |  31 ++++++--
 ofproto/ofproto-dpif-xlate-cache.c |  13 ---
 ofproto/ofproto-dpif-xlate-cache.h |   1 -
 ofproto/ofproto-dpif-xlate.c       | 159 +++++++++++++++----------------------
 ofproto/ofproto-dpif.c             |   1 -
 6 files changed, 87 insertions(+), 120 deletions(-)

Comments

Ben Pfaff Jan. 2, 2018, 6:39 p.m. | #1
On Thu, Dec 21, 2017 at 02:25:15PM -0800, Justin Pettit wrote:
> A previous patch removed slow-pathing for controller actions with the
> exception of ones that specified "pause".  This commit removes that
> restriction so that no controller actions are slow-pathed.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

One worthwhile checkpatch warning:
WARNING: Line length is >79-characters long
#175 FILE: ofproto/ofproto-dpif-xlate.c:4372:
                                               ctx->xin->flow.in_port.ofp_port);

Another valuable simplification, thanks again.

Acked-by: Ben Pfaff <blp@ovn.org>
Ben Pfaff Jan. 4, 2018, 1:16 a.m. | #2
On Thu, Dec 21, 2017 at 02:25:15PM -0800, Justin Pettit wrote:
> A previous patch removed slow-pathing for controller actions with the
> exception of ones that specified "pause".  This commit removes that
> restriction so that no controller actions are slow-pathed.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

On second look, I don't have anything more to add.  I guess you could
delete the extra blank line here:

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 1558dd970cdc..98e3d832eb7d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4494,7 +4494,6 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
                                    ctx->pause->userdata,
                                    ctx->pause->userdata_len);
     } else {
-
         if (ctx->recirc_update_dp_hash) {
             struct ovs_action_hash *act_hash;
Justin Pettit Jan. 10, 2018, 6:47 a.m. | #3
> On Jan 2, 2018, at 10:39 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Thu, Dec 21, 2017 at 02:25:15PM -0800, Justin Pettit wrote:
>> A previous patch removed slow-pathing for controller actions with the
>> exception of ones that specified "pause".  This commit removes that
>> restriction so that no controller actions are slow-pathed.
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> One worthwhile checkpatch warning:
> WARNING: Line length is >79-characters long
> #175 FILE: ofproto/ofproto-dpif-xlate.c:4372:
>                                               ctx->xin->flow.in_port.ofp_port);

Thanks.  I fixed this and the newline I introduced that you mentioned in your followup.

> Another valuable simplification, thanks again.

Thanks!

--Justin

Patch

diff --git a/lib/odp-util.h b/lib/odp-util.h
index ff9ecf00e3c2..365194404eea 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -43,7 +43,6 @@  struct pkt_metadata;
     SPR(SLOW_LACP,       "lacp",       "Consists of LACP packets")      \
     SPR(SLOW_STP,        "stp",        "Consists of STP packets")       \
     SPR(SLOW_LLDP,       "lldp",       "Consists of LLDP packets")      \
-    SPR(SLOW_PAUSE,      "pause",      "Controller action with pause")  \
     SPR(SLOW_ACTION,     "action",                                      \
         "Uses action(s) not supported by datapath")
 
@@ -299,6 +298,7 @@  enum user_action_cookie_type {
 /* Flags for controller cookies. */
 enum user_action_controller_flags {
     UACF_DONT_SEND    = 1 << 0,      /* Don't send the packet to controller. */
+    UACF_CONTINUATION = 1 << 1,      /* Send packet-in as a continuation. */
 };
 
 struct user_action_cookie_header {
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 405a3f74368e..5b6e2b002816 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1449,6 +1449,8 @@  process_upcall(struct udpif *udpif, struct upcall *upcall,
                 break;
             }
 
+            const struct frozen_state *state = &recirc_node->state;
+
             struct ofproto_async_msg *am = xmalloc(sizeof *am);
             *am = (struct ofproto_async_msg) {
                 .controller_id = cookie->controller.controller_id,
@@ -1460,7 +1462,7 @@  process_upcall(struct udpif *udpif, struct upcall *upcall,
                                               dp_packet_size(packet)),
                             .packet_len = dp_packet_size(packet),
                             .reason = cookie->controller.reason,
-                            .table_id = recirc_node->state.table_id,
+                            .table_id = state->table_id,
                             .cookie = get_32aligned_be64(
                                          &cookie->controller.rule_cookie),
                             .userdata = (cookie->controller.userdata_len
@@ -1474,18 +1476,36 @@  process_upcall(struct udpif *udpif, struct upcall *upcall,
                 },
             };
 
+            if (cookie->controller.flags & UACF_CONTINUATION) {
+                am->pin.up.stack = (state->stack_size
+                          ? xmemdup(state->stack, state->stack_size)
+                          : NULL),
+                am->pin.up.stack_size = state->stack_size,
+                am->pin.up.mirrors = state->mirrors,
+                am->pin.up.conntracked = state->conntracked,
+                am->pin.up.actions = (state->ofpacts_len
+                            ? xmemdup(state->ofpacts,
+                                      state->ofpacts_len) : NULL),
+                am->pin.up.actions_len = state->ofpacts_len,
+                am->pin.up.action_set = (state->action_set_len
+                               ? xmemdup(state->action_set,
+                                         state->action_set_len)
+                               : NULL),
+                am->pin.up.action_set_len = state->action_set_len,
+                am->pin.up.bridge = upcall->ofproto->uuid;
+            }
+
             /* We don't want to use the upcall 'flow', since it may be
              * more specific than the point at which the "controller"
              * action was specified. */
             struct flow frozen_flow;
 
             frozen_flow = *flow;
-            if (!recirc_node->state.conntracked) {
+            if (!state->conntracked) {
                 flow_clear_conntrack(&frozen_flow);
             }
 
-            frozen_metadata_to_flow(&recirc_node->state.metadata,
-                                    &frozen_flow);
+            frozen_metadata_to_flow(&state->metadata, &frozen_flow);
             flow_get_metadata(&frozen_flow, &am->pin.up.base.flow_metadata);
 
             ofproto_dpif_send_async_msg(upcall->ofproto, am);
@@ -1515,9 +1535,6 @@  handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
      *
      *   - For SLOW_ACTION, translation executes the actions directly.
      *
-     *   - For SLOW_PAUSE, translation needs to handle a pause request
-     *     from the controller.
-     *
      * The loop fills 'ops' with an array of operations to execute in the
      * datapath. */
     n_ops = 0;
diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c
index 24fc769a7a0d..2789d1361f51 100644
--- a/ofproto/ofproto-dpif-xlate-cache.c
+++ b/ofproto/ofproto-dpif-xlate-cache.c
@@ -153,13 +153,6 @@  xlate_push_stats_entry(struct xc_entry *entry,
         tnl_neigh_lookup(entry->tnl_neigh_cache.br_name,
                          &entry->tnl_neigh_cache.d_ipv6, &dmac);
         break;
-    case XC_CONTROLLER:
-        if (entry->controller.am) {
-            ofproto_dpif_send_async_msg(entry->controller.ofproto,
-                                        entry->controller.am);
-            entry->controller.am = NULL; /* One time only. */
-        }
-        break;
     case XC_TUNNEL_HEADER:
         if (entry->tunnel_hdr.operation == ADD) {
             stats->n_bytes += stats->n_packets * entry->tunnel_hdr.hdr_size;
@@ -247,12 +240,6 @@  xlate_cache_clear_entry(struct xc_entry *entry)
         break;
     case XC_TNL_NEIGH:
         break;
-    case XC_CONTROLLER:
-        if (entry->controller.am) {
-            ofproto_async_msg_free(entry->controller.am);
-            entry->controller.am = NULL;
-        }
-        break;
     case XC_TUNNEL_HEADER:
         break;
     default:
diff --git a/ofproto/ofproto-dpif-xlate-cache.h b/ofproto/ofproto-dpif-xlate-cache.h
index e62719740cb2..ae3cd7fdc822 100644
--- a/ofproto/ofproto-dpif-xlate-cache.h
+++ b/ofproto/ofproto-dpif-xlate-cache.h
@@ -51,7 +51,6 @@  enum xc_type {
     XC_FIN_TIMEOUT,      /* Calls back to ofproto. */
     XC_GROUP,
     XC_TNL_NEIGH,
-    XC_CONTROLLER,
     XC_TUNNEL_HEADER,
 };
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9ec8d54be5ed..383ca63b0629 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -364,7 +364,6 @@  struct xlate_ctx {
     uint32_t dp_hash_basis;
     struct ofpbuf frozen_actions;
     const struct ofpact_controller *pause;
-    struct flow *paused_flow;
 
     /* True if a packet was but is no longer MPLS (due to an MPLS pop action).
      * This is a trigger for recirculation in cases where translating an action
@@ -4346,6 +4345,40 @@  flood_packets(struct xlate_ctx *ctx, bool all, bool is_last_action)
 }
 
 static void
+put_controller_user_action(struct xlate_ctx *ctx,
+                           enum user_action_controller_flags flags,
+                           uint32_t recirc_id, int len,
+                           enum ofp_packet_in_reason reason,
+                           uint16_t controller_id,
+                           const uint8_t *userdata, size_t userdata_len)
+{
+    union user_action_cookie *cookie;
+    cookie = xmalloc(sizeof cookie->controller + userdata_len);
+
+    memset(&cookie->controller, 0, sizeof cookie->controller);
+    cookie->header.type = USER_ACTION_COOKIE_CONTROLLER;
+    cookie->header.ofproto_uuid = ctx->xbridge->ofproto->uuid;
+    cookie->controller.flags = flags;
+    cookie->controller.recirc_id = recirc_id;
+    cookie->controller.max_len = len;
+    cookie->controller.controller_id = controller_id;
+    cookie->controller.reason = reason;
+    cookie->controller.userdata_len = userdata_len;
+    put_32aligned_be64(&cookie->controller.rule_cookie, ctx->rule_cookie);
+    memcpy(cookie->controller.userdata, userdata, userdata_len);
+
+    odp_port_t odp_port = ofp_port_to_odp_port(ctx->xbridge,
+                                               ctx->xin->flow.in_port.ofp_port);
+    uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port,
+                                     flow_hash_5tuple(&ctx->xin->flow, 0));
+    odp_put_userspace_action(pid, cookie,
+                             sizeof cookie->controller + userdata_len,
+                             ODPP_NONE, false, ctx->odp_actions);
+
+    free(cookie);
+}
+
+static void
 xlate_controller_action(struct xlate_ctx *ctx, int len,
                         enum ofp_packet_in_reason reason,
                         uint16_t controller_id,
@@ -4397,34 +4430,14 @@  xlate_controller_action(struct xlate_ctx *ctx, int len,
         nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
     }
 
-    union user_action_cookie *cookie;
-    cookie = xmalloc(sizeof cookie->controller + userdata_len);
-
-    memset(&cookie->controller, 0, sizeof cookie->controller);
-    cookie->header.type = USER_ACTION_COOKIE_CONTROLLER,
-    cookie->header.ofproto_uuid = ctx->xbridge->ofproto->uuid,
-    cookie->controller.recirc_id = recirc_id,
-    cookie->controller.max_len = len,
-    cookie->controller.controller_id = controller_id,
-    cookie->controller.reason = reason,
-    cookie->controller.userdata_len = userdata_len,
-    put_32aligned_be64(&cookie->controller.rule_cookie, ctx->rule_cookie);
-    memcpy(cookie->controller.userdata, userdata, userdata_len);
-
     /* Generate the datapath flows even if we don't send the packet-in
      * so that debugging more closely represents normal state. */
+    enum user_action_controller_flags flags = 0;
     if (!ctx->xin->allow_side_effects && !ctx->xin->xcache) {
-        cookie->controller.flags |= UACF_DONT_SEND;
+        flags = UACF_DONT_SEND;
     }
-
-    odp_port_t odp_port = ofp_port_to_odp_port(
-        ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
-    uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port,
-                                     flow_hash_5tuple(&ctx->xin->flow, 0));
-    odp_put_userspace_action(pid, cookie,
-                             sizeof cookie->controller + userdata_len,
-                             ODPP_NONE, false, ctx->odp_actions);
-    free(cookie);
+    put_controller_user_action(ctx, flags, recirc_id, len, reason,
+                               controller_id, userdata, userdata_len);
 
     if (meter_id != UINT32_MAX) {
         nl_msg_end_nested(ctx->odp_actions, ac_offset);
@@ -4432,57 +4445,6 @@  xlate_controller_action(struct xlate_ctx *ctx, int len,
     }
 }
 
-static void
-emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state)
-{
-    if (!ctx->xin->allow_side_effects && !ctx->xin->xcache) {
-        return;
-    }
-
-    struct ofproto_async_msg *am = xmalloc(sizeof *am);
-    *am = (struct ofproto_async_msg) {
-        .controller_id = ctx->pause->controller_id,
-        .oam = OAM_PACKET_IN,
-        .pin = {
-            .up = {
-                .base = {
-                    .userdata = xmemdup(ctx->pause->userdata,
-                                        ctx->pause->userdata_len),
-                    .userdata_len = ctx->pause->userdata_len,
-                    .packet = xmemdup(dp_packet_data(ctx->xin->packet),
-                                      dp_packet_size(ctx->xin->packet)),
-                    .packet_len = dp_packet_size(ctx->xin->packet),
-                    .reason = ctx->pause->reason,
-                },
-                .bridge = ctx->xbridge->ofproto->uuid,
-                .stack = xmemdup(state->stack, state->stack_size),
-                .stack_size = state->stack_size,
-                .mirrors = state->mirrors,
-                .conntracked = state->conntracked,
-                .actions = xmemdup(state->ofpacts, state->ofpacts_len),
-                .actions_len = state->ofpacts_len,
-                .action_set = xmemdup(state->action_set,
-                                      state->action_set_len),
-                .action_set_len = state->action_set_len,
-            },
-            .max_len = UINT16_MAX,
-        },
-    };
-    flow_get_metadata(ctx->paused_flow, &am->pin.up.base.flow_metadata);
-
-    /* Async messages are only sent once, so if we send one now, no
-     * xlate cache entry is created.  */
-    if (ctx->xin->allow_side_effects) {
-        ofproto_dpif_send_async_msg(ctx->xbridge->ofproto, am);
-    } else /* xcache */ {
-        struct xc_entry *entry;
-
-        entry = xlate_cache_add_entry(ctx->xin->xcache, XC_CONTROLLER);
-        entry->controller.ofproto = ctx->xbridge->ofproto;
-        entry->controller.am = am;
-    }
-}
-
 /* Creates a frozen state, and allocates a unique recirc id for the given
  * state.  Returns a non-zero recirc id if it is allocated successfully.
  * Returns 0 otherwise.
@@ -4490,7 +4452,6 @@  emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state)
 static uint32_t
 finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
 {
-    uint32_t id = 0;
     ovs_assert(ctx->freezing);
 
     struct frozen_state state = {
@@ -4507,23 +4468,31 @@  finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
     };
     frozen_metadata_from_flow(&state.metadata, &ctx->xin->flow);
 
+    /* Allocate a unique recirc id for the given metadata state in the
+     * flow.  An existing id, with a new reference to the corresponding
+     * recirculation context, will be returned if possible.
+     * The life-cycle of this recirc id is managed by associating it
+     * with the udpif key ('ukey') created for each new datapath flow. */
+    uint32_t recirc_id = recirc_alloc_id_ctx(&state);
+    if (!recirc_id) {
+        xlate_report_error(ctx, "Failed to allocate recirculation id");
+        ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
+        return 0;
+    }
+    recirc_refs_add(&ctx->xout->recircs, recirc_id);
+
     if (ctx->pause) {
-        if (ctx->xin->packet) {
-            emit_continuation(ctx, &state);
-        }
-    } else {
-        /* Allocate a unique recirc id for the given metadata state in the
-         * flow.  An existing id, with a new reference to the corresponding
-         * recirculation context, will be returned if possible.
-         * The life-cycle of this recirc id is managed by associating it
-         * with the udpif key ('ukey') created for each new datapath flow. */
-        id = recirc_alloc_id_ctx(&state);
-        if (!id) {
-            xlate_report_error(ctx, "Failed to allocate recirculation id");
-            ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
+        if (!ctx->xin->allow_side_effects && !ctx->xin->xcache) {
             return 0;
         }
-        recirc_refs_add(&ctx->xout->recircs, id);
+
+        put_controller_user_action(ctx, UACF_CONTINUATION, recirc_id,
+                                   ctx->pause->max_len,
+                                   ctx->pause->reason,
+                                   ctx->pause->controller_id,
+                                   ctx->pause->userdata,
+                                   ctx->pause->userdata_len);
+    } else {
 
         if (ctx->recirc_update_dp_hash) {
             struct ovs_action_hash *act_hash;
@@ -4535,12 +4504,12 @@  finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
             act_hash->hash_alg = OVS_HASH_ALG_L4;  /* Make configurable. */
             act_hash->hash_basis = 0;              /* Make configurable. */
         }
-        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, id);
+        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, recirc_id);
     }
 
     /* Undo changes done by freezing. */
     ctx_cancel_freeze(ctx);
-    return id;
+    return recirc_id;
 }
 
 /* Called only when we're freezing. */
@@ -6119,8 +6088,6 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             controller = ofpact_get_CONTROLLER(a);
             if (controller->pause) {
                 ctx->pause = controller;
-                ctx->xout->slow |= SLOW_PAUSE;
-                *ctx->paused_flow = ctx->xin->flow;
                 ctx_trigger_freeze(ctx);
                 a = ofpact_next(a);
             } else {
@@ -6755,7 +6722,6 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     uint64_t frozen_actions_stub[1024 / 8];
     uint64_t actions_stub[256 / 8];
     struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub);
-    struct flow paused_flow;
     struct xlate_ctx ctx = {
         .xin = xin,
         .xout = xout,
@@ -6791,7 +6757,6 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         .recirc_update_dp_hash = false,
         .frozen_actions = OFPBUF_STUB_INITIALIZER(frozen_actions_stub),
         .pause = NULL,
-        .paused_flow = &paused_flow,
 
         .was_mpls = false,
         .conntracked = false,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b0a1390d816b..ab70244396bf 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4668,7 +4668,6 @@  ofproto_dpif_xcache_execute(struct ofproto_dpif *ofproto,
         case XC_NORMAL:
         case XC_GROUP:
         case XC_TNL_NEIGH:
-        case XC_CONTROLLER:
         case XC_TUNNEL_HEADER:
             xlate_push_stats_entry(entry, stats);
             break;