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

["Wang, Yipeng1" <yipeng1.wang@intel.com>]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Tuesday, November 9, 2021 11:46 AM
> To: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Wang, Yipeng1 <yipeng1.wang@intel.com>;
> Gobriel, Sameh <sameh.gobriel@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH] hash: clarify comments for
> RTE_HASH_BUCKET_ENTRIES
> 
> <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?
[Wang, Yipeng] 
Since it only affects local variables and we don’t create any, let's keep the attribute for now.

Thanks!