DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ori Kam <orika@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>,
	Shahaf Shuler <shahafs@nvidia.com>,
	Gregory Etelson <getelson@nvidia.com>,
	NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH 1/3] ethdev: update modify field flow action
Date: Mon, 4 Oct 2021 09:38:51 +0000
Message-ID: <DM8PR12MB5400BC382E032FA89AF0992DD6AE9@DM8PR12MB5400.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20211001195223.31909-2-viacheslavo@nvidia.com>

Hi Slava,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Friday, October 1, 2021 10:52 PM
> Subject: [PATCH 1/3] ethdev: update modify field flow action
> 
> The generic modify field flow action introduced in [1] has some issues related
> to the immediate source operand:
> 
>   - immediate source can be presented either as an unsigned
>     64-bit integer or pointer to data pattern in memory.
>     There was no explicit pointer field defined in the union
> 
>   - the byte ordering for 64-bit integer was not specified.
>     Many fields have lesser lengths and byte ordering
>     is crucial.
> 
>   - how the bit offset is applied to the immediate source
>     field was not defined and documented
> 
>   - 64-bit integer size is not enough to provide MAC and

I think for mac it is enough.

>     IPv6 addresses
> 
> In order to cover the issues and exclude any ambiguities the following is
> done:
> 
>   - introduce the explicit pointer field
>     in rte_flow_action_modify_data structure
> 
>   - replace the 64-bit unsigned integer with 16-byte array
> 
>   - update the modify field flow action documentation
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     |  8 ++++++++
>  doc/guides/rel_notes/release_21_11.rst |  7 +++++++
>  lib/ethdev/rte_flow.h                  | 15 ++++++++++++---
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..a54760a7b4 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2835,6 +2835,14 @@ a packet to any other part of it.
>  ``value`` sets an immediate value to be used as a source or points to a
> location of the value in memory. It is used instead of ``level`` and ``offset``
> for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER``
> respectively.
> +The data in memory should be presented exactly in the same byte order
> +and length as in the relevant flow item, i.e. data for field with type
> +RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field in
> +rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
> +rte_flow_item_ipv6 conventions, and so on. The bitfield exatracted from
> +the memory being applied as second operation parameter is defined by
> +width and the destination field offset. If the field size is large than
> +16 bytes the pattern can be provided as pointer only.
> 
You should specify where is the offset of the src is taken from.
Per your example if the application wants to change the 2 byte of source mac
it should giveas an imidate value 6 bytes, with the second byte as the new value to set
so from where do it takes the offset? Since offset is not valid in case of immediate value.
I assume it is based on the offset of the destination.

>  .. _table_rte_flow_action_modify_field:
> 
> diff --git a/doc/guides/rel_notes/release_21_11.rst
> b/doc/guides/rel_notes/release_21_11.rst
> index 73e377a007..7db6cccab0 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -170,6 +170,10 @@ API Changes
>    the crypto/security operation. This field will be used to communicate
>    events such as soft expiry with IPsec in lookaside mode.
> 
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated, immediate
> +data
> +  array is extended, data pointer field is explicitly added to union,
> +the
> +  action behavior is defined in more strict fashion and documentation
> uddated.
> +
Uddated ->updated?
I think it is important to document here that the behavior has changed,
from seting only the relevant value to update to setting all the field and
the mask is done internally.

> 
>  ABI Changes
>  -----------
> @@ -206,6 +210,9 @@ ABI Changes
>    and hard expiry limits. Limits can be either in number of packets or bytes.
> 
> 
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
> +
> +
>  Known Issues
>  ------------
> 
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> 7b1ed7f110..af4c693ead 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3204,6 +3204,9 @@ enum rte_flow_field_id {  };
> 
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
>   * Field description for MODIFY_FIELD action.
>   */
>  struct rte_flow_action_modify_data {
> @@ -3217,10 +3220,16 @@ struct rte_flow_action_modify_data {
>  			uint32_t offset;
>  		};
>  		/**
> -		 * Immediate value for RTE_FLOW_FIELD_VALUE or
> -		 * memory address for RTE_FLOW_FIELD_POINTER.
> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
> in the
> +		 * same byte order and length as in relevant
> rte_flow_item_xxx.

Please see my comment about how to get the offset.

>  		 */
> -		uint64_t value;
> +		uint8_t value[16];
> +		/*
> +		 * Memory address for RTE_FLOW_FIELD_POINTER, memory
> layout
> +		 * should be the same as for relevant field in the
> +		 * rte_flow_item_xxx structure.

I assume also in this case the offset comes from the dest right?

> +		 */
> +		void *pvalue;
>  	};
>  };
> 
> --
> 2.18.1

