On 4/9/2018 5:38 PM, Van Haaren, Harry wrote: >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Dumitrescu, Cristian >> Sent: Monday, April 9, 2018 4:59 PM >> To: Stephen Hemminger <step...@networkplumber.org>; Singh, Jasvinder >> <jasvinder.si...@intel.com>; Richardson, Bruce <bruce.richard...@intel.com> >> Cc: dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8 >> >> >> >>> -----Original Message----- >>> From: Stephen Hemminger [mailto:step...@networkplumber.org] >>> Sent: Monday, April 9, 2018 4:10 PM >>> To: Singh, Jasvinder <jasvinder.si...@intel.com> >>> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitre...@intel.com> >>> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8 >>> >>> On Mon, 9 Apr 2018 13:49:48 +0100 >>> Jasvinder Singh <jasvinder.si...@intel.com> 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 <jasvinder.si...@intel.com> >>>> --- >>>> 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 > :) >
