[dpdk-dev,1/7] Use an accessor for rte_hash_key
Checks
Commit Message
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
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?
> -----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
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
>
> 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
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
>
>
@@ -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;