diff mbox

[dpdk-dev,1/7] Use an accessor for rte_hash_key

Message ID 1503086972-32649-1-git-send-email-mike.stolarchuk@bigswitch.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

mstolarchuk Aug. 18, 2017, 8:09 p.m. UTC 
  Improves consistency, allows identifcation of use-sites

Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 64 +++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 33 deletions(-)

Comments

Thomas Monjalon Oct. 17, 2017, 1:59 p.m. UTC | #1 
18/08/2017 22:09, mstolarchuk:
> Improves consistency, allows identifcation of use-sites
> 
> Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com>

Any comment on this patch and others from the same author?
De Lara Guarch, Pablo Oct. 19, 2017, 8:10 a.m. UTC | #2 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, October 17, 2017 2:59 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; mstolarchuk <mike.stolarchuk@bigswitch.com>
> Subject: Re: [dpdk-dev] [PATCH 1/7] Use an accessor for rte_hash_key
> 
> 18/08/2017 22:09, mstolarchuk:
> > Improves consistency, allows identifcation of use-sites
> >
> > Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com>
> 
> Any comment on this patch and others from the same author?

Hi,

Two of the patches submitted are actually the same, although they have a different enumeration.
The patches look like they were part of two different patchsets, so it doesn't look right.
Also, patch 6/6 is not applicable anymore, as a similar fix was sent previously.

Mike, could you send another patchset, that is rebased on top of the latest code?

Thanks,
Pablo
mstolarchuk Oct. 27, 2017, 3 p.m. UTC | #3 
Pablo,

Also, what about the other patch?
the use of a static variable in a recursive call?
obviously incorrect for a threaded environment ... has that been accepted?

regards,
mts.



On Thu, Oct 19, 2017 at 4:10 AM, De Lara Guarch, Pablo <
pablo.de.lara.guarch@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Tuesday, October 17, 2017 2:59 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch,
> > Pablo <pablo.de.lara.guarch@intel.com>
> > Cc: dev@dpdk.org; mstolarchuk <mike.stolarchuk@bigswitch.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/7] Use an accessor for rte_hash_key
> >
> > 18/08/2017 22:09, mstolarchuk:
> > > Improves consistency, allows identifcation of use-sites
> > >
> > > Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com>
> >
> > Any comment on this patch and others from the same author?
>
> Hi,
>
> Two of the patches submitted are actually the same, although they have a
> different enumeration.
> The patches look like they were part of two different patchsets, so it
> doesn't look right.
> Also, patch 6/6 is not applicable anymore, as a similar fix was sent
> previously.
>
> Mike, could you send another patchset, that is rebased on top of the
> latest code?
>
> Thanks,
> Pablo
>
De Lara Guarch, Pablo Oct. 27, 2017, 3:31 p.m. UTC | #4 
> From: Mike Stolarchuk [mailto:mike.stolarchuk@bigswitch.com]

> Sent: Friday, October 27, 2017 4:00 PM

> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>

> Cc: Thomas Monjalon <thomas@monjalon.net>; Richardson, Bruce

> <bruce.richardson@intel.com>; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 1/7] Use an accessor for rte_hash_key

> 

> Pablo,

> 

> Also, what about the other patch?

> the use of a static variable in a recursive call? obviously incorrect for a

> threaded environment ... has that been accepted?


Hi Mike,

Yes, that patch was accepted.

Thanks,
Pablo

> 

> regards,

> mts.

> 

> 

> 

> On Thu, Oct 19, 2017 at 4:10 AM, De Lara Guarch, Pablo

> <pablo.de.lara.guarch@intel.com> wrote:

> 

> 

> > -----Original Message-----

> > From: Thomas Monjalon [mailto:thomas@monjalon.net]

> > Sent: Tuesday, October 17, 2017 2:59 PM

> > To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch,

> > Pablo <pablo.de.lara.guarch@intel.com>

> > Cc: dev@dpdk.org; mstolarchuk <mike.stolarchuk@bigswitch.com>

