From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id E2CE98E67 for ; Wed, 18 Apr 2018 16:58:52 +0200 (CEST) Received: by mail-wr0-f195.google.com with SMTP id z73-v6so5722100wrb.0 for ; Wed, 18 Apr 2018 07:58:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=PLLcSOcVgpUNjSLqIqTKPMVjgfZ8K8X8HzPt0yd5uyU=; b=1S6PUJgd3nKFnt+tv8QipRmD2JyrDYLnr4PWpd4Dcx1q0YFC+eoALU/Eu5UBGeM6Cf 5Pi1nmpL4r66sKJ+7Y38maQMXxpD1CyFFq30nVSj+RRRQTevJ78OAW//65Ut1ZeIRUPl tWYEjZbC+yOc8gOAixj6gWYfxHxdkf06K7CXR/eOBdjiUR4KRck+pt+5I1w5FsiwatfQ 6y9vWnFYYnmrgfMdW43TtRk79ss8eUd2GQwIjr8idpGS1Tw//lzdQjpX/bK6lIoYRKq5 mXWuWOjC117wJ2amv/xLuTATU2hyLeDGzdmSr2Ghuz1Gx5VccM6oL5l4+SzrTeOH2wb3 EyNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=PLLcSOcVgpUNjSLqIqTKPMVjgfZ8K8X8HzPt0yd5uyU=; b=gLiFYSBRo1RoNr9bKVRpE06Z2HEKV5lVOx/EJCdYB9msrhFxB1DHaZeipCo5tvQ2U/ 2wu19p0TLCfANjN/aJLaEpp46FbrqqetdyZDthpmp8VTCyFK9tuSyxFZ0PgPzeDZarDk OUV9Q7PkrEYLOgEl7Mot//ptk4GTYblYc1KIPHUt4+b45bMAmaEv9xG3odDTUu+c8AUv QZYCiqsSR1LwRlcqS1s2gUOY/DK78bMPgmliH/2kq0hFxKhHYNhxI9spMwoUraWwjVLk H3d66tlNRhu1g8AaDry9bjQtF1XDp64hRGlixR1leIbtLOmVH7nH4Pc1TXN/r4On+Z4q pRGw== X-Gm-Message-State: ALQs6tDw/eLKIFCJhrJUavFoyUc0pt08VeOlIaHsb4fE1qQYob8mjCn5 IvVwQ51sChVj+sfi/+2VrtleCkiq X-Google-Smtp-Source: AIpwx4/ABKEutCJ/ljP/NFmspSEFACheULWZ1fPABtP1FH9rpDqdhBE5WX+/os13Vm8x+STypsUJHA== X-Received: by 10.28.155.81 with SMTP id d78mr2046662wme.61.1524063532617; Wed, 18 Apr 2018 07:58:52 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id a139sm2559921wma.43.2018.04.18.07.58.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Apr 2018 07:58:51 -0700 (PDT) Date: Wed, 18 Apr 2018 16:58:38 +0200 From: Adrien Mazarguil To: Andrew Rybchenko Cc: Thomas Monjalon , Ferruh Yigit , dev@dpdk.org, Ajit Khaparde , Wenzhuo Lu , John Daley , Gaetan Rivet , Beilei Xing , Konstantin Ananyev , Nelio Laranjeiro , Pascal Mazon Message-ID: <20180418145838.GU4957@6wind.com> References: <20180410162022.9101-1-adrien.mazarguil@6wind.com> <20180416150525.2817-1-adrien.mazarguil@6wind.com> <20180416150525.2817-6-adrien.mazarguil@6wind.com> <1caed91d-c762-7e5c-af33-a6dd5f1ef319@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1caed91d-c762-7e5c-af33-a6dd5f1ef319@solarflare.com> Subject: Re: [dpdk-dev] [PATCH v4 05/16] ethdev: alter behavior of flow API actions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Apr 2018 14:58:53 -0000 On Wed, Apr 18, 2018 at 03:26:00PM +0300, Andrew Rybchenko wrote: > On 04/16/2018 07:22 PM, Adrien Mazarguil wrote: > > This patch makes the following changes to flow rule actions: > > > > - List order now matters, they are redefined as performed first to last > > instead of "all simultaneously". > > > > - Repeated actions are now supported (e.g. specifying QUEUE multiple times > > now duplicates traffic among them). Previously only the last action of > > any given kind was taken into account. > > > > - No more distinction between terminating/non-terminating/meta actions. > > Flow rules themselves are now defined as always terminating unless a > > PASSTHRU action is specified. > > > > These changes alter the behavior of flow rules in corner cases in order to > > prepare the flow API for actions that modify traffic contents or properties > > (e.g. encapsulation, compression) and for which order matter when combined. > > > > Previously one would have to do so through multiple flow rules by combining > > PASSTRHU with priority levels, however this proved overly complex to > > implement at the PMD level, hence this simpler approach. > > > > This breaks ABI compatibility for the following public functions: > > > > - rte_flow_create() > > - rte_flow_validate() > > > > PMDs with rte_flow support are modified accordingly: > > > > - bnxt: no change, implementation already forbids multiple actions and does > > not support PASSTHRU. > > > > - e1000: no change, same as bnxt. > > > > - enic: modified to forbid redundant actions, no support for default drop. > > > > - failsafe: no change needed. > > > > - i40e: no change, implementation already forbids multiple actions. > > > > - ixgbe: same as i40e. > > > > - mlx4: modified to forbid multiple fate-deciding actions and drop when > > unspecified. > > > > - mlx5: same as mlx4, with other redundant actions also forbidden. > > > > - sfc: same as mlx4. > > > > - tap: implementation already complies with the new behavior except for > > the default pass-through modified as a default drop. > > > > Signed-off-by: Adrien Mazarguil > > Reviewed-by: Andrew Rybchenko > > Cc: Ajit Khaparde > > Cc: Wenzhuo Lu > > Cc: John Daley > > Cc: Gaetan Rivet > > Cc: Beilei Xing > > Cc: Konstantin Ananyev > > Cc: Nelio Laranjeiro > > Cc: Andrew Rybchenko > > Cc: Pascal Mazon > > --- > > doc/guides/prog_guide/rte_flow.rst | 67 +++++++++++++------------------- > > drivers/net/enic/enic_flow.c | 25 ++++++++++++ > > drivers/net/mlx4/mlx4_flow.c | 21 +++++++--- > > drivers/net/mlx5/mlx5_flow.c | 69 ++++++++++++++------------------- > > drivers/net/sfc/sfc_flow.c | 22 +++++++---- > > drivers/net/tap/tap_flow.c | 11 ++++++ > > lib/librte_ether/rte_flow.h | 54 +++++++------------------- > > 7 files changed, 138 insertions(+), 131 deletions(-) > > [...] > > > diff --git a/drivers/net/enic/enic_flow.c b/drivers/net/enic/enic_flow.c > > index b9f36587c..a5c6a1670 100644 > > --- a/drivers/net/enic/enic_flow.c > > +++ b/drivers/net/enic/enic_flow.c > > @@ -975,6 +979,10 @@ enic_copy_action_v1(const struct rte_flow_action actions[], > > const struct rte_flow_action_queue *queue = > > (const struct rte_flow_action_queue *) > > actions->conf; > > + > > + if (overlap & FATE) > > + return ENOTSUP; > > + overlap |= FATE; > > enic_action->rq_idx = > > enic_rte_rq_idx_to_sop_idx(queue->index); > > break; > > @@ -984,6 +992,8 @@ enic_copy_action_v1(const struct rte_flow_action actions[], > > break; > > } > > } > > + if (!overlap & FATE) > > Build using clang on Ubuntu 17.10 fails: > > /var/tmp/dpdk-next-net/drivers/net/enic/enic_flow.c:1000:6: fatal error: > logical not is only applied to >       the left hand side of this bitwise operator > [-Wlogical-not-parentheses] >         if (!overlap & FATE) >             ^        ~ > /var/tmp/dpdk-next-net/drivers/net/enic/enic_flow.c:1000:6: note: add > parentheses after the '!' to >       evaluate the bitwise operator first >         if (!overlap & FATE) >             ^ >              (             ) > /var/tmp/dpdk-next-net/drivers/net/enic/enic_flow.c:1000:6: note: add > parentheses around left hand side >       expression to silence this warning >         if (!overlap & FATE) >             ^ >             (       ) > 1 error generated. > /var/tmp/dpdk-next-net/mk/internal/rte.compile-pre.mk:114: recipe for target > 'enic_flow.o' failed > make[4]: *** [enic_flow.o] Error 1 > /var/tmp/dpdk-next-net/mk/rte.subdir.mk:35: recipe for target 'enic' failed > make[3]: *** [enic] Error 2 >   CC nfp_cpp_pcie_ops.o > make[3]: *** Waiting for unfinished jobs.... > > $ clang --version > clang version 4.0.1-6 (tags/RELEASE_401/final) > Target: x86_64-pc-linux-gnu > Thread model: posix > InstalledDir: /usr/bin > > > > + return ENOTSUP; > > enic_action->type = FILTER_ACTION_RQ_STEERING; > > return 0; > > } > > @@ -1001,6 +1011,9 @@ static int > > enic_copy_action_v2(const struct rte_flow_action actions[], > > struct filter_action_v2 *enic_action) > > { > > + enum { FATE = 1, MARK = 2, }; > > + uint32_t overlap = 0; > > + > > FLOW_TRACE(); > > for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > > @@ -1009,6 +1022,10 @@ enic_copy_action_v2(const struct rte_flow_action actions[], > > const struct rte_flow_action_queue *queue = > > (const struct rte_flow_action_queue *) > > actions->conf; > > + > > + if (overlap & FATE) > > + return ENOTSUP; > > + overlap |= FATE; > > enic_action->rq_idx = > > enic_rte_rq_idx_to_sop_idx(queue->index); > > enic_action->flags |= FILTER_ACTION_RQ_STEERING_FLAG; > > @@ -1019,6 +1036,9 @@ enic_copy_action_v2(const struct rte_flow_action actions[], > > (const struct rte_flow_action_mark *) > > actions->conf; > > + if (overlap & MARK) > > Same > > > + return ENOTSUP; > > + overlap |= MARK; > > /* ENIC_MAGIC_FILTER_ID is reserved and is the highest > > * in the range of allows mark ids. > > */ > > @@ -1029,6 +1049,9 @@ enic_copy_action_v2(const struct rte_flow_action actions[], > > break; > > } > > case RTE_FLOW_ACTION_TYPE_FLAG: { > > + if (overlap & MARK) > > + return ENOTSUP; > > + overlap |= MARK; > > enic_action->filter_id = ENIC_MAGIC_FILTER_ID; > > enic_action->flags |= FILTER_ACTION_FILTER_ID_FLAG; > > break; > > @@ -1044,6 +1067,8 @@ enic_copy_action_v2(const struct rte_flow_action actions[], > > break; > > } > > } > > + if (!overlap & FATE) > > Same > > > + return ENOTSUP; > > enic_action->type = FILTER_ACTION_V2; > > return 0; > > } > > [...] Thanks for reporting. These "!overlap" checks are indeed messy, oddly my own compilation tests with GCC and clang did not report them. I'll submit an updated version. -- Adrien Mazarguil 6WIND