<snip>
>
> On Thu, Jan 16, 2020 at 6:25 AM Honnappa Nagarahalli
> <honnappa.nagaraha...@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.nagaraha...@arm.com>
> > Reviewed-by: Gavin Hu <gavin...@arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljed...@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 think this is a false positive.
Users include 'rte_hash.h' file which does not define the structure. It just
has the declaration 'struct rte_hash'. The actual structure is defined in
'rte_cuckoo_hash.h'. But this is not included by the user. So, the application
does not have visibility into 'struct rte_hash' as defined in
'rte_cuckoo_hash.h'.
The 'rte_create_hash' API returns a pointer to the 'struct rte_hash'. All the
APIs are non-inline and just take this pointer as the argument. So, the 'struct
rte_hash' as defined in 'rte_cuckoo_hash.h' is not used by the user.
You can take a look at test_hash_readwrite_lf.c and function 'check_bucket'.
This function is written as the test case cannot access the 'struct rte_hash'
from 'rte_cuckoo_hash.h'.
>
> 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
