[dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter

[Ferruh Yigit]

On 12/2/2016 10:42 AM, Wei Zhao wrote:
> From: wei zhao1 <wei.zhao1 at intel.com>
> 
> Add support for storing flow director filter in SW.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
> Signed-off-by: wei zhao1 <wei.zhao1 at intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c |  48 ++++++++++++++++++
>  drivers/net/ixgbe/ixgbe_ethdev.h |  19 ++++++-
>  drivers/net/ixgbe/ixgbe_fdir.c   | 105 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 169 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 7f10cca..f8e5fe1 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
<...>
> @@ -1289,6 +1302,25 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
>  
>  	/* initialize SYN filter */
>  	filter_info->syn_info = 0;
> +	/* initialize flow director filter list & hash */
> +	TAILQ_INIT(&fdir_info->fdir_list);
> +	snprintf(fdir_hash_name, RTE_HASH_NAMESIZE,
> +		 "fdir_%s", eth_dev->data->name);
> +	fdir_info->hash_handle = rte_hash_create(&fdir_hash_params);

Do we really create a hash table in device init? Is there a way to do
this if we know user will use it?

> +	if (!fdir_info->hash_handle) {
> +		PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
> +		return -EINVAL;

And should we exit here? What if user will not use flow director at all?

> +	}
> +	fdir_info->hash_map = rte_zmalloc("ixgbe",
> +					  sizeof(struct ixgbe_fdir_filter *) *
> +					  IXGBE_MAX_FDIR_FILTER_NUM,
> +					  0);
> +	if (!fdir_info->hash_map) {
> +		PMD_INIT_LOG(ERR,
> +			     "Failed to allocate memory for fdir hash map!");
> +		return -ENOMEM;
> +	}
> +

Can you please extract these into functions, to have more clear
init/uninit functions?

>  	return 0;
>  }
>  
> @@ -1297,6 +1329,9 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev)
>  {
>  	struct rte_pci_device *pci_dev;
>  	struct ixgbe_hw *hw;
> +	struct ixgbe_hw_fdir_info *fdir_info =
> +		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(eth_dev->data->dev_private);
> +	struct ixgbe_fdir_filter *fdir_filter;
>  
>  	PMD_INIT_FUNC_TRACE();
>  
> @@ -1330,6 +1365,19 @@ eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev)
>  	rte_free(eth_dev->data->hash_mac_addrs);
>  	eth_dev->data->hash_mac_addrs = NULL;
>  
> +	/* remove all the fdir filters & hash */
> +	if (fdir_info->hash_map)
> +		rte_free(fdir_info->hash_map);
> +	if (fdir_info->hash_handle)
> +		rte_hash_free(fdir_info->hash_handle);

All rte_hash_xxx() APIs gives build error for shared library build, [1]
needs to be added into Makefile.

But, this makes hash library a dependency to the PMD, do you think is
there a way to escape from this?

[1]
+DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_net lib/librte_hash

> +
> +	while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list))) {
> +		TAILQ_REMOVE(&fdir_info->fdir_list,
> +			     fdir_filter,
> +			     entries);
> +		rte_free(fdir_filter);
> +	}
> +
>  	return 0;
>  }
>  
<...>