From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Wang, Yipeng1" <yipeng1.wang@intel.com>
Subject: Re: [PATCH v4 1/4] hash: fix race condition in iterate
Date: Tue, 2 Oct 2018 00:17:59 +0000
Message-ID: <D2C4A16CA39F7F4E8E384D204491D7A6614EA321@FMSMSX151.amr.corp.intel.com>
References: <1537993618-92630-1-git-send-email-yipeng1.wang@intel.com>
 <1538155426-145177-1-git-send-email-yipeng1.wang@intel.com>
 <1538155426-145177-2-git-send-email-yipeng1.wang@intel.com>
 <AM6PR08MB3672BF2941A02884C656AD4698EF0@AM6PR08MB3672.eurprd08.prod.outlook.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "dev@dpdk.org"
 <dev@dpdk.org>, "Gobriel, Sameh" <sameh.gobriel@intel.com>, nd <nd@arm.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>, "Richardson, Bruce"
 <bruce.richardson@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id 993965323
 for <dev@dpdk.org>; Tue,  2 Oct 2018 02:18:26 +0200 (CEST)
In-Reply-To: <AM6PR08MB3672BF2941A02884C656AD4698EF0@AM6PR08MB3672.eurprd08.prod.outlook.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

>-----Original Message-----
>From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
>> @@ -1317,16 +1317,19 @@ rte_hash_iterate(const struct rte_hash *h, const
>> void **key, void **data, uint32
>>  	bucket_idx =3D *next / RTE_HASH_BUCKET_ENTRIES;
>>  	idx =3D *next % RTE_HASH_BUCKET_ENTRIES;
>>
>> +	__hash_rw_reader_lock(h);
>This does not work well with the lock-less changes I am making.  We should=
 leave the lock in its original position. Instead change the
>while loop as follows:
>
>while ((position =3D h->buckets[bucket_idx].key_idx[idx]) =3D=3D EMPTY_SLO=
T)
>
>>  	/* If current position is empty, go to the next one */
>>  	while (h->buckets[bucket_idx].key_idx[idx] =3D=3D EMPTY_SLOT) {
>>  		(*next)++;
>>  		/* End of table */
>> -		if (*next =3D=3D total_entries)
>> +		if (*next =3D=3D total_entries) {
>> +			__hash_rw_reader_unlock(h);
>>  			return -ENOENT;
>> +		}
>>  		bucket_idx =3D *next / RTE_HASH_BUCKET_ENTRIES;
>>  		idx =3D *next % RTE_HASH_BUCKET_ENTRIES;
>>  	}
>> -	__hash_rw_reader_lock(h);
>> +
>>  	/* Get position of entry in key table */
>>  	position =3D h->buckets[bucket_idx].key_idx[idx];
>If we change the while loop as I suggested as above, we can remove this li=
ne.
>
>>  	next_key =3D (struct rte_hash_key *) ((char *)h->key_store +

[Wang, Yipeng] Sorry that I did not realize you already have it in your pat=
ch set and I agree.
Do you want to export it as a bug fix in your patch set? I will remove my c=
hange.

For the lock free, do we need to protect it with version counter? Imagine t=
he following corner case:
While the iterator read the key and data, there is a writer deleted, remove=
d, and recycled the key-data pair,
and write a new key and data into it. While the writer are writing, will th=
e reader reads out wrong key/data, or
mismatched key/data?