[v3,2/3] ethdev: add compare item

Message ID 20240115091318.1053433-3-suanmingm@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add RTE_FLOW_ITEM_TYPE_COMPARE |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Suanming Mou Jan. 15, 2024, 9:13 a.m. UTC
  The new item type is added for the case user wants to match traffic
based on packet field compare result with other fields or immediate
value.

e.g. take advantage the compare item user will be able to accumulate
a IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
register, then compare the tag register with IPv4 header total length
to understand the packet has payload or not.

The supported operations can be as below:
 - RTE_FLOW_ITEM_COMPARE_EQ (equal)
 - RTE_FLOW_ITEM_COMPARE_NE (not equal)
 - RTE_FLOW_ITEM_COMPARE_LT (less than)
 - RTE_FLOW_ITEM_COMPARE_LE (less than or equal)
 - RTE_FLOW_ITEM_COMPARE_GT (great than)
 - RTE_FLOW_ITEM_COMPARE_GE (great than or equal)

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 app/test-pmd/cmdline_flow.c                 | 414 +++++++++++++++++++-
 doc/guides/nics/features/default.ini        |   1 +
 doc/guides/prog_guide/rte_flow.rst          |   7 +
 doc/guides/rel_notes/release_24_03.rst      |   5 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |   7 +
 lib/ethdev/rte_flow.c                       |   1 +
 lib/ethdev/rte_flow.h                       | 322 ++++++++-------
 7 files changed, 603 insertions(+), 154 deletions(-)
  

Comments

Ferruh Yigit Jan. 30, 2024, 5:33 p.m. UTC | #1
On 1/15/2024 9:13 AM, Suanming Mou wrote:
> The new item type is added for the case user wants to match traffic
> based on packet field compare result with other fields or immediate
> value.
> 
> e.g. take advantage the compare item user will be able to accumulate
> a IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
> register, then compare the tag register with IPv4 header total length
> to understand the packet has payload or not.
> 

ack, above sample makes it easier to understand.

This patch is adding testpmd commands, can you please provide some
sample commands in commit log?
The more samples are better, as far as I remember there was a testpmd
documentation that documents the sample usages, can you please check for it?

> The supported operations can be as below:
>  - RTE_FLOW_ITEM_COMPARE_EQ (equal)
>  - RTE_FLOW_ITEM_COMPARE_NE (not equal)
>  - RTE_FLOW_ITEM_COMPARE_LT (less than)
>  - RTE_FLOW_ITEM_COMPARE_LE (less than or equal)
>  - RTE_FLOW_ITEM_COMPARE_GT (great than)
>  - RTE_FLOW_ITEM_COMPARE_GE (great than or equal)
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

<...>

