From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 005A0A0487
for <public@inbox.dpdk.org>; Thu, 4 Jul 2019 02:39:19 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 3CEF41E20;
Thu, 4 Jul 2019 02:39:18 +0200 (CEST)
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
by dpdk.org (Postfix) with ESMTP id A05A51D9E;
Thu, 4 Jul 2019 02:39:15 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga006.fm.intel.com ([10.253.24.20])
by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
03 Jul 2019 17:39:14 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.63,449,1557212400"; d="scan'208";a="363205362"
Received: from orsmsx104.amr.corp.intel.com ([10.22.225.131])
by fmsmga006.fm.intel.com with ESMTP; 03 Jul 2019 17:39:13 -0700
Received: from orsmsx156.amr.corp.intel.com (10.22.240.22) by
ORSMSX104.amr.corp.intel.com (10.22.225.131) with Microsoft SMTP Server (TLS)
id 14.3.439.0; Wed, 3 Jul 2019 17:39:13 -0700
Received: from orsmsx104.amr.corp.intel.com ([169.254.4.70]) by
ORSMSX156.amr.corp.intel.com ([169.254.8.198]) with mapi id 14.03.0439.000;
Wed, 3 Jul 2019 17:39:13 -0700
From: "Wang, Yipeng1" <yipeng1.wang@intel.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>, "Gobriel, Sameh"
<sameh.gobriel@intel.com>, "Richardson, Bruce" <bruce.richardson@intel.com>,
"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
CC: "Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>, "Ruifeng Wang (Arm
Technology China)" <Ruifeng.Wang@arm.com>, "dev@dpdk.org" <dev@dpdk.org>, nd
<nd@arm.com>, "stable@dpdk.org" <stable@dpdk.org>, nd <nd@arm.com>
Thread-Topic: [PATCH 2/3] lib/hash: load pData after full key compare
Thread-Index: AQHVK5syNdL2m0zXvEuHE2Sc2GqGyKaxYNTwgAXcXYCAAmsW8A==
Date: Thu, 4 Jul 2019 00:39:12 +0000
Message-ID: <D2C4A16CA39F7F4E8E384D204491D7A673EC8D64@ORSMSX104.amr.corp.intel.com>
References: <20190625211520.43181-1-honnappa.nagarahalli@arm.com>
<20190625211520.43181-3-honnappa.nagarahalli@arm.com>
<D2C4A16CA39F7F4E8E384D204491D7A673EC7163@ORSMSX104.amr.corp.intel.com>
<VE1PR08MB5149554B9627B2ED79AA77EE98F80@VE1PR08MB5149.eurprd08.prod.outlook.com>
In-Reply-To: <VE1PR08MB5149554B9627B2ED79AA77EE98F80@VE1PR08MB5149.eurprd08.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
x-ctpclassification: CTP_NT
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDc0OGQ4MzItMTAxZC00OWM5LWIwOTgtZDQzYTZlNjM5YzZhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRG8zVG02cGNPQ0htNDliUWN5QnNcL2liVDBDN0VqK1N5SzM2dmxDS2Q0TnU2NE1PYngwYUZJMGtXOFpsUmo1eEEifQ==
x-originating-ip: [10.22.254.140]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 2/3] lib/hash: load pData after full key
compare
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
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]
>Sent: Monday, July 1, 2019 9:35 PM
>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: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Ruifeng Wang (Arm =
Technology China) <Ruifeng.Wang@arm.com>;
>dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@=
arm.com>; stable@dpdk.org; nd <nd@arm.com>
>Subject: RE: [PATCH 2/3] lib/hash: load pData after full key compare
>
>Thank you Yipeng for your comments.
>
>> >
>> >When a hash entry is added, there are 2 sets of stores.
>> >
>> >1) The application writes its data to memory (whose address is provided
>> >in rte_hash_add_key_with_hash_data API (or NULL))
>> >2) The rte_hash library writes to its own internal data structures; key
>> >store entry and the hash table.
>> >
>> >The only ordering requirement between these 2 is that - the store to
>> >the application data must complete before the store to key_index.
>> >There are no ordering requirements between the stores to the
>> >key/signature and store to application data. The synchronization point
>> >for application data can be any point between the 'store to application
>> >data' and 'store to the key_index'. So, pData should not be a guard
>> >variable for the data in hash table. It should be a guard variable only
>> >for the application data written to the memory location pointed by
>> >pData. Hence, pData can be loaded after full key comparison.
>> >
>> >Fixes: e605a1d36 ("hash: add lock-free r/w concurrency")
>> >Cc: stable@dpdk.org
>> >
>> >Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> >Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>> >Tested-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> >---
>> > lib/librte_hash/rte_cuckoo_hash.c | 67 +++++++++++++++----------------
>> > 1 file changed, 32 insertions(+), 35 deletions(-)
>> >
>> >diff --git a/lib/librte_hash/rte_cuckoo_hash.c
>> >b/lib/librte_hash/rte_cuckoo_hash.c
>> >index f37f6957d..077328fed 100644
>> >--- a/lib/librte_hash/rte_cuckoo_hash.c
>> >+++ b/lib/librte_hash/rte_cuckoo_hash.c
>> >@@ -649,9 +649,11 @@ search_and_update(const struct rte_hash *h, void
>> *data, const void *key,
>> > k =3D (struct rte_hash_key *) ((char *)keys +
>> > bkt->key_idx[i] * h->key_entry_size);
>> > if (rte_hash_cmp_eq(key, k->key, h) =3D=3D 0) {
>> >- /* 'pdata' acts as the synchronization point
>> >- * when an existing hash entry is updated.
>> >- * Key is not updated in this case.
>> >+ /* The store to application data at *data
>> >+ * should not leak after the store to pdata
>> >+ * in the key store. i.e. pdata is the guard
>> >+ * variable. Release the application data
>> >+ * to the readers.
>> > */
>> > __atomic_store_n(&k->pdata,
>> > data,
>> >@@ -711,11 +713,10 @@ rte_hash_cuckoo_insert_mw(const struct
>> rte_hash *h,
>> > /* Check if slot is available */
>> > if (likely(prim_bkt->key_idx[i] =3D=3D EMPTY_SLOT)) {
>> > prim_bkt->sig_current[i] =3D sig;
>> >- /* Key can be of arbitrary length, so it is
>> >- * not possible to store it atomically.
>> >- * Hence the new key element's memory stores
>> >- * (key as well as data) should be complete
>> >- * before it is referenced.
>> >+ /* Store to signature and key should not
>> >+ * leak after the store to key_idx. i.e.
>> >+ * key_idx is the guard variable for signature
>> >+ * and key.
>> > */
>> > __atomic_store_n(&prim_bkt->key_idx[i],
>> > new_idx,
>> >@@ -990,17 +991,15 @@ __rte_hash_add_key_with_hash(const struct
>> >rte_hash *h, const void *key,
>> >
>> > new_k =3D RTE_PTR_ADD(keys, (uintptr_t)slot_id * h->key_entry_size);
>> > new_idx =3D (uint32_t)((uintptr_t) slot_id);
>> >- /* Copy key */
>> >- memcpy(new_k->key, key, h->key_len);
>> >- /* Key can be of arbitrary length, so it is not possible to store
>> >- * it atomically. Hence the new key element's memory stores
>> >- * (key as well as data) should be complete before it is referenced.
>> >- * 'pdata' acts as the synchronization point when an existing hash
>> >- * entry is updated.
>> >+ /* The store to application data (by the application) at *data should
>> >+ * not leak after the store of pdata in the key store. i.e. pdata is
>> >+ * the guard variable. Release the application data to the readers.
>> > */
>> > __atomic_store_n(&new_k->pdata,
>> > data,
>> > __ATOMIC_RELEASE);
>> [Wang, Yipeng] Actually do we need to guard pdata for the newly inserted
>> key? I thought the guard of key_idx already can make sure The order for=
the
>> application to read data.
>Yes, you are correct. In the hash_add case, the store-release on key_idx w=
ould be sufficient. However, hash_update case requires
>store-release on pData. This was the reason to keep store-release for pDat=
a in hash_add when the lock-free algorithm was
>introduced.
[Wang, Yipeng] Sorry that I am still a bit confused, we already have store =
release in search_and_update function right? Isn't that enough
for the hash_update case?
>
>> >+ /* Copy key */
>> >+ memcpy(new_k->key, key, h->key_len);
>> [Wang, Yipeng] You don't need to do the order change just to show the po=
int
>> of unnecessary ordering I think.
>> I am afraid it may cause further confusion for future people who read th=
is
>> change, especially it is not in the commit Message (and it is a bug fix)=
.
>I made this change to keep it inline with the corresponding change in the =
lookup function. I can add this explanation to the commit
>message. Please let me know if this is ok for you.
[Wang, Yipeng] Thanks for the change.
To me it still looks unnecessary but If you think this cosmetic change woul=
d help others to understand the code better, I am OK with it.