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

["Zhang, Helin" <helin.zhang@intel.com>]

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