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

[Honnappa Nagarahalli]

<snip>

> 
> On Thu, Jan 16, 2020 at 6:25 AM Honnappa Nagarahalli
> <honnappa.nagarahalli at 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 at arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu at arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl at 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