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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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

-- 
Thomas