From patchwork Fri Oct 14 12:21:24 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ales Musil X-Patchwork-Id: 1690022 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=i/0ZmUjM; dkim-atps=neutral Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4Mplqj5tRJz23jc for ; Fri, 14 Oct 2022 23:21:37 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 973AE42233; Fri, 14 Oct 2022 12:21:35 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 973AE42233 Authentication-Results: smtp4.osuosl.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=i/0ZmUjM X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oNsQULBhJH4A; Fri, 14 Oct 2022 12:21:34 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id 46A844218F; Fri, 14 Oct 2022 12:21:33 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 46A844218F Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 146E2C0032; Fri, 14 Oct 2022 12:21:33 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 5EDBAC002D for ; Fri, 14 Oct 2022 12:21:31 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 46ABC41138 for ; Fri, 14 Oct 2022 12:21:31 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 46ABC41138 Authentication-Results: smtp2.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=i/0ZmUjM X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fpb8KkYhqq3N for ; Fri, 14 Oct 2022 12:21:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org EDDDF402B1 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp2.osuosl.org (Postfix) with ESMTPS id EDDDF402B1 for ; Fri, 14 Oct 2022 12:21:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1665750088; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=rDhvF0tRAHax7bmJCz5DfqDVbT1x1oW4wMh07HmVzQw=; b=i/0ZmUjMzlkM/jk2VvsPF67+xTCN8S3Se4XZmQVpYLw6Ij2Avz9P9KHSqKXRpipgLWN1LK sxpAbxHaaqXP5YVGNy4tL+UwAWb88sbWrsHHXx66k/TVJKU47p3luh7E96pyBRi/Kl4hgS S8XZ3touRt+twROJl0POqJpK9O3KpCw= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-144-KwmQ4Le1NQCQdQwdFSreyg-1; Fri, 14 Oct 2022 08:21:27 -0400 X-MC-Unique: KwmQ4Le1NQCQdQwdFSreyg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7109887B2A4 for ; Fri, 14 Oct 2022 12:21:27 +0000 (UTC) Received: from amusil.redhat.com (ovpn-192-110.brq.redhat.com [10.40.192.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 791E9C15BB3; Fri, 14 Oct 2022 12:21:26 +0000 (UTC) From: Ales Musil To: dev@openvswitch.org Date: Fri, 14 Oct 2022 14:21:24 +0200 Message-Id: <20221014122125.205013-1-amusil@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH v6 1/2] ofproto-dpif-xlate: Extract the freezing processing into a function X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" Through out the code there is the same pattern that occurs in regards to to finish_freezing when ctx->freezing=true or xlate_action_set when ctx->freezing=false. Extract it to common function that is called from those places instead. Signed-off-by: Ales Musil --- v6: Rebase on top of current master. Fix wrong if semantics. --- ofproto/ofproto-dpif-xlate.c | 53 ++++++++++++------------------------ 1 file changed, 17 insertions(+), 36 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 3b9b26da1..6ea71eb52 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3883,6 +3883,17 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, const struct flow *flow, co xport_in->xbundle->protected && xport_out->xbundle->protected); } +static void +xlate_ctx_process_freezing(struct xlate_ctx *ctx) +{ + if (!ctx->freezing) { + xlate_action_set(ctx); + } + if (ctx->freezing) { + finish_freezing(ctx); + } +} + /* Function handles when a packet is sent from one bridge to another bridge. * * The bridges are internally connected, either with patch ports or with @@ -3943,12 +3954,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, false, is_last_action, clone_xlate_actions); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); } else { /* Forwarding is disabled by STP and RSTP. Let OFPP_NORMAL and * the learning action look at the packet, then drop it. */ @@ -5866,12 +5872,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, if (reversible_actions(actions, actions_len) || is_last_action) { old_flow = ctx->xin->flow; do_xlate_actions(actions, actions_len, ctx, is_last_action, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); goto xlate_done; } @@ -5890,12 +5891,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, /* Use clone action as datapath clone. */ offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE); do_xlate_actions(actions, actions_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); nl_msg_end_non_empty_nested(ctx->odp_actions, offset); goto dp_clone_done; } @@ -5906,12 +5902,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, ac_offset = nl_msg_start_nested(ctx->odp_actions, OVS_SAMPLE_ATTR_ACTIONS); do_xlate_actions(actions, actions_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { nl_msg_cancel_nested(ctx->odp_actions, offset); } else { @@ -6472,12 +6463,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, value.u8_val = 1; mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow); do_xlate_actions(remaining_acts, remaining_acts_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); nl_msg_end_nested(ctx->odp_actions, offset_attr); ctx->base_flow = old_base; @@ -6497,12 +6483,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, value.u8_val = 0; mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow); do_xlate_actions(remaining_acts, remaining_acts_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); nl_msg_end_nested(ctx->odp_actions, offset_attr); nl_msg_end_nested(ctx->odp_actions, offset); From patchwork Fri Oct 14 12:21:25 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ales Musil X-Patchwork-Id: 1690023 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=bOl3y9Rl; dkim-atps=neutral Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4Mplqz30TRz23jc for ; Fri, 14 Oct 2022 23:21:51 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 60A3041138; Fri, 14 Oct 2022 12:21:49 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 60A3041138 Authentication-Results: smtp2.osuosl.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=bOl3y9Rl X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id SXB8RGwS-jjE; Fri, 14 Oct 2022 12:21:47 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp2.osuosl.org (Postfix) with ESMTPS id B98694116A; Fri, 14 Oct 2022 12:21:46 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org B98694116A Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 781D4C0032; Fri, 14 Oct 2022 12:21:46 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7C78AC0032 for ; Fri, 14 Oct 2022 12:21:44 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 4D94A41162 for ; Fri, 14 Oct 2022 12:21:40 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 4D94A41162 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qk072vYXbTLw for ; Fri, 14 Oct 2022 12:21:38 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 95F1541185 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp2.osuosl.org (Postfix) with ESMTPS id 95F1541185 for ; Fri, 14 Oct 2022 12:21:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1665750097; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gYmyi63BHLb1X+Djwe+MHnHJ0757J2Bn1LsKXIr9Puc=; b=bOl3y9Rle8UhH7hwCDxKRNMS4c+iNhKwnltATN5Y5TZIis2Wmxx/pPCi8SEBGZcHAEmBcj XDC5YPYgnyWThIjnCe0cxDzMowjvCKKZa6B7qWgNV+kjBvpunpVKkixTjhM2JvnyEB11hi 0wJVVaNMGqyGNMvxRwLEveL93qfZXm4= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-12-4gMWSUVqOqGsoWQglnjmng-1; Fri, 14 Oct 2022 08:21:36 -0400 X-MC-Unique: 4gMWSUVqOqGsoWQglnjmng-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 227B01C05EBC for ; Fri, 14 Oct 2022 12:21:31 +0000 (UTC) Received: from amusil.redhat.com (ovpn-192-110.brq.redhat.com [10.40.192.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id EC0C2C15BB3; Fri, 14 Oct 2022 12:21:27 +0000 (UTC) From: Ales Musil To: dev@openvswitch.org Date: Fri, 14 Oct 2022 14:21:25 +0200 Message-Id: <20221014122125.205013-2-amusil@redhat.com> In-Reply-To: <20221014122125.205013-1-amusil@redhat.com> References: <20221014122125.205013-1-amusil@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH v6 2/2] ofproto-dpif-xlate: Optimize the clone for patch ports X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When the packet was traveling through patch port boundary OvS would check if any of the actions is reversible, if not it would clone the packet. However, the check was only at the first level of the second bridge. That caused some issues when the packet had gone through more actions, some of them might have been irreversible. In order to keep the semantics the same we might need to run the actions twice in the worst case scenario. During the clone there are 4 scenarios that can happen. 1) The action is last one for that flow, in that case we just continue without clone. 2) There is irreversible action in the action set (first level). In this case we know that there is at leas one irreversible action which is enough to do clone. 3) All actions in first level are reversible, we can try to run all actions as if we don't need any clone and inspect the ofpbuf at the end. In positive case there are no irreversible actions so we will just submit the buffer and continue. 4) This is same as 3) with the difference that there is irreversible action in the ofpbuf. To keep the semantics we need to re-run the actions and treat it as clone. This requires resotration of the xlate_ctx. Add test cases for all irreversible actions to see if they are properly cloned. Signed-off-by: Ales Musil --- v4: Rebase on top of current master. Address comments from Eelco. v5: Make the code more readable and reduce duplication. v6: Rebase on top of current master. Address comments from Eelco. --- lib/odp-util.c | 125 ++++++++++++++++++++++ lib/odp-util.h | 1 + ofproto/ofproto-dpif-trace.c | 2 +- ofproto/ofproto-dpif-trace.h | 1 + ofproto/ofproto-dpif-xlate.c | 194 ++++++++++++++++++++++++++--------- tests/ofproto-dpif.at | 129 +++++++++++++++++++++++ 6 files changed, 404 insertions(+), 48 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index ba5be4bb3..e8cc247cb 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -8768,3 +8768,128 @@ commit_odp_actions(const struct flow *flow, struct flow *base, return slow1 ? slow1 : slow2; } + +static inline bool +nlattr_action_is_irreversible(const uint16_t type) +{ + switch ((enum ovs_action_attr) type) { + case OVS_ACTION_ATTR_CT: + case OVS_ACTION_ATTR_CT_CLEAR: + case OVS_ACTION_ATTR_TRUNC: + case OVS_ACTION_ATTR_PUSH_ETH: + case OVS_ACTION_ATTR_POP_ETH: + case OVS_ACTION_ATTR_PUSH_NSH: + case OVS_ACTION_ATTR_POP_NSH: + case OVS_ACTION_ATTR_METER: + case OVS_ACTION_ATTR_TUNNEL_PUSH: + case OVS_ACTION_ATTR_TUNNEL_POP: + case __OVS_ACTION_ATTR_MAX: + return true; + + case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_OUTPUT: + case OVS_ACTION_ATTR_USERSPACE: + case OVS_ACTION_ATTR_SET: + case OVS_ACTION_ATTR_PUSH_VLAN: + case OVS_ACTION_ATTR_POP_VLAN: + case OVS_ACTION_ATTR_SAMPLE: + case OVS_ACTION_ATTR_RECIRC: + case OVS_ACTION_ATTR_HASH: + case OVS_ACTION_ATTR_SET_MASKED: + case OVS_ACTION_ATTR_CLONE: + case OVS_ACTION_ATTR_CHECK_PKT_LEN: + case OVS_ACTION_ATTR_LB_OUTPUT: + case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_PUSH_MPLS: + case OVS_ACTION_ATTR_POP_MPLS: + case OVS_ACTION_ATTR_DROP: + return false; + } + return true; +} + +static bool +odp_cpl_contains_irreversible_actions(const struct nlattr *attr) +{ + static const struct nl_policy ovs_cpl_policy[] = { + [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = {.type = NL_A_U16}, + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = {.type = NL_A_NESTED}, + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL] = {.type = NL_A_NESTED}, + }; + struct nlattr *a[ARRAY_SIZE(ovs_cpl_policy)]; + + if (!nl_parse_nested(attr, ovs_cpl_policy, a, ARRAY_SIZE(a))) { + return false; + } + + const struct nlattr *greater = + a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER]; + const struct nlattr *less = + a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL]; + const void *greater_data = nl_attr_get(greater); + const void *less_data = nl_attr_get(less); + size_t greater_len = nl_attr_get_size(greater); + size_t less_len = nl_attr_get_size(less); + + return odp_contains_irreversible_action(greater_data, greater_len) || + odp_contains_irreversible_action(less_data, less_len); +} + +/* Check if any of the actions in the ofpbuf is irreversible. */ +bool +odp_contains_irreversible_action(const void *attrs, size_t attrs_len) +{ + const struct nlattr *attr; + int left; + + NL_ATTR_FOR_EACH (attr, left, attrs, attrs_len) { + uint16_t type = attr->nla_type; + + switch ((enum ovs_action_attr) type) { + /* Skip "clone" or "sample" because it already encapsulates + * irreversible actions. */ + case OVS_ACTION_ATTR_CLONE: + case OVS_ACTION_ATTR_SAMPLE: + continue; + /* Check nested actions of "check_packet_len". */ + case OVS_ACTION_ATTR_CHECK_PKT_LEN: + if (odp_cpl_contains_irreversible_actions(attr)) { + return true; + } + continue; + /* Check any other action. */ + case OVS_ACTION_ATTR_CT: + case OVS_ACTION_ATTR_CT_CLEAR: + case OVS_ACTION_ATTR_TRUNC: + case OVS_ACTION_ATTR_PUSH_ETH: + case OVS_ACTION_ATTR_POP_ETH: + case OVS_ACTION_ATTR_PUSH_NSH: + case OVS_ACTION_ATTR_POP_NSH: + case OVS_ACTION_ATTR_METER: + case OVS_ACTION_ATTR_TUNNEL_PUSH: + case OVS_ACTION_ATTR_TUNNEL_POP: + case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_OUTPUT: + case OVS_ACTION_ATTR_USERSPACE: + case OVS_ACTION_ATTR_SET: + case OVS_ACTION_ATTR_PUSH_VLAN: + case OVS_ACTION_ATTR_POP_VLAN: + case OVS_ACTION_ATTR_RECIRC: + case OVS_ACTION_ATTR_HASH: + case OVS_ACTION_ATTR_SET_MASKED: + case OVS_ACTION_ATTR_LB_OUTPUT: + case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_PUSH_MPLS: + case OVS_ACTION_ATTR_POP_MPLS: + case OVS_ACTION_ATTR_DROP: + if (nlattr_action_is_irreversible(type)) { + return true; + } + continue; + case __OVS_ACTION_ATTR_MAX: + break; + } + return true; + } + return false; +} diff --git a/lib/odp-util.h b/lib/odp-util.h index a1d0d0fba..d842974ba 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -373,6 +373,7 @@ void odp_put_pop_eth_action(struct ofpbuf *odp_actions); void odp_put_push_eth_action(struct ofpbuf *odp_actions, const struct eth_addr *eth_src, const struct eth_addr *eth_dst); +bool odp_contains_irreversible_action(const void *attrs, size_t attrs_len); struct attr_len_tbl { int len; diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index 527e2f17e..15e79c353 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -108,7 +108,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, return true; } -static void +void oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) { if (node) { diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index f579a5ca4..629327495 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -96,6 +96,7 @@ bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, const struct ofpact_nat *, const struct dp_packet *, uint32_t recirc_id, const uint16_t zone); +void oftrace_recirc_node_destroy(struct oftrace_recirc_node *node); void ofproto_append_ports_to_map(struct ofputil_port_map *, struct hmap ports); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 6ea71eb52..5766503cd 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5771,6 +5771,114 @@ xlate_sample_action(struct xlate_ctx *ctx, compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false); } +/* Struct to store the xlate_ctx that needs to be restored after +* clone is finished. */ +struct xlate_ctx_clone_state { + union mf_subvalue stack_buffer[1024 / sizeof(union mf_subvalue)]; + struct ofpbuf stack; + + uint64_t action_set_buffer[1024 / 8]; + struct ofpbuf action_set; + + uint32_t odp_actions_size; + + struct flow xin_flow; + struct flow base_flow; + + bool was_mpls; + bool conntracked; + + size_t recirc_size; + + int resubmit; + uint32_t sflow_n_outputs; +}; + +static struct xlate_ctx_clone_state +xlate_ctx_clone_state_store(struct xlate_ctx *ctx) +{ + struct xlate_ctx_clone_state state = { + .stack = ctx->stack, + .action_set = ctx->action_set, + .odp_actions_size = ctx->odp_actions->size, + .xin_flow = ctx->xin->flow, + .base_flow = ctx->base_flow, + .was_mpls = ctx->was_mpls, + .conntracked = ctx->conntracked, + .recirc_size = ctx->xin->recirc_queue + ? ovs_list_size(ctx->xin->recirc_queue) + : 0, + .resubmit = ctx->resubmits, + .sflow_n_outputs = ctx->sflow_n_outputs, + }; + + /* Set up a new stack from the provided stack buffer. */ + ofpbuf_use_stub(&ctx->stack, state.stack_buffer, + sizeof state.stack_buffer); + ofpbuf_put(&ctx->stack, state.stack.data, state.stack.size); + + /* Set up a new action_set from the provided stack buffer. */ + ofpbuf_use_stub(&ctx->action_set, state.action_set_buffer, + sizeof state.action_set_buffer); + ofpbuf_put(&ctx->action_set, state.action_set.data, state.action_set.size); + + return state; +} + +/* Restore xlate_ctx member to the old_state, with additional steps for + * revert. */ +static void +xlate_ctx_clone_state_restore(struct xlate_ctx *ctx, + struct xlate_ctx_clone_state *old_state, + bool no_clone_restore, bool revert) +{ + ofpbuf_uninit(&ctx->stack); + ofpbuf_uninit(&ctx->action_set); + + ctx->stack = old_state->stack; + ctx->action_set = old_state->action_set; + ctx->xin->flow = old_state->xin_flow; + + /* Restore just common parts when we don't clone. */ + if (no_clone_restore) { + return; + } + + /* The clone's conntrack execution should have no effect on the original + * packet. */ + ctx->conntracked = old_state->conntracked; + + /* Popping MPLS from the clone should have no effect on the original + * packet. */ + ctx->was_mpls = old_state->was_mpls; + + /* Restore the 'base_flow' for the next action. */ + ctx->base_flow = old_state->base_flow; + + if (revert) { + ctx->resubmits = old_state->resubmit; + ctx->sflow_n_outputs = old_state->sflow_n_outputs; + + /* Revert the actions that were added by non-clone path. */ + ctx->odp_actions->size = old_state->odp_actions_size; + + /* Clear recirculations that were added by non-clone path so that + * packets are not recirculated twice. */ + size_t old_recirc_size = old_state->recirc_size; + size_t recirc_size = ctx->xin->recirc_queue + ? ovs_list_size(ctx->xin->recirc_queue) + : 0; + + for (; old_recirc_size < recirc_size; old_recirc_size++) { + struct oftrace_recirc_node *node = + CONTAINER_OF(ovs_list_pop_back(ctx->xin->recirc_queue), + struct oftrace_recirc_node, node); + + oftrace_recirc_node_destroy(node); + } + } +} + /* Determine if an datapath action translated from the openflow action * can be reversed by another datapath action. * @@ -5852,37 +5960,18 @@ reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len) } static void -clone_xlate_actions(const struct ofpact *actions, size_t actions_len, - struct xlate_ctx *ctx, bool is_last_action, - bool group_bucket_action OVS_UNUSED) +do_clone_xlate_actions(const struct ofpact *actions, size_t actions_len, + struct xlate_ctx *ctx) { - struct ofpbuf old_stack = ctx->stack; - union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); - ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size); - - struct ofpbuf old_action_set = ctx->action_set; - uint64_t actset_stub[1024 / 8]; - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); - ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size); - size_t offset, ac_offset; - struct flow old_flow = ctx->xin->flow; - - if (reversible_actions(actions, actions_len) || is_last_action) { - old_flow = ctx->xin->flow; - do_xlate_actions(actions, actions_len, ctx, is_last_action, false); - xlate_ctx_process_freezing(ctx); - goto xlate_done; - } + struct xlate_ctx_clone_state old_state = xlate_ctx_clone_state_store(ctx); /* Commit datapath actions before emitting the clone action to * avoid emitting those actions twice. Once inside * the clone, another time for the action after clone. */ xlate_commit_actions(ctx); - struct flow old_base = ctx->base_flow; - bool old_was_mpls = ctx->was_mpls; - bool old_conntracked = ctx->conntracked; + /* The base needs to be stored after xlate_commit_actions. */ + old_state.base_flow = ctx->base_flow; /* The actions are not reversible, a datapath clone action is * required to encode the translation. Select the clone action @@ -5893,10 +5982,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, do_xlate_actions(actions, actions_len, ctx, true, false); xlate_ctx_process_freezing(ctx); nl_msg_end_non_empty_nested(ctx->odp_actions, offset); - goto dp_clone_done; - } - - if (ctx->xbridge->support.sample_nesting > 3) { + } else if (ctx->xbridge->support.sample_nesting > 3) { /* Use sample action as datapath clone. */ offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE); ac_offset = nl_msg_start_nested(ctx->odp_actions, @@ -5910,31 +5996,45 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, UINT32_MAX); /* 100% probability. */ nl_msg_end_nested(ctx->odp_actions, offset); } - goto dp_clone_done; + } else { + /* Datapath does not support clone, skip xlate 'oc' and + * report an error */ + xlate_report_error(ctx, "Failed to compose clone action"); } + xlate_ctx_clone_state_restore(ctx, &old_state, false, false); +} - /* Datapath does not support clone, skip xlate 'oc' and - * report an error */ - xlate_report_error(ctx, "Failed to compose clone action"); +static void +clone_xlate_actions(const struct ofpact *actions, size_t actions_len, + struct xlate_ctx *ctx, bool is_last_action, + bool group_bucket_action OVS_UNUSED) +{ + /* We have to clone if any of the actions is irreversible. */ + if (!is_last_action && !reversible_actions(actions, actions_len)) { + do_clone_xlate_actions(actions, actions_len, ctx); + return; + } -dp_clone_done: - /* The clone's conntrack execution should have no effect on the original - * packet. */ - ctx->conntracked = old_conntracked; + /* Let's run the actions as if they are reversible, if we find out that + * there was any irreversible action we can restore the xlate_ctx and run + * the do_clone instead. If the action is last we don't have to check and + * can just proceed with the actions. */ + struct xlate_ctx_clone_state old_state = xlate_ctx_clone_state_store(ctx); - /* Popping MPLS from the clone should have no effect on the original - * packet. */ - ctx->was_mpls = old_was_mpls; + do_xlate_actions(actions, actions_len, ctx, is_last_action, false); - /* Restore the 'base_flow' for the next action. */ - ctx->base_flow = old_base; + void *added_actions = + ofpbuf_at_assert(ctx->odp_actions, old_state.odp_actions_size, 0); + uint32_t added_size = ctx->odp_actions->size - old_state.odp_actions_size; -xlate_done: - ofpbuf_uninit(&ctx->action_set); - ctx->action_set = old_action_set; - ofpbuf_uninit(&ctx->stack); - ctx->stack = old_stack; - ctx->xin->flow = old_flow; + if (is_last_action + || !odp_contains_irreversible_action(added_actions, added_size)) { + xlate_ctx_process_freezing(ctx); + xlate_ctx_clone_state_restore(ctx, &old_state, true, false); + } else { + xlate_ctx_clone_state_restore(ctx, &old_state, false, true); + do_clone_xlate_actions(actions, actions_len, ctx); + } } static void diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 8e993c585..92a9d48ff 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -11943,3 +11943,132 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`]) OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([ofproto-dpif - nested patch ports clone]) + +OVS_VSWITCHD_START( + [add-port br0 br0-p0 -- set Interface br0-p0 type=dummy ofport_request=1 -- \ + add-port br0 br0-p1 -- set Interface br0-p1 type=patch options:peer=br1-p0 -- \ + add-br br1 -- \ + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:01 -- \ + set bridge br1 datapath-type=dummy other-config:datapath-id=1111 fail-mode=secure -- \ + add-port br1 br1-p0 -- set Interface br1-p0 type=patch options:peer=br0-p1 -- \ + add-port br1 br1-p1 -- set Interface br1-p1 type=patch options:peer=br2-p0 -- \ + add-port br1 br1-p2 -- set Interface br1-p2 type=dummy ofport_request=2 --\ + add-br br2 -- \ + set bridge br2 other-config:hwaddr=aa:66:aa:66:00:02 -- \ + set bridge br2 datapath-type=dummy other-config:datapath-id=2222 fail-mode=secure -- \ + add-port br2 br2-p0 -- set Interface br2-p0 type=patch options:peer=br1-p1 -- \ + add-port br2 br2-p1 -- set Interface br2-p1 type=dummy ofport_request=3]) + +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br1 'meter=1 pktps stats bands=type=drop rate=2']) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br2 'meter=1 pktps stats bands=type=drop rate=2']) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=br0-p1,br0-p0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 in_port=br1-p0,ip,actions=meter:1,br1-p1,br1-p2]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br2 in_port=br2-p0,ip,actions=meter:1,br2-p1]) + +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | grep "Datapath actions:"], [0], [dnl +Datapath actions: clone(meter(0),clone(meter(1),102),101),100 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto-dpif - patch ports - ct (clone)]) + +OVS_VSWITCHD_START( + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 ofport_request=2 -- \ + add-br br1 -- \ + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 fail-mode=secure -- \ + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=p1,p0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "in_port=1,ip,actions=resubmit(,3)"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=3,in_port=1,ip,ct_state=-trk,actions=ct(table=0)"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=3,in_port=1,ip,ct_state=+trk,actions=p3"]) + +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | strip_recirc | grep "Datapath actions:"], [0], [dnl +Datapath actions: clone(ct,recirc()),1 +Datapath actions: 3 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + + +AT_SETUP([ofproto-dpif - patch ports - trunc (clone)]) + +OVS_VSWITCHD_START( + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 ofport_request=2 -- \ + add-br br1 -- \ + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 fail-mode=secure -- \ + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=p1,p0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "in_port=1,ip,actions=resubmit(,3)"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=3,in_port=1,ip,actions=output(port=p3, max_len=20)"]) + +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | grep "Datapath actions:"], [0], [dnl +Datapath actions: clone(trunc(20),3),1 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto-dpif - patch ports - encap (clone)]) + +OVS_VSWITCHD_START( + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 ofport_request=2 -- \ + add-br br1 -- \ + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 fail-mode=secure -- \ + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "in_port=local,ip,actions=encap(nsh()),encap(ethernet),p1,p0"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "in_port=1,dl_type=0x894f,actions=decap(),decap(),p3"]) + +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | strip_recirc | grep "Datapath actions:"], [0], [dnl +Datapath actions: push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x0,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=00:00:00:00:00:00),clone(pop_eth,pop_nsh(),recirc()),1 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto-dpif - patch ports - check_pkt_larger (clone)]) + +OVS_VSWITCHD_START( + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 ofport_request=2 -- \ + add-br br1 -- \ + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 fail-mode=secure -- \ + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) + + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "in_port=local,ip,actions=p1,p0"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=0,in_port=1,actions=check_pkt_larger(200)->reg0[[0]],resubmit(,1)"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=1,in_port=1,priority=200,reg0=0x1/0x1,ip,actions=ct(table=1)"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=1,in_port=1,ip,ct_state=+trk,actions=p3"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 "table=1,in_port=1,priority=0,ip,actions=p3"]) + +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | strip_recirc | grep "Datapath actions:"], [0], [dnl +Datapath actions: clone(check_pkt_len(size=200,gt(ct,recirc()),le(3))),1 +Datapath actions: 3 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP