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

[Tiwei Bie <tiwei.bie@intel.com>]

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.

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

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

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