Re: [dpdk-dev] [PATCH] net/mlx5: support modify field rte flow action

[Alexander Kozyrev <akozyrev@nvidia.com>]

< From: Slava Ovsiienko <viacheslavo@nvidia.com> on Wednesday, January 27, 2021 6:37
>
> Hi, Alexander
> 
> Please, see my minor comments below.
> 
> Besides this:
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> 
> > -----Original Message-----
> > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > Sent: Wednesday, January 27, 2021 4:49
> > To: dev@dpdk.org
> > Cc: Raslan Darawsheh <rasland@nvidia.com>; Slava Ovsiienko
> > <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; Ori Kam
> > <orika@nvidia.com>
> > Subject: [PATCH] net/mlx5: support modify field rte flow action
> >
> > Add support for new MODIFY_FIELD action to the Mellanox PMD.
> > This is the generic API that allows to manipulate any packet header field by
> > copying data from another packet field or mark, metadata, tag, or
> immediate
> > value (or pointer to it).
> >
> > Since the API is generic and covers a lot of action under its umbrella it
> makes
> > sense to implement all the mechanics gradually in order to move to this API
> > for any packet field manipulations in the future. This is first step of RTE
> flows
> Typo: "the first"
> 
> > consolidation.
> >
> > The modify field RTE flow supports three operations: set, add and sub. This
> Missed API|action?   Should it be "The modify field RTE flow action supports"
> ?
Thanks, will fix typos.

> > patch brings to live only the "set" operation.
> > Support is provided for any packet header field as well as meta/tag/mark
> and
> > immediate value can be used as a source.
> >
> > There are few limitations for this first version of API support:
> > - encapsulation levels are not supported, just outermost header can be
> > manipulated for now.
> > - offsets can only be 4-bytes aligned: 32, 64 and 96 for IPv6.
> > - the special ITEM_START ID is not supported as we do not allow to cross
> > packet header field boundaries yet.
> >
> > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > ---
> >  drivers/net/mlx5/mlx5_flow.h    |   4 +-
> >  drivers/net/mlx5/mlx5_flow_dv.c | 507
> > +++++++++++++++++++++++++++++++-
> >  2 files changed, 508 insertions(+), 3 deletions(-)
> >
> mlx5 documentation update?
> release notes update?
Let me add them.

> > diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> > index 2178a04e3a..6f6828c6a1 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -219,6 +219,7 @@ enum mlx5_feature_name {  #define
> > MLX5_FLOW_ACTION_SAMPLE (1ull << 36)  #define
> > MLX5_FLOW_ACTION_TUNNEL_SET (1ull << 37)  #define
> > MLX5_FLOW_ACTION_TUNNEL_MATCH (1ull << 38)
> > +#define MLX5_FLOW_ACTION_MODIFY_FIELD (1ull << 39)
> >
> >  #define MLX5_FLOW_FATE_ACTIONS \
> >  	(MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_QUEUE | \
> > @@ -249,7 +250,8 @@ enum mlx5_feature_name {
> >  				      MLX5_FLOW_ACTION_MARK_EXT | \
> >  				      MLX5_FLOW_ACTION_SET_META | \
> >  				      MLX5_FLOW_ACTION_SET_IPV4_DSCP | \
> > -				      MLX5_FLOW_ACTION_SET_IPV6_DSCP)
> > +				      MLX5_FLOW_ACTION_SET_IPV6_DSCP | \
> > +				      MLX5_FLOW_ACTION_MODIFY_FIELD)
> >
> >  #define MLX5_FLOW_VLAN_ACTIONS
> (MLX5_FLOW_ACTION_OF_POP_VLAN
> > | \
> >  				MLX5_FLOW_ACTION_OF_PUSH_VLAN)
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > b/drivers/net/mlx5/mlx5_flow_dv.c index 1a0c0be680..d842dc9887 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -209,6 +209,8 @@ rte_col_2_mlx5_col(enum rte_color rcol)
> >  	return MLX5_FLOW_COLOR_UNDEFINED;
> >  }
> >
> > +#define MLX5DV_FLOW_MAX_MOD_FIELDS 5
> Let's move to the mlx5_flow.h, closer to other #defines
Ok.

> > +
> >  struct field_modify_info {
> >  	uint32_t size; /* Size of field in protocol header, in bytes. */
> >  	uint32_t offset; /* Offset of field in protocol header, in bytes. */ @@
> -
> > 431,6 +433,9 @@ flow_dv_convert_modify_action(struct rte_flow_item
> > *item,
> >  				(int)dcopy->offset < 0 ? off_b : dcopy-
> >offset;
> >  			/* Convert entire record to big-endian format. */
> >  			actions[i].data1 =
> rte_cpu_to_be_32(actions[i].data1);
> > +			++dcopy;
> > +			if (!dcopy)
> > +				break;
> Sorry, I do not follow. dcopy is the structure pointer (we assume it points to
> the array).
> How pointer can be NULL after update ++dcopy ?
Just a precaution if we go past the array due to some bug.

> >  		} else {
> >  			MLX5_ASSERT(item->spec);
> >  			data = flow_dv_fetch_field((const uint8_t *)item-
> > >spec + @@ -1324,6 +1329,339 @@
> > flow_dv_convert_action_modify_ipv6_dscp
> >  					     MLX5_MODIFICATION_TYPE_SET,
> > error);  }
> >
> > +static void
> > +mlx5_flow_field_id_to_modify_info
> > +		(const struct rte_flow_action_modify_data *data,
> > +		 struct field_modify_info *info,
> > +		 uint32_t *mask, uint32_t *value,
> > +		 struct rte_eth_dev *dev,
> > +		 const struct rte_flow_attr *attr,
> > +		 struct rte_flow_error *error)
> > +{
> > +	uint32_t idx = 0;
> > +	switch (data->field) {
> > +	case RTE_FLOW_FIELD_START:
> > +		/* not supported yet */
> > +		break;
> Let's move to default? To catch validation gap?
Ok.

> > +	case RTE_FLOW_FIELD_MAC_DST:
> > +		if (mask) {
> > +			info[idx] = (struct field_modify_info){4, 0,
> > +
> > 	MLX5_MODI_OUT_DMAC_47_16};
> > +			mask[idx] = (data->offset == 32) ? 0x0 : 0xffffffff;
> With zero mask we could just skip the element here
> (not critical - anyway, it will be dropped at conversion stage to HW actions)
That is the ides - to skip element at the conversion stage.