Re: [dpdk-dev] [PATCH v2 03/17] net/i40e: store flow director filter

["Xing, Beilei" <beilei.xing@intel.com>]

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, December 28, 2016 11:39 AM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 03/17] net/i40e: store flow director filter
> 
> On Tue, Dec 27, 2016 at 02:26:10PM +0800, Beilei Xing wrote:
> > Currently there's no flow director filter stored in SW. This patch
> > stores flow director filters in SW with cuckoo hash, also adds
> > protection if a flow director filter has been added.
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 48 +++++++++++++++++++++
> > drivers/net/i40e/i40e_ethdev.h | 12 ++++++
> >  drivers/net/i40e/i40e_fdir.c   | 98
> ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 158 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index c012d5d..427ebdc 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> [...]
> > @@ -1342,6 +1379,17 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
> >  		rte_free(p_tunnel);
> >  	}
> >
> > +	/* Remove all flow director rules and hash */
> > +	if (fdir_info->hash_map)
> > +		rte_free(fdir_info->hash_map);
> > +	if (fdir_info->hash_table)
> > +		rte_hash_free(fdir_info->hash_table);
> > +
> > +	while ((p_fdir = TAILQ_FIRST(&fdir_info->fdir_list))) {
> 
> There is a redundant pair of parentheses, or you should compare with NULL.

I think the another parentheses is used to compare with NULL. In fact there's similar using in PMD.

> 
> > +		TAILQ_REMOVE(&fdir_info->fdir_list, p_fdir, rules);
> > +		rte_free(p_fdir);
> > +	}
> > +
> >  	dev->dev_ops = NULL;
> >  	dev->rx_pkt_burst = NULL;
> >  	dev->tx_pkt_burst = NULL;
> [...]
> > diff --git a/drivers/net/i40e/i40e_fdir.c
> > b/drivers/net/i40e/i40e_fdir.c index 335bf15..faa2495 100644
> > --- a/drivers/net/i40e/i40e_fdir.c
> > +++ b/drivers/net/i40e/i40e_fdir.c
> [...]
> > +/* Check if there exists the flow director filter */ static struct
> > +i40e_fdir_filter * i40e_sw_fdir_filter_lookup(struct i40e_fdir_info
> > +*fdir_info,
> > +			const struct rte_eth_fdir_input *input) {
> > +	int ret = 0;
> > +
> 
> The initialization is meaningless, as it will be written by the below
> assignment unconditionally.

Yes, you're right.

> 
> > +	ret = rte_hash_lookup(fdir_info->hash_table, (const void *)input);
> > +	if (ret < 0)
> > +		return NULL;
> > +
> > +	return fdir_info->hash_map[ret];
> > +}
> > +
> > +/* Add a flow director filter into the SW list */
> > +static int
> > +i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter *filter)
> > +{
> > +	struct i40e_fdir_info *fdir_info = &pf->fdir;
> > +	int ret = 0;
> > +
> 
> Same here.
> 
> > +	ret = rte_hash_add_key(fdir_info->hash_table,
> > +			       &filter->fdir.input);
> > +	if (ret < 0)
> > +		PMD_DRV_LOG(ERR,
> > +			    "Failed to insert fdir filter to hash table %d!",
> > +			    ret);
> 
> Function should return when ret < 0.

Thanks for catching it.

> 
> > +	fdir_info->hash_map[ret] = filter;
> > +
> > +	TAILQ_INSERT_TAIL(&fdir_info->fdir_list, filter, rules);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Delete a flow director filter from the SW list */
> > +static int
> > +i40e_sw_fdir_filter_del(struct i40e_pf *pf, struct i40e_fdir_filter *filter)
> > +{
> > +	struct i40e_fdir_info *fdir_info = &pf->fdir;
> > +	int ret = 0;
> > +
> 
> Same here.
> 
> > +	ret = rte_hash_del_key(fdir_info->hash_table,
> > +			       &filter->fdir.input);
> > +	if (ret < 0)
> > +		PMD_DRV_LOG(ERR,
> > +			    "Failed to delete fdir filter to hash table %d!",
> > +			    ret);
> 
> Function should return when ret < 0.
> 
> > +	fdir_info->hash_map[ret] = NULL;
> > +
> > +	TAILQ_REMOVE(&fdir_info->fdir_list, filter, rules);
> > +	rte_free(filter);
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * i40e_add_del_fdir_filter - add or remove a flow director filter.
> >   * @pf: board private structure
> > @@ -1032,6 +1105,8 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
> >  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> >  	unsigned char *pkt = (unsigned char *)pf->fdir.prg_pkt;
> >  	enum i40e_filter_pctype pctype;
> > +	struct i40e_fdir_info *fdir_info = &pf->fdir;
> > +	struct i40e_fdir_filter *fdir_filter, *node;
> >  	int ret = 0;
> >
> >  	if (dev->data->dev_conf.fdir_conf.mode !=
> RTE_FDIR_MODE_PERFECT) {
> > @@ -1054,6 +1129,21 @@ i40e_add_del_fdir_filter(struct rte_eth_dev
> *dev,
> >  		return -EINVAL;
> >  	}
> >
> > +	fdir_filter = rte_zmalloc("fdir_filter", sizeof(*fdir_filter), 0);
> > +	i40e_fdir_filter_convert(filter, fdir_filter);
> > +	node = i40e_sw_fdir_filter_lookup(fdir_info, &fdir_filter->fdir.input);
> > +	if (add && node) {
> > +		PMD_DRV_LOG(ERR,
> > +			    "Conflict with existing flow director rules!");
> > +		rte_free(fdir_filter);
> > +		return -EINVAL;
> > +	} else if (!add && !node) {
> 
> When `if (add && node)' is true, function will return. There is no need
> to use `else' here.
> 
> Best regards,
> Tiwei Bie