diff mbox series

ethdev: introduce generic copy rte flow action

Message ID 20210108063234.7679-1-akozyrev@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series ethdev: introduce generic copy rte flow action |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing fail Testing issues
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Alexander Kozyrev Jan. 8, 2021, 6:32 a.m. UTC 
  Implement a generic copy flow API to allow copying of an arbitrary
header field (as well as mark, metadata or tag) to another item.

This generic copy mechanism removes the necessity to implement a
separate RTE Flow action every time we need to modify a new packet
field in the future. A user-provided value can be used from a
specified tag/metadata or directly copied from other packet field.

The number of bits to copy as well as the offset to start from can
be specified to allow a partial copy or copy into an arbitrary
place in a packet for greater flexibility.

RFC: http://patches.dpdk.org/patch/85384/

Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
 lib/librte_ethdev/rte_flow.c       |  1 +
 lib/librte_ethdev/rte_flow.h       | 59 ++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

Comments

Ori Kam Jan. 10, 2021, 8 a.m. UTC | #1 
Hi Alexander,

I guess that the test-pmd part will be available later right?

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Friday, January 8, 2021 8:33 AM
> Subject: [PATCH] ethdev: introduce generic copy rte flow action
> 
> Implement a generic copy flow API to allow copying of an arbitrary
> header field (as well as mark, metadata or tag) to another item.
> 
> This generic copy mechanism removes the necessity to implement a
> separate RTE Flow action every time we need to modify a new packet
> field in the future. A user-provided value can be used from a
> specified tag/metadata or directly copied from other packet field.
> 
> The number of bits to copy as well as the offset to start from can
> be specified to allow a partial copy or copy into an arbitrary
> place in a packet for greater flexibility.
> 

Since the question why you are using enum and not just offset from 
the start of the packet, was discussed and raised by number of people it will be best
if it will appear in the commit log, at least the advantages to this implementation.

> RFC:
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.d
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> 7Cd04c2e49c3a840994da408d8b39f3304%7C43083d15727340c1b7db39efd9cc
> c17a%7C0%7C0%7C637456843629116269%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 1000&amp;sdata=85096rASBtzbjU42pV6sPkl3nVt5HlR6%2BL9nxI3qgFA%3D&a
> mp;reserved=0
> 
> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
>  lib/librte_ethdev/rte_flow.c       |  1 +
>  lib/librte_ethdev/rte_flow.h       | 59 ++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 86b3444803..b737ff9dad 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2766,6 +2766,41 @@ The behaviour of the shared action defined by
> ``action`` argument of type
>     | no properties |
>     +---------------+
> 
> +Action: ``COPY_ITEM``
> +^^^^^^^^^^^^^^^^^^^^^
> +
> +Copy ``width`` bits from ``src`` item to ``dst`` item.
> +
> +An arbitrary header field (as well as mark, metadata or tag values)
> +can be used as both source and destination items as set by ``item``.
> +

For tag I think you should also use the index right?

> +Inner packet header fields can be accessed using the ``index`` and
> +it is possible to start the copy from the ``offset`` bits in an item.

Please specify  what means index 0 /1 ... 0 is outer most? Inner most?
You can look at the RSS level for reference.
I think it will be best to use the same values.

What happens if we want to copy between different sizes?
for example copy IPV6 src to number of tags? I assume we will be using offset and
split the copy command to number of actions right?

> +
> +.. _table_rte_flow_action_copy_item:
> +
> +.. table:: COPY_ITEM
> +
> +   +-----------------------------------------+
> +   | Field         | Value                   |
> +   +===============+=========================+
> +   | ``dst``       | destination item        |
> +   | ``src``       | source item             |
> +   | ``width``     | number of bits to copy  |
> +   +---------------+-------------------------+
> +
> +.. _table_rte_flow_action_copy_data:
> +
> +.. table:: destination/source item definition
> +
> +   +----------------------------------------------------------+
> +   | Field         | Value                                    |
> +   +===============+==========================================+
> +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> +   | ``index``     | index of outer/inner header or tag array |
> +   | ``offset``    | number of bits to skip during the copy   |
> +   +---------------+------------------------------------------+
> +
>  Negative types
>  ~~~~~~~~~~~~~~
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index a06f64c271..fdbabefc47 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -176,6 +176,7 @@ static const struct rte_flow_desc_data
> rte_flow_desc_action[] = {
>  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
>  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
>  	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> +	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct
> rte_flow_action_copy_item)),
>  	/**
>  	 * Shared action represented as handle of type
>  	 * (struct rte_flow_shared action *) stored in conf field (see
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 0977a78270..0540c861fb 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2198,6 +2198,16 @@ enum rte_flow_action_type {
>  	 * struct rte_flow_shared_action).
>  	 */
>  	RTE_FLOW_ACTION_TYPE_SHARED,
> +
> +	/**
> +	 * Copy a packet header field, tag, mark or metadata.
> +	 *
> +	 * Allow saving an arbitrary header field by copying its value
> +	 * to a tag/mark/metadata or copy it into another header field.
> +	 *
> +	 * See struct rte_flow_action_copy_item.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
>  };
> 
>  /**
> @@ -2791,6 +2801,55 @@ struct rte_flow_action_set_dscp {
>   */
>  struct rte_flow_shared_action;
> 
> +enum rte_flow_item_id {
> +	RTE_FLOW_ITEM_NONE = 0,
> +	RTE_FLOW_ITEM_MAC_DST,
> +	RTE_FLOW_ITEM_MAC_SRC,
> +	RTE_FLOW_ITEM_VLAN_TYPE,
> +	RTE_FLOW_ITEM_VLAN_ID,
> +	RTE_FLOW_ITEM_MAC_TYPE,
> +	RTE_FLOW_ITEM_IPV4_DSCP,
> +	RTE_FLOW_ITEM_IPV4_TTL,
> +	RTE_FLOW_ITEM_IPV4_SRC,
> +	RTE_FLOW_ITEM_IPV4_DST,
> +	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
> +	RTE_FLOW_ITEM_IPV6_SRC,
> +	RTE_FLOW_ITEM_IPV6_DST,
> +	RTE_FLOW_ITEM_TCP_PORT_SRC,
> +	RTE_FLOW_ITEM_TCP_PORT_DST,
> +	RTE_FLOW_ITEM_TCP_SEQ_NUM,
> +	RTE_FLOW_ITEM_TCP_ACK_NUM,
> +	RTE_FLOW_ITEM_TCP_FLAGS,
> +	RTE_FLOW_ITEM_UDP_PORT_SRC,
> +	RTE_FLOW_ITEM_UDP_PORT_DST,
> +	RTE_FLOW_ITEM_VXLAN_VNI,
> +	RTE_FLOW_ITEM_GENEVE_VNI,
> +	RTE_FLOW_ITEM_GTP_TEID,
> +	RTE_FLOW_ITEM_TAG,
> +	RTE_FLOW_ITEM_MARK,
> +	RTE_FLOW_ITEM_META,
> +};

I don't think this name is good since it not rte_flow_item this is just internal enumeration
for this action.

> +
> +struct rte_flow_action_copy_data {
> +	enum rte_flow_item_id item;
> +	uint32_t index;
> +	uint32_t offset;

Why use 32 bits? Since this copy only one register with max len of 32 bit.
The max offset can 31? Same for the index.

> +};
> +
> +/**
> + * RTE_FLOW_ACTION_TYPE_COPY_ITEM
> + *
> + * Copies a specified number of bits from a source header field
> + * to a destination header field. Tag, mark or metadata can also
> + * be used as a source/destination to allow saving/overwriting
> + * an arbituary header field with a user-specified value.
> + */
> +struct rte_flow_action_copy_item {
> +	struct rte_flow_action_copy_data dst;
> +	struct rte_flow_action_copy_data src;
> +	uint32_t width;

Why use 32 bit register?

> +};
> +
>  /* Mbuf dynamic field offset for metadata. */
>  extern int32_t rte_flow_dynf_metadata_offs;
> 
> --
> 2.24.1

Best,
Ori
Asaf Penso Jan. 10, 2021, 9:36 a.m. UTC | #2 
Correct, Ori. We'll soon send the testpmd part and pmd draft.

