[dpdk-dev] [PATCH v4 11/21] ethdev: define structures for getting flow director information
Wu, Jingjing
jingjing.wu at intel.com
Wed Oct 29 03:10:49 CET 2014
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
More information about the dev
mailing list