[v2,1/2] hash: fix bugs in 'free key with position'
Checks
Commit Message
This patch fixes 2 bugs-
1] Incorrect position returned to the free slots.
2] Incorrect computation of total_entries
Bugzilla ID: 261
Fixes: 9d033dac7d7c ("hash: support no free on delete")
Cc: honnappa.nagarahalli@arm.com
Cc: stable@dpdk.org
Reported-by: Linfan <zhongdahulinfan@163.com>
Suggested-by: Linfan <zhongdahulinfan@163.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
---
lib/librte_hash/rte_cuckoo_hash.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
Comments
09/05/2019 00:59, Dharmik Thakkar:
> This patch fixes 2 bugs-
> 1] Incorrect position returned to the free slots.
> 2] Incorrect computation of total_entries
Is it possible to split in 2 patches?
How critical is this bug? It looks old.
I'm afraid it will miss 19.05.
This bug makes 'lock free reader/writer concurrency hash' unusable.
There are two reasons:
1] memory leak because we cannot free keys which indexes greater than the number of total entries
2] the ring of free_slots may have unexpected key conflict with in use one
The patch fixes these 2 issues, both of which are in the same API:
int __rte_experimental
rte_hash_free_key_with_position(const struct rte_hash *h,
const int32_t position)
I don't think it is necessarily to split in 2 patches.
Thanks,
Linfan
At 2019-05-09 16:29:56, "Thomas Monjalon" <thomas@monjalon.net> wrote:
>09/05/2019 00:59, Dharmik Thakkar:
>> This patch fixes 2 bugs-
>> 1] Incorrect position returned to the free slots.
>> 2] Incorrect computation of total_entries
>
>Is it possible to split in 2 patches?
>
>How critical is this bug? It looks old.
>I'm afraid it will miss 19.05.
>
>
09/05/2019 12:40, 胡林帆:
> This bug makes 'lock free reader/writer concurrency hash' unusable.
> There are two reasons:
> 1] memory leak because we cannot free keys which indexes greater than the number of total entries
>
> 2] the ring of free_slots may have unexpected key conflict with in use one
>
> The patch fixes these 2 issues, both of which are in the same API:
>
> int __rte_experimental
> rte_hash_free_key_with_position(const struct rte_hash *h,
> const int32_t position)
>
> I don't think it is necessarily to split in 2 patches.
Sorry for insisting, I think it is necessary to split in 2 patches
with better explanations in the commit logs.
Then we'll need approval from the maintainers.
PS1: Please provide your full name in english alphabet
PS2: Please do not top-post
> At 2019-05-09 16:29:56, "Thomas Monjalon" <thomas@monjalon.net> wrote:
> >09/05/2019 00:59, Dharmik Thakkar:
> >> This patch fixes 2 bugs-
> >> 1] Incorrect position returned to the free slots.
> >> 2] Incorrect computation of total_entries
> >
> >Is it possible to split in 2 patches?
> >
> >How critical is this bug? It looks old.
> >I'm afraid it will miss 19.05.
> On May 9, 2019, at 6:25 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 09/05/2019 12:40, 胡林帆:
>> This bug makes 'lock free reader/writer concurrency hash' unusable.
>> There are two reasons:
>> 1] memory leak because we cannot free keys which indexes greater than the number of total entries
>>
>> 2] the ring of free_slots may have unexpected key conflict with in use one
>>
>> The patch fixes these 2 issues, both of which are in the same API:
>>
>> int __rte_experimental
>> rte_hash_free_key_with_position(const struct rte_hash *h,
>> const int32_t position)
>>
>> I don't think it is necessarily to split in 2 patches.
>
> Sorry for insisting, I think it is necessary to split in 2 patches
> with better explanations in the commit logs.
I will update the patch. Thanks!
> Then we'll need approval from the maintainers.
>
> PS1: Please provide your full name in english alphabet
> PS2: Please do not top-post
>
>
>> At 2019-05-09 16:29:56, "Thomas Monjalon" <thomas@monjalon.net> wrote:
>>> 09/05/2019 00:59, Dharmik Thakkar:
>>>> This patch fixes 2 bugs-
>>>> 1] Incorrect position returned to the free slots.
>>>> 2] Incorrect computation of total_entries
>>>
>>> Is it possible to split in 2 patches?
>>>
>>> How critical is this bug? It looks old.
>>> I'm afraid it will miss 19.05.
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
@@ -1587,14 +1587,19 @@ int __rte_experimental
rte_hash_free_key_with_position(const struct rte_hash *h,
const int32_t position)
{
- RETURN_IF_TRUE(((h == NULL) || (position == EMPTY_SLOT)), -EINVAL);
+ /* Key index where key is stored, adding the first dummy index*/
+ uint32_t key_idx = position + 1;
+
+ RETURN_IF_TRUE(((h == NULL) || (key_idx == EMPTY_SLOT)), -EINVAL);
unsigned int lcore_id, n_slots;
struct lcore_cache *cached_free_slots;
- const int32_t total_entries = h->num_buckets * RTE_HASH_BUCKET_ENTRIES;
+ const uint32_t total_entries = h->use_local_cache ?
+ h->entries + (RTE_MAX_LCORE - 1) * (LCORE_CACHE_SIZE - 1) + 1
+ : h->entries + 1;
/* Out of bounds */
- if (position >= total_entries)
+ if (key_idx >= total_entries)
return -EINVAL;
if (h->ext_table_support && h->readwrite_concur_lf_support) {
uint32_t index = h->ext_bkt_to_free[position];
@@ -1619,11 +1624,11 @@ rte_hash_free_key_with_position(const struct rte_hash *h,
}
/* Put index of new free slot in cache. */
cached_free_slots->objs[cached_free_slots->len] =
- (void *)((uintptr_t)position);
+ (void *)((uintptr_t)key_idx);
cached_free_slots->len++;
} else {
rte_ring_sp_enqueue(h->free_slots,
- (void *)((uintptr_t)position));
+ (void *)((uintptr_t)key_idx));
}
return 0;