DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] table: fix build error with gcc 8
@ 2018-04-09 12:49 Jasvinder Singh
  2018-04-09 12:59 ` Dumitrescu, Cristian
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Jasvinder Singh @ 2018-04-09 12:49 UTC (permalink / raw)
  To: dev; +Cc: cristian.dumitrescu

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.singh@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
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 12:49 [dpdk-dev] [PATCH] table: fix build error with gcc 8 Jasvinder Singh
@ 2018-04-09 12:59 ` Dumitrescu, Cristian
  2018-04-09 13:09 ` Bruce Richardson
  2018-04-09 15:09 ` Stephen Hemminger
  2 siblings, 0 replies; 16+ messages in thread
From: Dumitrescu, Cristian @ 2018-04-09 12:59 UTC (permalink / raw)
  To: Singh, Jasvinder, dev



> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Monday, April 9, 2018 1:50 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Subject: [PATCH] table: fix build error with gcc 8
> 
> 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.singh@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
> --
> 2.9.3

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 12:49 [dpdk-dev] [PATCH] table: fix build error with gcc 8 Jasvinder Singh
  2018-04-09 12:59 ` Dumitrescu, Cristian
@ 2018-04-09 13:09 ` Bruce Richardson
  2018-04-09 14:51   ` Singh, Jasvinder
  2018-04-09 15:09 ` Stephen Hemminger
  2 siblings, 1 reply; 16+ messages in thread
From: Bruce Richardson @ 2018-04-09 13:09 UTC (permalink / raw)
  To: Jasvinder Singh; +Cc: dev, cristian.dumitrescu

On Mon, Apr 09, 2018 at 01:49:48PM +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 <jasvinder.singh@intel.com>

What's the actual error message? Why do the types not match?

/Bruce

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 13:09 ` Bruce Richardson
@ 2018-04-09 14:51   ` Singh, Jasvinder
  2018-04-09 15:28     ` Bruce Richardson
  0 siblings, 1 reply; 16+ messages in thread
From: Singh, Jasvinder @ 2018-04-09 14:51 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: dev, Dumitrescu, Cristian


> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, April 9, 2018 2:09 PM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> 
> On Mon, Apr 09, 2018 at 01:49:48PM +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 <jasvinder.singh@intel.com>
> 
> What's the actual error message? Why do the types not match?
> 
> /Bruce
 
Error log is captured below;

  CC rte_table_hash_cuckoo.o
/librte_table/rte_table_hash_cuckoo.c: In function 'rte_table_hash_cuckoo_create':
/librte_table/rte_table_hash_cuckoo.c:110:16: error: cast between incompatible
 function types from 'rte_table_hash_op_hash' {aka 'long unsigned int (*)(void *, void *, unsigned int,  long unsigned int)'}
 to 'uint32_t (*)(const void *, uint32_t,  uint32_t)' {aka 'unsigned int (*)(const void *, unsigned int,  unsigned int)'} [-Werror=cast-function-type]
   .hash_func = (rte_hash_function) p->f_hash,
                ^
cc1: all warnings being treated as errors

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 12:49 [dpdk-dev] [PATCH] table: fix build error with gcc 8 Jasvinder Singh
  2018-04-09 12:59 ` Dumitrescu, Cristian
  2018-04-09 13:09 ` Bruce Richardson
@ 2018-04-09 15:09 ` Stephen Hemminger
  2018-04-09 15:58   ` Dumitrescu, Cristian
  2 siblings, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2018-04-09 15:09 UTC (permalink / raw)
  To: Jasvinder Singh; +Cc: dev, cristian.dumitrescu

On Mon,  9 Apr 2018 13:49:48 +0100
Jasvinder Singh <jasvinder.singh@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.singh@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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 14:51   ` Singh, Jasvinder
@ 2018-04-09 15:28     ` Bruce Richardson
  2018-04-09 16:41       ` Stephen Hemminger
  0 siblings, 1 reply; 16+ messages in thread
From: Bruce Richardson @ 2018-04-09 15:28 UTC (permalink / raw)
  To: Singh, Jasvinder; +Cc: dev, Dumitrescu, Cristian