Regards,
Asaf Penso

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Ori Kam
>Sent: Sunday, January 10, 2021 10:01 AM
>To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
>Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>Monjalon <thomas@monjalon.net>; ferruh.yigit@intel.com;
>andrew.rybchenko@oktetlabs.ru
>Subject: Re: [dpdk-dev] [PATCH] ethdev: introduce generic copy rte flow
>action
>
>Hi Alexander,
>
>I guess that the test-pmd part will be available later right?
>
>> -----Original Message-----
>> From: Alexander Kozyrev <akozyrev@nvidia.com>
>> Sent: Friday, January 8, 2021 8:33 AM
>> Subject: [PATCH] ethdev: introduce generic copy rte flow action
>>
>> Implement a generic copy flow API to allow copying of an arbitrary
>> header field (as well as mark, metadata or tag) to another item.
>>
>> This generic copy mechanism removes the necessity to implement a
>> separate RTE Flow action every time we need to modify a new packet
>> field in the future. A user-provided value can be used from a
>> specified tag/metadata or directly copied from other packet field.
>>
>> The number of bits to copy as well as the offset to start from can be
>> specified to allow a partial copy or copy into an arbitrary place in a
>> packet for greater flexibility.
>>
>
>Since the question why you are using enum and not just offset from the start
>of the packet, was discussed and raised by number of people it will be best if
>it will appear in the commit log, at least the advantages to this
>implementation.
>
>> RFC:
>>
>https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
>> es.d
>pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com
>%
>> 7Cd04c2e49c3a840994da408d8b39f3304%7C43083d15727340c1b7db39efd9cc
>>
>c17a%7C0%7C0%7C637456843629116269%7CUnknown%7CTWFpbGZsb3d8eyJ
>>
>WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>C
>>
>1000&amp;sdata=85096rASBtzbjU42pV6sPkl3nVt5HlR6%2BL9nxI3qgFA%3D&a
>> mp;reserved=0
>>
>> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
>> ---
>>  doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
>>  lib/librte_ethdev/rte_flow.c       |  1 +
>>  lib/librte_ethdev/rte_flow.h       | 59
>++++++++++++++++++++++++++++++
>>  3 files changed, 95 insertions(+)
>>
>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>> b/doc/guides/prog_guide/rte_flow.rst
>> index 86b3444803..b737ff9dad 100644
>> --- a/doc/guides/prog_guide/rte_flow.rst
>> +++ b/doc/guides/prog_guide/rte_flow.rst
>> @@ -2766,6 +2766,41 @@ The behaviour of the shared action defined by
>> ``action`` argument of type
>>     | no properties |
>>     +---------------+
>>
>> +Action: ``COPY_ITEM``
>> +^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Copy ``width`` bits from ``src`` item to ``dst`` item.
>> +
>> +An arbitrary header field (as well as mark, metadata or tag values)
>> +can be used as both source and destination items as set by ``item``.
>> +
>
>For tag I think you should also use the index right?
>
>> +Inner packet header fields can be accessed using the ``index`` and it
>> +is possible to start the copy from the ``offset`` bits in an item.
>
>Please specify  what means index 0 /1 ... 0 is outer most? Inner most?
>You can look at the RSS level for reference.
>I think it will be best to use the same values.
>
>What happens if we want to copy between different sizes?
>for example copy IPV6 src to number of tags? I assume we will be using offset
>and split the copy command to number of actions right?
>
>> +
>> +.. _table_rte_flow_action_copy_item:
>> +
>> +.. table:: COPY_ITEM
>> +
>> +   +-----------------------------------------+
>> +   | Field         | Value                   |
>> +   +===============+=========================+
>> +   | ``dst``       | destination item        |
>> +   | ``src``       | source item             |
>> +   | ``width``     | number of bits to copy  |
>> +   +---------------+-------------------------+
>> +
>> +.. _table_rte_flow_action_copy_data:
>> +
>> +.. table:: destination/source item definition
>> +
>> +   +----------------------------------------------------------+
>> +   | Field         | Value                                    |
>> +
>+===============+==========================================
>+
>> +   | ``item``      | ID of a packet field/mark/metadata/tag   |
>> +   | ``index``     | index of outer/inner header or tag array |
>> +   | ``offset``    | number of bits to skip during the copy   |
>> +   +---------------+------------------------------------------+
>> +
>>  Negative types
>>  ~~~~~~~~~~~~~~
>>
>> diff --git a/lib/librte_ethdev/rte_flow.c
>> b/lib/librte_ethdev/rte_flow.c index a06f64c271..fdbabefc47 100644
>> --- a/lib/librte_ethdev/rte_flow.c
>> +++ b/lib/librte_ethdev/rte_flow.c
>> @@ -176,6 +176,7 @@ static const struct rte_flow_desc_data
>> rte_flow_desc_action[] = {
>>  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
>> rte_flow_action_set_dscp)),
>>  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
>>  	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
>> +	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct
>> rte_flow_action_copy_item)),
>>  	/**
>>  	 * Shared action represented as handle of type
>>  	 * (struct rte_flow_shared action *) stored in conf field (see diff
>> --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
>> index 0977a78270..0540c861fb 100644
>> --- a/lib/librte_ethdev/rte_flow.h
>> +++ b/lib/librte_ethdev/rte_flow.h
>> @@ -2198,6 +2198,16 @@ enum rte_flow_action_type {
>>  	 * struct rte_flow_shared_action).
>>  	 */
>>  	RTE_FLOW_ACTION_TYPE_SHARED,
>> +
>> +	/**
>> +	 * Copy a packet header field, tag, mark or metadata.
>> +	 *
>> +	 * Allow saving an arbitrary header field by copying its value
>> +	 * to a tag/mark/metadata or copy it into another header field.
>> +	 *
>> +	 * See struct rte_flow_action_copy_item.
>> +	 */
>> +	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
>>  };
>>
>>  /**
>> @@ -2791,6 +2801,55 @@ struct rte_flow_action_set_dscp {
>>   */
>>  struct rte_flow_shared_action;
>>
>> +enum rte_flow_item_id {
>> +	RTE_FLOW_ITEM_NONE = 0,
>> +	RTE_FLOW_ITEM_MAC_DST,
>> +	RTE_FLOW_ITEM_MAC_SRC,
>> +	RTE_FLOW_ITEM_VLAN_TYPE,
>> +	RTE_FLOW_ITEM_VLAN_ID,
>> +	RTE_FLOW_ITEM_MAC_TYPE,
>> +	RTE_FLOW_ITEM_IPV4_DSCP,
>> +	RTE_FLOW_ITEM_IPV4_TTL,
>> +	RTE_FLOW_ITEM_IPV4_SRC,
>> +	RTE_FLOW_ITEM_IPV4_DST,
>> +	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
>> +	RTE_FLOW_ITEM_IPV6_SRC,
>> +	RTE_FLOW_ITEM_IPV6_DST,
>> +	RTE_FLOW_ITEM_TCP_PORT_SRC,
>> +	RTE_FLOW_ITEM_TCP_PORT_DST,
>> +	RTE_FLOW_ITEM_TCP_SEQ_NUM,
>> +	RTE_FLOW_ITEM_TCP_ACK_NUM,
>> +	RTE_FLOW_ITEM_TCP_FLAGS,
>> +	RTE_FLOW_ITEM_UDP_PORT_SRC,
>> +	RTE_FLOW_ITEM_UDP_PORT_DST,
>> +	RTE_FLOW_ITEM_VXLAN_VNI,
>> +	RTE_FLOW_ITEM_GENEVE_VNI,
>> +	RTE_FLOW_ITEM_GTP_TEID,
>> +	RTE_FLOW_ITEM_TAG,
>> +	RTE_FLOW_ITEM_MARK,
>> +	RTE_FLOW_ITEM_META,
>> +};
>
>I don't think this name is good since it not rte_flow_item this is just internal
>enumeration for this action.
>
>> +
>> +struct rte_flow_action_copy_data {
>> +	enum rte_flow_item_id item;
>> +	uint32_t index;
>> +	uint32_t offset;
>
>Why use 32 bits? Since this copy only one register with max len of 32 bit.
>The max offset can 31? Same for the index.
>
>> +};
>> +
>> +/**
>> + * RTE_FLOW_ACTION_TYPE_COPY_ITEM
>> + *
>> + * Copies a specified number of bits from a source header field
>> + * to a destination header field. Tag, mark or metadata can also
>> + * be used as a source/destination to allow saving/overwriting
>> + * an arbituary header field with a user-specified value.
>> + */
>> +struct rte_flow_action_copy_item {
>> +	struct rte_flow_action_copy_data dst;
>> +	struct rte_flow_action_copy_data src;
>> +	uint32_t width;
>
>Why use 32 bit register?
>
>> +};
>> +
>>  /* Mbuf dynamic field offset for metadata. */  extern int32_t
>> rte_flow_dynf_metadata_offs;
>>
>> --
>> 2.24.1
>
>Best,
>Ori
Alexander Kozyrev Jan. 12, 2021, 2:15 p.m. UTC | #3 
> From: Ori Kam <orika@nvidia.com> on Sunday, January 10, 2021 3:01
> Hi Alexander,
> 
> I guess that the test-pmd part will be available later right?
Yes, please take a look at v2 version for testpmd implementation.

> > -----Original Message-----
> > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > Sent: Friday, January 8, 2021 8:33 AM
> > Subject: [PATCH] ethdev: introduce generic copy rte flow action
> >
> > Implement a generic copy flow API to allow copying of an arbitrary
> > header field (as well as mark, metadata or tag) to another item.
> >
> > This generic copy mechanism removes the necessity to implement a
> > separate RTE Flow action every time we need to modify a new packet
> > field in the future. A user-provided value can be used from a
> > specified tag/metadata or directly copied from other packet field.
> >
> > The number of bits to copy as well as the offset to start from can
> > be specified to allow a partial copy or copy into an arbitrary
> > place in a packet for greater flexibility.
> >
> 
> Since the question why you are using enum and not just offset from
> the start of the packet, was discussed and raised by number of people it will be
> best
> if it will appear in the commit log, at least the advantages to this
> implementation.
Ok, will add to v3 commit message.

> > RFC:
> >
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.
> d
> >
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> >
> 7Cd04c2e49c3a840994da408d8b39f3304%7C43083d15727340c1b7db39efd9cc
> >
> c17a%7C0%7C0%7C637456843629116269%7CUnknown%7CTWFpbGZsb3d8eyJ
> >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > 1000&amp;sdata=85096rASBtzbjU42pV6sPkl3nVt5HlR6%2BL9nxI3qgFA%3D&a
> > mp;reserved=0
> >
> > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
> >  lib/librte_ethdev/rte_flow.c       |  1 +
> >  lib/librte_ethdev/rte_flow.h       | 59 ++++++++++++++++++++++++++++++
> >  3 files changed, 95 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 86b3444803..b737ff9dad 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2766,6 +2766,41 @@ The behaviour of the shared action defined by
> > ``action`` argument of type
> >     | no properties |
> >     +---------------+
> >
> > +Action: ``COPY_ITEM``
> > +^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > +
> > +An arbitrary header field (as well as mark, metadata or tag values)
> > +can be used as both source and destination items as set by ``item``.
> > +
> 
> For tag I think you should also use the index right?
Right, index is here to access any element in the tag array in addition to
outermost or any inner packet header fields. Will specify explicitly in the doc.

> > +Inner packet header fields can be accessed using the ``index`` and
> > +it is possible to start the copy from the ``offset`` bits in an item.
> 
> Please specify  what means index 0 /1 ... 0 is outer most? Inner most?
> You can look at the RSS level for reference.
> I think it will be best to use the same values.
0 is outer most, of course,  I'Il add some clarification about that.

