Re: [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> >>
> >> This patch adds a comment for RTE_HASH_BUCKET_ENTRIES explaining
> why
> >> a particular value was chosen.
> >>
> >> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> >> ---
> >>   lib/hash/rte_cuckoo_hash.h | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/hash/rte_cuckoo_hash.h b/lib/hash/rte_cuckoo_hash.h
> >> index 85be49d3bb..84dc55d86e 100644
> >> --- a/lib/hash/rte_cuckoo_hash.h
> >> +++ b/lib/hash/rte_cuckoo_hash.h
> >> @@ -101,7 +101,13 @@ const rte_hash_cmp_eq_t
> >> cmp_jump_table[NUM_KEY_CMP_CASES] = {  #endif
> >>
> >>
> >> -/** Number of items per bucket. */
> >> +/**
> >> + * Number of items per bucket.
> >> + * 8 is a tradeoff between performance and memory consumption.
> >> + * When it is equal to 8, the sizeof(struct rte_hash_bucket) equal
> >> +to
> >> + * RTE_CACHE_LINE_SIZE, thus, there are no gaps in memory between
> >> +the hash
> >> + * buckets due to their alignment.
> >> + */
> > I think this should consider cache lines which are 128B. How about the
> following:
> > "when it is equal to 8, multiple 'struct rte_hash_bucket' can be fit on a single
> cache line without any gaps in memory between them".
> >
> 
> Sounds good, will add it in v2.
> 
> > On the other hand, I am wondering if 'struct rte_hash_bucket' needs to have
> __rte_cache_aligned attribute. When the memory is allocated for the buckets
> we are requesting that it is aligned on the cache line boundary. That should be
> sufficient. Removing the attribute will help for local variables. Some functions
> (for ex: rte_hash_cuckoo_move_insert_mw) have 2 local variables of this type
> and they can be placed on the same cache line if this attribute is removed.
> >
> 
> I see, however I can't find inside the rte_hash_cuckoo_move_insert_mw()
> 'struct rte_hash_bucket' as local variables, there are only pointers:
Agree, it is all pointers.

> 
>          struct rte_hash_bucket *cur_bkt;
>          struct rte_hash_bucket *prev_bkt, *curr_bkt = leaf->bkt;
> 
> Yipeng, Sameh, Bruce, what do you think about removing __rte_cache_aligned
> attribute?
> 
> >>   #define RTE_HASH_BUCKET_ENTRIES		8
> >>
> >>   #if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES)
> >> --
> >> 2.25.1
> >
> 
> --
> Regards,
> Vladimir