On Mon, Apr 09, 2018 at 03:51:10PM +0100, Singh, Jasvinder wrote:
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday, April 9, 2018 2:09 PM
> > To: Singh, Jasvinder <jasvinder.singh@intel.com>
> > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> > 
> > On Mon, Apr 09, 2018 at 01:49:48PM +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 <jasvinder.singh@intel.com>
> > 
> > What's the actual error message? Why do the types not match?
> > 
> > /Bruce
>  
> Error log is captured below;
> 
>   CC rte_table_hash_cuckoo.o
> /librte_table/rte_table_hash_cuckoo.c: In function 'rte_table_hash_cuckoo_create':
> /librte_table/rte_table_hash_cuckoo.c:110:16: error: cast between incompatible
>  function types from 'rte_table_hash_op_hash' {aka 'long unsigned int (*)(void *, void *, unsigned int,  long unsigned int)'}
>  to 'uint32_t (*)(const void *, uint32_t,  uint32_t)' {aka 'unsigned int (*)(const void *, unsigned int,  unsigned int)'} [-Werror=cast-function-type]
>    .hash_func = (rte_hash_function) p->f_hash,
>                 ^
> cc1: all warnings being treated as errors
> 
Even if the compiler isn't complaining now, how can that cast work? Looking
at the error message given, it appears there are two big issues:

1. The expected function call takes 3 parameters:
	(const void *, uint32_t,  uint32_t),
   but you are giving it a function that takes 4 parameters:
	(void *, void *, unsigned int,  long unsigned int)
2. The return type expected is "unsigned int", but you are giving a
   function returning "long unsigned int". On 32-bit systems, these are
   going to be the same size, but on 64-bit, they will be different.
   Similarly for the last function argument.

Is the error message correct?

/Bruce

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 15:09 ` Stephen Hemminger
@ 2018-04-09 15:58   ` Dumitrescu, Cristian
  2018-04-09 16:38     ` Van Haaren, Harry
  0 siblings, 1 reply; 16+ messages in thread
From: Dumitrescu, Cristian @ 2018-04-09 15:58 UTC (permalink / raw)
  To: Stephen Hemminger, Singh, Jasvinder, Richardson, Bruce; +Cc: dev



> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, April 9, 2018 4:10 PM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@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.singh@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.singh@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.

Regards,
Cristian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 15:58   ` Dumitrescu, Cristian
@ 2018-04-09 16:38     ` Van Haaren, Harry
  2018-04-09 16:43       ` Ferruh Yigit
                         ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Van Haaren, Harry @ 2018-04-09 16:38 UTC (permalink / raw)
  To: Dumitrescu, Cristian, Stephen Hemminger, Singh, Jasvinder,
	Richardson, Bruce
  Cc: dev

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu, Cristian
> Sent: Monday, April 9, 2018 4:59 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> 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 <jasvinder.singh@intel.com>
> > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@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.singh@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.singh@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?

Given the current code, I don't see a better solution - but I hope I'm wrong :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 15:28     ` Bruce Richardson
@ 2018-04-09 16:41       ` Stephen Hemminger
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2018-04-09 16:41 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Singh, Jasvinder, dev, Dumitrescu, Cristian

