From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH] table: fix build error with gcc 8 Date: Mon, 9 Apr 2018 17:43:18 +0100 Message-ID: References: <20180409124948.130974-1-jasvinder.singh@intel.com> <20180409080936.58ecb66c@xeon-e3> <3EB4FA525960D640B5BDFFD6A3D891267BB3D389@IRSMSX108.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" To: "Van Haaren, Harry" , "Dumitrescu, Cristian" , Stephen Hemminger , "Singh, Jasvinder" , "Richardson, Bruce" Return-path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 2B2B31B854 for ; Mon, 9 Apr 2018 18:43:22 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 4/9/2018 5:38 PM, Van Haaren, Harry wrote: >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu, Cristian >> Sent: Monday, April 9, 2018 4:59 PM >> To: Stephen Hemminger ; Singh, Jasvinder >> ; Richardson, Bruce >> Cc: dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8 >> >> >> >>> -----Original Message----- >>> From: Stephen Hemminger [mailto:stephen@networkplumber.org] >>> Sent: Monday, April 9, 2018 4:10 PM >>> To: Singh, Jasvinder >>> Cc: dev@dpdk.org; Dumitrescu, Cristian >>> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8 >>> >>> On Mon, 9 Apr 2018 13:49:48 +0100 >>> Jasvinder Singh wrote: >>> >>>> Fix build error with gcc 8.0 due to cast between function types. >>>> Fixes: 5a80bf0ae613 ("table: add cuckoo hash") >>>> >>>> Signed-off-by: Jasvinder Singh >>>> --- >>>> lib/librte_table/rte_table_hash_cuckoo.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/librte_table/rte_table_hash_cuckoo.c >>> b/lib/librte_table/rte_table_hash_cuckoo.c >>>> index dcb4fe9..f7eae27 100644 >>>> --- a/lib/librte_table/rte_table_hash_cuckoo.c >>>> +++ b/lib/librte_table/rte_table_hash_cuckoo.c >>>> @@ -103,11 +103,13 @@ rte_table_hash_cuckoo_create(void *params, >>>> return NULL; >>>> } >>>> >>>> + void *hash_func = p->f_hash; >>>> + >>>> /* Create cuckoo hash table */ >>>> struct rte_hash_parameters hash_cuckoo_params = { >>>> .entries = p->n_keys, >>>> .key_len = p->key_size, >>>> - .hash_func = (rte_hash_function)(p->f_hash), >>>> + .hash_func = (rte_hash_function) hash_func, >>>> .hash_func_init_val = p->seed, >>>> .socket_id = socket_id, >>>> .name = p->name >>> >>> This is just tricking the compiler into not complaining. >>> I would really rather see the two hash functions made the same. >> >> (Adding Bruce as well to consolidate all conversations in a single thread.) >> >> What we want to do here is be able to use the librte_hash under the same API >> as the several hash table flavors implemented in librte_table. >> >> Both of these libraries allow configuring the hash function per each hash >> table instance. Problem is: hash function in librte_hash has only 3 parameters >> (no key mask), while hash function in librte_table has 4 parameters (includes >> key mask). The key mask helps a lot for practical protocol implementations by >> avoiding key copy & pre-process on lookup. >> >> So then: how to plug in librte_hash under the same API as the suite of hash >> tables in librte_table? We don't want to re-implement cuckoo hash from >> librte_hash, we simply want to invoke it as a low-level primitive, similarly >> to how the LPM and ACL tables are plugged into librte_table. >> >> Solution is: as an exception, pass a 3-parameter hash function to cuckoo hash >> flavor under the librte_table. Maybe this should be documented better. This >> currently triggers a build warning with gcc 8, which is easy to fix, hence >> this trivial patch. >> >> Ideally, for every 3-parameter hash function, I would like to generate the >> corresponding 4-parameter hash function on-the-fly, but unfortunately this is >> not what C language can do. >> >> Of course, IMO the best solution is to add key mask support to librte_hash. > > > Looking at the previous discussion I see the following as a possible solution; > > Given the current code looks broken it should be fixed in this release. > Given the actual code fix is an API / ABI break (depending on solution) it cannot be merged official in this release. > We have a NEXT_ABI macro - it allows us to break API/ABI conditionally at compile time. > > With the above 3 points, I think the best solution is to correctly fix the problem that GCC 8 is identifying, and putting that new API inside the NEXT_ macros. > > In this case, we can preserve backwards (buggy) behavior if required, and provide correct (but API/ABI breaking) code as well. This is a tough decision - particularly for distros - what do they package? +1 to use RTE_NEXT_ABI and deliver fixed code, and agree this is kind of pushing decision to distros. > > Given the current code, I don't see a better solution - but I hope I'm wrong :) >