Re: [dpdk-dev] [PATCH v9 5/6] lib/hash: use ring with 32b element size to save memory

[David Marchand <david.marchand@redhat.com>]

On Thu, Jan 16, 2020 at 6:25 AM Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> wrote:
>
> The freelist and external bucket indices are 32b. Using rings
> that use 32b element sizes will save memory.
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> ---
>  lib/librte_hash/rte_cuckoo_hash.c | 94 ++++++++++++++++---------------
>  lib/librte_hash/rte_cuckoo_hash.h |  2 +-
>  2 files changed, 50 insertions(+), 46 deletions(-)
>

[snip]

> diff --git a/lib/librte_hash/rte_cuckoo_hash.h b/lib/librte_hash/rte_cuckoo_hash.h
> index fb19bb27d..345de6bf9 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.h
> +++ b/lib/librte_hash/rte_cuckoo_hash.h
> @@ -124,7 +124,7 @@ const rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
>
>  struct lcore_cache {
>         unsigned len; /**< Cache len */
> -       void *objs[LCORE_CACHE_SIZE]; /**< Cache objects */
> +       uint32_t objs[LCORE_CACHE_SIZE]; /**< Cache objects */

This triggers a warning in ABI checks:

1 function with some indirect sub-type change:

  [C]'function int32_t rte_hash_add_key(const rte_hash*, void*)' at
rte_cuckoo_hash.c:1118:1 has some indirect sub-type changes:
    parameter 1 of type 'const rte_hash*' has sub-type changes:
      in pointed to type 'const rte_hash':
        in unqualified underlying type 'struct rte_hash' at
rte_cuckoo_hash.h:160:1:
          type size hasn't changed
          1 data member change:
           type of 'lcore_cache* rte_hash::local_free_slots' changed:
             in pointed to type 'struct lcore_cache' at rte_cuckoo_hash.h:125:1:
               type size changed from 4608 to 2560 (in bits)
               1 data member change:
                type of 'void* lcore_cache::objs[64]' changed:
                  array element type 'void*' changed:
                    entity changed from 'void*' to 'typedef uint32_t'
at stdint-uintn.h:26:1
                    type size changed from 64 to 32 (in bits)
                  type name changed from 'void*[64]' to 'uint32_t[64]'
                  array type size changed from 4096 to 2048
                and offset changed from 64 to 32 (in bits) (by -32 bits)

As far as I can see, the local_free_slots field in rte_hash is
supposed to be internal and should just be hidden from users.
lib/librte_hash/rte_cuckoo_hash.c:              h->local_free_slots =
rte_zmalloc_socket(NULL,
lib/librte_hash/rte_cuckoo_hash.c:              rte_free(h->local_free_slots);
lib/librte_hash/rte_cuckoo_hash.c:                      cached_cnt +=
h->local_free_slots[i].len;
lib/librte_hash/rte_cuckoo_hash.c:
h->local_free_slots[i].len = 0;
lib/librte_hash/rte_cuckoo_hash.c:              cached_free_slots =
&h->local_free_slots[lcore_id];
lib/librte_hash/rte_cuckoo_hash.c:              cached_free_slots =
&h->local_free_slots[lcore_id];
lib/librte_hash/rte_cuckoo_hash.c:              cached_free_slots =
&h->local_free_slots[lcore_id];
lib/librte_hash/rte_cuckoo_hash.h:      struct lcore_cache *local_free_slots;

Not sure how users could make use of this.
But the abi check flags this as a breakage since this type was exported.

I can see three options:
- we stick to our "no abi breakage" policy, this change is postponed
to the next ABI breakage, and at the same time, we hide this type and
inspect the rest of the rte_hash API to avoid new issues in the
future,
- we duplicate structures and API by using function versioning to keep
the exact rte_hash v20.0 ABI and a v20.0.1 ABI with the resized and
cleaned structures,
- we override the ABI freeze here by ruling that this was an internal
structure that users should not access (ugh..)

Seeing how this is an optimisation, my preference goes to the first option.


-- 
David Marchand