>  
> -static const char *const modify_field_ids[] = {
> +static const char *const flow_field_ids[] = {
>

I wonder if this rename should be in previous patch, as it does the
logical change of the modify action specific fields to more generic fields.

<...>

> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
> index a691e794f4..8c8c661218 100644
> --- a/doc/guides/rel_notes/release_24_03.rst
> +++ b/doc/guides/rel_notes/release_24_03.rst
> @@ -55,6 +55,11 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +* **Added compare flow matching criteria.**
> +
> +  Added ``RTE_FLOW_ITEM_TYPE_COMPARE`` to allow matching on compare
> +  result between the packet fields or value.
> +
>  * **Updated NVIDIA mlx5 driver.**
>  
>    * Added support for accumulating from src field to dst field.
>

I guess you are rebasing on some internal repo, because above NVIDIA
note doesn't exist in upstream repo. Can you please rebase on latest
next-net, this also helps to resolve conflict with random action in
upstream repo.

<...>

> +/**
> + * Field IDs for packet field.
> + */
> +enum rte_flow_field_id {
> +	RTE_FLOW_FIELD_START = 0,	/**< Start of a packet. */
> +	RTE_FLOW_FIELD_MAC_DST,		/**< Destination MAC Address. */
> +	RTE_FLOW_FIELD_MAC_SRC,		/**< Source MAC Address. */
> +	RTE_FLOW_FIELD_VLAN_TYPE,	/**< VLAN Tag Identifier. */
> +	RTE_FLOW_FIELD_VLAN_ID,		/**< VLAN Identifier. */
> +	RTE_FLOW_FIELD_MAC_TYPE,	/**< EtherType. */
> +	RTE_FLOW_FIELD_IPV4_DSCP,	/**< IPv4 DSCP. */
> +	RTE_FLOW_FIELD_IPV4_TTL,	/**< IPv4 Time To Live. */
> +	RTE_FLOW_FIELD_IPV4_SRC,	/**< IPv4 Source Address. */
> +	RTE_FLOW_FIELD_IPV4_DST,	/**< IPv4 Destination Address. */
> +	RTE_FLOW_FIELD_IPV6_DSCP,	/**< IPv6 DSCP. */
> +	RTE_FLOW_FIELD_IPV6_HOPLIMIT,	/**< IPv6 Hop Limit. */
> +	RTE_FLOW_FIELD_IPV6_SRC,	/**< IPv6 Source Address. */
> +	RTE_FLOW_FIELD_IPV6_DST,	/**< IPv6 Destination Address. */
> +	RTE_FLOW_FIELD_TCP_PORT_SRC,	/**< TCP Source Port Number. */
> +	RTE_FLOW_FIELD_TCP_PORT_DST,	/**< TCP Destination Port Number. */
> +	RTE_FLOW_FIELD_TCP_SEQ_NUM,	/**< TCP Sequence Number. */
> +	RTE_FLOW_FIELD_TCP_ACK_NUM,	/**< TCP Acknowledgment Number. */
> +	RTE_FLOW_FIELD_TCP_FLAGS,	/**< TCP Flags. */
> +	RTE_FLOW_FIELD_UDP_PORT_SRC,	/**< UDP Source Port Number. */
> +	RTE_FLOW_FIELD_UDP_PORT_DST,	/**< UDP Destination Port Number. */
> +	RTE_FLOW_FIELD_VXLAN_VNI,	/**< VXLAN Network Identifier. */
> +	RTE_FLOW_FIELD_GENEVE_VNI,	/**< GENEVE Network Identifier. */
> +	RTE_FLOW_FIELD_GTP_TEID,	/**< GTP Tunnel Endpoint Identifier. */
> +	RTE_FLOW_FIELD_TAG,		/**< Tag value. */
> +	RTE_FLOW_FIELD_MARK,		/**< Mark value. */
> +	RTE_FLOW_FIELD_META,		/**< Metadata value. */
> +	RTE_FLOW_FIELD_POINTER,		/**< Memory pointer. */
> +	RTE_FLOW_FIELD_VALUE,		/**< Immediate value. */
> +	RTE_FLOW_FIELD_IPV4_ECN,	/**< IPv4 ECN. */
> +	RTE_FLOW_FIELD_IPV6_ECN,	/**< IPv6 ECN. */
> +	RTE_FLOW_FIELD_GTP_PSC_QFI,	/**< GTP QFI. */
> +	RTE_FLOW_FIELD_METER_COLOR,	/**< Meter color marker. */
> +	RTE_FLOW_FIELD_IPV6_PROTO,	/**< IPv6 next header. */
> +	RTE_FLOW_FIELD_FLEX_ITEM,	/**< Flex item. */
> +	RTE_FLOW_FIELD_HASH_RESULT,	/**< Hash result. */
> +	RTE_FLOW_FIELD_GENEVE_OPT_TYPE,	/**< GENEVE option type. */
> +	RTE_FLOW_FIELD_GENEVE_OPT_CLASS,/**< GENEVE option class. */
> +	RTE_FLOW_FIELD_GENEVE_OPT_DATA,	/**< GENEVE option data. */
> +	RTE_FLOW_FIELD_MPLS,		/**< MPLS header. */
> +	RTE_FLOW_FIELD_TCP_DATA_OFFSET,	/**< TCP data offset. */
> +	RTE_FLOW_FIELD_IPV4_IHL,	/**< IPv4 IHL. */
> +	RTE_FLOW_FIELD_IPV4_TOTAL_LEN,	/**< IPv4 total length. */
> +	RTE_FLOW_FIELD_IPV6_PAYLOAD_LEN	/**< IPv6 payload length. */
> +};
> +

+1 to move the structs to keep the proper order, but not sure if it is
better to do this in previous patch or this one.

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * Field description for packet field.
> + */
> +struct rte_flow_field_data {
> +	enum rte_flow_field_id field; /**< Field or memory type ID. */
> +	union {
> +		struct {
> +			/** Encapsulation level and tag index or flex item handle. */
> +			union {
> +				struct {
> +					/**
> +					 * Packet encapsulation level containing
> +					 * the field to modify.
> +					 *
> +					 * - @p 0 requests the default behavior.
> +					 *   Depending on the packet type, it
> +					 *   can mean outermost, innermost or
> +					 *   anything in between.
> +					 *
> +					 *   It basically stands for the
> +					 *   innermost encapsulation level.
> +					 *   Modification can be performed
> +					 *   according to PMD and device
> +					 *   capabilities.
> +					 *
> +					 * - @p 1 requests modification to be
> +					 *   performed on the outermost packet
> +					 *   encapsulation level.
> +					 *
> +					 * - @p 2 and subsequent values request
> +					 *   modification to be performed on
> +					 *   the specified inner packet
> +					 *   encapsulation level, from
> +					 *   outermost to innermost (lower to
> +					 *   higher values).
> +					 *
> +					 * Values other than @p 0 are not
> +					 * necessarily supported.
> +					 *
> +					 * @note that for MPLS field,
> +					 * encapsulation level also include
> +					 * tunnel since MPLS may appear in
> +					 * outer, inner or tunnel.
> +					 */
> +					uint8_t level;
> +					union {
> +						/**
> +						 * Tag index array inside
> +						 * encapsulation level.
> +						 * Used for VLAN, MPLS or TAG types.
> +						 */
> +						uint8_t tag_index;
> +						/**
> +						 * Geneve option identifier.
> +						 * Relevant only for
> +						 * RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> +						 * modification type.
> +						 */
> +						struct {
> +							/**
> +							 * Geneve option type.
> +							 */
> +							uint8_t type;
> +							/**
> +							 * Geneve option class.
> +							 */
> +							rte_be16_t class_id;
> +						};
> +					};
> +				};
> +				struct rte_flow_item_flex_handle *flex_handle;
> +			};
> +			/** Number of bits to skip from a field. */
> +			uint32_t offset;
> +		};
> +		/**
> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
> +		 * same byte order and length as in relevant rte_flow_item_xxx.
> +		 * The immediate source bitfield offset is inherited from
> +		 * the destination's one.
> +		 */
> +		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.
> +		 */
> +		void *pvalue;
> +	};
> +};
> +
>

I am aware that you are just moving the above struct, but it is nested
too much which is making it hard to read.

As you are touching it, can we extract some structs and make this struct
less nested, what do you think?
Of course it needs to be done in separate patch, as a
preperation/clean-up patch before moving it around.

<...>

> +/**
> + *
> + * RTE_FLOW_ITEM_TYPE_COMPARE
> + *
> + * Matches the packet with compare result.
> + *
> + * The operation means a compare with b result.
> + */
> +struct rte_flow_item_compare {
> +	enum rte_flow_item_compare_op operation; /* The compare operation. */
> +	struct rte_flow_field_data a;		 /* Field be compared.  */
> +	struct rte_flow_field_data b;		 /* Field as comparator. */
>

Variable names 'a' and 'b' are not descriptive although it may be OK
since there is no significance to the values, but other option can be
'first' and 'second', but overall not strong opinion.
  
Suanming Mou Jan. 31, 2024, 2:47 a.m. UTC | #2
Hi,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, January 31, 2024 1:34 AM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> 
> On 1/15/2024 9:13 AM, Suanming Mou wrote:
> > The new item type is added for the case user wants to match traffic
> > based on packet field compare result with other fields or immediate
> > value.
> >
> > e.g. take advantage the compare item user will be able to accumulate a
> > IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
> > register, then compare the tag register with IPv4 header total length
> > to understand the packet has payload or not.
> >
> 
> ack, above sample makes it easier to understand.
> 
> This patch is adding testpmd commands, can you please provide some sample
> commands in commit log?
> The more samples are better, as far as I remember there was a testpmd
> documentation that documents the sample usages, can you please check for it?

Yes, I think we have something to do in "testpmd_funcs.rst", will update.

> 
> > The supported operations can be as below:
> >  - RTE_FLOW_ITEM_COMPARE_EQ (equal)
> >  - RTE_FLOW_ITEM_COMPARE_NE (not equal)
> >  - RTE_FLOW_ITEM_COMPARE_LT (less than)
> >  - RTE_FLOW_ITEM_COMPARE_LE (less than or equal)
> >  - RTE_FLOW_ITEM_COMPARE_GT (great than)
> >  - RTE_FLOW_ITEM_COMPARE_GE (great than or equal)
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> > Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
> <...>
> 
> >
> > -static const char *const modify_field_ids[] = {
> > +static const char *const flow_field_ids[] = {
> >
> 
> I wonder if this rename should be in previous patch, as it does the logical change
> of the modify action specific fields to more generic fields.

Agree, will adjust.

> 
> <...>
> 
> > diff --git a/doc/guides/rel_notes/release_24_03.rst
> > b/doc/guides/rel_notes/release_24_03.rst
> > index a691e794f4..8c8c661218 100644
> > --- a/doc/guides/rel_notes/release_24_03.rst
> > +++ b/doc/guides/rel_notes/release_24_03.rst
> > @@ -55,6 +55,11 @@ New Features
> >       Also, make sure to start the actual text at the margin.
> >       =======================================================
> >
> > +* **Added compare flow matching criteria.**
> > +
> > +  Added ``RTE_FLOW_ITEM_TYPE_COMPARE`` to allow matching on compare
> > + result between the packet fields or value.
> > +
> >  * **Updated NVIDIA mlx5 driver.**
> >
> >    * Added support for accumulating from src field to dst field.
> >
> 
> I guess you are rebasing on some internal repo, because above NVIDIA note
> doesn't exist in upstream repo. Can you please rebase on latest next-net, this also
> helps to resolve conflict with random action in upstream repo.

Will rebase and update.

> 
> <...>
> 
> > +/**
> > + * Field IDs for packet field.
> > + */
> > +enum rte_flow_field_id {
> > +	RTE_FLOW_FIELD_START = 0,	/**< Start of a packet. */
> > +	RTE_FLOW_FIELD_MAC_DST,		/**< Destination MAC Address.
> */
> > +	RTE_FLOW_FIELD_MAC_SRC,		/**< Source MAC Address. */
> > +	RTE_FLOW_FIELD_VLAN_TYPE,	/**< VLAN Tag Identifier. */
> > +	RTE_FLOW_FIELD_VLAN_ID,		/**< VLAN Identifier. */
> > +	RTE_FLOW_FIELD_MAC_TYPE,	/**< EtherType. */
> > +	RTE_FLOW_FIELD_IPV4_DSCP,	/**< IPv4 DSCP. */
> > +	RTE_FLOW_FIELD_IPV4_TTL,	/**< IPv4 Time To Live. */
> > +	RTE_FLOW_FIELD_IPV4_SRC,	/**< IPv4 Source Address. */
> > +	RTE_FLOW_FIELD_IPV4_DST,	/**< IPv4 Destination Address. */
> > +	RTE_FLOW_FIELD_IPV6_DSCP,	/**< IPv6 DSCP. */
> > +	RTE_FLOW_FIELD_IPV6_HOPLIMIT,	/**< IPv6 Hop Limit. */
> > +	RTE_FLOW_FIELD_IPV6_SRC,	/**< IPv6 Source Address. */
> > +	RTE_FLOW_FIELD_IPV6_DST,	/**< IPv6 Destination Address. */
> > +	RTE_FLOW_FIELD_TCP_PORT_SRC,	/**< TCP Source Port Number.
> */
> > +	RTE_FLOW_FIELD_TCP_PORT_DST,	/**< TCP Destination Port
> Number. */
> > +	RTE_FLOW_FIELD_TCP_SEQ_NUM,	/**< TCP Sequence Number. */
> > +	RTE_FLOW_FIELD_TCP_ACK_NUM,	/**< TCP Acknowledgment
> Number. */
> > +	RTE_FLOW_FIELD_TCP_FLAGS,	/**< TCP Flags. */
> > +	RTE_FLOW_FIELD_UDP_PORT_SRC,	/**< UDP Source Port Number.
> */
> > +	RTE_FLOW_FIELD_UDP_PORT_DST,	/**< UDP Destination Port
> Number. */
> > +	RTE_FLOW_FIELD_VXLAN_VNI,	/**< VXLAN Network Identifier. */
> > +	RTE_FLOW_FIELD_GENEVE_VNI,	/**< GENEVE Network
> Identifier. */
> > +	RTE_FLOW_FIELD_GTP_TEID,	/**< GTP Tunnel Endpoint Identifier. */
> > +	RTE_FLOW_FIELD_TAG,		/**< Tag value. */
> > +	RTE_FLOW_FIELD_MARK,		/**< Mark value. */
> > +	RTE_FLOW_FIELD_META,		/**< Metadata value. */
> > +	RTE_FLOW_FIELD_POINTER,		/**< Memory pointer. */
> > +	RTE_FLOW_FIELD_VALUE,		/**< Immediate value. */
> > +	RTE_FLOW_FIELD_IPV4_ECN,	/**< IPv4 ECN. */
> > +	RTE_FLOW_FIELD_IPV6_ECN,	/**< IPv6 ECN. */
> > +	RTE_FLOW_FIELD_GTP_PSC_QFI,	/**< GTP QFI. */
> > +	RTE_FLOW_FIELD_METER_COLOR,	/**< Meter color marker. */
> > +	RTE_FLOW_FIELD_IPV6_PROTO,	/**< IPv6 next header. */
> > +	RTE_FLOW_FIELD_FLEX_ITEM,	/**< Flex item. */
> > +	RTE_FLOW_FIELD_HASH_RESULT,	/**< Hash result. */
> > +	RTE_FLOW_FIELD_GENEVE_OPT_TYPE,	/**< GENEVE option type. */
> > +	RTE_FLOW_FIELD_GENEVE_OPT_CLASS,/**< GENEVE option class. */
> > +	RTE_FLOW_FIELD_GENEVE_OPT_DATA,	/**< GENEVE option data. */
> > +	RTE_FLOW_FIELD_MPLS,		/**< MPLS header. */
> > +	RTE_FLOW_FIELD_TCP_DATA_OFFSET,	/**< TCP data offset. */
> > +	RTE_FLOW_FIELD_IPV4_IHL,	/**< IPv4 IHL. */
> > +	RTE_FLOW_FIELD_IPV4_TOTAL_LEN,	/**< IPv4 total length. */
> > +	RTE_FLOW_FIELD_IPV6_PAYLOAD_LEN	/**< IPv6 payload length. */
> > +};
> > +
> 
> +1 to move the structs to keep the proper order, but not sure if it is
> better to do this in previous patch or this one.

The previous patch is just for renaming, I assume moving the struct is too much in previous patch, what do you think?

> 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * Field description for packet field.
> > + */
> > +struct rte_flow_field_data {
> > +	enum rte_flow_field_id field; /**< Field or memory type ID. */
> > +	union {
> > +		struct {
> > +			/** Encapsulation level and tag index or flex item
> handle. */
> > +			union {
> > +				struct {
> > +					/**
> > +					 * Packet encapsulation level containing
> > +					 * the field to modify.
> > +					 *
> > +					 * - @p 0 requests the default behavior.
> > +					 *   Depending on the packet type, it
> > +					 *   can mean outermost, innermost or
> > +					 *   anything in between.
> > +					 *
> > +					 *   It basically stands for the
> > +					 *   innermost encapsulation level.
> > +					 *   Modification can be performed
> > +					 *   according to PMD and device
> > +					 *   capabilities.
> > +					 *
> > +					 * - @p 1 requests modification to be
> > +					 *   performed on the outermost packet
> > +					 *   encapsulation level.
> > +					 *
> > +					 * - @p 2 and subsequent values
> request
> > +					 *   modification to be performed on
> > +					 *   the specified inner packet
> > +					 *   encapsulation level, from
> > +					 *   outermost to innermost (lower to
> > +					 *   higher values).
> > +					 *
> > +					 * Values other than @p 0 are not
> > +					 * necessarily supported.
> > +					 *
> > +					 * @note that for MPLS field,
> > +					 * encapsulation level also include
> > +					 * tunnel since MPLS may appear in
> > +					 * outer, inner or tunnel.
> > +					 */
> > +					uint8_t level;
> > +					union {
> > +						/**
> > +						 * Tag index array inside
> > +						 * encapsulation level.
> > +						 * Used for VLAN, MPLS or TAG
> types.
> > +						 */
> > +						uint8_t tag_index;
> > +						/**
> > +						 * Geneve option identifier.
> > +						 * Relevant only for
> > +						 *
> RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> > +						 * modification type.
> > +						 */
> > +						struct {
> > +							/**
> > +							 * Geneve option type.
> > +							 */
> > +							uint8_t type;
> > +							/**
> > +							 * Geneve option class.
> > +							 */
> > +							rte_be16_t class_id;
> > +						};
> > +					};
> > +				};
> > +				struct rte_flow_item_flex_handle *flex_handle;
> > +			};
> > +			/** Number of bits to skip from a field. */
> > +			uint32_t offset;
> > +		};
> > +		/**
> > +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in
> the
> > +		 * same byte order and length as in relevant rte_flow_item_xxx.
> > +		 * The immediate source bitfield offset is inherited from
> > +		 * the destination's one.
> > +		 */
> > +		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.
> > +		 */
> > +		void *pvalue;
> > +	};
> > +};
> > +
> >
> 
> I am aware that you are just moving the above struct, but it is nested too much
> which is making it hard to read.
> 
> As you are touching it, can we extract some structs and make this struct less
> nested, what do you think?
> Of course it needs to be done in separate patch, as a preperation/clean-up patch
> before moving it around.

Agree the struct maybe a bit nested. But not sure how it was discussed before during the last new member was added... @Ori, Do you have any idea about this?

And if it is really expected, I believe another new thread is worth for that change,  better not be in that series.
Need to discuss the new struct name and other stuff. What do you think? 

> 
> <...>
> 
> > +/**
> > + *
> > + * RTE_FLOW_ITEM_TYPE_COMPARE
> > + *
> > + * Matches the packet with compare result.
> > + *
> > + * The operation means a compare with b result.
> > + */
> > +struct rte_flow_item_compare {
> > +	enum rte_flow_item_compare_op operation; /* The compare operation.
> */
> > +	struct rte_flow_field_data a;		 /* Field be compared.  */
> > +	struct rte_flow_field_data b;		 /* Field as comparator. */
> >
> 
> Variable names 'a' and 'b' are not descriptive although it may be OK since there is
> no significance to the values, but other option can be 'first' and 'second', but
> overall not strong opinion.

Yes, thanks for the suggestion, in fact we also discussed about the name a lot, finally we choose the widely used 'a' and 'b'

Thanks
  
Ori Kam Jan. 31, 2024, 3:56 p.m. UTC | #3
Hi

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Wednesday, January 31, 2024 4:48 AM
> 
> Hi,
> 
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@amd.com>
> > Sent: Wednesday, January 31, 2024 1:34 AM
> > To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> <orika@nvidia.com>;
> > Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> > <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > <thomas@monjalon.net>; Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> >
> > On 1/15/2024 9:13 AM, Suanming Mou wrote:
> > > The new item type is added for the case user wants to match traffic
> > > based on packet field compare result with other fields or immediate
> > > value.
> > >
> > > e.g. take advantage the compare item user will be able to accumulate a
> > > IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
> > > register, then compare the tag register with IPv4 header total length
> > > to understand the packet has payload or not.
> > >
> >
> > ack, above sample makes it easier to understand.
> >
> > This patch is adding testpmd commands, can you please provide some
> sample
> > commands in commit log?
> > The more samples are better, as far as I remember there was a testpmd
> > documentation that documents the sample usages, can you please check
> for it?

[Snip ..]

> >
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * Field description for packet field.
> > > + */
> > > +struct rte_flow_field_data {
> > > +	enum rte_flow_field_id field; /**< Field or memory type ID. */
> > > +	union {
> > > +		struct {
> > > +			/** Encapsulation level and tag index or flex item
> > handle. */
> > > +			union {
> > > +				struct {
> > > +					/**
> > > +					 * Packet encapsulation level
> containing
> > > +					 * the field to modify.
> > > +					 *
> > > +					 * - @p 0 requests the default
> behavior.
> > > +					 *   Depending on the packet type, it
> > > +					 *   can mean outermost, innermost
> or
> > > +					 *   anything in between.
> > > +					 *
> > > +					 *   It basically stands for the
> > > +					 *   innermost encapsulation level.
> > > +					 *   Modification can be performed
> > > +					 *   according to PMD and device
> > > +					 *   capabilities.
> > > +					 *
> > > +					 * - @p 1 requests modification to be
> > > +					 *   performed on the outermost
> packet
> > > +					 *   encapsulation level.
> > > +					 *
> > > +					 * - @p 2 and subsequent values
> > request
> > > +					 *   modification to be performed on
> > > +					 *   the specified inner packet
> > > +					 *   encapsulation level, from
> > > +					 *   outermost to innermost (lower to
> > > +					 *   higher values).
> > > +					 *
> > > +					 * Values other than @p 0 are not
> > > +					 * necessarily supported.
> > > +					 *
> > > +					 * @note that for MPLS field,
> > > +					 * encapsulation level also include
> > > +					 * tunnel since MPLS may appear in
> > > +					 * outer, inner or tunnel.
> > > +					 */
> > > +					uint8_t level;
> > > +					union {
> > > +						/**
> > > +						 * Tag index array inside
> > > +						 * encapsulation level.
> > > +						 * Used for VLAN, MPLS or
> TAG
> > types.
> > > +						 */
> > > +						uint8_t tag_index;
> > > +						/**
> > > +						 * Geneve option identifier.
> > > +						 * Relevant only for
> > > +						 *
> > RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> > > +						 * modification type.
> > > +						 */
> > > +						struct {
> > > +							/**
> > > +							 * Geneve option
> type.
> > > +							 */
> > > +							uint8_t type;
> > > +							/**
> > > +							 * Geneve option
> class.
> > > +							 */
> > > +							rte_be16_t class_id;
> > > +						};
> > > +					};
> > > +				};
> > > +				struct rte_flow_item_flex_handle
> *flex_handle;
> > > +			};
> > > +			/** Number of bits to skip from a field. */
> > > +			uint32_t offset;
> > > +		};
> > > +		/**
> > > +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
> in
> > the
> > > +		 * same byte order and length as in relevant
> rte_flow_item_xxx.
> > > +		 * The immediate source bitfield offset is inherited from
> > > +		 * the destination's one.
> > > +		 */
> > > +		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.
> > > +		 */
> > > +		void *pvalue;
> > > +	};
> > > +};
> > > +
> > >
> >
> > I am aware that you are just moving the above struct, but it is nested too
> much
> > which is making it hard to read.
> >
> > As you are touching it, can we extract some structs and make this struct less
> > nested, what do you think?
> > Of course it needs to be done in separate patch, as a preperation/clean-up
> patch
> > before moving it around.
> 
> Agree the struct maybe a bit nested. But not sure how it was discussed
> before during the last new member was added... @Ori, Do you have any idea
> about this?
> 

As far as I remember, it was never discussed, 

I think for this series we should keep it as is, and revise it later.

Best,
Ori
> And if it is really expected, I believe another new thread is worth for that
> change,  better not be in that series.
> Need to discuss the new struct name and other stuff. What do you think?
> 
> >
> > <...>
> >
> > > +/**
> > > + *
> > > + * RTE_FLOW_ITEM_TYPE_COMPARE
> > > + *
> > > + * Matches the packet with compare result.
> > > + *
> > > + * The operation means a compare with b result.
> > > + */
> > > +struct rte_flow_item_compare {
> > > +	enum rte_flow_item_compare_op operation; /* The compare
> operation.
> > */
> > > +	struct rte_flow_field_data a;		 /* Field be compared.  */
> > > +	struct rte_flow_field_data b;		 /* Field as comparator. */
> > >
> >
> > Variable names 'a' and 'b' are not descriptive although it may be OK since
> there is
> > no significance to the values, but other option can be 'first' and 'second',
> but
> > overall not strong opinion.
> 
> Yes, thanks for the suggestion, in fact we also discussed about the name a lot,
> finally we choose the widely used 'a' and 'b'
> 
> Thanks
  
Ferruh Yigit Jan. 31, 2024, 4:46 p.m. UTC | #4
On 1/31/2024 3:56 PM, Ori Kam wrote:
> Hi
> 
>> -----Original Message-----
>> From: Suanming Mou <suanmingm@nvidia.com>
>> Sent: Wednesday, January 31, 2024 4:48 AM
>>
>> Hi,
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>> Sent: Wednesday, January 31, 2024 1:34 AM
>>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
>> <orika@nvidia.com>;
>>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
>>> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
>>> <thomas@monjalon.net>; Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
>>>
>>> On 1/15/2024 9:13 AM, Suanming Mou wrote:
>>>> The new item type is added for the case user wants to match traffic
>>>> based on packet field compare result with other fields or immediate
>>>> value.
>>>>
>>>> e.g. take advantage the compare item user will be able to accumulate a
>>>> IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
>>>> register, then compare the tag register with IPv4 header total length
>>>> to understand the packet has payload or not.
>>>>
>>>
>>> ack, above sample makes it easier to understand.
>>>
>>> This patch is adding testpmd commands, can you please provide some
>> sample
>>> commands in commit log?
>>> The more samples are better, as far as I remember there was a testpmd
>>> documentation that documents the sample usages, can you please check
>> for it?
> 
> [Snip ..]
> 
>>>
>>>> +/**
>>>> + * @warning
>>>> + * @b EXPERIMENTAL: this structure may change without prior notice
>>>> + *
>>>> + * Field description for packet field.
>>>> + */
>>>> +struct rte_flow_field_data {
>>>> +	enum rte_flow_field_id field; /**< Field or memory type ID. */
>>>> +	union {
>>>> +		struct {
>>>> +			/** Encapsulation level and tag index or flex item
>>> handle. */
>>>> +			union {
>>>> +				struct {
>>>> +					/**
>>>> +					 * Packet encapsulation level
>> containing
>>>> +					 * the field to modify.
>>>> +					 *
>>>> +					 * - @p 0 requests the default
>> behavior.
>>>> +					 *   Depending on the packet type, it
>>>> +					 *   can mean outermost, innermost
>> or
>>>> +					 *   anything in between.
>>>> +					 *
>>>> +					 *   It basically stands for the
>>>> +					 *   innermost encapsulation level.
>>>> +					 *   Modification can be performed
>>>> +					 *   according to PMD and device
>>>> +					 *   capabilities.
>>>> +					 *
>>>> +					 * - @p 1 requests modification to be
>>>> +					 *   performed on the outermost
>> packet
>>>> +					 *   encapsulation level.
>>>> +					 *
>>>> +					 * - @p 2 and subsequent values
>>> request
>>>> +					 *   modification to be performed on
>>>> +					 *   the specified inner packet
>>>> +					 *   encapsulation level, from
>>>> +					 *   outermost to innermost (lower to
>>>> +					 *   higher values).
>>>> +					 *
>>>> +					 * Values other than @p 0 are not
>>>> +					 * necessarily supported.
>>>> +					 *
>>>> +					 * @note that for MPLS field,
>>>> +					 * encapsulation level also include
>>>> +					 * tunnel since MPLS may appear in
>>>> +					 * outer, inner or tunnel.
>>>> +					 */
>>>> +					uint8_t level;
>>>> +					union {
>>>> +						/**
>>>> +						 * Tag index array inside
>>>> +						 * encapsulation level.
>>>> +						 * Used for VLAN, MPLS or
>> TAG
>>> types.
>>>> +						 */
>>>> +						uint8_t tag_index;
>>>> +						/**
>>>> +						 * Geneve option identifier.
>>>> +						 * Relevant only for
>>>> +						 *
>>> RTE_FLOW_FIELD_GENEVE_OPT_XXXX
>>>> +						 * modification type.
>>>> +						 */
>>>> +						struct {
>>>> +							/**
>>>> +							 * Geneve option
>> type.
>>>> +							 */
>>>> +							uint8_t type;
>>>> +							/**
>>>> +							 * Geneve option
>> class.
>>>> +							 */
>>>> +							rte_be16_t class_id;
>>>> +						};
>>>> +					};
>>>> +				};
>>>> +				struct rte_flow_item_flex_handle
>> *flex_handle;
>>>> +			};
>>>> +			/** Number of bits to skip from a field. */
>>>> +			uint32_t offset;
>>>> +		};
>>>> +		/**
>>>> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
>> in
>>> the
>>>> +		 * same byte order and length as in relevant
>> rte_flow_item_xxx.
>>>> +		 * The immediate source bitfield offset is inherited from
>>>> +		 * the destination's one.
>>>> +		 */
>>>> +		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.
>>>> +		 */
>>>> +		void *pvalue;
>>>> +	};
>>>> +};
>>>> +
>>>>
>>>
>>> I am aware that you are just moving the above struct, but it is nested too
>> much
>>> which is making it hard to read.
>>>
>>> As you are touching it, can we extract some structs and make this struct less
>>> nested, what do you think?
>>> Of course it needs to be done in separate patch, as a preperation/clean-up
>> patch
>>> before moving it around.
>>
>> Agree the struct maybe a bit nested. But not sure how it was discussed
>> before during the last new member was added... @Ori, Do you have any idea
>> about this?
>>
> 
> As far as I remember, it was never discussed, 
> 
> I think for this series we should keep it as is, and revise it later.
> 

If you don't want to make this set more complex with this, that is OK as
long as it is addressed at some point.

> Best,
> Ori
>> And if it is really expected, I believe another new thread is worth for that
>> change,  better not be in that series.
>> Need to discuss the new struct name and other stuff. What do you think?
>>
>>>
>>> <...>
>>>
>>>> +/**
>>>> + *
>>>> + * RTE_FLOW_ITEM_TYPE_COMPARE
>>>> + *
>>>> + * Matches the packet with compare result.
>>>> + *
>>>> + * The operation means a compare with b result.
>>>> + */
>>>> +struct rte_flow_item_compare {
>>>> +	enum rte_flow_item_compare_op operation; /* The compare
>> operation.
>>> */
>>>> +	struct rte_flow_field_data a;		 /* Field be compared.  */
>>>> +	struct rte_flow_field_data b;		 /* Field as comparator. */
>>>>
>>>
>>> Variable names 'a' and 'b' are not descriptive although it may be OK since
>> there is
>>> no significance to the values, but other option can be 'first' and 'second',
>> but
>>> overall not strong opinion.
>>
>> Yes, thanks for the suggestion, in fact we also discussed about the name a lot,
>> finally we choose the widely used 'a' and 'b'
>>
>> Thanks
>
  
Ori Kam Jan. 31, 2024, 5:43 p.m. UTC | #5
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, January 31, 2024 6:46 PM
> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> 
> On 1/31/2024 3:56 PM, Ori Kam wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Suanming Mou <suanmingm@nvidia.com>
> >> Sent: Wednesday, January 31, 2024 4:48 AM
> >>
> >> Hi,
> >>
> >>> -----Original Message-----
> >>> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >>> Sent: Wednesday, January 31, 2024 1:34 AM
> >>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> >> <orika@nvidia.com>;
> >>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> >>> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> >>> <thomas@monjalon.net>; Andrew Rybchenko
> >>> <andrew.rybchenko@oktetlabs.ru>
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> >>>
> >>> On 1/15/2024 9:13 AM, Suanming Mou wrote:
> >>>> The new item type is added for the case user wants to match traffic
> >>>> based on packet field compare result with other fields or immediate
> >>>> value.
> >>>>
> >>>> e.g. take advantage the compare item user will be able to accumulate a
> >>>> IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
> >>>> register, then compare the tag register with IPv4 header total length
> >>>> to understand the packet has payload or not.
> >>>>
> >>>
> >>> ack, above sample makes it easier to understand.
> >>>
> >>> This patch is adding testpmd commands, can you please provide some
> >> sample
> >>> commands in commit log?
> >>> The more samples are better, as far as I remember there was a testpmd
> >>> documentation that documents the sample usages, can you please check
> >> for it?
> >
> > [Snip ..]
> >
> >>>
> >>>> +/**
> >>>> + * @warning
> >>>> + * @b EXPERIMENTAL: this structure may change without prior notice
> >>>> + *
> >>>> + * Field description for packet field.
> >>>> + */
> >>>> +struct rte_flow_field_data {
> >>>> +	enum rte_flow_field_id field; /**< Field or memory type ID. */
> >>>> +	union {
> >>>> +		struct {
> >>>> +			/** Encapsulation level and tag index or flex item
> >>> handle. */
> >>>> +			union {
> >>>> +				struct {
> >>>> +					/**
> >>>> +					 * Packet encapsulation level
> >> containing
> >>>> +					 * the field to modify.
> >>>> +					 *
> >>>> +					 * - @p 0 requests the default
> >> behavior.
> >>>> +					 *   Depending on the packet type, it
> >>>> +					 *   can mean outermost, innermost
> >> or
> >>>> +					 *   anything in between.
> >>>> +					 *
> >>>> +					 *   It basically stands for the
> >>>> +					 *   innermost encapsulation level.
> >>>> +					 *   Modification can be performed
> >>>> +					 *   according to PMD and device
> >>>> +					 *   capabilities.
> >>>> +					 *
> >>>> +					 * - @p 1 requests modification to be
> >>>> +					 *   performed on the outermost
> >> packet
> >>>> +					 *   encapsulation level.
> >>>> +					 *
> >>>> +					 * - @p 2 and subsequent values
> >>> request
> >>>> +					 *   modification to be performed on
> >>>> +					 *   the specified inner packet
> >>>> +					 *   encapsulation level, from
> >>>> +					 *   outermost to innermost (lower to
> >>>> +					 *   higher values).
> >>>> +					 *
> >>>> +					 * Values other than @p 0 are not
> >>>> +					 * necessarily supported.
> >>>> +					 *
> >>>> +					 * @note that for MPLS field,
> >>>> +					 * encapsulation level also include
> >>>> +					 * tunnel since MPLS may appear in
> >>>> +					 * outer, inner or tunnel.
> >>>> +					 */
> >>>> +					uint8_t level;
> >>>> +					union {
> >>>> +						/**
> >>>> +						 * Tag index array inside
> >>>> +						 * encapsulation level.
> >>>> +						 * Used for VLAN, MPLS or
> >> TAG
> >>> types.
> >>>> +						 */
> >>>> +						uint8_t tag_index;
> >>>> +						/**
> >>>> +						 * Geneve option identifier.
> >>>> +						 * Relevant only for
> >>>> +						 *
> >>> RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> >>>> +						 * modification type.
> >>>> +						 */
> >>>> +						struct {
> >>>> +							/**
> >>>> +							 * Geneve option
> >> type.
> >>>> +							 */
> >>>> +							uint8_t type;
> >>>> +							/**
> >>>> +							 * Geneve option
> >> class.
> >>>> +							 */
> >>>> +							rte_be16_t class_id;
> >>>> +						};
> >>>> +					};
> >>>> +				};
> >>>> +				struct rte_flow_item_flex_handle
> >> *flex_handle;
> >>>> +			};
> >>>> +			/** Number of bits to skip from a field. */
> >>>> +			uint32_t offset;
> >>>> +		};
> >>>> +		/**
> >>>> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
> >> in
> >>> the
> >>>> +		 * same byte order and length as in relevant
> >> rte_flow_item_xxx.
> >>>> +		 * The immediate source bitfield offset is inherited from
> >>>> +		 * the destination's one.
> >>>> +		 */
> >>>> +		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.
> >>>> +		 */
> >>>> +		void *pvalue;
> >>>> +	};
> >>>> +};
> >>>> +
> >>>>
> >>>
> >>> I am aware that you are just moving the above struct, but it is nested too
> >> much
> >>> which is making it hard to read.
> >>>
> >>> As you are touching it, can we extract some structs and make this struct
> less
> >>> nested, what do you think?
> >>> Of course it needs to be done in separate patch, as a preperation/clean-
> up
> >> patch
> >>> before moving it around.
> >>
> >> Agree the struct maybe a bit nested. But not sure how it was discussed
> >> before during the last new member was added... @Ori, Do you have any
> idea
> >> about this?
> >>
> >
> > As far as I remember, it was never discussed,
> >
> > I think for this series we should keep it as is, and revise it later.
> >
> 
> If you don't want to make this set more complex with this, that is OK as
> long as it is addressed at some point.

Agree,
If you have suggestions, I will be more than happy to hear.

> 
> > Best,
> > Ori
> >> And if it is really expected, I believe another new thread is worth for that
> >> change,  better not be in that series.
> >> Need to discuss the new struct name and other stuff. What do you think?
> >>
> >>>
> >>> <...>
> >>>
> >>>> +/**
> >>>> + *
> >>>> + * RTE_FLOW_ITEM_TYPE_COMPARE
> >>>> + *
> >>>> + * Matches the packet with compare result.
> >>>> + *
> >>>> + * The operation means a compare with b result.
> >>>> + */
> >>>> +struct rte_flow_item_compare {
> >>>> +	enum rte_flow_item_compare_op operation; /* The compare
> >> operation.
> >>> */
> >>>> +	struct rte_flow_field_data a;		 /* Field be compared.  */
> >>>> +	struct rte_flow_field_data b;		 /* Field as comparator. */
> >>>>
> >>>
> >>> Variable names 'a' and 'b' are not descriptive although it may be OK since
> >> there is
> >>> no significance to the values, but other option can be 'first' and 'second',
> >> but
> >>> overall not strong opinion.
> >>
> >> Yes, thanks for the suggestion, in fact we also discussed about the name a
> lot,
> >> finally we choose the widely used 'a' and 'b'
> >>
> >> Thanks
> >
  
Ferruh Yigit Jan. 31, 2024, 5:54 p.m. UTC | #6
On 1/31/2024 5:43 PM, Ori Kam wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Wednesday, January 31, 2024 6:46 PM
>> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
>>
>> On 1/31/2024 3:56 PM, Ori Kam wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Suanming Mou <suanmingm@nvidia.com>
>>>> Sent: Wednesday, January 31, 2024 4:48 AM
>>>>
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>>> Sent: Wednesday, January 31, 2024 1:34 AM
>>>>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
>>>> <orika@nvidia.com>;
>>>>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
>>>>> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
>>>>> <thomas@monjalon.net>; Andrew Rybchenko
>>>>> <andrew.rybchenko@oktetlabs.ru>
>>>>> Cc: dev@dpdk.org
>>>>> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
>>>>>
>>>>> On 1/15/2024 9:13 AM, Suanming Mou wrote:
>>>>>> The new item type is added for the case user wants to match traffic
>>>>>> based on packet field compare result with other fields or immediate
>>>>>> value.
>>>>>>
>>>>>> e.g. take advantage the compare item user will be able to accumulate a
>>>>>> IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
>>>>>> register, then compare the tag register with IPv4 header total length
>>>>>> to understand the packet has payload or not.
>>>>>>
>>>>>
>>>>> ack, above sample makes it easier to understand.
>>>>>
>>>>> This patch is adding testpmd commands, can you please provide some
>>>> sample
>>>>> commands in commit log?
>>>>> The more samples are better, as far as I remember there was a testpmd
>>>>> documentation that documents the sample usages, can you please check
>>>> for it?
>>>
>>> [Snip ..]
>>>
>>>>>
>>>>>> +/**
>>>>>> + * @warning
>>>>>> + * @b EXPERIMENTAL: this structure may change without prior notice
>>>>>> + *
>>>>>> + * Field description for packet field.
>>>>>> + */
>>>>>> +struct rte_flow_field_data {
>>>>>> +	enum rte_flow_field_id field; /**< Field or memory type ID. */
>>>>>> +	union {
>>>>>> +		struct {
>>>>>> +			/** Encapsulation level and tag index or flex item
>>>>> handle. */
>>>>>> +			union {
>>>>>> +				struct {
>>>>>> +					/**
>>>>>> +					 * Packet encapsulation level
>>>> containing
>>>>>> +					 * the field to modify.
>>>>>> +					 *
>>>>>> +					 * - @p 0 requests the default
>>>> behavior.
>>>>>> +					 *   Depending on the packet type, it
>>>>>> +					 *   can mean outermost, innermost
>>>> or
>>>>>> +					 *   anything in between.
>>>>>> +					 *
>>>>>> +					 *   It basically stands for the
>>>>>> +					 *   innermost encapsulation level.
>>>>>> +					 *   Modification can be performed
>>>>>> +					 *   according to PMD and device
>>>>>> +					 *   capabilities.
>>>>>> +					 *
>>>>>> +					 * - @p 1 requests modification to be
>>>>>> +					 *   performed on the outermost
>>>> packet
>>>>>> +					 *   encapsulation level.
>>>>>> +					 *
>>>>>> +					 * - @p 2 and subsequent values
>>>>> request
>>>>>> +					 *   modification to be performed on
>>>>>> +					 *   the specified inner packet
>>>>>> +					 *   encapsulation level, from
>>>>>> +					 *   outermost to innermost (lower to
>>>>>> +					 *   higher values).
>>>>>> +					 *
>>>>>> +					 * Values other than @p 0 are not
>>>>>> +					 * necessarily supported.
>>>>>> +					 *
>>>>>> +					 * @note that for MPLS field,
>>>>>> +					 * encapsulation level also include
>>>>>> +					 * tunnel since MPLS may appear in
>>>>>> +					 * outer, inner or tunnel.
>>>>>> +					 */
>>>>>> +					uint8_t level;
>>>>>> +					union {
>>>>>> +						/**
>>>>>> +						 * Tag index array inside
>>>>>> +						 * encapsulation level.
>>>>>> +						 * Used for VLAN, MPLS or
>>>> TAG
>>>>> types.
>>>>>> +						 */
>>>>>> +						uint8_t tag_index;
>>>>>> +						/**
>>>>>> +						 * Geneve option identifier.
>>>>>> +						 * Relevant only for
>>>>>> +						 *
>>>>> RTE_FLOW_FIELD_GENEVE_OPT_XXXX
>>>>>> +						 * modification type.
>>>>>> +						 */
>>>>>> +						struct {
>>>>>> +							/**
>>>>>> +							 * Geneve option
>>>> type.
>>>>>> +							 */
>>>>>> +							uint8_t type;
>>>>>> +							/**
>>>>>> +							 * Geneve option
>>>> class.
>>>>>> +							 */
>>>>>> +							rte_be16_t class_id;
>>>>>> +						};
>>>>>> +					};
>>>>>> +				};
>>>>>> +				struct rte_flow_item_flex_handle
>>>> *flex_handle;
>>>>>> +			};
>>>>>> +			/** Number of bits to skip from a field. */
>>>>>> +			uint32_t offset;
>>>>>> +		};
>>>>>> +		/**
>>>>>> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
>>>> in
>>>>> the
>>>>>> +		 * same byte order and length as in relevant
>>>> rte_flow_item_xxx.
>>>>>> +		 * The immediate source bitfield offset is inherited from
>>>>>> +		 * the destination's one.
>>>>>> +		 */
>>>>>> +		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.
>>>>>> +		 */
>>>>>> +		void *pvalue;
>>>>>> +	};
>>>>>> +};
>>>>>> +
>>>>>>
>>>>>
>>>>> I am aware that you are just moving the above struct, but it is nested too
>>>> much
>>>>> which is making it hard to read.
>>>>>
>>>>> As you are touching it, can we extract some structs and make this struct
>> less
>>>>> nested, what do you think?
>>>>> Of course it needs to be done in separate patch, as a preperation/clean-
>> up
>>>> patch
>>>>> before moving it around.
>>>>
>>>> Agree the struct maybe a bit nested. But not sure how it was discussed
>>>> before during the last new member was added... @Ori, Do you have any
>> idea
>>>> about this?
>>>>
>>>
>>> As far as I remember, it was never discussed,
>>>
>>> I think for this series we should keep it as is, and revise it later.
>>>
>>
>> If you don't want to make this set more complex with this, that is OK as
>> long as it is addressed at some point.
> 
> Agree,
> If you have suggestions, I will be more than happy to hear.
> 

For the struct?
Simply extracting the inner structs as named structs to reduce the
nested structs, does this make sense?
  
Suanming Mou Feb. 1, 2024, 12:31 a.m. UTC | #7
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, February 1, 2024 12:46 AM
> To: Ori Kam <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>;
> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> 
> On 1/31/2024 3:56 PM, Ori Kam wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Suanming Mou <suanmingm@nvidia.com>
> >> Sent: Wednesday, January 31, 2024 4:48 AM
> >>
> >> Hi,
> >>
> >>> -----Original Message-----
> >>> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >>> Sent: Wednesday, January 31, 2024 1:34 AM
> >>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> >> <orika@nvidia.com>;
> >>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> >>> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> >>> <thomas@monjalon.net>; Andrew Rybchenko
> >>> <andrew.rybchenko@oktetlabs.ru>
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> >>>
> >>> On 1/15/2024 9:13 AM, Suanming Mou wrote:
> >>>> The new item type is added for the case user wants to match traffic
> >>>> based on packet field compare result with other fields or immediate
> >>>> value.
> >>>>
> >>>> e.g. take advantage the compare item user will be able to
> >>>> accumulate a IPv4/TCP packet's TCP data_offset and IPv4 IHL field
> >>>> to a tag register, then compare the tag register with IPv4 header
> >>>> total length to understand the packet has payload or not.
> >>>>
> >>>
> >>> ack, above sample makes it easier to understand.
> >>>
> >>> This patch is adding testpmd commands, can you please provide some
> >> sample
> >>> commands in commit log?
> >>> The more samples are better, as far as I remember there was a
> >>> testpmd documentation that documents the sample usages, can you
> >>> please check
> >> for it?
> >
> > [Snip ..]
> >
> >>>
> >>>> +/**
> >>>> + * @warning
> >>>> + * @b EXPERIMENTAL: this structure may change without prior notice
> >>>> + *
> >>>> + * Field description for packet field.
> >>>> + */
> >>>> +struct rte_flow_field_data {
> >>>> +	enum rte_flow_field_id field; /**< Field or memory type ID. */
> >>>> +	union {
> >>>> +		struct {
> >>>> +			/** Encapsulation level and tag index or flex item
> >>> handle. */
> >>>> +			union {
> >>>> +				struct {
> >>>> +					/**
> >>>> +					 * Packet encapsulation level
> >> containing
> >>>> +					 * the field to modify.
> >>>> +					 *
> >>>> +					 * - @p 0 requests the default
> >> behavior.
> >>>> +					 *   Depending on the packet type, it
> >>>> +					 *   can mean outermost, innermost
> >> or
> >>>> +					 *   anything in between.
> >>>> +					 *
> >>>> +					 *   It basically stands for the
> >>>> +					 *   innermost encapsulation level.
> >>>> +					 *   Modification can be performed
> >>>> +					 *   according to PMD and device
> >>>> +					 *   capabilities.
> >>>> +					 *
> >>>> +					 * - @p 1 requests modification to be
> >>>> +					 *   performed on the outermost
> >> packet
> >>>> +					 *   encapsulation level.
> >>>> +					 *
> >>>> +					 * - @p 2 and subsequent values
> >>> request
> >>>> +					 *   modification to be performed on
> >>>> +					 *   the specified inner packet
> >>>> +					 *   encapsulation level, from
> >>>> +					 *   outermost to innermost (lower to
> >>>> +					 *   higher values).
> >>>> +					 *
> >>>> +					 * Values other than @p 0 are not
> >>>> +					 * necessarily supported.
> >>>> +					 *
> >>>> +					 * @note that for MPLS field,
> >>>> +					 * encapsulation level also include
> >>>> +					 * tunnel since MPLS may appear in
> >>>> +					 * outer, inner or tunnel.
> >>>> +					 */
> >>>> +					uint8_t level;
> >>>> +					union {
> >>>> +						/**
> >>>> +						 * Tag index array inside
> >>>> +						 * encapsulation level.
> >>>> +						 * Used for VLAN, MPLS or
> >> TAG
> >>> types.
> >>>> +						 */
> >>>> +						uint8_t tag_index;
> >>>> +						/**
> >>>> +						 * Geneve option identifier.
> >>>> +						 * Relevant only for
> >>>> +						 *
> >>> RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> >>>> +						 * modification type.
> >>>> +						 */
> >>>> +						struct {
> >>>> +							/**
> >>>> +							 * Geneve option
> >> type.
> >>>> +							 */
> >>>> +							uint8_t type;
> >>>> +							/**
> >>>> +							 * Geneve option
> >> class.
> >>>> +							 */
> >>>> +							rte_be16_t class_id;
> >>>> +						};
> >>>> +					};
> >>>> +				};
> >>>> +				struct rte_flow_item_flex_handle
> >> *flex_handle;
> >>>> +			};
> >>>> +			/** Number of bits to skip from a field. */
> >>>> +			uint32_t offset;
> >>>> +		};
> >>>> +		/**
> >>>> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
> >> in
> >>> the
> >>>> +		 * same byte order and length as in relevant
> >> rte_flow_item_xxx.
> >>>> +		 * The immediate source bitfield offset is inherited from
> >>>> +		 * the destination's one.
> >>>> +		 */
> >>>> +		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.
> >>>> +		 */
> >>>> +		void *pvalue;
> >>>> +	};
> >>>> +};
> >>>> +
> >>>>
> >>>
> >>> I am aware that you are just moving the above struct, but it is
> >>> nested too
> >> much
> >>> which is making it hard to read.
> >>>
> >>> As you are touching it, can we extract some structs and make this
> >>> struct less nested, what do you think?
> >>> Of course it needs to be done in separate patch, as a
> >>> preperation/clean-up
> >> patch
> >>> before moving it around.
> >>
> >> Agree the struct maybe a bit nested. But not sure how it was
> >> discussed before during the last new member was added... @Ori, Do you
> >> have any idea about this?
> >>
> >
> > As far as I remember, it was never discussed,
> >
> > I think for this series we should keep it as is, and revise it later.
> >
> 
> If you don't want to make this set more complex with this, that is OK as long
> as it is addressed at some point.

OK, thanks. As it is addressed, will update v4 soon.

> 
> > Best,
> > Ori
> >> And if it is really expected, I believe another new thread is worth
> >> for that change,  better not be in that series.
> >> Need to discuss the new struct name and other stuff. What do you think?
> >>
> >>>
> >>> <...>
> >>>
> >>>> +/**
> >>>> + *
> >>>> + * RTE_FLOW_ITEM_TYPE_COMPARE
> >>>> + *
> >>>> + * Matches the packet with compare result.
> >>>> + *
> >>>> + * The operation means a compare with b result.
> >>>> + */
> >>>> +struct rte_flow_item_compare {
> >>>> +	enum rte_flow_item_compare_op operation; /* The compare
> >> operation.
> >>> */
> >>>> +	struct rte_flow_field_data a;		 /* Field be compared.  */
> >>>> +	struct rte_flow_field_data b;		 /* Field as comparator. */
> >>>>
> >>>
> >>> Variable names 'a' and 'b' are not descriptive although it may be OK
> >>> since
> >> there is
> >>> no significance to the values, but other option can be 'first' and
> >>> 'second',
> >> but
> >>> overall not strong opinion.
> >>
> >> Yes, thanks for the suggestion, in fact we also discussed about the
> >> name a lot, finally we choose the widely used 'a' and 'b'
> >>
> >> Thanks
> >
  
Ori Kam Feb. 1, 2024, 6:51 a.m. UTC | #8
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, January 31, 2024 7:54 PM
> 
> On 1/31/2024 5:43 PM, Ori Kam wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Wednesday, January 31, 2024 6:46 PM
> >> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> >>
> >> On 1/31/2024 3:56 PM, Ori Kam wrote:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: Suanming Mou <suanmingm@nvidia.com>
> >>>> Sent: Wednesday, January 31, 2024 4:48 AM
> >>>>
> >>>> Hi,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >>>>> Sent: Wednesday, January 31, 2024 1:34 AM
> >>>>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> >>>> <orika@nvidia.com>;
> >>>>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> >>>>> <yuying.zhang@intel.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL)
> >>>>> <thomas@monjalon.net>; Andrew Rybchenko
> >>>>> <andrew.rybchenko@oktetlabs.ru>
> >>>>> Cc: dev@dpdk.org
> >>>>> Subject: Re: [PATCH v3 2/3] ethdev: add compare item
> >>>>>
> >>>>> On 1/15/2024 9:13 AM, Suanming Mou wrote:
> >>>>>> The new item type is added for the case user wants to match traffic
> >>>>>> based on packet field compare result with other fields or immediate
> >>>>>> value.
> >>>>>>
> >>>>>> e.g. take advantage the compare item user will be able to accumulate
> a
> >>>>>> IPv4/TCP packet's TCP data_offset and IPv4 IHL field to a tag
> >>>>>> register, then compare the tag register with IPv4 header total length
> >>>>>> to understand the packet has payload or not.
> >>>>>>
> >>>>>
> >>>>> ack, above sample makes it easier to understand.
> >>>>>
> >>>>> This patch is adding testpmd commands, can you please provide some
> >>>> sample
> >>>>> commands in commit log?
> >>>>> The more samples are better, as far as I remember there was a
> testpmd
> >>>>> documentation that documents the sample usages, can you please
> check
> >>>> for it?
> >>>
> >>> [Snip ..]
> >>>
> >>>>>
> >>>>>> +/**
> >>>>>> + * @warning
> >>>>>> + * @b EXPERIMENTAL: this structure may change without prior
> notice
> >>>>>> + *
> >>>>>> + * Field description for packet field.
> >>>>>> + */
> >>>>>> +struct rte_flow_field_data {
> >>>>>> +	enum rte_flow_field_id field; /**< Field or memory type ID.
> */
> >>>>>> +	union {
> >>>>>> +		struct {
> >>>>>> +			/** Encapsulation level and tag index or flex
> item
> >>>>> handle. */
> >>>>>> +			union {
> >>>>>> +				struct {
> >>>>>> +					/**
> >>>>>> +					 * Packet encapsulation level
> >>>> containing
> >>>>>> +					 * the field to modify.
> >>>>>> +					 *
> >>>>>> +					 * - @p 0 requests the default
> >>>> behavior.
> >>>>>> +					 *   Depending on the packet
> type, it
> >>>>>> +					 *   can mean outermost,
> innermost
> >>>> or
> >>>>>> +					 *   anything in between.
> >>>>>> +					 *
> >>>>>> +					 *   It basically stands for the
> >>>>>> +					 *   innermost encapsulation
> level.
> >>>>>> +					 *   Modification can be
> performed
> >>>>>> +					 *   according to PMD and
> device
> >>>>>> +					 *   capabilities.
> >>>>>> +					 *
> >>>>>> +					 * - @p 1 requests
> modification to be
> >>>>>> +					 *   performed on the
> outermost
> >>>> packet
> >>>>>> +					 *   encapsulation level.
> >>>>>> +					 *
> >>>>>> +					 * - @p 2 and subsequent
> values
> >>>>> request
> >>>>>> +					 *   modification to be
> performed on
> >>>>>> +					 *   the specified inner packet
> >>>>>> +					 *   encapsulation level, from
> >>>>>> +					 *   outermost to innermost
> (lower to
> >>>>>> +					 *   higher values).
> >>>>>> +					 *
> >>>>>> +					 * Values other than @p 0 are
> not
> >>>>>> +					 * necessarily supported.
> >>>>>> +					 *
> >>>>>> +					 * @note that for MPLS field,
> >>>>>> +					 * encapsulation level also
> include
> >>>>>> +					 * tunnel since MPLS may
> appear in
> >>>>>> +					 * outer, inner or tunnel.
> >>>>>> +					 */
> >>>>>> +					uint8_t level;
> >>>>>> +					union {
> >>>>>> +						/**
> >>>>>> +						 * Tag index array
> inside
> >>>>>> +						 * encapsulation level.
> >>>>>> +						 * Used for VLAN,
> MPLS or
> >>>> TAG
> >>>>> types.
> >>>>>> +						 */
> >>>>>> +						uint8_t tag_index;
> >>>>>> +						/**
> >>>>>> +						 * Geneve option
> identifier.
> >>>>>> +						 * Relevant only for
> >>>>>> +						 *
> >>>>> RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> >>>>>> +						 * modification type.
> >>>>>> +						 */
> >>>>>> +						struct {
> >>>>>> +							/**
> >>>>>> +							 * Geneve
> option
> >>>> type.
> >>>>>> +							 */
> >>>>>> +							uint8_t type;
> >>>>>> +							/**
> >>>>>> +							 * Geneve
> option
> >>>> class.
> >>>>>> +							 */
> >>>>>> +							rte_be16_t
> class_id;
> >>>>>> +						};
> >>>>>> +					};
> >>>>>> +				};
> >>>>>> +				struct rte_flow_item_flex_handle
> >>>> *flex_handle;
> >>>>>> +			};
> >>>>>> +			/** Number of bits to skip from a field. */
> >>>>>> +			uint32_t offset;
> >>>>>> +		};
> >>>>>> +		/**
> >>>>>> +		 * Immediate value for RTE_FLOW_FIELD_VALUE,
> presented
> >>>> in
> >>>>> the
> >>>>>> +		 * same byte order and length as in relevant
> >>>> rte_flow_item_xxx.
> >>>>>> +		 * The immediate source bitfield offset is inherited
> from
> >>>>>> +		 * the destination's one.
> >>>>>> +		 */
> >>>>>> +		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.
> >>>>>> +		 */
> >>>>>> +		void *pvalue;
> >>>>>> +	};
> >>>>>> +};
> >>>>>> +
> >>>>>>
> >>>>>
> >>>>> I am aware that you are just moving the above struct, but it is nested
> too
> >>>> much
> >>>>> which is making it hard to read.
> >>>>>
> >>>>> As you are touching it, can we extract some structs and make this
> struct
> >> less
> >>>>> nested, what do you think?
> >>>>> Of course it needs to be done in separate patch, as a
> preperation/clean-
> >> up
> >>>> patch
> >>>>> before moving it around.
> >>>>
> >>>> Agree the struct maybe a bit nested. But not sure how it was discussed
> >>>> before during the last new member was added... @Ori, Do you have
> any
> >> idea
> >>>> about this?
> >>>>
> >>>
> >>> As far as I remember, it was never discussed,
> >>>
> >>> I think for this series we should keep it as is, and revise it later.
> >>>
> >>
> >> If you don't want to make this set more complex with this, that is OK as
> >> long as it is addressed at some point.
> >
> > Agree,
> > If you have suggestions, I will be more than happy to hear.
> >
> 
> For the struct?
> Simply extracting the inner structs as named structs to reduce the
> nested structs, does this make sense?
> 
Yes that make sense.
  

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 3725e955c7..4f448ff8ec 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -543,6 +543,28 @@  enum index {
 	ITEM_PTYPE,
 	ITEM_PTYPE_VALUE,
 	ITEM_NSH,
+	ITEM_COMPARE,
+	ITEM_COMPARE_OP,
+	ITEM_COMPARE_OP_VALUE,
+	ITEM_COMPARE_FIELD_A_TYPE,
+	ITEM_COMPARE_FIELD_A_TYPE_VALUE,
+	ITEM_COMPARE_FIELD_A_LEVEL,
+	ITEM_COMPARE_FIELD_A_LEVEL_VALUE,
+	ITEM_COMPARE_FIELD_A_TAG_INDEX,
+	ITEM_COMPARE_FIELD_A_TYPE_ID,
+	ITEM_COMPARE_FIELD_A_CLASS_ID,
+	ITEM_COMPARE_FIELD_A_OFFSET,
+	ITEM_COMPARE_FIELD_B_TYPE,
+	ITEM_COMPARE_FIELD_B_TYPE_VALUE,
+	ITEM_COMPARE_FIELD_B_LEVEL,
+	ITEM_COMPARE_FIELD_B_LEVEL_VALUE,
+	ITEM_COMPARE_FIELD_B_TAG_INDEX,
+	ITEM_COMPARE_FIELD_B_TYPE_ID,
+	ITEM_COMPARE_FIELD_B_CLASS_ID,
+	ITEM_COMPARE_FIELD_B_OFFSET,
+	ITEM_COMPARE_FIELD_B_VALUE,
+	ITEM_COMPARE_FIELD_B_POINTER,
+	ITEM_COMPARE_FIELD_WIDTH,
 
 	/* Validate/create actions. */
 	ACTIONS,
@@ -740,13 +762,17 @@  enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+static const char *const compare_ops[] = {
+	"eq", "ne", "lt", "le", "gt", "ge", NULL
+};
+
 /** Maximum size for external pattern in struct rte_flow_field_data. */
-#define ACTION_MODIFY_PATTERN_SIZE 32
+#define FLOW_FIELD_PATTERN_SIZE 32
 
 /** Storage size for struct rte_flow_action_modify_field including pattern. */
 #define ACTION_MODIFY_SIZE \
 	(sizeof(struct rte_flow_action_modify_field) + \
-	ACTION_MODIFY_PATTERN_SIZE)
+	FLOW_FIELD_PATTERN_SIZE)
 
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
@@ -940,7 +966,7 @@  static const char *const modify_field_ops[] = {
 	"set", "add", "sub", NULL
 };
 
-static const char *const modify_field_ids[] = {
+static const char *const flow_field_ids[] = {
 	"start", "mac_dst", "mac_src",
 	"vlan_type", "vlan_id", "mac_type",
 	"ipv4_dscp", "ipv4_ttl", "ipv4_src", "ipv4_dst",
@@ -1590,6 +1616,7 @@  static const enum index next_item[] = {
 	ITEM_IB_BTH,
 	ITEM_PTYPE,
 	ITEM_NSH,
+	ITEM_COMPARE,
 	END_SET,
 	ZERO,
 };
@@ -2121,6 +2148,38 @@  static const enum index item_nsh[] = {
 	ZERO,
 };
 
+static const enum index item_compare_field[] = {
+	ITEM_COMPARE_OP,
+	ITEM_COMPARE_FIELD_A_TYPE,
+	ITEM_COMPARE_FIELD_B_TYPE,
+	ITEM_NEXT,
+	ZERO,
+};
+
+static const enum index compare_field_a[] = {
+	ITEM_COMPARE_FIELD_A_TYPE,
+	ITEM_COMPARE_FIELD_A_LEVEL,
+	ITEM_COMPARE_FIELD_A_TAG_INDEX,
+	ITEM_COMPARE_FIELD_A_TYPE_ID,
+	ITEM_COMPARE_FIELD_A_CLASS_ID,
+	ITEM_COMPARE_FIELD_A_OFFSET,
+	ITEM_COMPARE_FIELD_B_TYPE,
+	ZERO,
+};
+
+static const enum index compare_field_b[] = {
+	ITEM_COMPARE_FIELD_B_TYPE,
+	ITEM_COMPARE_FIELD_B_LEVEL,
+	ITEM_COMPARE_FIELD_B_TAG_INDEX,
+	ITEM_COMPARE_FIELD_B_TYPE_ID,
+	ITEM_COMPARE_FIELD_B_CLASS_ID,
+	ITEM_COMPARE_FIELD_B_OFFSET,
+	ITEM_COMPARE_FIELD_B_VALUE,
+	ITEM_COMPARE_FIELD_B_POINTER,
+	ITEM_COMPARE_FIELD_WIDTH,
+	ZERO,
+};
+
 static const enum index next_action[] = {
 	ACTION_END,
 	ACTION_VOID,
@@ -2863,6 +2922,24 @@  comp_quota_update_name(struct context *ctx, const struct token *token,
 static int
 comp_qu_mode_name(struct context *ctx, const struct token *token,
 		  unsigned int ent, char *buf, unsigned int size);
+static int
+comp_set_compare_field_id(struct context *ctx, const struct token *token,
+			  unsigned int ent, char *buf, unsigned int size);
+static int
+comp_set_compare_op(struct context *ctx, const struct token *token,
+		    unsigned int ent, char *buf, unsigned int size);
+static int
+parse_vc_compare_op(struct context *ctx, const struct token *token,
+			 const char *str, unsigned int len, void *buf,
+			 unsigned int size);
+static int
+parse_vc_compare_field_id(struct context *ctx, const struct token *token,
+			  const char *str, unsigned int len, void *buf,
+			  unsigned int size);
+static int
+parse_vc_compare_field_level(struct context *ctx, const struct token *token,
+			     const char *str, unsigned int len, void *buf,
+			     unsigned int size);
 
 struct indlst_conf {
 	uint32_t id;
@@ -5982,6 +6059,174 @@  static const struct token token_list[] = {
 		.next = NEXT(item_nsh),
 		.call = parse_vc,
 	},
+	[ITEM_COMPARE] = {
+		.name = "compare",
+		.help = "match with the comparison result",
+		.priv = PRIV_ITEM(COMPARE, sizeof(struct rte_flow_item_compare)),
+		.next = NEXT(NEXT_ENTRY(ITEM_COMPARE_OP)),
+		.call = parse_vc,
+	},
+	[ITEM_COMPARE_OP] = {
+		.name = "op",
+		.help = "operation type",
+		.next = NEXT(item_compare_field,
+			NEXT_ENTRY(ITEM_COMPARE_OP_VALUE), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare, operation)),
+	},
+	[ITEM_COMPARE_OP_VALUE] = {
+		.name = "{operation}",
+		.help = "operation type value",
+		.call = parse_vc_compare_op,
+		.comp = comp_set_compare_op,
+	},
+	[ITEM_COMPARE_FIELD_A_TYPE] = {
+		.name = "a_type",
+		.help = "compared field type",
+		.next = NEXT(compare_field_a,
+			NEXT_ENTRY(ITEM_COMPARE_FIELD_A_TYPE_VALUE), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare, a.field)),
+	},
+	[ITEM_COMPARE_FIELD_A_TYPE_VALUE] = {
+		.name = "{a_type}",
+		.help = "compared field type value",
+		.call = parse_vc_compare_field_id,
+		.comp = comp_set_compare_field_id,
+	},
+	[ITEM_COMPARE_FIELD_A_LEVEL] = {
+		.name = "a_level",
+		.help = "compared field level",
+		.next = NEXT(compare_field_a,
+			     NEXT_ENTRY(ITEM_COMPARE_FIELD_A_LEVEL_VALUE), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare, a.level)),
+	},
+	[ITEM_COMPARE_FIELD_A_LEVEL_VALUE] = {
+		.name = "{a_level}",
+		.help = "compared field level value",
+		.call = parse_vc_compare_field_level,
+		.comp = comp_none,
+	},
+	[ITEM_COMPARE_FIELD_A_TAG_INDEX] = {
+		.name = "a_tag_index",
+		.help = "compared field tag array",
+		.next = NEXT(compare_field_a,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					a.tag_index)),
+	},
+	[ITEM_COMPARE_FIELD_A_TYPE_ID] = {
+		.name = "a_type_id",
+		.help = "compared field type ID",
+		.next = NEXT(compare_field_a,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					a.type)),
+	},
+	[ITEM_COMPARE_FIELD_A_CLASS_ID] = {
+		.name = "a_class",
+		.help = "compared field class ID",
+		.next = NEXT(compare_field_a,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_compare,
+					     a.class_id)),
+	},
+	[ITEM_COMPARE_FIELD_A_OFFSET] = {
+		.name = "a_offset",
+		.help = "compared field bit offset",
+		.next = NEXT(compare_field_a,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					a.offset)),
+	},
+	[ITEM_COMPARE_FIELD_B_TYPE] = {
+		.name = "b_type",
+		.help = "comparator field type",
+		.next = NEXT(compare_field_b,
+			NEXT_ENTRY(ITEM_COMPARE_FIELD_B_TYPE_VALUE), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					b.field)),
+	},
+	[ITEM_COMPARE_FIELD_B_TYPE_VALUE] = {
+		.name = "{b_type}",
+		.help = "comparator field type value",
+		.call = parse_vc_compare_field_id,
+		.comp = comp_set_compare_field_id,
+	},
+	[ITEM_COMPARE_FIELD_B_LEVEL] = {
+		.name = "b_level",
+		.help = "comparator field level",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(ITEM_COMPARE_FIELD_B_LEVEL_VALUE), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					b.level)),
+	},
+	[ITEM_COMPARE_FIELD_B_LEVEL_VALUE] = {
+		.name = "{b_level}",
+		.help = "comparator field level value",
+		.call = parse_vc_compare_field_level,
+		.comp = comp_none,
+	},
+	[ITEM_COMPARE_FIELD_B_TAG_INDEX] = {
+		.name = "b_tag_index",
+		.help = "comparator field tag array",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					b.tag_index)),
+	},
+	[ITEM_COMPARE_FIELD_B_TYPE_ID] = {
+		.name = "b_type_id",
+		.help = "comparator field type ID",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					b.type)),
+	},
+	[ITEM_COMPARE_FIELD_B_CLASS_ID] = {
+		.name = "b_class",
+		.help = "comparator field class ID",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_compare,
+					     b.class_id)),
+	},
+	[ITEM_COMPARE_FIELD_B_OFFSET] = {
+		.name = "b_offset",
+		.help = "comparator field bit offset",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					b.offset)),
+	},
+	[ITEM_COMPARE_FIELD_B_VALUE] = {
+		.name = "b_value",
+		.help = "comparator immediate value",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(COMMON_HEX), item_param),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_item_compare,
+					b.value)),
+	},
+	[ITEM_COMPARE_FIELD_B_POINTER] = {
+		.name = "b_ptr",
+		.help = "pointer to comparator immediate value",
+		.next = NEXT(compare_field_b,
+			     NEXT_ENTRY(COMMON_HEX), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					b.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_item_compare),
+				 FLOW_FIELD_PATTERN_SIZE)),
+	},
+	[ITEM_COMPARE_FIELD_WIDTH] = {
+		.name = "width",
+		.help = "number of bits to compare",
+		.next = NEXT(item_compare_field,
+			NEXT_ENTRY(COMMON_UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_compare,
+					width)),
+	},
 
 	/* Validate/create actions. */
 	[ACTIONS] = {
@@ -6961,7 +7206,7 @@  static const struct token token_list[] = {
 			     ARGS_ENTRY_ARB(0, 0),
 			     ARGS_ENTRY_ARB
 				(sizeof(struct rte_flow_action_modify_field),
-				 ACTION_MODIFY_PATTERN_SIZE)),
+				 FLOW_FIELD_PATTERN_SIZE)),
 		.call = parse_vc_conf,
 	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
