[dpdk-dev] [PATCH 5/7] ethdev: unification of flow types
Zhang, Helin
helin.zhang at intel.com
Tue Feb 3 06:40:04 CET 2015
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, February 2, 2015 11:39 PM
> To: Zhang, Helin
> Cc: dev at 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 at 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
More information about the dev
mailing list