Re: [dpdk-dev] [PATCH 5/7] ethdev: unification of flow types

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

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, February 2, 2015 11:39 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 5/7] ethdev: unification of flow types
> 
> Hi Helin,
> 
> 2015-01-19 14:56, Helin Zhang:
> > Flow types was defined actually for i40e hardware specifically, and
> > wasn't able to be used for defining RSS offload types of all PMDs. It
> > removed the enum flow types, and uses macros instead with new names.
> > The new macros can be used for defining RSS offload types later. Also
> > modifications are made in i40e and testpmd accordingly.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> [...]
> > --- a/lib/librte_ether/rte_eth_ctrl.h
> > +++ b/lib/librte_ether/rte_eth_ctrl.h
> > @@ -46,6 +46,35 @@
> >  extern "C" {
> >  #endif
> >
> > +/*
> > + * A packet can be identified by hardware as different flow types.
> > +Different
> > + * NIC hardwares may support different flow types.
> > + * Basically, the NIC hardware identifies the flow type as deep
> > +protocol as
> > + * possible, and exclusively. For example, if a packet is identified
> > +as
> > + * 'ETH_FLOW_TYPE_NONFRAG_IPV4_TCP', it will not be any of other flow
> > +types,
> > + * though it is an actual IPV4 packet.
> > + * Note that the flow types are used to define RSS offload types in
> > + * rte_ethdev.h.
> > + */
> > +#define ETH_FLOW_TYPE_UNKNOWN            0
> > +#define ETH_FLOW_TYPE_IPV4               1
> > +#define ETH_FLOW_TYPE_FRAG_IPV4          2
> > +#define ETH_FLOW_TYPE_NONFRAG_IPV4_TCP   3
> > +#define ETH_FLOW_TYPE_NONFRAG_IPV4_UDP   4
> > +#define ETH_FLOW_TYPE_NONFRAG_IPV4_SCTP  5 #define
> > +ETH_FLOW_TYPE_NONFRAG_IPV4_OTHER 6
> > +#define ETH_FLOW_TYPE_IPV6               7
> > +#define ETH_FLOW_TYPE_FRAG_IPV6          8
> > +#define ETH_FLOW_TYPE_NONFRAG_IPV6_TCP   9
> > +#define ETH_FLOW_TYPE_NONFRAG_IPV6_UDP   10
> > +#define ETH_FLOW_TYPE_NONFRAG_IPV6_SCTP  11 #define
> > +ETH_FLOW_TYPE_NONFRAG_IPV6_OTHER 12
> > +#define ETH_FLOW_TYPE_L2_PAYLOAD         13
> > +#define ETH_FLOW_TYPE_IPV6_EX            14
> > +#define ETH_FLOW_TYPE_IPV6_TCP_EX        15
> > +#define ETH_FLOW_TYPE_IPV6_UDP_EX        16
> > +#define ETH_FLOW_TYPE_MAX                17
> 
> Why not using an enum?
Enum is 'int' which needs 32 bits, while flow_type is just 16 bits.
The old one which is not enum was in rte_ethdev.h, the enum one was added
recently in rte_eth_ctrl.h.

> Nitpicking: numbers from 0 to 9 should be right aligned.
In my source file, they are right aligned.

> 
> >  /**
> >   * Feature filter types
> >   */
> > @@ -179,24 +208,6 @@ struct rte_eth_tunnel_filter_conf {
> >  #define RTE_ETH_FDIR_MAX_FLEXLEN         16 /** < Max length of
> flexbytes. */
> >
> >  /**
> > - * Flow type
> > - */
> > -enum rte_eth_flow_type {
> > -	RTE_ETH_FLOW_TYPE_NONE = 0,
> > -	RTE_ETH_FLOW_TYPE_UDPV4,
> > -	RTE_ETH_FLOW_TYPE_TCPV4,
> > -	RTE_ETH_FLOW_TYPE_SCTPV4,
> > -	RTE_ETH_FLOW_TYPE_IPV4_OTHER,
> > -	RTE_ETH_FLOW_TYPE_FRAG_IPV4,
> > -	RTE_ETH_FLOW_TYPE_UDPV6,
> > -	RTE_ETH_FLOW_TYPE_TCPV6,
> > -	RTE_ETH_FLOW_TYPE_SCTPV6,
> > -	RTE_ETH_FLOW_TYPE_IPV6_OTHER,
> > -	RTE_ETH_FLOW_TYPE_FRAG_IPV6,
> > -	RTE_ETH_FLOW_TYPE_MAX = 64,
> > -};
> 
> You are renaming the prefix RTE_ETH_FLOW_TYPE_ to ETH_FLOW_TYPE.
> As this is an exported enum (in the API), we should keep RTE_ prefix.
> If you are trying to shorten the names, I suggest RTE_ETH_FLOW_.
OK. Started with RTE_ETH_FLOW_ is good for me. I will modified it in v2.

> 
> [...]
> >  struct rte_eth_fdir_input {
> > -	enum rte_eth_flow_type flow_type;      /**< Type of flow */
> > +	uint16_t flow_type;      /**< Type of flow */
> [...]
> >  struct rte_eth_fdir_flex_mask {
> > -	enum rte_eth_flow_type flow_type;  /**< Flow type */
> > +	uint16_t flow_type;  /**< Flow type */
> 
> I think this comment is useless ;)
OK. I will remove it. Thanks!

Regards,
Helin

> 
> --
> Thomas