> What happens if we want to copy between different sizes?
> for example copy IPV6 src to number of tags? I assume we will be using offset
> and
> split the copy command to number of actions right?
That is the correct understanding. We can utilize 4 copy_item actions in this case to
copy 32-bits of an IPv6 SRC at the time (specifying different offsets) to 4 different Tag fields.

> > +
> > +.. _table_rte_flow_action_copy_item:
> > +
> > +.. table:: COPY_ITEM
> > +
> > +   +-----------------------------------------+
> > +   | Field         | Value                   |
> > +   +===============+=========================+
> > +   | ``dst``       | destination item        |
> > +   | ``src``       | source item             |
> > +   | ``width``     | number of bits to copy  |
> > +   +---------------+-------------------------+
> > +
> > +.. _table_rte_flow_action_copy_data:
> > +
> > +.. table:: destination/source item definition
> > +
> > +   +----------------------------------------------------------+
> > +   | Field         | Value                                    |
> > +   +===============+==========================================+
> > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > +   | ``index``     | index of outer/inner header or tag array |
> > +   | ``offset``    | number of bits to skip during the copy   |
> > +   +---------------+------------------------------------------+
> > +
> >  Negative types
> >  ~~~~~~~~~~~~~~
> >
> > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > index a06f64c271..fdbabefc47 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -176,6 +176,7 @@ static const struct rte_flow_desc_data
> > rte_flow_desc_action[] = {
> >  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> > rte_flow_action_set_dscp)),
> >  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> >  	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > +	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct
> > rte_flow_action_copy_item)),
> >  	/**
> >  	 * Shared action represented as handle of type
> >  	 * (struct rte_flow_shared action *) stored in conf field (see
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 0977a78270..0540c861fb 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2198,6 +2198,16 @@ enum rte_flow_action_type {
> >  	 * struct rte_flow_shared_action).
> >  	 */
> >  	RTE_FLOW_ACTION_TYPE_SHARED,
> > +
> > +	/**
> > +	 * Copy a packet header field, tag, mark or metadata.
> > +	 *
> > +	 * Allow saving an arbitrary header field by copying its value
> > +	 * to a tag/mark/metadata or copy it into another header field.
> > +	 *
> > +	 * See struct rte_flow_action_copy_item.
> > +	 */
> > +	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
> >  };
> >
> >  /**
> > @@ -2791,6 +2801,55 @@ struct rte_flow_action_set_dscp {
> >   */
> >  struct rte_flow_shared_action;
> >
> > +enum rte_flow_item_id {
> > +	RTE_FLOW_ITEM_NONE = 0,
> > +	RTE_FLOW_ITEM_MAC_DST,
> > +	RTE_FLOW_ITEM_MAC_SRC,
> > +	RTE_FLOW_ITEM_VLAN_TYPE,
> > +	RTE_FLOW_ITEM_VLAN_ID,
> > +	RTE_FLOW_ITEM_MAC_TYPE,
> > +	RTE_FLOW_ITEM_IPV4_DSCP,
> > +	RTE_FLOW_ITEM_IPV4_TTL,
> > +	RTE_FLOW_ITEM_IPV4_SRC,
> > +	RTE_FLOW_ITEM_IPV4_DST,
> > +	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
> > +	RTE_FLOW_ITEM_IPV6_SRC,
> > +	RTE_FLOW_ITEM_IPV6_DST,
> > +	RTE_FLOW_ITEM_TCP_PORT_SRC,
> > +	RTE_FLOW_ITEM_TCP_PORT_DST,
> > +	RTE_FLOW_ITEM_TCP_SEQ_NUM,
> > +	RTE_FLOW_ITEM_TCP_ACK_NUM,
> > +	RTE_FLOW_ITEM_TCP_FLAGS,
> > +	RTE_FLOW_ITEM_UDP_PORT_SRC,
> > +	RTE_FLOW_ITEM_UDP_PORT_DST,
> > +	RTE_FLOW_ITEM_VXLAN_VNI,
> > +	RTE_FLOW_ITEM_GENEVE_VNI,
> > +	RTE_FLOW_ITEM_GTP_TEID,
> > +	RTE_FLOW_ITEM_TAG,
> > +	RTE_FLOW_ITEM_MARK,
> > +	RTE_FLOW_ITEM_META,
> > +};
> 
> I don't think this name is good since it not rte_flow_item this is just internal
> enumeration
> for this action.
Will rename to rte_flow_action_copy_item_id in v3? Is this ok?

> > +
> > +struct rte_flow_action_copy_data {
> > +	enum rte_flow_item_id item;
> > +	uint32_t index;
> > +	uint32_t offset;
> 
> Why use 32 bits? Since this copy only one register with max len of 32 bit.
> The max offset can 31? Same for the index.
This extends the flexibility of the copy API. This way you can modify any place
in a packet as you desire by specifying RTE_FLOW_ITEM_NONE to start with the
beginning of a packet and choosing a particular offset value to point in a desired place.
And since packet length can be pretty big we need to accommodate this.
Do you think this flexibility is not necessary and we should bound the offset to the
length of a selected packet field only? Index is 32 bit for the same field in RSS.

> > +};
> > +
> > +/**
> > + * RTE_FLOW_ACTION_TYPE_COPY_ITEM
> > + *
> > + * Copies a specified number of bits from a source header field
> > + * to a destination header field. Tag, mark or metadata can also
> > + * be used as a source/destination to allow saving/overwriting
> > + * an arbituary header field with a user-specified value.
> > + */
> > +struct rte_flow_action_copy_item {
> > +	struct rte_flow_action_copy_data dst;
> > +	struct rte_flow_action_copy_data src;
> > +	uint32_t width;
> 
> Why use 32 bit register?
Again, to make API as generic as possible in case there will be a PMD driver
that can copy a huge chunk of a packet into the another place of this packet.
What is your opinion on that?

> 
> > +};
> > +
> >  /* Mbuf dynamic field offset for metadata. */
> >  extern int32_t rte_flow_dynf_metadata_offs;
> >
> > --
> > 2.24.1
> 
> Best,
> Ori
Ori Kam Jan. 12, 2021, 2:52 p.m. UTC | #4 
Hi,

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, January 12, 2021 4:16 PM
> To: Ori Kam <orika@nvidia.com>; dev@dpdk.org
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> andrew.rybchenko@oktetlabs.ru
> Subject: RE: [PATCH] ethdev: introduce generic copy rte flow action
> 
> > From: Ori Kam <orika@nvidia.com> on Sunday, January 10, 2021 3:01
> > Hi Alexander,
> >
> > I guess that the test-pmd part will be available later right?
> Yes, please take a look at v2 version for testpmd implementation.
> 
> > > -----Original Message-----
> > > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > > Sent: Friday, January 8, 2021 8:33 AM
> > > Subject: [PATCH] ethdev: introduce generic copy rte flow action
> > >
> > > Implement a generic copy flow API to allow copying of an arbitrary
> > > header field (as well as mark, metadata or tag) to another item.
> > >
> > > This generic copy mechanism removes the necessity to implement a
> > > separate RTE Flow action every time we need to modify a new packet
> > > field in the future. A user-provided value can be used from a
> > > specified tag/metadata or directly copied from other packet field.
> > >
> > > The number of bits to copy as well as the offset to start from can
> > > be specified to allow a partial copy or copy into an arbitrary
> > > place in a packet for greater flexibility.
> > >
> >
> > Since the question why you are using enum and not just offset from
> > the start of the packet, was discussed and raised by number of people it will
> be
> > best
> > if it will appear in the commit log, at least the advantages to this
> > implementation.
> Ok, will add to v3 commit message.
> 
> > > RFC:
> > >
> > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.
> > d
> > >
> >
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> > >
> >
> 7Cd04c2e49c3a840994da408d8b39f3304%7C43083d15727340c1b7db39efd9cc
> > >
> >
> c17a%7C0%7C0%7C637456843629116269%7CUnknown%7CTWFpbGZsb3d8eyJ
> > >
> >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > >
> 1000&amp;sdata=85096rASBtzbjU42pV6sPkl3nVt5HlR6%2BL9nxI3qgFA%3D&a
> > > mp;reserved=0
> > >
> > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > ---
> > >  doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
> > >  lib/librte_ethdev/rte_flow.c       |  1 +
> > >  lib/librte_ethdev/rte_flow.h       | 59 ++++++++++++++++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >
> > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > b/doc/guides/prog_guide/rte_flow.rst
> > > index 86b3444803..b737ff9dad 100644
> > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > @@ -2766,6 +2766,41 @@ The behaviour of the shared action defined by
> > > ``action`` argument of type
> > >     | no properties |
> > >     +---------------+
> > >
> > > +Action: ``COPY_ITEM``
> > > +^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > > +
> > > +An arbitrary header field (as well as mark, metadata or tag values)
> > > +can be used as both source and destination items as set by ``item``.
> > > +
> >
> > For tag I think you should also use the index right?
> Right, index is here to access any element in the tag array in addition to
> outermost or any inner packet header fields. Will specify explicitly in the doc.
> 
> > > +Inner packet header fields can be accessed using the ``index`` and
> > > +it is possible to start the copy from the ``offset`` bits in an item.
> >
> > Please specify  what means index 0 /1 ... 0 is outer most? Inner most?
> > You can look at the RSS level for reference.
> > I think it will be best to use the same values.
> 0 is outer most, of course,  I'Il add some clarification about that.
> 
Not necessary, like I said please look at the RSS, 0 can mean the inner most.

