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

  define structures for getting flow director information

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
 lib/librte_ether/rte_eth_ctrl.h | 40 ++++++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h   | 23 -----------------------
 2 files changed, 40 insertions(+), 23 deletions(-)
  

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?

> +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.

> +/**
> + * 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)

> +struct rte_eth_fdir_info {
> +	int mode;                 /**< if 0 disbale, if 1 enable*/

Typo: disbale
  

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
  

2014-10-29 02:10, Wu, Jingjing:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 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;
> };

Yes, you are adding a new API while keeping temporarily the old one.
So it's better to add new structures with good names, so we will be able
to remove the whole old API without any other API change in next release.

Thanks