diff mbox

[dpdk-dev,v5,2/5] ethdev: add enum type and relevant structures for hash filter control

Message ID 1413861289-26662-3-git-send-email-helin.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Zhang, Helin Oct. 21, 2014, 3:14 a.m. UTC 
  enum type and relevant structures are added in rte_eth_ctrl.h to
support hash filter control.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 lib/librte_ether/rte_eth_ctrl.h | 75 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

v5 changes:
* Integrated with filter API defined recently.

Comments

Thomas Monjalon Nov. 3, 2014, 7:57 a.m. UTC | #1 
2014-10-21 11:14, Helin Zhang:
> +enum rte_eth_hash_filter_info_type {
> +	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
> +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,

PCTYPE is an unknown word in the API layer.
Could you replace it by something more generic?

> +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
> +	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
> +	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
> +	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
> +};

You should comment each constant.

> +struct rte_eth_sym_hash_ena_info {
> +	/**< packet classification type, defined in rte_ethdev.h */
> +	uint8_t pctype;

No, PCTYPE is not anymore defined in ethdev.

> +/**
> + * A structure used to set or get filter swap information, to support
> + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> + */
> +struct rte_eth_filter_swap_info {
> +	/**< Packet classification type, defined in rte_ethdev.h */
> +	uint8_t pctype;
> +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> +	uint8_t off0_src0;
> +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> +	uint8_t off0_src1;
> +	/**< Field length of the first couple. */
> +	uint8_t len0;
> +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> +	uint8_t off1_src0;
> +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> +	uint8_t off1_src1;
> +	/**< Field length of the second couple. */
> +	uint8_t len1;
> +};

I guess it would be easier to understand if
RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
Zhang, Helin Nov. 6, 2014, 3:41 a.m. UTC | #2 
Thanks for your good comments!

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 3, 2014 3:57 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-10-21 11:14, Helin Zhang:
> > +enum rte_eth_hash_filter_info_type {
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,
> 
> PCTYPE is an unknown word in the API layer.
> Could you replace it by something more generic?
So you suggested to remove words of PCTYPE, pctype, or packet classification type?
I am not trying to rename ETH_RSS_IPV4_SHIFT, ..., ETH_RSS_NONF_IPV4_UDP_SHIFT, ..., etc.
They are actually pctype in i40e, and not only for RSS. I would like to rename them into
generic names. Any good naming ideas from you or other guys? My idea is to rename
them like RTE_ETH_FLOW_TYPE_XX.

> 
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
> > +};
> 
> You should comment each constant.
OK. Good to know that!

> 
> > +struct rte_eth_sym_hash_ena_info {
> > +	/**< packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> 
> No, PCTYPE is not anymore defined in ethdev.
We need a generic name for that, how about flow_type? Good comments are welcome!

> 
> > +/**
> > + * A structure used to set or get filter swap information, to support
> > + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > + */
> > +struct rte_eth_filter_swap_info {
> > +	/**< Packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > +	uint8_t off0_src0;
> > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > +	uint8_t off0_src1;
> > +	/**< Field length of the first couple. */
> > +	uint8_t len0;
> > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src0;
> > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src1;
> > +	/**< Field length of the second couple. */
> > +	uint8_t len1;
> > +};
> 
> I guess it would be easier to understand if
> RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
It has already been defined before this structure definition.
I don't think I have understood your idea. Could you help to explain more? Thanks!

> 
> --
> Thomas

Regards,
Helin
Thomas Monjalon Nov. 6, 2014, 8:43 a.m. UTC | #3 
2014-11-06 03:41, Zhang, Helin:
> > > +/**
> > > + * A structure used to set or get filter swap information, to support
> > > + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > > + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > > + */
> > > +struct rte_eth_filter_swap_info {
> > > +	/**< Packet classification type, defined in rte_ethdev.h */
> > > +	uint8_t pctype;
> > > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > > +	uint8_t off0_src0;
> > > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > > +	uint8_t off0_src1;
> > > +	/**< Field length of the first couple. */
> > > +	uint8_t len0;
> > > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > > +	uint8_t off1_src0;
> > > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > > +	uint8_t off1_src1;
> > > +	/**< Field length of the second couple. */
> > > +	uint8_t len1;
> > > +};
> > 
> > I guess it would be easier to understand if
> > RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
> 
> It has already been defined before this structure definition.
> I don't think I have understood your idea. Could you help to explain more? Thanks!

By "defined", I mean "explained". What is the action of swap filter?
You offer new features in API without explaining them. It's probably obvious
for you but not for me.
Zhang, Helin Nov. 6, 2014, 8:54 a.m. UTC | #4 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, November 6, 2014 4:43 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-11-06 03:41, Zhang, Helin:
> > > > +/**
> > > > + * A structure used to set or get filter swap information, to
> > > > +support
> > > > + * 'RTE_ETH_FILTER_HASH',
> > > > +'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > > > + * with information type
> 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > > > + */
> > > > +struct rte_eth_filter_swap_info {
> > > > +	/**< Packet classification type, defined in rte_ethdev.h */
> > > > +	uint8_t pctype;
> > > > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > > > +	uint8_t off0_src0;
> > > > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > > > +	uint8_t off0_src1;
> > > > +	/**< Field length of the first couple. */
> > > > +	uint8_t len0;
> > > > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > > > +	uint8_t off1_src0;
> > > > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > > > +	uint8_t off1_src1;
> > > > +	/**< Field length of the second couple. */
> > > > +	uint8_t len1;
> > > > +};
> > >
> > > I guess it would be easier to understand if
> > > RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
> >
> > It has already been defined before this structure definition.
> > I don't think I have understood your idea. Could you help to explain more?
> Thanks!
> 
> By "defined", I mean "explained". What is the action of swap filter?
> You offer new features in API without explaining them. It's probably obvious for
> you but not for me.
Yes, they should be explained.

> 
> --
> Thomas

Regards,
Helin
Zhang, Helin Nov. 11, 2014, 3:27 a.m. UTC | #5 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 3, 2014 3:57 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-10-21 11:14, Helin Zhang:
> > +enum rte_eth_hash_filter_info_type {
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,
> 
> PCTYPE is an unknown word in the API layer.
> Could you replace it by something more generic?
In ethdev layer, the idea of 'flow type' will be used for generic purpose.

> 
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
> > +};
> 
> You should comment each constant.
Yes, agree.

> 
> > +struct rte_eth_sym_hash_ena_info {
> > +	/**< packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> 
> No, PCTYPE is not anymore defined in ethdev.
No pctype will be there on ethdev layer.

> 
> > +/**
> > + * A structure used to set or get filter swap information, to support
> > + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > + */
> > +struct rte_eth_filter_swap_info {
> > +	/**< Packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > +	uint8_t off0_src0;
> > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > +	uint8_t off0_src1;
> > +	/**< Field length of the first couple. */
> > +	uint8_t len0;
> > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src0;
> > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src1;
> > +	/**< Field length of the second couple. */
> > +	uint8_t len1;
> > +};
> 
> I guess it would be easier to understand if
> RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
> 
> --
> Thomas

Regards,
Helin
Zhang, Helin Nov. 11, 2014, 6:46 a.m. UTC | #6 
Hi Thomas

In order to get things more generic, and remove any mappings on specific NIC hardwares, I planned to change the macros in rte_ethdev.h from

/* Supported RSS offloads */
/* for 1G & 10G */
#define ETH_RSS_IPV4_SHIFT                    0
#define ETH_RSS_IPV4_TCP_SHIFT                1
#define ETH_RSS_IPV6_SHIFT                    2
#define ETH_RSS_IPV6_EX_SHIFT                 3
#define ETH_RSS_IPV6_TCP_SHIFT                4
#define ETH_RSS_IPV6_TCP_EX_SHIFT             5
#define ETH_RSS_IPV4_UDP_SHIFT                6
#define ETH_RSS_IPV6_UDP_SHIFT                7
#define ETH_RSS_IPV6_UDP_EX_SHIFT             8
/* for 40G only */
#define ETH_RSS_NONF_IPV4_UDP_SHIFT           31
#define ETH_RSS_NONF_IPV4_TCP_SHIFT           33
#define ETH_RSS_NONF_IPV4_SCTP_SHIFT          34
#define ETH_RSS_NONF_IPV4_OTHER_SHIFT         35
#define ETH_RSS_FRAG_IPV4_SHIFT               36
#define ETH_RSS_NONF_IPV6_UDP_SHIFT           41
#define ETH_RSS_NONF_IPV6_TCP_SHIFT           43
#define ETH_RSS_NONF_IPV6_SCTP_SHIFT          44
#define ETH_RSS_NONF_IPV6_OTHER_SHIFT         45
#define ETH_RSS_FRAG_IPV6_SHIFT               46
#define ETH_RSS_FCOE_OX_SHIFT                 48
#define ETH_RSS_FCOE_RX_SHIFT                 49
#define ETH_RSS_FCOE_OTHER_SHIFT              50
#define ETH_RSS_L2_PAYLOAD_SHIFT              63

to

/* Supported RSS offloads */
/* for 1G & 10G */
#define ETH_FLOW_TYPE_IPV4                    0
#define ETH_FLOW_TYPE_IPV4_TCP                1
#define ETH_FLOW_TYPE_IPV6                    2
#define ETH_FLOW_TYPE_IPV6_EX                 3
#define ETH_FLOW_TYPE_IPV6_TCP                4
#define ETH_FLOW_TYPE_IPV6_TCP_EX             5
#define ETH_FLOW_TYPE_IPV4_UDP                6
#define ETH_FLOW_TYPE_IPV6_UDP                7
#define ETH_FLOW_TYPE_IPV6_UDP_EX             8
/* for 40G only */
#define ETH_FLOW_TYPE_NONFRAG_IPV4_UDP           9
#define ETH_FLOW_TYPE_NONFRAG_IPV4_TCP           10
#define ETH_FLOW_TYPE_NONFRAG_IPV4_SCTP          11
#define ETH_FLOW_TYPE_NONFRAG_IPV4_OTHER         12
#define ETH_FLOW_TYPE_FRAG_IPV4               13
#define ETH_FLOW_TYPE_NONFRAG_IPV6_UDP           14
#define ETH_FLOW_TYPE_NONFRAG_IPV6_TCP           15
#define ETH_FLOW_TYPE_NONFRAG_IPV6_SCTP          16
#define ETH_FLOW_TYPE_NONFRAG_IPV6_OTHER         17
#define ETH_FLOW_TYPE_FRAG_IPV6              18
#define ETH_FLOW_TYPE_L2_PAYLOAD              19

Any comments or better ideas on that? Thanks!

Regards,
Helin
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 3, 2014 3:57 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-10-21 11:14, Helin Zhang:
> > +enum rte_eth_hash_filter_info_type {
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,
> 
> PCTYPE is an unknown word in the API layer.
> Could you replace it by something more generic?
> 
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
> > +};
> 
> You should comment each constant.
> 
> > +struct rte_eth_sym_hash_ena_info {
> > +	/**< packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> 
> No, PCTYPE is not anymore defined in ethdev.
> 
> > +/**
> > + * A structure used to set or get filter swap information, to support
> > + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > + */
> > +struct rte_eth_filter_swap_info {
> > +	/**< Packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > +	uint8_t off0_src0;
> > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > +	uint8_t off0_src1;
> > +	/**< Field length of the first couple. */
> > +	uint8_t len0;
> > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src0;
> > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src1;
> > +	/**< Field length of the second couple. */
> > +	uint8_t len1;
> > +};
> 
> I guess it would be easier to understand if
> RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
> 
> --
> Thomas
Thomas Monjalon Nov. 11, 2014, 9:08 p.m. UTC | #7 
2014-11-11 06:46, Zhang, Helin:
> In order to get things more generic, and remove any mappings on specific NIC hardwares, I planned to change the macros in rte_ethdev.h from
> 
> /* Supported RSS offloads */
> /* for 1G & 10G */
> #define ETH_RSS_IPV4_SHIFT                    0
> #define ETH_RSS_IPV4_TCP_SHIFT                1
> #define ETH_RSS_IPV6_SHIFT                    2
> #define ETH_RSS_IPV6_EX_SHIFT                 3
> #define ETH_RSS_IPV6_TCP_SHIFT                4
> #define ETH_RSS_IPV6_TCP_EX_SHIFT             5
> #define ETH_RSS_IPV4_UDP_SHIFT                6
> #define ETH_RSS_IPV6_UDP_SHIFT                7
> #define ETH_RSS_IPV6_UDP_EX_SHIFT             8
> /* for 40G only */
> #define ETH_RSS_NONF_IPV4_UDP_SHIFT           31
> #define ETH_RSS_NONF_IPV4_TCP_SHIFT           33
> #define ETH_RSS_NONF_IPV4_SCTP_SHIFT          34
> #define ETH_RSS_NONF_IPV4_OTHER_SHIFT         35
> #define ETH_RSS_FRAG_IPV4_SHIFT               36
> #define ETH_RSS_NONF_IPV6_UDP_SHIFT           41
> #define ETH_RSS_NONF_IPV6_TCP_SHIFT           43
> #define ETH_RSS_NONF_IPV6_SCTP_SHIFT          44
> #define ETH_RSS_NONF_IPV6_OTHER_SHIFT         45
> #define ETH_RSS_FRAG_IPV6_SHIFT               46
> #define ETH_RSS_FCOE_OX_SHIFT                 48
> #define ETH_RSS_FCOE_RX_SHIFT                 49
> #define ETH_RSS_FCOE_OTHER_SHIFT              50
> #define ETH_RSS_L2_PAYLOAD_SHIFT              63
> 
> to
> 
> /* Supported RSS offloads */
> /* for 1G & 10G */
> #define ETH_FLOW_TYPE_IPV4                    0
> #define ETH_FLOW_TYPE_IPV4_TCP                1
> #define ETH_FLOW_TYPE_IPV6                    2
> #define ETH_FLOW_TYPE_IPV6_EX                 3
> #define ETH_FLOW_TYPE_IPV6_TCP                4
> #define ETH_FLOW_TYPE_IPV6_TCP_EX             5
> #define ETH_FLOW_TYPE_IPV4_UDP                6
> #define ETH_FLOW_TYPE_IPV6_UDP                7
> #define ETH_FLOW_TYPE_IPV6_UDP_EX             8
> /* for 40G only */
> #define ETH_FLOW_TYPE_NONFRAG_IPV4_UDP           9
> #define ETH_FLOW_TYPE_NONFRAG_IPV4_TCP           10
> #define ETH_FLOW_TYPE_NONFRAG_IPV4_SCTP          11
> #define ETH_FLOW_TYPE_NONFRAG_IPV4_OTHER         12
> #define ETH_FLOW_TYPE_FRAG_IPV4               13
> #define ETH_FLOW_TYPE_NONFRAG_IPV6_UDP           14
> #define ETH_FLOW_TYPE_NONFRAG_IPV6_TCP           15
> #define ETH_FLOW_TYPE_NONFRAG_IPV6_SCTP          16
> #define ETH_FLOW_TYPE_NONFRAG_IPV6_OTHER         17
> #define ETH_FLOW_TYPE_FRAG_IPV6              18
> #define ETH_FLOW_TYPE_L2_PAYLOAD              19
> 
> Any comments or better ideas on that? Thanks!

About the renaming RSS -> FLOW_TYPE, I have no objection.
It seems a bit better.
Some comments are needed to explain what means the value.
I think the comments "1G & 10G" or "40G only" are possibly wrong.
Actually you use ETH_FLOW_TYPE_IPV4 for ixgbe and ETH_FLOW_TYPE_FRAG_IPV4
or ETH_FLOW_TYPE_NONFRAG_IPV4_* for i40e. It's not consistent and clearly
shows that you stick to the hardware definitions.

Something really generic could be a set of flags like this:
	IPV4
	IPV6
	NONFRAG
	UDP
	TCP
	SCTP
Zhang, Helin Nov. 12, 2014, 5:52 a.m. UTC | #8 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, November 12, 2014 5:09 AM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-11-11 06:46, Zhang, Helin:
> > In order to get things more generic, and remove any mappings on
> > specific NIC hardwares, I planned to change the macros in rte_ethdev.h
> > from
> >
> > /* Supported RSS offloads */
> > /* for 1G & 10G */
> > #define ETH_RSS_IPV4_SHIFT                    0
> > #define ETH_RSS_IPV4_TCP_SHIFT                1
> > #define ETH_RSS_IPV6_SHIFT                    2
> > #define ETH_RSS_IPV6_EX_SHIFT                 3
> > #define ETH_RSS_IPV6_TCP_SHIFT                4
> > #define ETH_RSS_IPV6_TCP_EX_SHIFT             5
> > #define ETH_RSS_IPV4_UDP_SHIFT                6
> > #define ETH_RSS_IPV6_UDP_SHIFT                7
> > #define ETH_RSS_IPV6_UDP_EX_SHIFT             8
> > /* for 40G only */
> > #define ETH_RSS_NONF_IPV4_UDP_SHIFT           31
> > #define ETH_RSS_NONF_IPV4_TCP_SHIFT           33
> > #define ETH_RSS_NONF_IPV4_SCTP_SHIFT          34
> > #define ETH_RSS_NONF_IPV4_OTHER_SHIFT         35
> > #define ETH_RSS_FRAG_IPV4_SHIFT               36
> > #define ETH_RSS_NONF_IPV6_UDP_SHIFT           41
> > #define ETH_RSS_NONF_IPV6_TCP_SHIFT           43
> > #define ETH_RSS_NONF_IPV6_SCTP_SHIFT          44
> > #define ETH_RSS_NONF_IPV6_OTHER_SHIFT         45
> > #define ETH_RSS_FRAG_IPV6_SHIFT               46
> > #define ETH_RSS_FCOE_OX_SHIFT                 48
> > #define ETH_RSS_FCOE_RX_SHIFT                 49
> > #define ETH_RSS_FCOE_OTHER_SHIFT              50
> > #define ETH_RSS_L2_PAYLOAD_SHIFT              63
> >
> > to
> >
> > /* Supported RSS offloads */
> > /* for 1G & 10G */
> > #define ETH_FLOW_TYPE_IPV4                    0
> > #define ETH_FLOW_TYPE_IPV4_TCP                1
> > #define ETH_FLOW_TYPE_IPV6                    2
> > #define ETH_FLOW_TYPE_IPV6_EX                 3
> > #define ETH_FLOW_TYPE_IPV6_TCP                4
> > #define ETH_FLOW_TYPE_IPV6_TCP_EX             5
> > #define ETH_FLOW_TYPE_IPV4_UDP                6
> > #define ETH_FLOW_TYPE_IPV6_UDP                7
> > #define ETH_FLOW_TYPE_IPV6_UDP_EX             8
> > /* for 40G only */
> > #define ETH_FLOW_TYPE_NONFRAG_IPV4_UDP           9
> > #define ETH_FLOW_TYPE_NONFRAG_IPV4_TCP           10
> > #define ETH_FLOW_TYPE_NONFRAG_IPV4_SCTP          11
> > #define ETH_FLOW_TYPE_NONFRAG_IPV4_OTHER         12
> > #define ETH_FLOW_TYPE_FRAG_IPV4               13
> > #define ETH_FLOW_TYPE_NONFRAG_IPV6_UDP           14
> > #define ETH_FLOW_TYPE_NONFRAG_IPV6_TCP           15
> > #define ETH_FLOW_TYPE_NONFRAG_IPV6_SCTP          16
> > #define ETH_FLOW_TYPE_NONFRAG_IPV6_OTHER         17
> > #define ETH_FLOW_TYPE_FRAG_IPV6              18
> > #define ETH_FLOW_TYPE_L2_PAYLOAD              19
> >
> > Any comments or better ideas on that? Thanks!
> 
> About the renaming RSS -> FLOW_TYPE, I have no objection.
> It seems a bit better.
Nice to hear that!

> Some comments are needed to explain what means the value.
> I think the comments "1G & 10G" or "40G only" are possibly wrong.
Yes, I agree with you.

> Actually you use ETH_FLOW_TYPE_IPV4 for ixgbe and
> ETH_FLOW_TYPE_FRAG_IPV4 or ETH_FLOW_TYPE_NONFRAG_IPV4_* for i40e.
> It's not consistent and clearly shows that you stick to the hardware definitions.
> 
> Something really generic could be a set of flags like this:
> 	IPV4
> 	IPV6
> 	NONFRAG
> 	UDP
> 	TCP
> 	SCTP
Good conclusion! We could think of it in a new patch set. I don't want to put everything into this patch. :)

> 
> --
> Thomas

Regards,
Helin
Thomas Monjalon Nov. 12, 2014, 9:30 a.m. UTC | #9 
2014-11-12 05:52, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Actually you use ETH_FLOW_TYPE_IPV4 for ixgbe and
> > ETH_FLOW_TYPE_FRAG_IPV4 or ETH_FLOW_TYPE_NONFRAG_IPV4_* for i40e.
> > It's not consistent and clearly shows that you stick to the hardware definitions.
> > 
> > Something really generic could be a set of flags like this:
> > 	IPV4
> > 	IPV6
> > 	NONFRAG
> > 	UDP
> > 	TCP
> > 	SCTP
> 
> Good conclusion! We could think of it in a new patch set.
> I don't want to put everything into this patch. :)

If you agree flags must be used, the old defines must be removed.
So no need to rename the defines.
I think you should directly change to flags (in this patchset or another).

Thanks
Zhang, Helin Nov. 12, 2014, 2:22 p.m. UTC | #10 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, November 12, 2014 5:31 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-11-12 05:52, Zhang, Helin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Actually you use ETH_FLOW_TYPE_IPV4 for ixgbe and
> > > ETH_FLOW_TYPE_FRAG_IPV4 or ETH_FLOW_TYPE_NONFRAG_IPV4_* for
> i40e.
> > > It's not consistent and clearly shows that you stick to the hardware
> definitions.
> > >
> > > Something really generic could be a set of flags like this:
> > > 	IPV4
> > > 	IPV6
> > > 	NONFRAG
> > > 	UDP
> > > 	TCP
> > > 	SCTP
> >
> > Good conclusion! We could think of it in a new patch set.
> > I don't want to put everything into this patch. :)
> 
> If you agree flags must be used, the old defines must be removed.
> So no need to rename the defines.
> I think you should directly change to flags (in this patchset or another).
I will keep it as it is in this patch for configuring hash functions. But I really
agree with your idea of using flags instead.
I will rework it in another patch set and send out a RFC to see if there is any
objections. For this patch set, I will focus on feature itself.

> 
> Thanks
> --
> Thomas

Regards,
Helin
diff mbox

Patch

diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
index df21ac6..c469b57 100644
--- a/lib/librte_ether/rte_eth_ctrl.h
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -51,6 +51,7 @@  extern "C" {
  */
 enum rte_filter_type {
 	RTE_ETH_FILTER_NONE = 0,
+	RTE_ETH_FILTER_HASH,
 	RTE_ETH_FILTER_MAX
 };
 
@@ -71,6 +72,80 @@  enum rte_filter_op {
 	RTE_ETH_FILTER_OP_MAX
 };
 
+/**
+ * Hash filter information types.
+ */
+enum rte_eth_hash_filter_info_type {
+	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
+	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,
+	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
+	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
+	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
+	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
+};
+
+/**
+ * Hash function types.
+ */
+enum rte_eth_hash_function {
+	RTE_ETH_HASH_FUNCTION_UNKNOWN = 0,
+	RTE_ETH_HASH_FUNCTION_TOEPLITZ,
+	RTE_ETH_HASH_FUNCTION_SIMPLE_XOR,
+	RTE_ETH_HASH_FUNCTION_MAX,
+};
+
+/**
+ * A structure used to set or get symmetric hash enable information, to support
+ * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET', with
+ * information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE'.
+ */
+struct rte_eth_sym_hash_ena_info {
+	/**< packet classification type, defined in rte_ethdev.h */
+	uint8_t pctype;
+	uint8_t enable; /**< enable or disable flag */
+};
+
+/**
+ * A structure used to set or get filter swap information, to support
+ * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
+ * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
+ */
+struct rte_eth_filter_swap_info {
+	/**< Packet classification type, defined in rte_ethdev.h */
+	uint8_t pctype;
+	/**< Offset of the 1st field of the 1st couple to be swapped. */
+	uint8_t off0_src0;
+	/**< Offset of the 2nd field of the 1st couple to be swapped. */
+	uint8_t off0_src1;
+	/**< Field length of the first couple. */
+	uint8_t len0;
+	/**< Offset of the 1st field of the 2nd couple to be swapped. */
+	uint8_t off1_src0;
+	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
+	uint8_t off1_src1;
+	/**< Field length of the second couple. */
+	uint8_t len1;
+};
+
+/**
+ * A structure used to set or get hash filter information, to support filter
+ * type of 'RTE_ETH_FILTER_HASH' and its operations.
+ */
+struct rte_eth_hash_filter_info {
+	enum rte_eth_hash_filter_info_type info_type; /**< Information type. */
+	/**< Details of hash filter infomation */
+	union {
+		/* For RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE */
+		struct rte_eth_sym_hash_ena_info sym_hash_ena;
+		/* For RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP */
+		struct rte_eth_filter_swap_info filter_swap;
+		/* For RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT */
+		uint8_t enable;
+		/* For RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION */
+		enum rte_eth_hash_function hash_function;
+	} info;
+};
+
 #ifdef __cplusplus
 }
 #endif