> > What happens if we want to copy between different sizes?
> > for example copy IPV6 src to number of tags? I assume we will be using offset
> > and
> > split the copy command to number of actions right?
> That is the correct understanding. We can utilize 4 copy_item actions in this
> case to
> copy 32-bits of an IPv6 SRC at the time (specifying different offsets) to 4
> different Tag fields.
> 
> > > +
> > > +.. _table_rte_flow_action_copy_item:
> > > +
> > > +.. table:: COPY_ITEM
> > > +
> > > +   +-----------------------------------------+
> > > +   | Field         | Value                   |
> > > +   +===============+=========================+
> > > +   | ``dst``       | destination item        |
> > > +   | ``src``       | source item             |
> > > +   | ``width``     | number of bits to copy  |
> > > +   +---------------+-------------------------+
> > > +
> > > +.. _table_rte_flow_action_copy_data:
> > > +
> > > +.. table:: destination/source item definition
> > > +
> > > +   +----------------------------------------------------------+
> > > +   | Field         | Value                                    |
> > > +
> +===============+==========================================+
> > > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > > +   | ``index``     | index of outer/inner header or tag array |
> > > +   | ``offset``    | number of bits to skip during the copy   |
> > > +   +---------------+------------------------------------------+
> > > +
> > >  Negative types
> > >  ~~~~~~~~~~~~~~
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > > index a06f64c271..fdbabefc47 100644
> > > --- a/lib/librte_ethdev/rte_flow.c
> > > +++ b/lib/librte_ethdev/rte_flow.c
> > > @@ -176,6 +176,7 @@ static const struct rte_flow_desc_data
> > > rte_flow_desc_action[] = {
> > >  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> > > rte_flow_action_set_dscp)),
> > >  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > >  	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > > +	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct
> > > rte_flow_action_copy_item)),
> > >  	/**
> > >  	 * Shared action represented as handle of type
> > >  	 * (struct rte_flow_shared action *) stored in conf field (see
> > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > index 0977a78270..0540c861fb 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -2198,6 +2198,16 @@ enum rte_flow_action_type {
> > >  	 * struct rte_flow_shared_action).
> > >  	 */
> > >  	RTE_FLOW_ACTION_TYPE_SHARED,
> > > +
> > > +	/**
> > > +	 * Copy a packet header field, tag, mark or metadata.
> > > +	 *
> > > +	 * Allow saving an arbitrary header field by copying its value
> > > +	 * to a tag/mark/metadata or copy it into another header field.
> > > +	 *
> > > +	 * See struct rte_flow_action_copy_item.
> > > +	 */
> > > +	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
> > >  };
> > >
> > >  /**
> > > @@ -2791,6 +2801,55 @@ struct rte_flow_action_set_dscp {
> > >   */
> > >  struct rte_flow_shared_action;
> > >
> > > +enum rte_flow_item_id {
> > > +	RTE_FLOW_ITEM_NONE = 0,
> > > +	RTE_FLOW_ITEM_MAC_DST,
> > > +	RTE_FLOW_ITEM_MAC_SRC,
> > > +	RTE_FLOW_ITEM_VLAN_TYPE,
> > > +	RTE_FLOW_ITEM_VLAN_ID,
> > > +	RTE_FLOW_ITEM_MAC_TYPE,
> > > +	RTE_FLOW_ITEM_IPV4_DSCP,
> > > +	RTE_FLOW_ITEM_IPV4_TTL,
> > > +	RTE_FLOW_ITEM_IPV4_SRC,
> > > +	RTE_FLOW_ITEM_IPV4_DST,
> > > +	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
> > > +	RTE_FLOW_ITEM_IPV6_SRC,
> > > +	RTE_FLOW_ITEM_IPV6_DST,
> > > +	RTE_FLOW_ITEM_TCP_PORT_SRC,
> > > +	RTE_FLOW_ITEM_TCP_PORT_DST,
> > > +	RTE_FLOW_ITEM_TCP_SEQ_NUM,
> > > +	RTE_FLOW_ITEM_TCP_ACK_NUM,
> > > +	RTE_FLOW_ITEM_TCP_FLAGS,
> > > +	RTE_FLOW_ITEM_UDP_PORT_SRC,
> > > +	RTE_FLOW_ITEM_UDP_PORT_DST,
> > > +	RTE_FLOW_ITEM_VXLAN_VNI,
> > > +	RTE_FLOW_ITEM_GENEVE_VNI,
> > > +	RTE_FLOW_ITEM_GTP_TEID,
> > > +	RTE_FLOW_ITEM_TAG,
> > > +	RTE_FLOW_ITEM_MARK,
> > > +	RTE_FLOW_ITEM_META,
> > > +};
> >
> > I don't think this name is good since it not rte_flow_item this is just internal
> > enumeration
> > for this action.
> Will rename to rte_flow_action_copy_item_id in v3? Is this ok?
> 
Yes better,
Or rte_flow_copy_item_id?

> > > +
> > > +struct rte_flow_action_copy_data {
> > > +	enum rte_flow_item_id item;
> > > +	uint32_t index;
> > > +	uint32_t offset;
> >
> > Why use 32 bits? Since this copy only one register with max len of 32 bit.
> > The max offset can 31? Same for the index.
> This extends the flexibility of the copy API. This way you can modify any place
> in a packet as you desire by specifying RTE_FLOW_ITEM_NONE to start with
> the
> beginning of a packet and choosing a particular offset value to point in a
> desired place.
> And since packet length can be pretty big we need to accommodate this.
> Do you think this flexibility is not necessary and we should bound the offset to
> the
> length of a selected packet field only? Index is 32 bit for the same field in RSS.
> 
Nice idea but then I would add ITEM_OFFSET or something
around this name, I don't think NONE is a valid value.

> > > +};
> > > +
> > > +/**
> > > + * RTE_FLOW_ACTION_TYPE_COPY_ITEM
> > > + *
> > > + * Copies a specified number of bits from a source header field
> > > + * to a destination header field. Tag, mark or metadata can also
> > > + * be used as a source/destination to allow saving/overwriting
> > > + * an arbituary header field with a user-specified value.
> > > + */
> > > +struct rte_flow_action_copy_item {
> > > +	struct rte_flow_action_copy_data dst;
> > > +	struct rte_flow_action_copy_data src;
> > > +	uint32_t width;
> >
> > Why use 32 bit register?
> Again, to make API as generic as possible in case there will be a PMD driver
> that can copy a huge chunk of a packet into the another place of this packet.
> What is your opinion on that?
> 
Nice thinking,

> >
> > > +};
> > > +
> > >  /* Mbuf dynamic field offset for metadata. */
> > >  extern int32_t rte_flow_dynf_metadata_offs;
> > >
> > > --
> > > 2.24.1
> >
> > Best,
> > Ori
Ori Kam Jan. 12, 2021, 3:13 p.m. UTC | #5 
Hi,