> > Subject: Re: [dpdk-dev] [PATCH 1/7] Use an accessor for rte_hash_key

> >

> > 18/08/2017 22:09, mstolarchuk:

> > > Improves consistency, allows identifcation of use-sites

> > >

> > > Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com>

> >

> > Any comment on this patch and others from the same author?

> Hi,

> 

> Two of the patches submitted are actually the same, although they have a

> different enumeration.

> The patches look like they were part of two different patchsets, so it doesn't

> look right.

> Also, patch 6/6 is not applicable anymore, as a similar fix was sent

> previously.

> 

> Mike, could you send another patchset, that is rebased on top of the latest

> code?

> 

> Thanks,

> Pablo
Ferruh Yigit March 1, 2019, 5:15 p.m. UTC | #5 
On 10/27/2017 4:31 PM, pablo.de.lara.guarch at intel.com (De Lara Guarch, Pablo)
wrote:
>> From: Mike Stolarchuk [mailto:mike.stolarchuk at bigswitch.com]
>> Sent: Friday, October 27, 2017 4:00 PM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>
>> Cc: Thomas Monjalon <thomas at monjalon.net>; Richardson, Bruce
>> <bruce.richardson at intel.com>; dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/7] Use an accessor for rte_hash_key
>>
>> Pablo,
>>
>> Also, what about the other patch?
>> the use of a static variable in a recursive call? obviously incorrect for a
>> threaded environment ... has that been accepted?
> 
> Hi Mike,
> 
> Yes, that patch was accepted.

Hi Pablo, Mike,

The patch is remaining from 2017 and sitting on the patchwork without any comment.

I am marking the patchset as rejected, if they are still relevant please send a
new version on top of latest repo.

Sorry for any inconvenience caused.

For reference patches:
https://patches.dpdk.org/patch/27668/
https://patches.dpdk.org/patch/27669/
https://patches.dpdk.org/patch/27670/
https://patches.dpdk.org/patch/27671/

> 
> Thanks,
> Pablo
> 
>>
>> regards,
>> mts.
>>
>>
>>
>> On Thu, Oct 19, 2017 at 4:10 AM, De Lara Guarch, Pablo
>> <pablo.de.lara.guarch at intel.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Thomas Monjalon [mailto:thomas at monjalon.net]
>>> Sent: Tuesday, October 17, 2017 2:59 PM
>>> To: Richardson, Bruce <bruce.richardson at intel.com>; De Lara Guarch,
>>> Pablo <pablo.de.lara.guarch at intel.com>
>>> Cc: dev at dpdk.org; mstolarchuk <mike.stolarchuk at bigswitch.com>
>>> Subject: Re: [dpdk-dev] [PATCH 1/7] Use an accessor for rte_hash_key
>>>
>>> 18/08/2017 22:09, mstolarchuk:
>>>> Improves consistency, allows identifcation of use-sites
>>>>
>>>> Signed-off-by: mstolarchuk <mike.stolarchuk at bigswitch.com>
>>>
>>> Any comment on this patch and others from the same author?
>> Hi,
>>
>> Two of the patches submitted are actually the same, although they have a
>> different enumeration.
>> The patches look like they were part of two different patchsets, so it doesn't
>> look right.
>> Also, patch 6/6 is not applicable anymore, as a similar fix was sent
>> previously.
>>
>> Mike, could you send another patchset, that is rebased on top of the latest
>> code?
>>
>> Thanks,
>> Pablo
> 
>
diff mbox

Patch

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 87b25c0..d1fbb0b 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -500,6 +500,20 @@  struct rte_hash *
 		rte_ring_sp_enqueue(h->free_slots, slot_id);
 }
 
