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 <adrien.mazarg...@6wind.com>
> > Reviewed-by: Andrew Rybchenko <arybche...@oktetlabs.ru>
> > Cc: Ajit Khaparde <ajit.khapa...@broadcom.com>
> > Cc: Wenzhuo Lu <wenzhuo...@intel.com>
> > Cc: John Daley <johnd...@cisco.com>
> > Cc: Gaetan Rivet <gaetan.ri...@6wind.com>
> > Cc: Beilei Xing <beilei.x...@intel.com>
> > Cc: Konstantin Ananyev <konstantin.anan...@intel.com>
> > Cc: Nelio Laranjeiro <nelio.laranje...@6wind.com>
> > Cc: Andrew Rybchenko <arybche...@solarflare.com>
> > Cc: Pascal Mazon <pascal.ma...@6wind.com>
> > ---
> > 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
