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.x...@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
