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

[Zhao1, Wei]

Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:58 AM
> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter
> 
> 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?

By now, we have no idea about whether user wiil use flow director or not.
So, our strategy is init all types of filter and give option to users.

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

Maybe, we can delete "return -EINVAL;" and only print error log as a warning.
But some uer maybe do not pay attention to warning error log sometimes, so we chose to 
exit if any init fail.

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

ok, that seems like a good idea to creat two new functions to do initialize of flow director and l2 tunnel fliter.
That will make dev init function more clear. I wiil add two init function for this purpose in v2.  

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

Ok, I will change as your suggestion in v2.

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