> -----Original Message-----
> From: Ori Kam
> Sent: Tuesday, January 12, 2021 4:53 PM
> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> andrew.rybchenko@oktetlabs.ru
> Subject: RE: [PATCH] ethdev: introduce generic copy rte flow action
> 
> Hi,
> 
> > -----Original Message-----
> > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > Sent: Tuesday, January 12, 2021 4:16 PM
> > To: Ori Kam <orika@nvidia.com>; dev@dpdk.org
> > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon
> > <thomas@monjalon.net>; ferruh.yigit@intel.com;
> > andrew.rybchenko@oktetlabs.ru
> > Subject: RE: [PATCH] ethdev: introduce generic copy rte flow action
> >
> > > From: Ori Kam <orika@nvidia.com> on Sunday, January 10, 2021 3:01
> > > Hi Alexander,
> > >
> > > I guess that the test-pmd part will be available later right?
> > Yes, please take a look at v2 version for testpmd implementation.
> >
> > > > -----Original Message-----
> > > > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > > > Sent: Friday, January 8, 2021 8:33 AM
> > > > Subject: [PATCH] ethdev: introduce generic copy rte flow action
> > > >
> > > > Implement a generic copy flow API to allow copying of an arbitrary
> > > > header field (as well as mark, metadata or tag) to another item.
> > > >
> > > > This generic copy mechanism removes the necessity to implement a
> > > > separate RTE Flow action every time we need to modify a new packet
> > > > field in the future. A user-provided value can be used from a
> > > > specified tag/metadata or directly copied from other packet field.
> > > >
> > > > The number of bits to copy as well as the offset to start from can
> > > > be specified to allow a partial copy or copy into an arbitrary
> > > > place in a packet for greater flexibility.
> > > >
> > >
> > > Since the question why you are using enum and not just offset from
> > > the start of the packet, was discussed and raised by number of people it will
> > be
> > > best
> > > if it will appear in the commit log, at least the advantages to this
> > > implementation.
> > Ok, will add to v3 commit message.
> >
> > > > RFC:
> > > >
> > >
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.
> > > d
> > > >
> > >
> >
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> > > >
> > >
> >
> 7Cd04c2e49c3a840994da408d8b39f3304%7C43083d15727340c1b7db39efd9cc
> > > >
> > >
> >
> c17a%7C0%7C0%7C637456843629116269%7CUnknown%7CTWFpbGZsb3d8eyJ
> > > >
> > >
> >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > > >
> >
> 1000&amp;sdata=85096rASBtzbjU42pV6sPkl3nVt5HlR6%2BL9nxI3qgFA%3D&a
> > > > mp;reserved=0
> > > >
> > > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > > ---
> > > >  doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
> > > >  lib/librte_ethdev/rte_flow.c       |  1 +
> > > >  lib/librte_ethdev/rte_flow.h       | 59
> ++++++++++++++++++++++++++++++
> > > >  3 files changed, 95 insertions(+)
> > > >
> > > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > > b/doc/guides/prog_guide/rte_flow.rst
> > > > index 86b3444803..b737ff9dad 100644
> > > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > > @@ -2766,6 +2766,41 @@ The behaviour of the shared action defined by
> > > > ``action`` argument of type
> > > >     | no properties |
> > > >     +---------------+
> > > >
> > > > +Action: ``COPY_ITEM``
> > > > +^^^^^^^^^^^^^^^^^^^^^
> > > > +
> > > > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > > > +
> > > > +An arbitrary header field (as well as mark, metadata or tag values)
> > > > +can be used as both source and destination items as set by ``item``.
> > > > +
> > >
> > > For tag I think you should also use the index right?
> > Right, index is here to access any element in the tag array in addition to
> > outermost or any inner packet header fields. Will specify explicitly in the doc.
> >
> > > > +Inner packet header fields can be accessed using the ``index`` and
> > > > +it is possible to start the copy from the ``offset`` bits in an item.
> > >
> > > Please specify  what means index 0 /1 ... 0 is outer most? Inner most?
> > > You can look at the RSS level for reference.
> > > I think it will be best to use the same values.
> > 0 is outer most, of course,  I'Il add some clarification about that.
> >
> Not necessary, like I said please look at the RSS, 0 can mean the inner most.
> 
> > > What happens if we want to copy between different sizes?
> > > for example copy IPV6 src to number of tags? I assume we will be using
> offset
> > > and
> > > split the copy command to number of actions right?
> > That is the correct understanding. We can utilize 4 copy_item actions in this
> > case to
> > copy 32-bits of an IPv6 SRC at the time (specifying different offsets) to 4
> > different Tag fields.
> >
> > > > +
> > > > +.. _table_rte_flow_action_copy_item:
> > > > +
> > > > +.. table:: COPY_ITEM
> > > > +
> > > > +   +-----------------------------------------+
> > > > +   | Field         | Value                   |
> > > > +   +===============+=========================+
> > > > +   | ``dst``       | destination item        |
> > > > +   | ``src``       | source item             |
> > > > +   | ``width``     | number of bits to copy  |
> > > > +   +---------------+-------------------------+
> > > > +
> > > > +.. _table_rte_flow_action_copy_data:
> > > > +
> > > > +.. table:: destination/source item definition
> > > > +
> > > > +   +----------------------------------------------------------+
> > > > +   | Field         | Value                                    |
> > > > +
> > +===============+==========================================+
> > > > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > > > +   | ``index``     | index of outer/inner header or tag array |
> > > > +   | ``offset``    | number of bits to skip during the copy   |
> > > > +   +---------------+------------------------------------------+
> > > > +
> > > >  Negative types
> > > >  ~~~~~~~~~~~~~~
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > > > index a06f64c271..fdbabefc47 100644
> > > > --- a/lib/librte_ethdev/rte_flow.c
> > > > +++ b/lib/librte_ethdev/rte_flow.c
> > > > @@ -176,6 +176,7 @@ static const struct rte_flow_desc_data
> > > > rte_flow_desc_action[] = {
> > > >  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> > > > rte_flow_action_set_dscp)),
> > > >  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > > >  	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > > > +	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct
> > > > rte_flow_action_copy_item)),
> > > >  	/**
> > > >  	 * Shared action represented as handle of type
> > > >  	 * (struct rte_flow_shared action *) stored in conf field (see
> > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > > index 0977a78270..0540c861fb 100644
> > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > @@ -2198,6 +2198,16 @@ enum rte_flow_action_type {
> > > >  	 * struct rte_flow_shared_action).
> > > >  	 */
> > > >  	RTE_FLOW_ACTION_TYPE_SHARED,
> > > > +
> > > > +	/**
> > > > +	 * Copy a packet header field, tag, mark or metadata.
> > > > +	 *
> > > > +	 * Allow saving an arbitrary header field by copying its value
> > > > +	 * to a tag/mark/metadata or copy it into another header field.
> > > > +	 *
> > > > +	 * See struct rte_flow_action_copy_item.
> > > > +	 */
> > > > +	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
> > > >  };
> > > >
> > > >  /**
> > > > @@ -2791,6 +2801,55 @@ struct rte_flow_action_set_dscp {
> > > >   */
> > > >  struct rte_flow_shared_action;
> > > >
> > > > +enum rte_flow_item_id {
> > > > +	RTE_FLOW_ITEM_NONE = 0,
> > > > +	RTE_FLOW_ITEM_MAC_DST,
> > > > +	RTE_FLOW_ITEM_MAC_SRC,
> > > > +	RTE_FLOW_ITEM_VLAN_TYPE,
> > > > +	RTE_FLOW_ITEM_VLAN_ID,
> > > > +	RTE_FLOW_ITEM_MAC_TYPE,
> > > > +	RTE_FLOW_ITEM_IPV4_DSCP,
> > > > +	RTE_FLOW_ITEM_IPV4_TTL,
> > > > +	RTE_FLOW_ITEM_IPV4_SRC,
> > > > +	RTE_FLOW_ITEM_IPV4_DST,
> > > > +	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
> > > > +	RTE_FLOW_ITEM_IPV6_SRC,
> > > > +	RTE_FLOW_ITEM_IPV6_DST,
> > > > +	RTE_FLOW_ITEM_TCP_PORT_SRC,
> > > > +	RTE_FLOW_ITEM_TCP_PORT_DST,
> > > > +	RTE_FLOW_ITEM_TCP_SEQ_NUM,
> > > > +	RTE_FLOW_ITEM_TCP_ACK_NUM,
> > > > +	RTE_FLOW_ITEM_TCP_FLAGS,
> > > > +	RTE_FLOW_ITEM_UDP_PORT_SRC,
> > > > +	RTE_FLOW_ITEM_UDP_PORT_DST,
> > > > +	RTE_FLOW_ITEM_VXLAN_VNI,
> > > > +	RTE_FLOW_ITEM_GENEVE_VNI,
> > > > +	RTE_FLOW_ITEM_GTP_TEID,
> > > > +	RTE_FLOW_ITEM_TAG,
> > > > +	RTE_FLOW_ITEM_MARK,
> > > > +	RTE_FLOW_ITEM_META,
> > > > +};
> > >
> > > I don't think this name is good since it not rte_flow_item this is just internal
> > > enumeration
> > > for this action.
> > Will rename to rte_flow_action_copy_item_id in v3? Is this ok?
> >
> Yes better,
> Or rte_flow_copy_item_id?
> 
On second thought I think a better name should be:
rte_flow_field_id - I removed  the copy since we may use it in other actions
for example set/add

