Re: [dpdk-dev] [PATCH v3 1/3] hash: fix position bug in 'free key with position'

[Dharmik Thakkar <Dharmik.Thakkar@arm.com>]

> On May 9, 2019, at 10:48 AM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
>> -----Original Message-----
>> From: Dharmik Thakkar [mailto:dharmik.thakkar@arm.com]
>> Sent: Thursday, May 9, 2019 6:39 AM
>> To: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh
>> <sameh.gobriel@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
>> De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>> Cc: dev@dpdk.org; honnappa.nagarahalli@arm.com;
>> zhongdahulinfan@163.com; Dharmik Thakkar <dharmik.thakkar@arm.com>;
>> stable@dpdk.org
>> Subject: [PATCH v3 1/3] hash: fix position bug in 'free key with position'
>> 
>> Currently, in rte_hash_free_key_with_position(), the position returned to the
>> ring of free_slots leads to an unexpected conflict with a key already in use.
>> 
>> This patch fixes incorrect position returned to the ring of free_slots.
>> 
>> 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 | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/librte_hash/rte_cuckoo_hash.c
>> b/lib/librte_hash/rte_cuckoo_hash.c
>> index 261267b7fd3d..8646ca52e60b 100644
>> --- a/lib/librte_hash/rte_cuckoo_hash.c
>> +++ b/lib/librte_hash/rte_cuckoo_hash.c
>> @@ -1587,14 +1587,17 @@ 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;
>> 
>> 	/* Out of bounds */
>> -	if (position >= total_entries)
>> +	if (key_idx >= total_entries)
> [Wang, Yipeng] 
> Compiling fail after this commit.
> /rte_cuckoo_hash.c:1600:14: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>  if (key_idx >= total_entries)
> please fix.
Thank you for reporting! I have fixed the issue and updated the patch.
> 
> Please run the test-build and test-meson-build scripts. I currently have some issue with my meson setup on my new system so I will
> rely on other reviewers or you guys to pass the meson test.
I ran the scripts on my setup. Thanks!
> 
> Logically these two bug fixes are valid and should be upstreamed otherwise this API breaks. But this is the experimental tag for :)
> 
> Thanks
> Yipeng