From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 97024FBA1 for ; Tue, 20 Dec 2016 17:58:03 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP; 20 Dec 2016 08:58:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,379,1477983600"; d="scan'208";a="20798118" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.29]) ([10.237.220.29]) by orsmga002.jf.intel.com with ESMTP; 20 Dec 2016 08:58:01 -0800 From: Ferruh Yigit To: Wei Zhao , dev@dpdk.org References: <1480675394-59179-1-git-send-email-wei.zhao1@intel.com> <1480675394-59179-3-git-send-email-wei.zhao1@intel.com> Cc: wenzhuo.lu@intel.com Message-ID: <71288c3d-2abb-d99d-45a4-f26d399194c2@intel.com> Date: Tue, 20 Dec 2016 16:58:00 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1480675394-59179-3-git-send-email-wei.zhao1@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Dec 2016 16:58:04 -0000 On 12/2/2016 10:42 AM, Wei Zhao wrote: > From: wei zhao1 > > Add support for storing flow director filter in SW. > > Signed-off-by: Wenzhuo Lu > Signed-off-by: wei zhao1 > --- > 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; > } > <...>