> > > > +
> > > > +struct rte_flow_action_copy_data {
> > > > +	enum rte_flow_item_id item;
> > > > +	uint32_t index;
> > > > +	uint32_t offset;
> > >
> > > Why use 32 bits? Since this copy only one register with max len of 32 bit.
> > > The max offset can 31? Same for the index.
> > This extends the flexibility of the copy API. This way you can modify any place
> > in a packet as you desire by specifying RTE_FLOW_ITEM_NONE to start with
> > the
> > beginning of a packet and choosing a particular offset value to point in a
> > desired place.
> > And since packet length can be pretty big we need to accommodate this.
> > Do you think this flexibility is not necessary and we should bound the offset to
> > the
> > length of a selected packet field only? Index is 32 bit for the same field in RSS.
> >
> Nice idea but then I would add ITEM_OFFSET or something
> around this name, I don't think NONE is a valid value.
> 
> > > > +};
> > > > +
> > > > +/**
> > > > + * RTE_FLOW_ACTION_TYPE_COPY_ITEM
> > > > + *
> > > > + * Copies a specified number of bits from a source header field
> > > > + * to a destination header field. Tag, mark or metadata can also
> > > > + * be used as a source/destination to allow saving/overwriting
> > > > + * an arbituary header field with a user-specified value.
> > > > + */
> > > > +struct rte_flow_action_copy_item {
> > > > +	struct rte_flow_action_copy_data dst;
> > > > +	struct rte_flow_action_copy_data src;
> > > > +	uint32_t width;
> > >
> > > Why use 32 bit register?
> > Again, to make API as generic as possible in case there will be a PMD driver
> > that can copy a huge chunk of a packet into the another place of this packet.
> > What is your opinion on that?
> >
> Nice thinking,
> 
> > >
> > > > +};
> > > > +
> > > >  /* Mbuf dynamic field offset for metadata. */
> > > >  extern int32_t rte_flow_dynf_metadata_offs;
> > > >
> > > > --
> > > > 2.24.1
> > >
> > > Best,
> > > Ori
Alexander Kozyrev Jan. 12, 2021, 5:19 p.m. UTC | #6 
> > From: Ori Kam onTuesday, January 12, 2021 4:53 PM
> > To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
> > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon
> > <thomas@monjalon.net>; ferruh.yigit@intel.com;
> > andrew.rybchenko@oktetlabs.ru
> > Subject: RE: [PATCH] ethdev: introduce generic copy rte flow action
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > > Sent: Tuesday, January 12, 2021 4:16 PM
> > > To: Ori Kam <orika@nvidia.com>; dev@dpdk.org
> > > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> > Monjalon
> > > <thomas@monjalon.net>; ferruh.yigit@intel.com;
> > > andrew.rybchenko@oktetlabs.ru
> > > Subject: RE: [PATCH] ethdev: introduce generic copy rte flow action
> > >
> > > > From: Ori Kam <orika@nvidia.com> on Sunday, January 10, 2021 3:01
> > > > Hi Alexander,
> > > >
> > > > I guess that the test-pmd part will be available later right?
> > > Yes, please take a look at v2 version for testpmd implementation.
> > >
> > > > > -----Original Message-----
> > > > > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > > > > Sent: Friday, January 8, 2021 8:33 AM
> > > > > Subject: [PATCH] ethdev: introduce generic copy rte flow action
> > > > >
> > > > > Implement a generic copy flow API to allow copying of an arbitrary
> > > > > header field (as well as mark, metadata or tag) to another item.
> > > > >
> > > > > This generic copy mechanism removes the necessity to implement a
> > > > > separate RTE Flow action every time we need to modify a new packet
> > > > > field in the future. A user-provided value can be used from a
> > > > > specified tag/metadata or directly copied from other packet field.
> > > > >
> > > > > The number of bits to copy as well as the offset to start from can
> > > > > be specified to allow a partial copy or copy into an arbitrary
> > > > > place in a packet for greater flexibility.
> > > > >
> > > >
> > > > Since the question why you are using enum and not just offset from
> > > > the start of the packet, was discussed and raised by number of people it
> will
> > > be
> > > > best
> > > > if it will appear in the commit log, at least the advantages to this
> > > > implementation.
> > > Ok, will add to v3 commit message.
> > >
> > > > > RFC:
> > > > >
> > > >
> >
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.
> > > > d
> > > > >
> > > >
> > >
> >
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> > > > >
> > > >
> > >
> >
> 7Cd04c2e49c3a840994da408d8b39f3304%7C43083d15727340c1b7db39efd9cc
> > > > >
> > > >
> > >
> >
> c17a%7C0%7C0%7C637456843629116269%7CUnknown%7CTWFpbGZsb3d8eyJ
> > > > >
> > > >
> > >
> >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > > > >
> > >
> > 1000&amp;sdata=85096rASBtzbjU42pV6sPkl3nVt5HlR6%2BL9nxI3qgFA%3D&a
> > > > > mp;reserved=0
> > > > >
> > > > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > > > ---
> > > > >  doc/guides/prog_guide/rte_flow.rst | 35 ++++++++++++++++++
> > > > >  lib/librte_ethdev/rte_flow.c       |  1 +
> > > > >  lib/librte_ethdev/rte_flow.h       | 59
> > ++++++++++++++++++++++++++++++
> > > > >  3 files changed, 95 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > > > b/doc/guides/prog_guide/rte_flow.rst
> > > > > index 86b3444803..b737ff9dad 100644
> > > > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > > > @@ -2766,6 +2766,41 @@ The behaviour of the shared action defined
> by
> > > > > ``action`` argument of type
> > > > >     | no properties |
> > > > >     +---------------+
> > > > >
> > > > > +Action: ``COPY_ITEM``
> > > > > +^^^^^^^^^^^^^^^^^^^^^
> > > > > +
> > > > > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > > > > +
> > > > > +An arbitrary header field (as well as mark, metadata or tag values)
> > > > > +can be used as both source and destination items as set by ``item``.
> > > > > +
> > > >
> > > > For tag I think you should also use the index right?
> > > Right, index is here to access any element in the tag array in addition to
> > > outermost or any inner packet header fields. Will specify explicitly in the doc.
> > >
> > > > > +Inner packet header fields can be accessed using the ``index`` and
> > > > > +it is possible to start the copy from the ``offset`` bits in an item.
> > > >
> > > > Please specify  what means index 0 /1 ... 0 is outer most? Inner most?
> > > > You can look at the RSS level for reference.
> > > > I think it will be best to use the same values.
> > > 0 is outer most, of course,  I'Il add some clarification about that.
> > >
> > Not necessary, like I said please look at the RSS, 0 can mean the inner most.
> >
> > > > What happens if we want to copy between different sizes?
> > > > for example copy IPV6 src to number of tags? I assume we will be using
> > offset
> > > > and
> > > > split the copy command to number of actions right?
> > > That is the correct understanding. We can utilize 4 copy_item actions in this
> > > case to
> > > copy 32-bits of an IPv6 SRC at the time (specifying different offsets) to 4
> > > different Tag fields.
> > >
> > > > > +
> > > > > +.. _table_rte_flow_action_copy_item:
> > > > > +
> > > > > +.. table:: COPY_ITEM
> > > > > +
> > > > > +   +-----------------------------------------+
> > > > > +   | Field         | Value                   |
> > > > > +   +===============+=========================+
> > > > > +   | ``dst``       | destination item        |
> > > > > +   | ``src``       | source item             |
> > > > > +   | ``width``     | number of bits to copy  |
> > > > > +   +---------------+-------------------------+
> > > > > +
> > > > > +.. _table_rte_flow_action_copy_data:
> > > > > +
> > > > > +.. table:: destination/source item definition
> > > > > +
> > > > > +   +----------------------------------------------------------+
> > > > > +   | Field         | Value                                    |
> > > > > +
> > > +===============+==========================================+
> > > > > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > > > > +   | ``index``     | index of outer/inner header or tag array |
> > > > > +   | ``offset``    | number of bits to skip during the copy   |
> > > > > +   +---------------+------------------------------------------+
> > > > > +
> > > > >  Negative types
> > > > >  ~~~~~~~~~~~~~~
> > > > >
> > > > > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > > > > index a06f64c271..fdbabefc47 100644
> > > > > --- a/lib/librte_ethdev/rte_flow.c
> > > > > +++ b/lib/librte_ethdev/rte_flow.c
> > > > > @@ -176,6 +176,7 @@ static const struct rte_flow_desc_data
> > > > > rte_flow_desc_action[] = {
> > > > >  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> > > > > rte_flow_action_set_dscp)),
> > > > >  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > > > >  	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > > > > +	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct
> > > > > rte_flow_action_copy_item)),
> > > > >  	/**
> > > > >  	 * Shared action represented as handle of type
> > > > >  	 * (struct rte_flow_shared action *) stored in conf field (see
> > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > > > index 0977a78270..0540c861fb 100644
> > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > @@ -2198,6 +2198,16 @@ enum rte_flow_action_type {
> > > > >  	 * struct rte_flow_shared_action).
> > > > >  	 */
> > > > >  	RTE_FLOW_ACTION_TYPE_SHARED,
> > > > > +
> > > > > +	/**
> > > > > +	 * Copy a packet header field, tag, mark or metadata.
> > > > > +	 *
> > > > > +	 * Allow saving an arbitrary header field by copying its value
> > > > > +	 * to a tag/mark/metadata or copy it into another header field.
> > > > > +	 *
> > > > > +	 * See struct rte_flow_action_copy_item.
> > > > > +	 */
> > > > > +	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
> > > > >  };
> > > > >
> > > > >  /**
> > > > > @@ -2791,6 +2801,55 @@ struct rte_flow_action_set_dscp {
> > > > >   */
> > > > >  struct rte_flow_shared_action;
> > > > >
> > > > > +enum rte_flow_item_id {
> > > > > +	RTE_FLOW_ITEM_NONE = 0,
> > > > > +	RTE_FLOW_ITEM_MAC_DST,
> > > > > +	RTE_FLOW_ITEM_MAC_SRC,
> > > > > +	RTE_FLOW_ITEM_VLAN_TYPE,
> > > > > +	RTE_FLOW_ITEM_VLAN_ID,
> > > > > +	RTE_FLOW_ITEM_MAC_TYPE,
> > > > > +	RTE_FLOW_ITEM_IPV4_DSCP,
> > > > > +	RTE_FLOW_ITEM_IPV4_TTL,
> > > > > +	RTE_FLOW_ITEM_IPV4_SRC,
> > > > > +	RTE_FLOW_ITEM_IPV4_DST,
> > > > > +	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
> > > > > +	RTE_FLOW_ITEM_IPV6_SRC,
> > > > > +	RTE_FLOW_ITEM_IPV6_DST,
> > > > > +	RTE_FLOW_ITEM_TCP_PORT_SRC,
> > > > > +	RTE_FLOW_ITEM_TCP_PORT_DST,
> > > > > +	RTE_FLOW_ITEM_TCP_SEQ_NUM,
> > > > > +	RTE_FLOW_ITEM_TCP_ACK_NUM,
> > > > > +	RTE_FLOW_ITEM_TCP_FLAGS,
> > > > > +	RTE_FLOW_ITEM_UDP_PORT_SRC,
> > > > > +	RTE_FLOW_ITEM_UDP_PORT_DST,
> > > > > +	RTE_FLOW_ITEM_VXLAN_VNI,
> > > > > +	RTE_FLOW_ITEM_GENEVE_VNI,
> > > > > +	RTE_FLOW_ITEM_GTP_TEID,
> > > > > +	RTE_FLOW_ITEM_TAG,
> > > > > +	RTE_FLOW_ITEM_MARK,
> > > > > +	RTE_FLOW_ITEM_META,
> > > > > +};
> > > >
> > > > I don't think this name is good since it not rte_flow_item this is just internal
> > > > enumeration
> > > > for this action.
> > > Will rename to rte_flow_action_copy_item_id in v3? Is this ok?
> > >
> > Yes better,
> > Or rte_flow_copy_item_id?
> >
> On second thought I think a better name should be:
> rte_flow_field_id - I removed  the copy since we may use it in other actions
> for example set/add
Then I would prefer sticking to the original rte_flow_item_id naming.
It allows room for anyone to use it as a general enum and incrorporates tag/metadata
and other future extensions, not just packet fields.

