Re: [dpdk-dev] [PATCH] table: add dedicated params struct for cuckoo hash

[Andy Green <andy@warmcat.com>]

On May 8, 2018 10:17:18 PM GMT+08:00, Jasvinder Singh <jasvinder.singh@intel.com> wrote:
>Add dedicated parameter structure for cuckoo hash. The cuckoo hash from
>librte_hash uses slightly different prototype for the hash function (no
>key_mask parameter, 32-bit seed and return value) that require either
>of the following approaches:
>   1/ Function pointer conversion: gcc 8.1 warning [1], misleading [2]

As I wrote earlier this is broken on master currently... gcc 8.0.1, shipping on Fedora 28, is able to appreciate the existing cast is completely wrong and build errors out.  It's not a compiler version quirk so much as what is in master is actually broken.

>   2/ Union within the parameter structure: pollutes a very generic API
>      parameter structure with some implementation dependent detail
>      (i.e. key mask not available for one of the available
>      implementations)
>   3/ Using opaque pointer for hash function: same issue from 2/
>   4/ Different parameter structure: avoid issue from 2/; hopefully,
>   it won't be long before librte_hash implements the key mask feature,
>      so the generic API structure could be used.

Unifying them in a single function pointer type is obviously best since they're trying to do the same thing.

> static int
>-check_params_create_hash_cuckoo(struct rte_table_hash_params *params)
>+check_params_create_hash_cuckoo(struct rte_table_hash_cuckoo_params
>*params)

...

> {
> 	if (params == NULL) {
> 		RTE_LOG(ERR, TABLE, "NULL Input Parameters.\n");
>@@ -82,7 +81,7 @@ rte_table_hash_cuckoo_create(void *params,
> 			int socket_id,
> 			uint32_t entry_size)
> {
>-	struct rte_table_hash_params *p = params;
>+	struct rte_table_hash_cuckoo_params *p = params;

I think a proper solution will have to get rid of the void * params...

>-		.hash_func = (rte_hash_function)(p->f_hash),
>+		.hash_func = p->f_hash,

-Andy