Re: [PATCH v4 05/16] ethdev: alter behavior of flow API actions

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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.mazarguil@6wind.com>
> > Reviewed-by: Andrew Rybchenko <arybchenko@oktetlabs.ru>
> > Cc: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Cc: John Daley <johndale@cisco.com>
> > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > Cc: Beilei Xing <beilei.xing@intel.com>
> > Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Cc: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> > Cc: Pascal Mazon <pascal.mazon@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