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

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
[...]
> --- 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?
Nitpicking: numbers from 0 to 9 should be 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_.

[...]
>  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 ;)

-- 
Thomas