From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 447B52952 for ; Mon, 26 Dec 2016 03:50:34 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga105.fm.intel.com with ESMTP; 25 Dec 2016 18:50:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,404,1477983600"; d="scan'208";a="916126098" Received: from pgsmsx108.gar.corp.intel.com ([10.221.44.103]) by orsmga003.jf.intel.com with ESMTP; 25 Dec 2016 18:50:33 -0800 Received: from pgsmsx103.gar.corp.intel.com ([169.254.2.52]) by PGSMSX108.gar.corp.intel.com ([169.254.8.13]) with mapi id 14.03.0248.002; Mon, 26 Dec 2016 10:50:32 +0800 From: "Zhao1, Wei" To: "Yigit, Ferruh" , "dev@dpdk.org" CC: "Lu, Wenzhuo" Thread-Topic: [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter Thread-Index: AQHSTIlnJ/Gc+mJgT0GbKVI48P2QA6EQpUEAgAj1/HA= Date: Mon, 26 Dec 2016 02:50:31 +0000 Message-ID: References: <1480675394-59179-1-git-send-email-wei.zhao1@intel.com> <1480675394-59179-3-git-send-email-wei.zhao1@intel.com> <71288c3d-2abb-d99d-45a4-f26d399194c2@intel.com> In-Reply-To: <71288c3d-2abb-d99d-45a4-f26d399194c2@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.30.20.205] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Mon, 26 Dec 2016 02:50:35 -0000 Hi, Ferruh > -----Original Message----- > From: Yigit, Ferruh > Sent: Wednesday, December 21, 2016 12:58 AM > To: Zhao1, Wei ; dev@dpdk.org > Cc: Lu, Wenzhuo > Subject: Re: [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filt= er >=20 > 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 =3D 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 =3D rte_hash_create(&fdir_hash_params); >=20 > Do we really create a hash table in device init? Is there a way to do thi= s 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. >=20 > > + if (!fdir_info->hash_handle) { > > + PMD_INIT_LOG(ERR, "Failed to create fdir hash table!"); > > + return -EINVAL; >=20 > 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 warnin= g. But some uer maybe do not pay attention to warning error log sometimes, so = we chose to=20 exit if any init fail. >=20 > > + } > > + fdir_info->hash_map =3D 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; > > + } > > + >=20 > Can you please extract these into functions, to have more clear init/unin= it > 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 f= or this purpose in v2. =20 >=20 > > 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 =3D > > + 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 =3D 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); >=20 > All rte_hash_xxx() APIs gives build error for shared library build, [1] n= eeds to > be added into Makefile. >=20 > But, this makes hash library a dependency to the PMD, do you think is the= re > a way to escape from this? >=20 > [1] > +DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) +=3D lib/librte_net > +lib/librte_hash >=20 Ok, I will change as your suggestion in v2. > > + > > + while ((fdir_filter =3D TAILQ_FIRST(&fdir_info->fdir_list))) { > > + TAILQ_REMOVE(&fdir_info->fdir_list, > > + fdir_filter, > > + entries); > > + rte_free(fdir_filter); > > + } > > + > > return 0; > > } > > > <...>