From patchwork Thu Dec 21 22:25:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 852156 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3z2mVv1T4Yz9rvt for ; Fri, 22 Dec 2017 09:28:07 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id EC194BE6; Thu, 21 Dec 2017 22:25:48 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id B8F3CB1E for ; Thu, 21 Dec 2017 22:25:45 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 854BD431 for ; Thu, 21 Dec 2017 22:25:44 +0000 (UTC) X-Originating-IP: 208.91.1.34 Received: from sc9-mailhost2.vmware.com (unknown [208.91.1.34]) (Authenticated sender: jpettit@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D672CC5A54 for ; Thu, 21 Dec 2017 23:25:42 +0100 (CET) From: Justin Pettit To: dev@openvswitch.org Date: Thu, 21 Dec 2017 14:25:15 -0800 Message-Id: <1513895115-35879-6-git-send-email-jpettit@ovn.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1513895115-35879-1-git-send-email-jpettit@ovn.org> References: <1513895115-35879-1-git-send-email-jpettit@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [no-slow 6/6] ofproto-dpif: Don't slow-path controller actions with pause. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org 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 Acked-by: Ben Pfaff --- 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(-) 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;