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

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

Thanks for your good comments!

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 3, 2014 3:57 PM
> 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-10-21 11:14, Helin Zhang:
> > +enum rte_eth_hash_filter_info_type {
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,
> 
> PCTYPE is an unknown word in the API layer.
> Could you replace it by something more generic?
So you suggested to remove words of PCTYPE, pctype, or packet classification type?
I am not trying to rename ETH_RSS_IPV4_SHIFT, ..., ETH_RSS_NONF_IPV4_UDP_SHIFT, ..., etc.
They are actually pctype in i40e, and not only for RSS. I would like to rename them into
generic names. Any good naming ideas from you or other guys? My idea is to rename
them like RTE_ETH_FLOW_TYPE_XX.

> 
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
> > +};
> 
> You should comment each constant.
OK. Good to know that!

> 
> > +struct rte_eth_sym_hash_ena_info {
> > +	/**< packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> 
> No, PCTYPE is not anymore defined in ethdev.
We need a generic name for that, how about flow_type? Good comments are welcome!

> 
> > +/**
> > + * A structure used to set or get filter swap information, to support
> > + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > + */
> > +struct rte_eth_filter_swap_info {
> > +	/**< Packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > +	uint8_t off0_src0;
> > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > +	uint8_t off0_src1;
> > +	/**< Field length of the first couple. */
> > +	uint8_t len0;
> > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src0;
> > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src1;
> > +	/**< Field length of the second couple. */
> > +	uint8_t len1;
> > +};
> 
> I guess it would be easier to understand if
> RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
It has already been defined before this structure definition.
I don't think I have understood your idea. Could you help to explain more? Thanks!

> 
> --
> Thomas

Regards,
Helin