Re: [dpdk-dev] [PATCH v4 11/21] ethdev: define structures for getting flow director information

["Wu, Jingjing" <jingjing.wu@intel.com>]

Hi, Thomas

Thanks for your comments.

Jingjing

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 28, 2014 9:45 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 11/21] ethdev: define structures for
> getting flow director information
> 
> 2014-10-22 09:01, Jingjing Wu:
> > +/**
> > + * A structure used to report the status of the flow director filters in use.
> > + */
> > +struct rte_eth_fdir {
> > +	/** Number of filters with collision indication. */
> > +	uint16_t collision;
> > +	/** Number of free (non programmed) filters. */
> > +	uint16_t free;
> > +	/** The Lookup hash value of the added filter that updated the value
> > +	   of the MAXLEN field */
> > +	uint16_t maxhash;
> > +	/** Longest linked list of filters in the table. */
> > +	uint8_t maxlen;
> > +	/** Number of added filters. */
> > +	uint64_t add;
> > +	/** Number of removed filters. */
> > +	uint64_t remove;
> > +	/** Number of failed added filters (no more space in device). */
> > +	uint64_t f_add;
> > +	/** Number of failed removed filters. */
> > +	uint64_t f_remove;
> > +};
> 
> rte_eth_fdir is a name which doesn't say what it really is.
> This structure looks like a collection of statistics.
> Why not rte_eth_fdir_stats?
> 
This structure is used in old flow director API. I moved it from rte_ethdev.h to rte_eth_ctrl.h and reuse it. As we discussed before, I think the old and new APIs will co-exist for one version. I also thought the name is not good enough, but I didn't change it because I want to keep the compatibility with the API used for ixgbe.
I think we can rename it when we are ready to remove the old flow director APIs. 

> > +struct rte_eth_fdir_ext {
> > +	uint16_t guarant_spc;  /**< guaranteed spaces.*/
> > +	uint16_t guarant_cnt;  /**< Number of filters in guaranteed spaces.
> */
> > +	uint16_t best_spc;     /**< best effort spaces.*/
> > +	uint16_t best_cnt;     /**< Number of filters in best effort spaces. */
> > +};
> 
> I don't understand why this "extended" structure is not merged in the first
> one.
> Adding new fields don't break old API.
> 
Just as you say the name of rte_eth_fdir is not suitable, but I didn't want to change 
It. What I want to use for operation RTE_ETH_FILTER_INFO is structure 
rte_eth_fdir_info. And then I define a new rte_eth_fdir_ext to get the info
rte_eth_fdir doesn't contain.
Of course we also can merger all the elements to the struct rte_eth_fdir_info 
Like below without reusing the old struct rte_eth_fdir, what do you think? 
struct rte_eth_fdir_info {
	int mode; 
	uint16_t collision;
	uint16_t free;
	uint16_t maxhash;
	uint8_t maxlen;
	uint64_t add;
	uint64_t remove;
	uint64_t f_add;
	uint64_t f_remove;
 	uint16_t guarant_spc;
	uint16_t guarant_cnt;
	uint16_t best_spc;
	uint16_t best_cnt;
};

> > +/**
> > + * A structure used to get the status information of flow director filter.
> > + * to support RTE_ETH_FILTER_FDIR with RTE_ETH_FILTER_INFO
> operation.
> > + */
> 
> OK content of this comment is good.
> But the second sentence has no start.
> Please try to have an uppercase letter at the beginning of your sentences,
> and a subject followed by a verb.
> (side note: this is also true for commit logs)
> 
OK, will change.

> > +struct rte_eth_fdir_info {
> > +	int mode;                 /**< if 0 disbale, if 1 enable*/
> 
> Typo: disbale
> 
OK, will change.
> --
> Thomas