> > > > > +
> > > > > +struct rte_flow_action_copy_data {
> > > > > +	enum rte_flow_item_id item;
> > > > > +	uint32_t index;
> > > > > +	uint32_t offset;
> > > >
> > > > Why use 32 bits? Since this copy only one register with max len of 32 bit.
> > > > The max offset can 31? Same for the index.
> > > This extends the flexibility of the copy API. This way you can modify any
> place
> > > in a packet as you desire by specifying RTE_FLOW_ITEM_NONE to start with
> > > the
> > > beginning of a packet and choosing a particular offset value to point in a
> > > desired place.
> > > And since packet length can be pretty big we need to accommodate this.
> > > Do you think this flexibility is not necessary and we should bound the offset
> to
> > > the
> > > length of a selected packet field only? Index is 32 bit for the same field in
> RSS.
> > >
> > Nice idea but then I would add ITEM_OFFSET or something
> > around this name, I don't think NONE is a valid value.
> >
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * RTE_FLOW_ACTION_TYPE_COPY_ITEM
> > > > > + *
> > > > > + * Copies a specified number of bits from a source header field
> > > > > + * to a destination header field. Tag, mark or metadata can also
> > > > > + * be used as a source/destination to allow saving/overwriting
> > > > > + * an arbituary header field with a user-specified value.
> > > > > + */
> > > > > +struct rte_flow_action_copy_item {
> > > > > +	struct rte_flow_action_copy_data dst;
> > > > > +	struct rte_flow_action_copy_data src;
> > > > > +	uint32_t width;
> > > >
> > > > Why use 32 bit register?
> > > Again, to make API as generic as possible in case there will be a PMD driver
> > > that can copy a huge chunk of a packet into the another place of this packet.
> > > What is your opinion on that?
> > >
> > Nice thinking,
> >
> > > >
> > > > > +};
> > > > > +
> > > > >  /* Mbuf dynamic field offset for metadata. */
> > > > >  extern int32_t rte_flow_dynf_metadata_offs;
> > > > >
> > > > > --
> > > > > 2.24.1
> > > >
> > > > Best,
> > > > Ori
Jerin Jacob Jan. 14, 2021, 1:59 p.m. UTC | #7 
On Fri, Jan 8, 2021 at 12:02 PM Alexander Kozyrev <akozyrev@nvidia.com> wrote:
>
> Implement a generic copy flow API to allow copying of an arbitrary
> header field (as well as mark, metadata or tag) to another item.
>
> This generic copy mechanism removes the necessity to implement a
> separate RTE Flow action every time we need to modify a new packet
> field in the future. A user-provided value can be used from a
> specified tag/metadata or directly copied from other packet field.
>
> The number of bits to copy as well as the offset to start from can
> be specified to allow a partial copy or copy into an arbitrary
> place in a packet for greater flexibility.
>
> RFC: http://patches.dpdk.org/patch/85384/
>
> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> ---

>
> +Action: ``COPY_ITEM``
> +^^^^^^^^^^^^^^^^^^^^^
> +
> +Copy ``width`` bits from ``src`` item to ``dst`` item.
> +
> +An arbitrary header field (as well as mark, metadata or tag values)
> +can be used as both source and destination items as set by ``item``.
> +
> +Inner packet header fields can be accessed using the ``index`` and
> +it is possible to start the copy from the ``offset`` bits in an item.
> +
> +.. _table_rte_flow_action_copy_item:
> +
> +.. table:: COPY_ITEM
> +
> +   +-----------------------------------------+
> +   | Field         | Value                   |
> +   +===============+=========================+
> +   | ``dst``       | destination item        |
> +   | ``src``       | source item             |
> +   | ``width``     | number of bits to copy  |



Overall it is a good improvement.

I think, if we add transform "op" here then it can be more generic. In
other words, A generic packet transform and copy operation is just one
of the operations.
ie.. making it as rte_flow_action_xform_item and introduce COPY, ADD,
SUB, etc transform along with existing rte_flow_action_copy_item
fields.

It may useful for expressing P4 packet transforms to rte_flow.

The current generation of Marvell HW does not have COPY transform so I
am leaving suggestions to vendors with this HW capablity.


> +   +---------------+-------------------------+




> +
> +.. _table_rte_flow_action_copy_data:
> +
> +.. table:: destination/source item definition
> +
> +   +----------------------------------------------------------+
> +   | Field         | Value                                    |
> +   +===============+==========================================+
> +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> +   | ``index``     | index of outer/inner header or tag array |
> +   | ``offset``    | number of bits to skip during the copy   |
> +   +---------------+------------------------------------------+
> +
Ori Kam Jan. 14, 2021, 3:02 p.m. UTC | #8 
Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Thursday, January 14, 2021 4:00 PM
> Subject: Re: [dpdk-dev] [PATCH] ethdev: introduce generic copy rte flow action
> 
> On Fri, Jan 8, 2021 at 12:02 PM Alexander Kozyrev <akozyrev@nvidia.com>
> wrote:
> >
> > Implement a generic copy flow API to allow copying of an arbitrary
> > header field (as well as mark, metadata or tag) to another item.
> >
> > This generic copy mechanism removes the necessity to implement a
> > separate RTE Flow action every time we need to modify a new packet
> > field in the future. A user-provided value can be used from a
> > specified tag/metadata or directly copied from other packet field.
> >
> > The number of bits to copy as well as the offset to start from can
> > be specified to allow a partial copy or copy into an arbitrary
> > place in a packet for greater flexibility.
> >
> > RFC:
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.d
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> 7C62ab41b9ed5948d056c308d8b894af02%7C43083d15727340c1b7db39efd9cc
> c17a%7C0%7C0%7C637462296023413253%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 1000&amp;sdata=ovpEss3%2B7TgZRYFiDkrvuMFW52747Gno5oOIeDLwrBQ%3D
> &amp;reserved=0
> >
> > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > ---
> 
> >
> > +Action: ``COPY_ITEM``
> > +^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > +
> > +An arbitrary header field (as well as mark, metadata or tag values)
> > +can be used as both source and destination items as set by ``item``.
> > +
> > +Inner packet header fields can be accessed using the ``index`` and
> > +it is possible to start the copy from the ``offset`` bits in an item.
> > +
> > +.. _table_rte_flow_action_copy_item:
> > +
> > +.. table:: COPY_ITEM
> > +
> > +   +-----------------------------------------+
> > +   | Field         | Value                   |
> > +   +===============+=========================+
> > +   | ``dst``       | destination item        |
> > +   | ``src``       | source item             |
> > +   | ``width``     | number of bits to copy  |
> 
> 
> 
> Overall it is a good improvement.
> 
> I think, if we add transform "op" here then it can be more generic. In
> other words, A generic packet transform and copy operation is just one
> of the operations.
> ie.. making it as rte_flow_action_xform_item and introduce COPY, ADD,
> SUB, etc transform along with existing rte_flow_action_copy_item
> fields.
> 
> It may useful for expressing P4 packet transforms to rte_flow.
> 
> The current generation of Marvell HW does not have COPY transform so I
> am leaving suggestions to vendors with this HW capablity.
> 
> 

+1 
Lest have dst, src, width, op members,
and change the action name to rte_flow_action_modify_field()

also lest add new field name immediate  so the copy can be used as set.
(copy of an immediate value is a set)

Possible op = copy / add / sub

> > +   +---------------+-------------------------+
> 
> 
> 
> 
> > +
> > +.. _table_rte_flow_action_copy_data:
> > +
> > +.. table:: destination/source item definition
> > +
> > +   +----------------------------------------------------------+
> > +   | Field         | Value                                    |
> > +   +===============+==========================================+
> > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > +   | ``index``     | index of outer/inner header or tag array |
> > +   | ``offset``    | number of bits to skip during the copy   |
> > +   +---------------+------------------------------------------+
> > +
Jerin Jacob Jan. 15, 2021, 2 p.m. UTC | #9 
On Thu, Jan 14, 2021 at 8:32 PM Ori Kam <orika@nvidia.com> wrote:
>
> Hi Jerin,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Thursday, January 14, 2021 4:00 PM
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: introduce generic copy rte flow action
> >
> > On Fri, Jan 8, 2021 at 12:02 PM Alexander Kozyrev <akozyrev@nvidia.com>
> > wrote:
> > >
> > > Implement a generic copy flow API to allow copying of an arbitrary
> > > header field (as well as mark, metadata or tag) to another item.
> > >
> > > This generic copy mechanism removes the necessity to implement a
> > > separate RTE Flow action every time we need to modify a new packet
> > > field in the future. A user-provided value can be used from a
> > > specified tag/metadata or directly copied from other packet field.
> > >
> > > The number of bits to copy as well as the offset to start from can
> > > be specified to allow a partial copy or copy into an arbitrary
> > > place in a packet for greater flexibility.
> > >
> > > RFC:
> > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.d
> > pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com%
> > 7C62ab41b9ed5948d056c308d8b894af02%7C43083d15727340c1b7db39efd9cc
> > c17a%7C0%7C0%7C637462296023413253%7CUnknown%7CTWFpbGZsb3d8eyJ
> > WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > 1000&amp;sdata=ovpEss3%2B7TgZRYFiDkrvuMFW52747Gno5oOIeDLwrBQ%3D
> > &amp;reserved=0
> > >
> > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > ---
> >
> > >
> > > +Action: ``COPY_ITEM``
> > > +^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > > +
> > > +An arbitrary header field (as well as mark, metadata or tag values)
> > > +can be used as both source and destination items as set by ``item``.
> > > +
> > > +Inner packet header fields can be accessed using the ``index`` and
> > > +it is possible to start the copy from the ``offset`` bits in an item.
> > > +
> > > +.. _table_rte_flow_action_copy_item:
> > > +
> > > +.. table:: COPY_ITEM
> > > +
> > > +   +-----------------------------------------+
> > > +   | Field         | Value                   |
> > > +   +===============+=========================+
> > > +   | ``dst``       | destination item        |
> > > +   | ``src``       | source item             |
> > > +   | ``width``     | number of bits to copy  |
> >
> >
> >
> > Overall it is a good improvement.
> >
> > I think, if we add transform "op" here then it can be more generic. In
> > other words, A generic packet transform and copy operation is just one
> > of the operations.
> > ie.. making it as rte_flow_action_xform_item and introduce COPY, ADD,
> > SUB, etc transform along with existing rte_flow_action_copy_item
> > fields.
> >
> > It may useful for expressing P4 packet transforms to rte_flow.
> >
> > The current generation of Marvell HW does not have COPY transform so I
> > am leaving suggestions to vendors with this HW capablity.
> >
> >
>
> +1
> Lest have dst, src, width, op members,
> and change the action name to rte_flow_action_modify_field()
>
> also lest add new field name immediate  so the copy can be used as set.
> (copy of an immediate value is a set)
>
> Possible op = copy / add / sub

+1

>
> > > +   +---------------+-------------------------+
> >
> >
> >
> >
> > > +
> > > +.. _table_rte_flow_action_copy_data:
> > > +
> > > +.. table:: destination/source item definition
> > > +
> > > +   +----------------------------------------------------------+
> > > +   | Field         | Value                                    |
> > > +   +===============+==========================================+
> > > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > > +   | ``index``     | index of outer/inner header or tag array |
> > > +   | ``offset``    | number of bits to skip during the copy   |
> > > +   +---------------+------------------------------------------+
> > > +
Alexander Kozyrev Jan. 15, 2021, 3:33 p.m. UTC | #10 
> From: Jerin Jacob <jerinjacobk@gmail.com> on  Friday, January 15, 2021 9:01
> Subject: Re: [dpdk-dev] [PATCH] ethdev: introduce generic copy rte flow
> action
> 
> On Thu, Jan 14, 2021 at 8:32 PM Ori Kam <orika@nvidia.com> wrote:
> >
> > Hi Jerin,
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Thursday, January 14, 2021 4:00 PM
> > > Subject: Re: [dpdk-dev] [PATCH] ethdev: introduce generic copy rte flow
> action
> > >
> > > On Fri, Jan 8, 2021 at 12:02 PM Alexander Kozyrev
> <akozyrev@nvidia.com>
> > > wrote:
> > > >
> > > > Implement a generic copy flow API to allow copying of an arbitrary
> > > > header field (as well as mark, metadata or tag) to another item.
> > > >
> > > > This generic copy mechanism removes the necessity to implement a
> > > > separate RTE Flow action every time we need to modify a new packet
> > > > field in the future. A user-provided value can be used from a
> > > > specified tag/metadata or directly copied from other packet field.
> > > >
> > > > The number of bits to copy as well as the offset to start from can
> > > > be specified to allow a partial copy or copy into an arbitrary
> > > > place in a packet for greater flexibility.
> > > >
> > > > RFC:
> > >
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.d
> > >
> pdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Corika%40nvidia.com
> %
> > >
> 7C62ab41b9ed5948d056c308d8b894af02%7C43083d15727340c1b7db39efd9cc
> > >
> c17a%7C0%7C0%7C637462296023413253%7CUnknown%7CTWFpbGZsb3d8ey
> J
> > >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C
> > >
> 1000&amp;sdata=ovpEss3%2B7TgZRYFiDkrvuMFW52747Gno5oOIeDLwrBQ%
> 3D
> > > &amp;reserved=0
> > > >
> > > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > > ---
> > >
> > > >
> > > > +Action: ``COPY_ITEM``
> > > > +^^^^^^^^^^^^^^^^^^^^^
> > > > +
> > > > +Copy ``width`` bits from ``src`` item to ``dst`` item.
> > > > +
> > > > +An arbitrary header field (as well as mark, metadata or tag values)
> > > > +can be used as both source and destination items as set by ``item``.
> > > > +
> > > > +Inner packet header fields can be accessed using the ``index`` and
> > > > +it is possible to start the copy from the ``offset`` bits in an item.
> > > > +
> > > > +.. _table_rte_flow_action_copy_item:
> > > > +
> > > > +.. table:: COPY_ITEM
> > > > +
> > > > +   +-----------------------------------------+
> > > > +   | Field         | Value                   |
> > > > +   +===============+=========================+
> > > > +   | ``dst``       | destination item        |
> > > > +   | ``src``       | source item             |
> > > > +   | ``width``     | number of bits to copy  |
> > >
> > >
> > >
> > > Overall it is a good improvement.
> > >
> > > I think, if we add transform "op" here then it can be more generic. In
> > > other words, A generic packet transform and copy operation is just one
> > > of the operations.
> > > ie.. making it as rte_flow_action_xform_item and introduce COPY, ADD,
> > > SUB, etc transform along with existing rte_flow_action_copy_item
> > > fields.
> > >
> > > It may useful for expressing P4 packet transforms to rte_flow.
> > >
> > > The current generation of Marvell HW does not have COPY transform so I
> > > am leaving suggestions to vendors with this HW capablity.
> > >
> > >
> >
> > +1
> > Lest have dst, src, width, op members,
> > and change the action name to rte_flow_action_modify_field()
> >
> > also lest add new field name immediate  so the copy can be used as set.
> > (copy of an immediate value is a set)
> >
> > Possible op = copy / add / sub
> 
> +1
Thank you, Jerin, for a great idea. I'm issuing a v4 patch with generic modify_field design. 

> 
> >
> > > > +   +---------------+-------------------------+
> > >
> > >
> > >
> > >
> > > > +
> > > > +.. _table_rte_flow_action_copy_data:
> > > > +
> > > > +.. table:: destination/source item definition
> > > > +
> > > > +   +----------------------------------------------------------+
> > > > +   | Field         | Value                                    |
> > > > +
> +===============+=========================================
> =+
> > > > +   | ``item``      | ID of a packet field/mark/metadata/tag   |
> > > > +   | ``index``     | index of outer/inner header or tag array |
> > > > +   | ``offset``    | number of bits to skip during the copy   |
> > > > +   +---------------+------------------------------------------+
> > > > +
diff mbox series

Patch

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 86b3444803..b737ff9dad 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2766,6 +2766,41 @@  The behaviour of the shared action defined by ``action`` argument of type
    | no properties |
    +---------------+
 
+Action: ``COPY_ITEM``
+^^^^^^^^^^^^^^^^^^^^^
+
+Copy ``width`` bits from ``src`` item to ``dst`` item.
+
+An arbitrary header field (as well as mark, metadata or tag values)
+can be used as both source and destination items as set by ``item``.
+
+Inner packet header fields can be accessed using the ``index`` and
+it is possible to start the copy from the ``offset`` bits in an item.
+
+.. _table_rte_flow_action_copy_item:
+
+.. table:: COPY_ITEM
+
+   +-----------------------------------------+
+   | Field         | Value                   |
+   +===============+=========================+
+   | ``dst``       | destination item        |
+   | ``src``       | source item             |
+   | ``width``     | number of bits to copy  |
+   +---------------+-------------------------+
+
+.. _table_rte_flow_action_copy_data:
+
+.. table:: destination/source item definition
+
+   +----------------------------------------------------------+
+   | Field         | Value                                    |
+   +===============+==========================================+
+   | ``item``      | ID of a packet field/mark/metadata/tag   |
+   | ``index``     | index of outer/inner header or tag array |
+   | ``offset``    | number of bits to skip during the copy   |
+   +---------------+------------------------------------------+
+
 Negative types
 ~~~~~~~~~~~~~~
 
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index a06f64c271..fdbabefc47 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -176,6 +176,7 @@  static const struct rte_flow_desc_data rte_flow_desc_action[] = {
 	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
 	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
 	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
+	MK_FLOW_ACTION(COPY_ITEM, sizeof(struct rte_flow_action_copy_item)),
 	/**
 	 * Shared action represented as handle of type
 	 * (struct rte_flow_shared action *) stored in conf field (see
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 0977a78270..0540c861fb 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -2198,6 +2198,16 @@  enum rte_flow_action_type {
 	 * struct rte_flow_shared_action).
 	 */
 	RTE_FLOW_ACTION_TYPE_SHARED,
+
+	/**
+	 * Copy a packet header field, tag, mark or metadata.
+	 *
+	 * Allow saving an arbitrary header field by copying its value
+	 * to a tag/mark/metadata or copy it into another header field.
+	 *
+	 * See struct rte_flow_action_copy_item.
+	 */
+	RTE_FLOW_ACTION_TYPE_COPY_ITEM,
 };
 
 /**
@@ -2791,6 +2801,55 @@  struct rte_flow_action_set_dscp {
  */
 struct rte_flow_shared_action;
 
+enum rte_flow_item_id {
+	RTE_FLOW_ITEM_NONE = 0,
+	RTE_FLOW_ITEM_MAC_DST,
+	RTE_FLOW_ITEM_MAC_SRC,
+	RTE_FLOW_ITEM_VLAN_TYPE,
+	RTE_FLOW_ITEM_VLAN_ID,
+	RTE_FLOW_ITEM_MAC_TYPE,
+	RTE_FLOW_ITEM_IPV4_DSCP,
+	RTE_FLOW_ITEM_IPV4_TTL,
+	RTE_FLOW_ITEM_IPV4_SRC,
+	RTE_FLOW_ITEM_IPV4_DST,
+	RTE_FLOW_ITEM_IPV6_HOPLIMIT,
+	RTE_FLOW_ITEM_IPV6_SRC,
+	RTE_FLOW_ITEM_IPV6_DST,
+	RTE_FLOW_ITEM_TCP_PORT_SRC,
+	RTE_FLOW_ITEM_TCP_PORT_DST,
+	RTE_FLOW_ITEM_TCP_SEQ_NUM,
+	RTE_FLOW_ITEM_TCP_ACK_NUM,
+	RTE_FLOW_ITEM_TCP_FLAGS,
+	RTE_FLOW_ITEM_UDP_PORT_SRC,
+	RTE_FLOW_ITEM_UDP_PORT_DST,
+	RTE_FLOW_ITEM_VXLAN_VNI,
+	RTE_FLOW_ITEM_GENEVE_VNI,
+	RTE_FLOW_ITEM_GTP_TEID,
+	RTE_FLOW_ITEM_TAG,
+	RTE_FLOW_ITEM_MARK,
+	RTE_FLOW_ITEM_META,
+};
+
+struct rte_flow_action_copy_data {
+	enum rte_flow_item_id item;
+	uint32_t index;
+	uint32_t offset;
+};
+
+/**
+ * RTE_FLOW_ACTION_TYPE_COPY_ITEM
+ *
+ * Copies a specified number of bits from a source header field
+ * to a destination header field. Tag, mark or metadata can also
+ * be used as a source/destination to allow saving/overwriting
+ * an arbituary header field with a user-specified value.
+ */
+struct rte_flow_action_copy_item {
+	struct rte_flow_action_copy_data dst;
+	struct rte_flow_action_copy_data src;
+	uint32_t width;
+};
+
 /* Mbuf dynamic field offset for metadata. */
 extern int32_t rte_flow_dynf_metadata_offs;