On Mon, 9 Apr 2018 16:28:12 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Mon, Apr 09, 2018 at 03:51:10PM +0100, Singh, Jasvinder wrote:
> >   
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Monday, April 9, 2018 2:09 PM
> > > To: Singh, Jasvinder <jasvinder.singh@intel.com>
> > > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> > > 
> > > On Mon, Apr 09, 2018 at 01:49:48PM +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 <jasvinder.singh@intel.com>  
> > > 
> > > What's the actual error message? Why do the types not match?
> > > 
> > > /Bruce  
> >  
> > Error log is captured below;
> > 
> >   CC rte_table_hash_cuckoo.o
> > /librte_table/rte_table_hash_cuckoo.c: In function 'rte_table_hash_cuckoo_create':
> > /librte_table/rte_table_hash_cuckoo.c:110:16: error: cast between incompatible
> >  function types from 'rte_table_hash_op_hash' {aka 'long unsigned int (*)(void *, void *, unsigned int,  long unsigned int)'}
> >  to 'uint32_t (*)(const void *, uint32_t,  uint32_t)' {aka 'unsigned int (*)(const void *, unsigned int,  unsigned int)'} [-Werror=cast-function-type]
> >    .hash_func = (rte_hash_function) p->f_hash,
> >                 ^
> > cc1: all warnings being treated as errors
> >   
> Even if the compiler isn't complaining now, how can that cast work? Looking
> at the error message given, it appears there are two big issues:
> 
> 1. The expected function call takes 3 parameters:
> 	(const void *, uint32_t,  uint32_t),
>    but you are giving it a function that takes 4 parameters:
> 	(void *, void *, unsigned int,  long unsigned int)
> 2. The return type expected is "unsigned int", but you are giving a
>    function returning "long unsigned int". On 32-bit systems, these are
>    going to be the same size, but on 64-bit, they will be different.
>    Similarly for the last function argument.
> 
> Is the error message correct?

Yes. the current code is working only by luck

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 16:38     ` Van Haaren, Harry
@ 2018-04-09 16:43       ` Ferruh Yigit
  2018-04-09 17:05         ` Dumitrescu, Cristian
  2018-04-09 17:02       ` Dumitrescu, Cristian
  2018-04-10 11:43       ` Neil Horman
  2 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2018-04-09 16:43 UTC (permalink / raw)
  To: Van Haaren, Harry, Dumitrescu, Cristian, Stephen Hemminger,
	Singh, Jasvinder, Richardson, Bruce
  Cc: 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 <stephen@networkplumber.org>; Singh, Jasvinder
>> <jasvinder.singh@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> 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 <jasvinder.singh@intel.com>
>>> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@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.singh@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.singh@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 :)
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 16:38     ` Van Haaren, Harry
  2018-04-09 16:43       ` Ferruh Yigit
@ 2018-04-09 17:02       ` Dumitrescu, Cristian
  2018-04-09 17:09         ` Ananyev, Konstantin
  2018-04-10 11:43       ` Neil Horman
  2 siblings, 1 reply; 16+ messages in thread
From: Dumitrescu, Cristian @ 2018-04-09 17:02 UTC (permalink / raw)
  To: Van Haaren, Harry, Stephen Hemminger, Singh, Jasvinder,
	Richardson, Bruce
  Cc: dev



> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Monday, April 9, 2018 5:38 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu,
> Cristian
> > Sent: Monday, April 9, 2018 4:59 PM
> > To: Stephen Hemminger <stephen@networkplumber.org>; Singh,
> Jasvinder
> > <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> > 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 <jasvinder.singh@intel.com>
> > > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@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.singh@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.singh@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.

The code is not broken. This is not a bug, it is a limitation for that particular table type. The only gap that I see is adding a Doxygen comment in the API header file.

User explicitly picks the hash table type it wants; when using this particular hash table type, that pointer needs to point to a 3-parameter function instead of 4. Given the limitation is clearly documented in Doxygen (current gap that we can quickly address), I don't see any problem.

If people think that this function conversion is not nice, it can be reworked in multiple ways at the expense of API (but not ABI) change:
1. Define the hash function field in the table parameter structure as opaque void * rather than 4-parameter version.
2. Create a separate parameter structure just for this hash table type.

> 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.

This is not new code introduced in this release cycle, this is just fixing the compiler warning, I fail to see how your ABI breakage mention is applicable.

Maybe we should talk more specifics over the code, where exactly in the code would you like to place your NEXT_ABI macro?

> 
> 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?
> 
> Given the current code, I don't see a better solution - but I hope I'm wrong :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 16:43       ` Ferruh Yigit
@ 2018-04-09 17:05         ` Dumitrescu, Cristian
  0 siblings, 0 replies; 16+ messages in thread
