Hi, Thomas Thanks for your comments.
Jingjing > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, October 28, 2014 9:45 PM > To: Wu, Jingjing > Cc: dev at 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
