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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

From: Andy Green [mailto:andy@warmcat.com]
Sent: Tuesday, May 8, 2018 3:35 PM
To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
Subject: Re: [dpdk-dev] [PATCH] table: add dedicated params struct for cuckoo hash



On May 8, 2018 10:17:18 PM GMT+08:00, Jasvinder Singh <jasvinder.singh@intel.com<mailto: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.

[Cristian] Right, this is why we fixed it with this patch.


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

[Cristian] It is not a bad solution, but we decided to go for a dedicated params structure for cuckoo hash for the above reasons. It is functionally equivalent and it removes the root cause of the problem (i.e. no function pointer conversion required).


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

[Cristian] You should probably go and spend some time understand how the rte_table.h API works.


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

-Andy