From: Dumitrescu, Cristian @ 2018-04-09 17:05 UTC (permalink / raw)
  To: Yigit, Ferruh, Van Haaren, Harry, Stephen Hemminger, Singh,
	Jasvinder, Richardson, Bruce
  Cc: dev



> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, April 9, 2018 5:43 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> 
> 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 <stephen@networkplumber.org>; Singh,
> Jasvinder
> >> <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> >> 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 <jasvinder.singh@intel.com>
> >>> Cc: dev@dpdk.org; Dumitrescu, Cristian
> <cristian.dumitrescu@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.singh@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.singh@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.
> 

Again, where is the bug, and where exactly in the code you want to put RTE_NEXT_ABI macro, and what is the problem fixed by using this macro?

As stated in the reply to Harry, this could be reworked in multiple ways if people think the function pointer conversion is misleading to the user.

> >
> > Given the current code, I don't see a better solution - but I hope I'm wrong
> :)
> >


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 17:02       ` Dumitrescu, Cristian
@ 2018-04-09 17:09         ` Ananyev, Konstantin
  2018-04-09 17:26           ` Dumitrescu, Cristian
  0 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2018-04-09 17:09 UTC (permalink / raw)
  To: Dumitrescu, Cristian, Van Haaren, Harry, Stephen Hemminger,
	Singh, Jasvinder, Richardson, Bruce
  Cc: dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu, Cristian
> Sent: Monday, April 9, 2018 6:02 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Stephen Hemminger <stephen@networkplumber.org>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> 
> 
> 
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Monday, April 9, 2018 5:38 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Stephen
> > Hemminger <stephen@networkplumber.org>; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> >
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu,
> > Cristian
> > > Sent: Monday, April 9, 2018 4:59 PM
> > > To: Stephen Hemminger <stephen@networkplumber.org>; Singh,
> > Jasvinder
> > > <jasvinder.singh@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > > 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 <jasvinder.singh@intel.com>
> > > > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@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.singh@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.singh@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.
> 
> The code is not broken. This is not a bug, it is a limitation for that particular table type. The only gap that I see is adding a Doxygen
> comment in the API header file.
> 
> User explicitly picks the hash table type it wants; when using this particular hash table type, that pointer needs to point to a 3-parameter
> function instead of 4. Given the limitation is clearly documented in Doxygen (current gap that we can quickly address), I don't see any
> problem.
> 
> If people think that this function conversion is not nice, it can be reworked in multiple ways at the expense of API (but not ABI) change:
> 1. Define the hash function field in the table parameter structure as opaque void * rather than 4-parameter version.
> 2. Create a separate parameter structure just for this hash table type.

Why just not define your f_hash member as a union:

struct rte_table_hash_params {
...
union {
    rte_table_hash_op_hash  f_hash_4params;
    rte_hash_function f_hash_3_params;
}; 

?

> 
> > 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.
> 
> This is not new code introduced in this release cycle, this is just fixing the compiler warning, I fail to see how your ABI breakage mention is
> applicable.
> 
> Maybe we should talk more specifics over the code, where exactly in the code would you like to place your NEXT_ABI macro?
> 
> >
> > 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?
> >
> > Given the current code, I don't see a better solution - but I hope I'm wrong :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 17:09         ` Ananyev, Konstantin
@ 2018-04-09 17:26           ` Dumitrescu, Cristian
  2018-04-10 12:32             ` Van Haaren, Harry
  0 siblings, 1 reply; 16+ messages in thread
From: Dumitrescu, Cristian @ 2018-04-09 17:26 UTC (permalink / raw)
  To: Ananyev, Konstantin, Van Haaren, Harry, Stephen Hemminger, Singh,
	Jasvinder, Richardson, Bruce
  Cc: dev

