From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id AEA332935 for ; Thu, 14 Sep 2017 12:10:30 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Sep 2017 03:10:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,392,1500966000"; d="scan'208";a="135266261" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by orsmga002.jf.intel.com with ESMTP; 14 Sep 2017 03:10:28 -0700 From: Ferruh Yigit To: "David Harton (dharton)" , "konstantin.ananyev@intel.com" Cc: "dev@dpdk.org" , Wenzhuo Lu References: <20170825192104.37699-1-dharton@cisco.com> <190f7c009bca4a10af244f7aa1ee1385@XCH-RCD-016.cisco.com> <8331235b-cd7a-f9c5-1b18-9ad9da8e87a4@intel.com> Message-ID: <97199839-6bef-d960-bf3c-51244ebaf427@intel.com> Date: Thu, 14 Sep 2017 11:10:27 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <8331235b-cd7a-f9c5-1b18-9ad9da8e87a4@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist symbols 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: Thu, 14 Sep 2017 10:10:31 -0000 On 8/29/2017 5:23 PM, Ferruh Yigit wrote: > On 8/28/2017 6:13 PM, David Harton (dharton) wrote: >> >> >>> -----Original Message----- >>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >>> Sent: Monday, August 28, 2017 9:27 AM >>> To: David Harton (dharton) ; >>> konstantin.ananyev@intel.com >>> Cc: dev@dpdk.org >>> Subject: Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist >>> symbols >>> >>> On 8/25/2017 8:21 PM, David Harton wrote: >>>> Some compilers generate warnings for duplicate symbols for the set of >>>> filter lists current defined in ixgbe_ethdev.h. >>>> This commits moves the defintion and declaration to the source file >>>> that actually uses them and provides a function to initialize the >>>> values akin to its flush function. >>>> >>>> Signed-off-by: David Harton >>>> --- >>>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++------ >>>> drivers/net/ixgbe/ixgbe_ethdev.h | 7 +------ >>>> drivers/net/ixgbe/ixgbe_flow.c | 18 ++++++++++++++++++ >>>> 3 files changed, 21 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >>>> b/drivers/net/ixgbe/ixgbe_ethdev.c >>>> index 1ec5aaf..ed21af5 100644 >>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >>>> @@ -1332,12 +1332,8 @@ struct rte_ixgbe_xstats_name_off { >>>> /* initialize l2 tunnel filter list & hash */ >>>> ixgbe_l2_tn_filter_init(eth_dev); >>>> >>>> - TAILQ_INIT(&filter_ntuple_list); >>>> - TAILQ_INIT(&filter_ethertype_list); >>>> - TAILQ_INIT(&filter_syn_list); >>>> - TAILQ_INIT(&filter_fdir_list); >>>> - TAILQ_INIT(&filter_l2_tunnel_list); >>>> - TAILQ_INIT(&ixgbe_flow_list); >>>> + /* initialize flow filter lists */ >>>> + ixgbe_filterlist_init(); >>> >>> Will it work if list initialization converted to the static >>> initialization? That would remove requirement of this function call. >> >> I was trying to keep the behavior the same: >> .probe -> eth_ixgbe_dev_init -> ixgbe_filterlist_init >> .remove -> eth_ixgbe_dev_uninit -> ixgbe_filterlist_flush >> >> While I think the below would work I always like mirror/parallel relationships like ctor/dtor for objects. > > I would agree if ixgbe_filterlist_init() initializes something more than > list itself, but for this case initialization can be avoided completely > which I would prefer. > > Driver maintainer also cc'ed for more comment. It seems there is no more comment. So, I suggest lets focus on your fix, my suggestion to convert to static initialization can be done later if desired. Still, what do you think about moving declaration of structs as well to the ixgbe_flow.c? After your change, only ixgbe_flow.c will be using them, no need to keep them in ixgbe_ethdev.h. Thanks, ferruh > >> >> Is that reasonable? >> >> Thanks, >> Dave >> >>> >>> something like: >>> >>> diff --git a/drivers/net/ixgbe/ixgbe_flow.c >>> b/drivers/net/ixgbe/ixgbe_flow.c index c8645f056..6184eb4af 100644 >>> --- a/drivers/net/ixgbe/ixgbe_flow.c >>> +++ b/drivers/net/ixgbe/ixgbe_flow.c >>> @@ -79,6 +79,24 @@ >>> #define IXGBE_MAX_N_TUPLE_PRIO 7 >>> #define IXGBE_MAX_FLX_SOURCE_OFF 62 >>> >>> +static TAILQ_HEAD(ixgbe_ntuple_filter_list, ixgbe_ntuple_filter_ele) >>> + filter_ntuple_list = TAILQ_HEAD_INITIALIZER(filter_ntuple_list); >>> + >>> +static TAILQ_HEAD(ixgbe_ethertype_filter_list, >>> ixgbe_ethertype_filter_ele) >>> + filter_ethertype_list = >>> TAILQ_HEAD_INITIALIZER(filter_ethertype_list); >>> + >>> +static TAILQ_HEAD(ixgbe_syn_filter_list, ixgbe_eth_syn_filter_ele) >>> + filter_syn_list = TAILQ_HEAD_INITIALIZER(filter_syn_list); >>> + >>> +static TAILQ_HEAD(ixgbe_fdir_rule_filter_list, ixgbe_fdir_rule_ele) >>> + filter_fdir_list = TAILQ_HEAD_INITIALIZER(filter_fdir_list); >>> + >>> +static TAILQ_HEAD(ixgbe_l2_tunnel_filter_list, >>> ixgbe_eth_l2_tunnel_conf_ele) >>> + filter_l2_tunnel_list = >>> TAILQ_HEAD_INITIALIZER(filter_l2_tunnel_list); >>> + >>> +static TAILQ_HEAD(ixgbe_flow_mem_list, ixgbe_flow_mem) >>> + ixgbe_flow_list = TAILQ_HEAD_INITIALIZER(ixgbe_flow_list); >>> + >