DPDK patches and discussions
 help / color / mirror / Atom feed
From: Alexander Kozyrev <akozyrev@nvidia.com>
To: Slava Ovsiienko <viacheslavo@nvidia.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Raslan Darawsheh <rasland@nvidia.com>,
	Matan Azrad <matan@nvidia.com>, Ori Kam <orika@nvidia.com>
Subject: Re: [dpdk-dev] [PATCH] net/mlx5: support modify field rte flow action
Date: Wed, 27 Jan 2021 14:58:25 +0000
Message-ID: <BN7PR12MB27078B5788381B1A5DA4E609AFBB9@BN7PR12MB2707.namprd12.prod.outlook.com> (raw)
In-Reply-To: <DM6PR12MB37534E3EC378A042C1D74FBFDFBB9@DM6PR12MB3753.namprd12.prod.outlook.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.

  reply	other threads:[~2021-01-27 14:58 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-27  2:48 Alexander Kozyrev
2021-01-27 11:36 ` Slava Ovsiienko
2021-01-27 14:58   ` Alexander Kozyrev [this message]
2021-01-28  7:39 ` [dpdk-dev] [PATCH v2] " Alexander Kozyrev
2021-01-28  8:00 ` [dpdk-dev] [PATCH v3] " Alexander Kozyrev
2021-01-28  9:26   ` Raslan Darawsheh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BN7PR12MB27078B5788381B1A5DA4E609AFBB9@BN7PR12MB2707.namprd12.prod.outlook.com \
    --to=akozyrev@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=matan@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=rasland@nvidia.com \
    --cc=viacheslavo@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git