+/*
+ * Function to compute the location of rte_hash_key from an index
+ */
+static inline struct rte_hash_key *
+__rte_hash_key_from_idx(const struct rte_hash *h, const uint32_t idx)
+{
+	/* private structure only to compute rte_hash_key offset */
+	struct key_entry {
+	    char element_size[h->key_entry_size];
+	} __attribute__((packed)) *key_store = h->key_store;
+
+	return (struct rte_hash_key *)(key_store + idx);
+}
+
 static inline int32_t
 __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key,
 						hash_sig_t sig, void *data)
@@ -508,7 +522,7 @@  struct rte_hash *
 	uint32_t prim_bucket_idx, sec_bucket_idx;
 	unsigned i;
 	struct rte_hash_bucket *prim_bkt, *sec_bkt;
-	struct rte_hash_key *new_k, *k, *keys = h->key_store;
+	struct rte_hash_key *new_k, *k;
 	void *slot_id = NULL;
 	uint32_t new_idx;
 	int ret;
@@ -556,7 +570,7 @@  struct rte_hash *
 		}
 	}
 
-	new_k = RTE_PTR_ADD(keys, (uintptr_t)slot_id * h->key_entry_size);
+	new_k = __rte_hash_key_from_idx(h, (uintptr_t)slot_id);
 	rte_prefetch0(new_k);
 	new_idx = (uint32_t)((uintptr_t) slot_id);
 