Thanks,
Ori


  reply	other threads:[~2021-10-04  9:38 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 14:16 [dpdk-dev] [RFC " Viacheslav Ovsiienko
2021-09-10 14:16 ` [dpdk-dev] [RFC 2/3] app/testpmd: update modify field flow action support Viacheslav Ovsiienko
2021-09-10 14:16 ` [dpdk-dev] [RFC 3/3] app/testpmd: fix hex string parser in flow commands Viacheslav Ovsiienko
2021-10-01 19:52 ` [dpdk-dev] [PATCH 0/3] ethdev: update modify field flow action Viacheslav Ovsiienko
2021-10-01 19:52   ` [dpdk-dev] [PATCH 1/3] " Viacheslav Ovsiienko
2021-10-04  9:38     ` Ori Kam [this message]
2021-10-01 19:52   ` [dpdk-dev] [PATCH 2/3] app/testpmd: update modify field flow action support Viacheslav Ovsiienko
2021-10-01 19:52   ` [dpdk-dev] [PATCH 3/3] app/testpmd: fix hex string parser in flow commands Viacheslav Ovsiienko
2021-10-10 23:45 ` [dpdk-dev] [PATCH v2 0/5] ethdev: update modify field flow action Viacheslav Ovsiienko
2021-10-10 23:45   ` [dpdk-dev] [PATCH v2 1/5] " Viacheslav Ovsiienko
2021-10-11  7:19     ` Ori Kam
2021-10-11  9:54     ` Andrew Rybchenko
2021-10-10 23:45   ` [dpdk-dev] [PATCH v2 2/5] app/testpmd: update modify field flow action support Viacheslav Ovsiienko
2021-10-11  9:13     ` Ori Kam
2021-10-10 23:45   ` [dpdk-dev] [PATCH v2 3/5] app/testpmd: fix hex string parser in flow commands Viacheslav Ovsiienko
2021-10-11  9:15     ` Ori Kam
2021-10-10 23:45   ` [dpdk-dev] [PATCH v2 4/5] net/mlx5: update modify field action Viacheslav Ovsiienko
2021-10-10 23:45   ` [dpdk-dev] [PATCH v2 5/5] doc: remove modify field action data deprecation notice Viacheslav Ovsiienko
2021-10-11  9:14     ` Ori Kam
2021-10-11  9:31     ` Andrew Rybchenko
2021-10-12  8:06 ` [dpdk-dev] [PATCH v3 0/5] ethdev: update modify field flow action Viacheslav Ovsiienko
2021-10-12  8:06   ` [dpdk-dev] [PATCH v3 1/5] " Viacheslav Ovsiienko
2021-10-12  8:16     ` Andrew Rybchenko
2021-10-12  8:06   ` [dpdk-dev] [PATCH v3 2/5] ethdev: fix missed experimental tag for modify field action Viacheslav Ovsiienko
2021-10-12  8:13     ` Ori Kam
2021-10-12  8:14     ` Andrew Rybchenko
2021-10-12  8:06   ` [dpdk-dev] [PATCH v3 3/5] app/testpmd: update modify field flow action support Viacheslav Ovsiienko
2021-10-12  8:06   ` [dpdk-dev] [PATCH v3 4/5] app/testpmd: fix hex string parser in flow commands Viacheslav Ovsiienko
2021-10-12  8:06   ` [dpdk-dev] [PATCH v3 5/5] net/mlx5: update modify field action Viacheslav Ovsiienko
2021-10-12 20:25 ` [dpdk-dev] [PATCH v5 0/5] ethdev: update modify field flow action Viacheslav Ovsiienko
2021-10-12 20:25   ` [dpdk-dev] [PATCH v5 1/5] " Viacheslav Ovsiienko
2021-10-12 20:25   ` [dpdk-dev] [PATCH v5 2/5] ethdev: fix missed experimental tag for modify field action Viacheslav Ovsiienko
2021-10-12 20:25   ` [dpdk-dev] [PATCH v5 3/5] app/testpmd: update modify field flow action support Viacheslav Ovsiienko
2021-10-12 20:25   ` [dpdk-dev] [PATCH v5 4/5] app/testpmd: fix hex string parser in flow commands Viacheslav Ovsiienko
2021-10-12 20:25   ` [dpdk-dev] [PATCH v5 5/5] net/mlx5: update modify field action Viacheslav Ovsiienko
2021-10-13 13:46   ` [dpdk-dev] [PATCH v5 0/5] ethdev: update modify field flow action Ferruh Yigit
2021-10-13 18:45 ` [dpdk-dev] [PATCH v6 " Viacheslav Ovsiienko
2021-10-13 18:45   ` [dpdk-dev] [PATCH v6 1/5] " Viacheslav Ovsiienko
2021-10-14 12:08     ` Ferruh Yigit
2021-10-14 20:07       ` Slava Ovsiienko
2021-10-13 18:45   ` [dpdk-dev] [PATCH v6 2/5] ethdev: fix missed experimental tag for modify field action Viacheslav Ovsiienko
2021-10-13 18:45   ` [dpdk-dev] [PATCH v6 3/5] app/testpmd: update modify field flow action support Viacheslav Ovsiienko
2021-10-13 18:45   ` [dpdk-dev] [PATCH v6 4/5] app/testpmd: fix hex string parser in flow commands Viacheslav Ovsiienko
2021-10-13 18:45   ` [dpdk-dev] [PATCH v6 5/5] net/mlx5: update modify field action Viacheslav Ovsiienko
2021-10-14 12:37   ` [dpdk-dev] [PATCH v6 0/5] ethdev: update modify field flow action Ferruh Yigit

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=DM8PR12MB5400BC382E032FA89AF0992DD6AE9@DM8PR12MB5400.namprd12.prod.outlook.com \
    --to=orika@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=getelson@nvidia.com \
    --cc=matan@nvidia.com \
    --cc=rasland@nvidia.com \
    --cc=shahafs@nvidia.com \
    --cc=thomas@monjalon.net \
    --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 http://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/ http://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