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

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

> -----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