On Fri, Jan 17, 2020 at 9:54 PM Honnappa Nagarahalli
<honnappa.nagaraha...@arm.com> wrote:
>
> <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.
Indeed, it seems properly hidden.
Scratching the rest of the mail.
Looked at abidiff, I can see it takes a public headers directory to
filter the ABI changes.
Need to make this work.
--
David Marchand