@@ -8404,6 +8649,122 @@  parse_vc_item_l2tpv2_type(struct context *ctx, const struct token *token,
 	return len;
 }
 
+/** Parse operation for compare match item. */
+static int
+parse_vc_compare_op(struct context *ctx, const struct token *token,
+			 const char *str, unsigned int len, void *buf,
+			 unsigned int size)
+{
+	struct rte_flow_item_compare *compare_item;
+	unsigned int i;
+
+	(void)token;
+	(void)buf;
+	(void)size;
+	if (ctx->curr != ITEM_COMPARE_OP_VALUE)
+		return -1;
+	for (i = 0; compare_ops[i]; ++i)
+		if (!strcmp_partial(compare_ops[i], str, len))
+			break;
+	if (!compare_ops[i])
+		return -1;
+	if (!ctx->object)
+		return len;
+	compare_item = ctx->object;
+	compare_item->operation = (enum rte_flow_item_compare_op)i;
+	return len;
+}
+
+/** Parse id for compare match item. */
+static int
+parse_vc_compare_field_id(struct context *ctx, const struct token *token,
+			  const char *str, unsigned int len, void *buf,
+			  unsigned int size)
+{
+	struct rte_flow_item_compare *compare_item;
+	unsigned int i;
+
+	(void)token;
+	(void)buf;
+	(void)size;
+	if (ctx->curr != ITEM_COMPARE_FIELD_A_TYPE_VALUE &&
+		ctx->curr != ITEM_COMPARE_FIELD_B_TYPE_VALUE)
+		return -1;
+	for (i = 0; flow_field_ids[i]; ++i)
+		if (!strcmp_partial(flow_field_ids[i], str, len))
+			break;
+	if (!flow_field_ids[i])
+		return -1;
+	if (!ctx->object)
+		return len;
+	compare_item = ctx->object;
+	if (ctx->curr == ITEM_COMPARE_FIELD_A_TYPE_VALUE)
+		compare_item->a.field = (enum rte_flow_field_id)i;
+	else
+		compare_item->b.field = (enum rte_flow_field_id)i;
+	return len;
+}
+
+/** Parse level for compare match item. */
+static int
+parse_vc_compare_field_level(struct context *ctx, const struct token *token,
+			     const char *str, unsigned int len, void *buf,
+			     unsigned int size)
+{
+	struct rte_flow_item_compare *compare_item;
+	struct flex_item *fp = NULL;
+	uint32_t val;
+	struct buffer *out = buf;
+	char *end;
+
+	(void)token;
+	(void)size;
+	if (ctx->curr != ITEM_COMPARE_FIELD_A_LEVEL_VALUE &&
+		ctx->curr != ITEM_COMPARE_FIELD_B_LEVEL_VALUE)
+		return -1;
+	if (!ctx->object)
+		return len;
+	compare_item = ctx->object;
+	errno = 0;
+	val = strtoumax(str, &end, 0);
+	if (errno || (size_t)(end - str) != len)
+		return -1;
+	/* No need to validate action template mask value */
+	if (out->args.vc.masks) {
+		if (ctx->curr == ITEM_COMPARE_FIELD_A_LEVEL_VALUE)
+			compare_item->a.level = val;
+		else
+			compare_item->b.level = val;
+		return len;
+	}
+	if ((ctx->curr == ITEM_COMPARE_FIELD_A_LEVEL_VALUE &&
+		compare_item->a.field == RTE_FLOW_FIELD_FLEX_ITEM) ||
+		(ctx->curr == ITEM_COMPARE_FIELD_B_LEVEL_VALUE &&
+		compare_item->b.field == RTE_FLOW_FIELD_FLEX_ITEM)) {
+		if (val >= FLEX_MAX_PARSERS_NUM) {
+			printf("Bad flex item handle\n");
+			return -1;
+		}
+		fp = flex_items[ctx->port][val];
+		if (!fp) {
+			printf("Bad flex item handle\n");
+			return -1;
+		}
+	}
+	if (ctx->curr == ITEM_COMPARE_FIELD_A_LEVEL_VALUE) {
+		if (compare_item->a.field != RTE_FLOW_FIELD_FLEX_ITEM)
+			compare_item->a.level = val;
+		else
+			compare_item->a.flex_handle = fp->flex_handle;
+	} else if (ctx->curr == ITEM_COMPARE_FIELD_B_LEVEL_VALUE) {
+		if (compare_item->b.field != RTE_FLOW_FIELD_FLEX_ITEM)
+			compare_item->b.level = val;
+		else
+			compare_item->b.flex_handle = fp->flex_handle;
+	}
+	return len;
+}
+
 /** Parse meter color action type. */
 static int
 parse_vc_action_meter_color_type(struct context *ctx, const struct token *token,
@@ -9773,10 +10134,10 @@  parse_vc_modify_field_id(struct context *ctx, const struct token *token,
 	if (ctx->curr != ACTION_MODIFY_FIELD_DST_TYPE_VALUE &&
 		ctx->curr != ACTION_MODIFY_FIELD_SRC_TYPE_VALUE)
 		return -1;
-	for (i = 0; modify_field_ids[i]; ++i)
-		if (!strcmp_partial(modify_field_ids[i], str, len))
+	for (i = 0; flow_field_ids[i]; ++i)
+		if (!strcmp_partial(flow_field_ids[i], str, len))
 			break;
-	if (!modify_field_ids[i])
+	if (!flow_field_ids[i])
 		return -1;
 	if (!ctx->object)
 		return len;
@@ -11890,6 +12251,39 @@  comp_rule_id(struct context *ctx, const struct token *token,
 	return i;
 }
 
+/** Complete operation for compare match item. */
+static int
+comp_set_compare_op(struct context *ctx, const struct token *token,
+		    unsigned int ent, char *buf, unsigned int size)
+{
+	RTE_SET_USED(ctx);
+	RTE_SET_USED(token);
+	if (!buf)
+		return RTE_DIM(compare_ops);
+	if (ent < RTE_DIM(compare_ops) - 1)
+		return strlcpy(buf, compare_ops[ent], size);
+	return -1;
+}
+
+/** Complete field id for compare match item. */
+static int
+comp_set_compare_field_id(struct context *ctx, const struct token *token,
+			  unsigned int ent, char *buf, unsigned int size)
+{
+	const char *name;
+
+	RTE_SET_USED(token);
+	if (!buf)
+		return RTE_DIM(flow_field_ids);
+	if (ent >= RTE_DIM(flow_field_ids) - 1)
+		return -1;
+	name = flow_field_ids[ent];
+	if (ctx->curr == ITEM_COMPARE_FIELD_B_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
+	return -1;
+}
+
 /** Complete type field for RSS action. */
 static int
 comp_vc_action_rss_type(struct context *ctx, const struct token *token,
@@ -12003,10 +12397,10 @@  comp_set_modify_field_id(struct context *ctx, const struct token *token,
 
 	RTE_SET_USED(token);
 	if (!buf)
-		return RTE_DIM(modify_field_ids);
-	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return RTE_DIM(flow_field_ids);
+	if (ent >= RTE_DIM(flow_field_ids) - 1)
 		return -1;
-	name = modify_field_ids[ent];
+	name = flow_field_ids[ent];
 	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
 	    (strcmp(name, "pointer") && strcmp(name, "value")))
 		return strlcpy(buf, name, size);
diff --git a/doc/guides/nics/features/default.ini b/doc/guides/nics/features/default.ini
index 806cb033ff..565338731f 100644
--- a/doc/guides/nics/features/default.ini
+++ b/doc/guides/nics/features/default.ini
@@ -87,6 +87,7 @@  aggr_affinity        =
 ah                   =
 any                  =
 arp_eth_ipv4         =
+compare              =
 conntrack            =
 ecpri                =
 esp                  =
diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index bf25c849fb..b720157d18 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1573,6 +1573,13 @@  Matches the packet type as defined in rte_mbuf_ptype.
 
 - ``packet_type``: L2/L3/L4 and tunnel information.
 
+Item: ``COMPARE``
+^^^^^^^^^^^^^^^^^
+
+Matches the comparison result between packet fields or value.
+
+- ``compare``: Comparison information.
+
 Actions
 ~~~~~~~
 
diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
index a691e794f4..8c8c661218 100644
--- a/doc/guides/rel_notes/release_24_03.rst
+++ b/doc/guides/rel_notes/release_24_03.rst
@@ -55,6 +55,11 @@  New Features
      Also, make sure to start the actual text at the margin.
      =======================================================
 
+* **Added compare flow matching criteria.**
+
+  Added ``RTE_FLOW_ITEM_TYPE_COMPARE`` to allow matching on compare
+  result between the packet fields or value.
+
 * **Updated NVIDIA mlx5 driver.**
 
   * Added support for accumulating from src field to dst field.
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 447e28e694..2d96deae75 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3841,6 +3841,13 @@  This section lists supported pattern items and their attributes, if any.
 
         - ``packet_type {unsigned}``: packet type.
 
+- ``compare``: match the comparison result between packet fields or value.
+
+        - ``op {string}``: comparison operation type.
+        - ``a_type {string}``: compared field.
+        - ``b_type {string}``: comparator field.
+        - ``width {unsigned}``: comparison width.
+
 
 Actions list
 ^^^^^^^^^^^^
diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index 549e329558..88cf003b2b 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -167,6 +167,7 @@  static const struct rte_flow_desc_data rte_flow_desc_item[] = {
 	MK_FLOW_ITEM(TX_QUEUE, sizeof(struct rte_flow_item_tx_queue)),
 	MK_FLOW_ITEM(IB_BTH, sizeof(struct rte_flow_item_ib_bth)),
 	MK_FLOW_ITEM(PTYPE, sizeof(struct rte_flow_item_ptype)),
+	MK_FLOW_ITEM(COMPARE, sizeof(struct rte_flow_item_compare)),
 };
 
 /** Generate flow_action[] entry. */
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 40f6dcaacd..8506184a76 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -704,6 +704,13 @@  enum rte_flow_item_type {
 	 *
 	 */
 	RTE_FLOW_ITEM_TYPE_PTYPE,
+
+	/**
+	 * Matches the packet with compare result.
+	 *
+	 * See struct rte_flow_item_compare.
+	 */
+	RTE_FLOW_ITEM_TYPE_COMPARE,
 };
 
 /**
@@ -2336,6 +2343,177 @@  static const struct rte_flow_item_ptype rte_flow_item_ptype_mask = {
 };
 #endif
 
+/**
+ * Field IDs for packet field.
+ */
+enum rte_flow_field_id {
+	RTE_FLOW_FIELD_START = 0,	/**< Start of a packet. */
+	RTE_FLOW_FIELD_MAC_DST,		/**< Destination MAC Address. */
+	RTE_FLOW_FIELD_MAC_SRC,		/**< Source MAC Address. */
+	RTE_FLOW_FIELD_VLAN_TYPE,	/**< VLAN Tag Identifier. */
+	RTE_FLOW_FIELD_VLAN_ID,		/**< VLAN Identifier. */
+	RTE_FLOW_FIELD_MAC_TYPE,	/**< EtherType. */
+	RTE_FLOW_FIELD_IPV4_DSCP,	/**< IPv4 DSCP. */
+	RTE_FLOW_FIELD_IPV4_TTL,	/**< IPv4 Time To Live. */
+	RTE_FLOW_FIELD_IPV4_SRC,	/**< IPv4 Source Address. */
+	RTE_FLOW_FIELD_IPV4_DST,	/**< IPv4 Destination Address. */
+	RTE_FLOW_FIELD_IPV6_DSCP,	/**< IPv6 DSCP. */
+	RTE_FLOW_FIELD_IPV6_HOPLIMIT,	/**< IPv6 Hop Limit. */
+	RTE_FLOW_FIELD_IPV6_SRC,	/**< IPv6 Source Address. */
+	RTE_FLOW_FIELD_IPV6_DST,	/**< IPv6 Destination Address. */
+	RTE_FLOW_FIELD_TCP_PORT_SRC,	/**< TCP Source Port Number. */
+	RTE_FLOW_FIELD_TCP_PORT_DST,	/**< TCP Destination Port Number. */
+	RTE_FLOW_FIELD_TCP_SEQ_NUM,	/**< TCP Sequence Number. */
+	RTE_FLOW_FIELD_TCP_ACK_NUM,	/**< TCP Acknowledgment Number. */
+	RTE_FLOW_FIELD_TCP_FLAGS,	/**< TCP Flags. */
+	RTE_FLOW_FIELD_UDP_PORT_SRC,	/**< UDP Source Port Number. */
+	RTE_FLOW_FIELD_UDP_PORT_DST,	/**< UDP Destination Port Number. */
+	RTE_FLOW_FIELD_VXLAN_VNI,	/**< VXLAN Network Identifier. */
+	RTE_FLOW_FIELD_GENEVE_VNI,	/**< GENEVE Network Identifier. */
+	RTE_FLOW_FIELD_GTP_TEID,	/**< GTP Tunnel Endpoint Identifier. */
+	RTE_FLOW_FIELD_TAG,		/**< Tag value. */
+	RTE_FLOW_FIELD_MARK,		/**< Mark value. */
+	RTE_FLOW_FIELD_META,		/**< Metadata value. */
+	RTE_FLOW_FIELD_POINTER,		/**< Memory pointer. */
+	RTE_FLOW_FIELD_VALUE,		/**< Immediate value. */
+	RTE_FLOW_FIELD_IPV4_ECN,	/**< IPv4 ECN. */
+	RTE_FLOW_FIELD_IPV6_ECN,	/**< IPv6 ECN. */
+	RTE_FLOW_FIELD_GTP_PSC_QFI,	/**< GTP QFI. */
+	RTE_FLOW_FIELD_METER_COLOR,	/**< Meter color marker. */
+	RTE_FLOW_FIELD_IPV6_PROTO,	/**< IPv6 next header. */
+	RTE_FLOW_FIELD_FLEX_ITEM,	/**< Flex item. */
+	RTE_FLOW_FIELD_HASH_RESULT,	/**< Hash result. */
+	RTE_FLOW_FIELD_GENEVE_OPT_TYPE,	/**< GENEVE option type. */
+	RTE_FLOW_FIELD_GENEVE_OPT_CLASS,/**< GENEVE option class. */
+	RTE_FLOW_FIELD_GENEVE_OPT_DATA,	/**< GENEVE option data. */
+	RTE_FLOW_FIELD_MPLS,		/**< MPLS header. */
+	RTE_FLOW_FIELD_TCP_DATA_OFFSET,	/**< TCP data offset. */
+	RTE_FLOW_FIELD_IPV4_IHL,	/**< IPv4 IHL. */
+	RTE_FLOW_FIELD_IPV4_TOTAL_LEN,	/**< IPv4 total length. */
+	RTE_FLOW_FIELD_IPV6_PAYLOAD_LEN	/**< IPv6 payload length. */
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
+ * Field description for packet field.
+ */
+struct rte_flow_field_data {
+	enum rte_flow_field_id field; /**< Field or memory type ID. */
+	union {
+		struct {
+			/** Encapsulation level and tag index or flex item handle. */
+			union {
+				struct {
+					/**
+					 * Packet encapsulation level containing
+					 * the field to modify.
+					 *
+					 * - @p 0 requests the default behavior.
+					 *   Depending on the packet type, it
+					 *   can mean outermost, innermost or
+					 *   anything in between.
+					 *
+					 *   It basically stands for the
+					 *   innermost encapsulation level.
+					 *   Modification can be performed
+					 *   according to PMD and device
+					 *   capabilities.
+					 *
+					 * - @p 1 requests modification to be
+					 *   performed on the outermost packet
+					 *   encapsulation level.
+					 *
+					 * - @p 2 and subsequent values request
+					 *   modification to be performed on
+					 *   the specified inner packet
+					 *   encapsulation level, from
+					 *   outermost to innermost (lower to
+					 *   higher values).
+					 *
+					 * Values other than @p 0 are not
+					 * necessarily supported.
+					 *
+					 * @note that for MPLS field,
+					 * encapsulation level also include
+					 * tunnel since MPLS may appear in
+					 * outer, inner or tunnel.
+					 */
+					uint8_t level;
+					union {
+						/**
+						 * Tag index array inside
+						 * encapsulation level.
+						 * Used for VLAN, MPLS or TAG types.
+						 */
+						uint8_t tag_index;
+						/**
+						 * Geneve option identifier.
+						 * Relevant only for
+						 * RTE_FLOW_FIELD_GENEVE_OPT_XXXX
+						 * modification type.
+						 */
+						struct {
+							/**
+							 * Geneve option type.
+							 */
+							uint8_t type;
+							/**
+							 * Geneve option class.
+							 */
+							rte_be16_t class_id;
+						};
+					};
+				};
+				struct rte_flow_item_flex_handle *flex_handle;
+			};
+			/** Number of bits to skip from a field. */
+			uint32_t offset;
+		};
+		/**
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
+		 * The immediate source bitfield offset is inherited from
+		 * the destination's one.
+		 */
+		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.
+		 */
+		void *pvalue;
+	};
+};
+
+/**
+ * Expected operation types for compare item.
+ */
+enum rte_flow_item_compare_op {
+	RTE_FLOW_ITEM_COMPARE_EQ,	/* Compare result equal. */
+	RTE_FLOW_ITEM_COMPARE_NE,	/* Compare result not equal. */
+	RTE_FLOW_ITEM_COMPARE_LT,	/* Compare result less than. */
+	RTE_FLOW_ITEM_COMPARE_LE,	/* Compare result less than or equal. */
+	RTE_FLOW_ITEM_COMPARE_GT,	/* Compare result great than. */
+	RTE_FLOW_ITEM_COMPARE_GE,	/* Compare result great than or equal. */
+};
+
+/**
+ *
+ * RTE_FLOW_ITEM_TYPE_COMPARE
+ *
+ * Matches the packet with compare result.
+ *
+ * The operation means a compare with b result.
+ */
+struct rte_flow_item_compare {
+	enum rte_flow_item_compare_op operation; /* The compare operation. */
+	struct rte_flow_field_data a;		 /* Field be compared.  */
+	struct rte_flow_field_data b;		 /* Field as comparator. */
+	uint32_t width;				 /* Compare width. */
+};
+
 /**
  * Action types.
  *
@@ -3856,150 +4034,6 @@  struct rte_flow_action_ethdev {
 	uint16_t port_id; /**< ethdev port ID */
 };
 
-/**
- * Field IDs for MODIFY_FIELD action.
- */
-enum rte_flow_field_id {
-	RTE_FLOW_FIELD_START = 0,	/**< Start of a packet. */
-	RTE_FLOW_FIELD_MAC_DST,		/**< Destination MAC Address. */
-	RTE_FLOW_FIELD_MAC_SRC,		/**< Source MAC Address. */
-	RTE_FLOW_FIELD_VLAN_TYPE,	/**< VLAN Tag Identifier. */
-	RTE_FLOW_FIELD_VLAN_ID,		/**< VLAN Identifier. */
-	RTE_FLOW_FIELD_MAC_TYPE,	/**< EtherType. */
-	RTE_FLOW_FIELD_IPV4_DSCP,	/**< IPv4 DSCP. */
-	RTE_FLOW_FIELD_IPV4_TTL,	/**< IPv4 Time To Live. */
-	RTE_FLOW_FIELD_IPV4_SRC,	/**< IPv4 Source Address. */
-	RTE_FLOW_FIELD_IPV4_DST,	/**< IPv4 Destination Address. */
-	RTE_FLOW_FIELD_IPV6_DSCP,	/**< IPv6 DSCP. */
-	RTE_FLOW_FIELD_IPV6_HOPLIMIT,	/**< IPv6 Hop Limit. */
-	RTE_FLOW_FIELD_IPV6_SRC,	/**< IPv6 Source Address. */
-	RTE_FLOW_FIELD_IPV6_DST,	/**< IPv6 Destination Address. */
-	RTE_FLOW_FIELD_TCP_PORT_SRC,	/**< TCP Source Port Number. */
-	RTE_FLOW_FIELD_TCP_PORT_DST,	/**< TCP Destination Port Number. */
-	RTE_FLOW_FIELD_TCP_SEQ_NUM,	/**< TCP Sequence Number. */
-	RTE_FLOW_FIELD_TCP_ACK_NUM,	/**< TCP Acknowledgment Number. */
-	RTE_FLOW_FIELD_TCP_FLAGS,	/**< TCP Flags. */
-	RTE_FLOW_FIELD_UDP_PORT_SRC,	/**< UDP Source Port Number. */
-	RTE_FLOW_FIELD_UDP_PORT_DST,	/**< UDP Destination Port Number. */
-	RTE_FLOW_FIELD_VXLAN_VNI,	/**< VXLAN Network Identifier. */
-	RTE_FLOW_FIELD_GENEVE_VNI,	/**< GENEVE Network Identifier. */
-	RTE_FLOW_FIELD_GTP_TEID,	/**< GTP Tunnel Endpoint Identifier. */
-	RTE_FLOW_FIELD_TAG,		/**< Tag value. */
-	RTE_FLOW_FIELD_MARK,		/**< Mark value. */
-	RTE_FLOW_FIELD_META,		/**< Metadata value. */
-	RTE_FLOW_FIELD_POINTER,		/**< Memory pointer. */
-	RTE_FLOW_FIELD_VALUE,		/**< Immediate value. */
-	RTE_FLOW_FIELD_IPV4_ECN,	/**< IPv4 ECN. */
-	RTE_FLOW_FIELD_IPV6_ECN,	/**< IPv6 ECN. */
-	RTE_FLOW_FIELD_GTP_PSC_QFI,	/**< GTP QFI. */
-	RTE_FLOW_FIELD_METER_COLOR,	/**< Meter color marker. */
-	RTE_FLOW_FIELD_IPV6_PROTO,	/**< IPv6 next header. */
-	RTE_FLOW_FIELD_FLEX_ITEM,	/**< Flex item. */
-	RTE_FLOW_FIELD_HASH_RESULT,	/**< Hash result. */
-	RTE_FLOW_FIELD_GENEVE_OPT_TYPE,	/**< GENEVE option type. */
-	RTE_FLOW_FIELD_GENEVE_OPT_CLASS,/**< GENEVE option class. */
-	RTE_FLOW_FIELD_GENEVE_OPT_DATA,	/**< GENEVE option data. */
-	RTE_FLOW_FIELD_MPLS,		/**< MPLS header. */
-	RTE_FLOW_FIELD_TCP_DATA_OFFSET,	/**< TCP data offset. */
-	RTE_FLOW_FIELD_IPV4_IHL,	/**< IPv4 IHL. */
-	RTE_FLOW_FIELD_IPV4_TOTAL_LEN,	/**< IPv4 total length. */
-	RTE_FLOW_FIELD_IPV6_PAYLOAD_LEN	/**< IPv6 payload length. */
-};
-
-/**
- * @warning
- * @b EXPERIMENTAL: this structure may change without prior notice
- *
- * Field description for packet field.
- */
-struct rte_flow_field_data {
-	enum rte_flow_field_id field; /**< Field or memory type ID. */
-	union {
-		struct {
-			/** Encapsulation level and tag index or flex item handle. */
-			union {
-				struct {
-					/**
-					 * Packet encapsulation level containing
-					 * the field to modify.
-					 *
-					 * - @p 0 requests the default behavior.
-					 *   Depending on the packet type, it
-					 *   can mean outermost, innermost or
-					 *   anything in between.
-					 *
-					 *   It basically stands for the
-					 *   innermost encapsulation level.
-					 *   Modification can be performed
-					 *   according to PMD and device
-					 *   capabilities.
-					 *
-					 * - @p 1 requests modification to be
-					 *   performed on the outermost packet
-					 *   encapsulation level.
-					 *
-					 * - @p 2 and subsequent values request
-					 *   modification to be performed on
-					 *   the specified inner packet
-					 *   encapsulation level, from
-					 *   outermost to innermost (lower to
-					 *   higher values).
-					 *
-					 * Values other than @p 0 are not
-					 * necessarily supported.
-					 *
-					 * @note that for MPLS field,
-					 * encapsulation level also include
-					 * tunnel since MPLS may appear in
-					 * outer, inner or tunnel.
-					 */
-					uint8_t level;
-					union {
-						/**
-						 * Tag index array inside
-						 * encapsulation level.
-						 * Used for VLAN, MPLS or TAG types.
-						 */
-						uint8_t tag_index;
-						/**
-						 * Geneve option identifier.
-						 * Relevant only for
-						 * RTE_FLOW_FIELD_GENEVE_OPT_XXXX
-						 * modification type.
-						 */
-						struct {
-							/**
-							 * Geneve option type.
-							 */
-							uint8_t type;
-							/**
-							 * Geneve option class.
-							 */
-							rte_be16_t class_id;
-						};
-					};
-				};
-				struct rte_flow_item_flex_handle *flex_handle;
-			};
-			/** Number of bits to skip from a field. */
-			uint32_t offset;
-		};
-		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
-		 * same byte order and length as in relevant rte_flow_item_xxx.
-		 * The immediate source bitfield offset is inherited from
-		 * the destination's one.
-		 */
-		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.
-		 */
-		void *pvalue;
-	};
-};
-
 /**
  * Operation types for MODIFY_FIELD action.
  */