> >
> > If people think that this function conversion is not nice, it can be reworked
> in multiple ways at the expense of API (but not ABI) change:
> > 1. Define the hash function field in the table parameter structure as
> opaque void * rather than 4-parameter version.
> > 2. Create a separate parameter structure just for this hash table type.
> 
> Why just not define your f_hash member as a union:
> 
> struct rte_table_hash_params {
> ...
> union {
>     rte_table_hash_op_hash  f_hash_4params;
>     rte_hash_function f_hash_3_params;
> };
> 
> ?
> 

Yes, agreed, this is yet another way to handle this, thanks Konstantin.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 16:38     ` Van Haaren, Harry
  2018-04-09 16:43       ` Ferruh Yigit
  2018-04-09 17:02       ` Dumitrescu, Cristian
@ 2018-04-10 11:43       ` Neil Horman
  2 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2018-04-10 11:43 UTC (permalink / raw)
  To: Van Haaren, Harry
  Cc: Dumitrescu, Cristian, Stephen Hemminger, Singh, Jasvinder,
	Richardson, Bruce, dev

On Mon, Apr 09, 2018 at 04:38:11PM +0000, 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 <stephen@networkplumber.org>; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> > 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 <jasvinder.singh@intel.com>
> > > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@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.singh@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.singh@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?
> 
> Given the current code, I don't see a better solution - but I hope I'm wrong :)
> 
Why not make the hash_func pointer in the rte_hash_parameters structure an
anonymous union, and reserve a bit in the extra_flag field to denote if the
function pointer has 3 arguments or 4?  Then rte_hash_hash can use the
appropriate calling convention on hash_func.

Neil

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
  2018-04-09 17:26           ` Dumitrescu, Cristian
@ 2018-04-10 12:32             ` Van Haaren, Harry
  0 siblings, 0 replies; 16+ messages in thread
From: Van Haaren, Harry @ 2018-04-10 12:32 UTC (permalink / raw)
  To: Dumitrescu, Cristian, Neil Horman
  Cc: dev, Ananyev, Konstantin, Stephen Hemminger, Singh, Jasvinder,
	Richardson, Bruce

+CC Neil from other reply

> From: Dumitrescu, Cristian
> Sent: Monday, April 9, 2018 6:27 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Stephen Hemminger <stephen@networkplumber.org>;
> Singh, Jasvinder <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> 
> > >
> > > If people think that this function conversion is not nice, it can be
> reworked
> > in multiple ways at the expense of API (but not ABI) change:
> > > 1. Define the hash function field in the table parameter structure as
> > opaque void * rather than 4-parameter version.
> > > 2. Create a separate parameter structure just for this hash table type.
> >
> > Why just not define your f_hash member as a union:
> >
> > struct rte_table_hash_params {
> > ...
> > union {
> >     rte_table_hash_op_hash  f_hash_4params;
> >     rte_hash_function f_hash_3_params;
> > };
> >
> > ?
> >
> 
> Yes, agreed, this is yet another way to handle this, thanks Konstantin.

Agree that this solution is a lot better than raw casting.

The issue I have with casting is that it doesn't explicitly show that the signature is different, and that the code must be aware of that fact. With a union, at least the code explicitly states that there is a difference in signature, and that this is being handled by the code, so this looks a better solution.

Neil proposed an alternative solution using a bit to indicate calling params in a separate reply - another possibility.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-04-10 12:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-09 12:49 [dpdk-dev] [PATCH] table: fix build error with gcc 8 Jasvinder Singh
2018-04-09 12:59 ` Dumitrescu, Cristian
2018-04-09 13:09 ` Bruce Richardson
2018-04-09 14:51   ` Singh, Jasvinder
2018-04-09 15:28     ` Bruce Richardson
2018-04-09 16:41       ` Stephen Hemminger
2018-04-09 15:09 ` Stephen Hemminger
2018-04-09 15:58   ` Dumitrescu, Cristian
2018-04-09 16:38     ` Van Haaren, Harry
2018-04-09 16:43       ` Ferruh Yigit
2018-04-09 17:05         ` Dumitrescu, Cristian
2018-04-09 17:02       ` Dumitrescu, Cristian
2018-04-09 17:09         ` Ananyev, Konstantin
2018-04-09 17:26           ` Dumitrescu, Cristian
2018-04-10 12:32             ` Van Haaren, Harry
2018-04-10 11:43       ` Neil Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).