@@ -564,8 +578,7 @@  struct rte_hash *
 	for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
 		if (prim_bkt->sig_current[i] == sig &&
 				prim_bkt->sig_alt[i] == alt_hash) {
-			k = (struct rte_hash_key *) ((char *)keys +
-					prim_bkt->key_idx[i] * h->key_entry_size);
+			k = __rte_hash_key_from_idx(h, prim_bkt->key_idx[i]);
 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
@@ -584,8 +597,7 @@  struct rte_hash *
 	for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
 		if (sec_bkt->sig_alt[i] == sig &&
 				sec_bkt->sig_current[i] == alt_hash) {
-			k = (struct rte_hash_key *) ((char *)keys +
-					sec_bkt->key_idx[i] * h->key_entry_size);
+			k = __rte_hash_key_from_idx(h, sec_bkt->key_idx[i]);
 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				/* Enqueue index of free slot back in the ring. */
 				enqueue_slot_back(h, cached_free_slots, slot_id);
@@ -711,6 +723,7 @@  struct rte_hash *
 	else
 		return ret;
 }
+
 static inline int32_t
 __rte_hash_lookup_with_hash(const struct rte_hash *h, const void *key,
 					hash_sig_t sig, void **data)
@@ -719,7 +732,7 @@  struct rte_hash *
 	hash_sig_t alt_hash;
 	unsigned i;
 	struct rte_hash_bucket *bkt;
-	struct rte_hash_key *k, *keys = h->key_store;
+	struct rte_hash_key *k;
 
 	bucket_idx = sig & h->bucket_bitmask;
 	bkt = &h->buckets[bucket_idx];
@@ -728,8 +741,7 @@  struct rte_hash *
 	for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
 		if (bkt->sig_current[i] == sig &&
 				bkt->key_idx[i] != EMPTY_SLOT) {
-			k = (struct rte_hash_key *) ((char *)keys +
-					bkt->key_idx[i] * h->key_entry_size);
+			k = __rte_hash_key_from_idx(h, bkt->key_idx[i]);
 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
@@ -751,8 +763,7 @@  struct rte_hash *
 	for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
 		if (bkt->sig_current[i] == alt_hash &&
 				bkt->sig_alt[i] == sig) {
-			k = (struct rte_hash_key *) ((char *)keys +
-					bkt->key_idx[i] * h->key_entry_size);
+			k = __rte_hash_key_from_idx(h, bkt->key_idx[i]);
 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				if (data != NULL)
 					*data = k->pdata;
@@ -835,7 +846,7 @@  struct rte_hash *
 	hash_sig_t alt_hash;
 	unsigned i;
 	struct rte_hash_bucket *bkt;
-	struct rte_hash_key *k, *keys = h->key_store;
+	struct rte_hash_key *k;
 	int32_t ret;
 
 	bucket_idx = sig & h->bucket_bitmask;
@@ -845,8 +856,7 @@  struct rte_hash *
 	for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
 		if (bkt->sig_current[i] == sig &&
 				bkt->key_idx[i] != EMPTY_SLOT) {
-			k = (struct rte_hash_key *) ((char *)keys +
-					bkt->key_idx[i] * h->key_entry_size);
+			k = __rte_hash_key_from_idx(h, bkt->key_idx[i]);
 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
@@ -870,8 +880,7 @@  struct rte_hash *
 	for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) {
 		if (bkt->sig_current[i] == alt_hash &&
 				bkt->key_idx[i] != EMPTY_SLOT) {
-			k = (struct rte_hash_key *) ((char *)keys +
-					bkt->key_idx[i] * h->key_entry_size);
+			k = __rte_hash_key_from_idx(h,  bkt->key_idx[i]);
 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
 				remove_entry(h, bkt, i);
 
@@ -910,9 +919,7 @@  struct rte_hash *
 {
 	RETURN_IF_TRUE(((h == NULL) || (key == NULL)), -EINVAL);
 
-	struct rte_hash_key *k, *keys = h->key_store;
-	k = (struct rte_hash_key *) ((char *) keys + (position + 1) *
-				     h->key_entry_size);
+	struct rte_hash_key *k = __rte_hash_key_from_idx(h, position + 1);
 	*key = k->key;
 
 	if (position !=
@@ -1037,9 +1044,7 @@  struct rte_hash *
 			uint32_t first_hit = __builtin_ctzl(prim_hitmask[i]);
 			uint32_t key_idx = primary_bkt[i]->key_idx[first_hit];
 			const struct rte_hash_key *key_slot =
-				(const struct rte_hash_key *)(
-				(const char *)h->key_store +
-				key_idx * h->key_entry_size);
+				__rte_hash_key_from_idx(h, key_idx);
 			rte_prefetch0(key_slot);
 			continue;
 		}
@@ -1048,9 +1053,7 @@  struct rte_hash *
 			uint32_t first_hit = __builtin_ctzl(sec_hitmask[i]);
 			uint32_t key_idx = secondary_bkt[i]->key_idx[first_hit];
 			const struct rte_hash_key *key_slot =
-				(const struct rte_hash_key *)(
-				(const char *)h->key_store +
-				key_idx * h->key_entry_size);
+				__rte_hash_key_from_idx(h, key_idx);
 			rte_prefetch0(key_slot);
 		}
 	}
@@ -1063,9 +1066,7 @@  struct rte_hash *
 
 			uint32_t key_idx = primary_bkt[i]->key_idx[hit_index];
 			const struct rte_hash_key *key_slot =
-				(const struct rte_hash_key *)(
-				(const char *)h->key_store +
-				key_idx * h->key_entry_size);
+				__rte_hash_key_from_idx(h, key_idx);
 			/*
 			 * If key index is 0, do not compare key,
 			 * as it is checking the dummy slot
@@ -1086,9 +1087,7 @@  struct rte_hash *
 
 			uint32_t key_idx = secondary_bkt[i]->key_idx[hit_index];
 			const struct rte_hash_key *key_slot =
-				(const struct rte_hash_key *)(
-				(const char *)h->key_store +
-				key_idx * h->key_entry_size);
+				__rte_hash_key_from_idx(h, key_idx);
 			/*
 			 * If key index is 0, do not compare key,
 			 * as it is checking the dummy slot
@@ -1170,8 +1169,7 @@  struct rte_hash *
 
 	/* Get position of entry in key table */
 	position = h->buckets[bucket_idx].key_idx[idx];
-	next_key = (struct rte_hash_key *) ((char *)h->key_store +
-				position * h->key_entry_size);
+	next_key = __rte_hash_key_from_idx(h, position);
 	/* Return key and data */
 	*key = next_key->key;
 	*data = next_key->pdata;