* [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES @ 2021-11-08 17:39 Vladimir Medvedkin 2021-11-08 22:04 ` Honnappa Nagarahalli 2021-11-10 18:36 ` [PATCH v2] " Vladimir Medvedkin 0 siblings, 2 replies; 8+ messages in thread From: Vladimir Medvedkin @ 2021-11-08 17:39 UTC (permalink / raw) To: dev; +Cc: thomas, Yipeng Wang, Sameh Gobriel, Bruce Richardson 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. + */ #define RTE_HASH_BUCKET_ENTRIES 8 #if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-08 17:39 [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES Vladimir Medvedkin @ 2021-11-08 22:04 ` Honnappa Nagarahalli 2021-11-09 19:11 ` Medvedkin, Vladimir 2021-11-10 18:36 ` [PATCH v2] " Vladimir Medvedkin 1 sibling, 1 reply; 8+ messages in thread From: Honnappa Nagarahalli @ 2021-11-08 22:04 UTC (permalink / raw) To: Vladimir Medvedkin, dev Cc: thomas, Yipeng Wang, Sameh Gobriel, Bruce Richardson, nd, Honnappa Nagarahalli, nd <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". 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. > #define RTE_HASH_BUCKET_ENTRIES 8 > > #if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES) > -- > 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-08 22:04 ` Honnappa Nagarahalli @ 2021-11-09 19:11 ` Medvedkin, Vladimir 2021-11-09 19:46 ` Honnappa Nagarahalli 0 siblings, 1 reply; 8+ messages in thread From: Medvedkin, Vladimir @ 2021-11-09 19:11 UTC (permalink / raw) To: Honnappa Nagarahalli, dev Cc: thomas, Yipeng Wang, Sameh Gobriel, Bruce Richardson, nd Hi Honnappa, On 08/11/2021 23:04, Honnappa Nagarahalli wrote: > <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: 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-09 19:11 ` Medvedkin, Vladimir @ 2021-11-09 19:46 ` Honnappa Nagarahalli 2021-11-10 18:26 ` Wang, Yipeng1 0 siblings, 1 reply; 8+ messages in thread From: Honnappa Nagarahalli @ 2021-11-09 19:46 UTC (permalink / raw) To: Medvedkin, Vladimir, dev Cc: thomas, Yipeng Wang, Sameh Gobriel, Bruce Richardson, nd, nd <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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-09 19:46 ` Honnappa Nagarahalli @ 2021-11-10 18:26 ` Wang, Yipeng1 0 siblings, 0 replies; 8+ messages in thread From: Wang, Yipeng1 @ 2021-11-10 18:26 UTC (permalink / raw) To: Honnappa Nagarahalli, Medvedkin, Vladimir, dev Cc: thomas, Gobriel, Sameh, Richardson, Bruce, nd, nd > -----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! ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-08 17:39 [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES Vladimir Medvedkin 2021-11-08 22:04 ` Honnappa Nagarahalli @ 2021-11-10 18:36 ` Vladimir Medvedkin 2021-11-10 19:16 ` Honnappa Nagarahalli 1 sibling, 1 reply; 8+ messages in thread From: Vladimir Medvedkin @ 2021-11-10 18:36 UTC (permalink / raw) To: dev Cc: Honnappa.Nagarahalli, thomas, Yipeng Wang, Sameh Gobriel, Bruce Richardson 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..eb2644f74b 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, multiple 'struct rte_hash_bucket' can be fit + * on a single cache line (64 or 128 bytes long) without any gaps + * in memory between them due to alignment. + */ #define RTE_HASH_BUCKET_ENTRIES 8 #if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-10 18:36 ` [PATCH v2] " Vladimir Medvedkin @ 2021-11-10 19:16 ` Honnappa Nagarahalli 2021-11-17 17:34 ` Thomas Monjalon 0 siblings, 1 reply; 8+ messages in thread From: Honnappa Nagarahalli @ 2021-11-10 19:16 UTC (permalink / raw) To: Vladimir Medvedkin, dev Cc: thomas, Yipeng Wang, Sameh Gobriel, Bruce Richardson, nd <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> Looks good Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.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..eb2644f74b 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, multiple 'struct rte_hash_bucket' can be fit > + * on a single cache line (64 or 128 bytes long) without any gaps > + * in memory between them due to alignment. > + */ > #define RTE_HASH_BUCKET_ENTRIES 8 > > #if !RTE_IS_POWER_OF_2(RTE_HASH_BUCKET_ENTRIES) > -- > 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES 2021-11-10 19:16 ` Honnappa Nagarahalli @ 2021-11-17 17:34 ` Thomas Monjalon 0 siblings, 0 replies; 8+ messages in thread From: Thomas Monjalon @ 2021-11-17 17:34 UTC (permalink / raw) To: Vladimir Medvedkin Cc: dev, Yipeng Wang, Sameh Gobriel, Bruce Richardson, nd, Honnappa Nagarahalli > > 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> > Looks good > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Applied, thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-17 17:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-08 17:39 [dpdk-dev] [PATCH] hash: clarify comments for RTE_HASH_BUCKET_ENTRIES Vladimir Medvedkin 2021-11-08 22:04 ` Honnappa Nagarahalli 2021-11-09 19:11 ` Medvedkin, Vladimir 2021-11-09 19:46 ` Honnappa Nagarahalli 2021-11-10 18:26 ` Wang, Yipeng1 2021-11-10 18:36 ` [PATCH v2] " Vladimir Medvedkin 2021-11-10 19:16 ` Honnappa Nagarahalli 2021-11-17 17:34 ` Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).