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 Fri, Jan 17, 2020 at 9:54 PM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> <snip>
>
> >
> > 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 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