From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v4 05/16] ethdev: alter behavior of flow API actions Date: Wed, 18 Apr 2018 16:58:38 +0200 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-Transfer-Encoding: 8bit Cc: Thomas Monjalon , Ferruh Yigit , dev@dpdk.org, Ajit Khaparde , Wenzhuo Lu , John Daley , Gaetan Rivet , Beilei Xing , Konstantin Ananyev , Nelio Laranjeiro , Pascal Mazon To: Andrew Rybchenko 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) Content-Disposition: inline In-Reply-To: <1caed91d-c762-7e5c-af33-a6dd5f1ef319@solarflare.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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