Thanks for your good comments!
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, November 3, 2014 3:57 PM
> To: Zhang, Helin
> Cc: dev at 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
