From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2F03AA0487 for ; Tue, 2 Jul 2019 06:35:13 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 67A077CBC; Tue, 2 Jul 2019 06:35:12 +0200 (CEST) Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80077.outbound.protection.outlook.com [40.107.8.77]) by dpdk.org (Postfix) with ESMTP id 15EB04C8F; Tue, 2 Jul 2019 06:35:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=RA4j/N5SfLNgphMjjXjJDXt1GGdf9tW/jTMNfJ1pGnQ=; b=6mm5WmKtCvF335UgGNuFTD7qU/cCOiRPXLkPLDrAQPEpfXMAvouw+m5bLU+B9Izjzvnf9YM+163lJAGTJ+E6tOdGtRIPP/LeQqHHYy+rjEv5EgRVMIREReAwONBKZi0iPVzX9WTijZ5CAy77SjwgDi7CsQn1pTbs8T0eTBHXmKE= Received: from VE1PR08MB5149.eurprd08.prod.outlook.com (20.179.30.152) by VE1PR08MB5181.eurprd08.prod.outlook.com (20.179.31.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2032.20; Tue, 2 Jul 2019 04:35:09 +0000 Received: from VE1PR08MB5149.eurprd08.prod.outlook.com ([fe80::a89e:33:fbda:ed35]) by VE1PR08MB5149.eurprd08.prod.outlook.com ([fe80::a89e:33:fbda:ed35%4]) with mapi id 15.20.2032.019; Tue, 2 Jul 2019 04:35:09 +0000 From: Honnappa Nagarahalli To: "Wang, Yipeng1" , "Gobriel, Sameh" , "Richardson, Bruce" , "De Lara Guarch, Pablo" CC: "Gavin Hu (Arm Technology China)" , "Ruifeng Wang (Arm Technology China)" , "dev@dpdk.org" , Honnappa Nagarahalli , nd , "stable@dpdk.org" , nd Thread-Topic: [PATCH 2/3] lib/hash: load pData after full key compare Thread-Index: AQHVLeD2jHh9aL8rKU6eVhpBi8s9w6a2wHwA Date: Tue, 2 Jul 2019 04:35:09 +0000 Message-ID: References: <20190625211520.43181-1-honnappa.nagarahalli@arm.com> <20190625211520.43181-3-honnappa.nagarahalli@arm.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: 1791873b-947b-440a-bbb3-1dd3e4188638.0 x-checkrecipientchecked: true authentication-results: spf=none (sender IP is ) smtp.mailfrom=Honnappa.Nagarahalli@arm.com; x-originating-ip: [217.140.111.135] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: e039cd1b-67cb-441c-06c0-08d6fea6aa7a x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(4618075)(2017052603328)(7193020); SRVR:VE1PR08MB5181; x-ms-traffictypediagnostic: VE1PR08MB5181: x-microsoft-antispam-prvs: nodisclaimer: True x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 008663486A x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(346002)(39860400002)(396003)(376002)(366004)(136003)(199004)(189003)(102836004)(9686003)(53936002)(8676002)(76176011)(55016002)(99286004)(3846002)(71190400001)(66476007)(6436002)(71200400001)(26005)(6246003)(8936002)(81156014)(81166006)(186003)(229853002)(6116002)(7696005)(316002)(14454004)(6506007)(25786009)(5660300002)(110136005)(4326008)(476003)(478600001)(446003)(72206003)(73956011)(486006)(66066001)(256004)(14444005)(74316002)(11346002)(66556008)(86362001)(68736007)(54906003)(64756008)(2906002)(66446008)(76116006)(7736002)(66946007)(305945005)(33656002)(52536014); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR08MB5181; H:VE1PR08MB5149.eurprd08.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: pAG7we7Yqj24MJLmo+UpZ5g9dnVk+2ePsKs3s3l57OKTYGSfq8vTf+sXVuRxwVzj/dbggIJg23y0u+3WJG0nc1RrRUmIkhCO+k85o+QkGIxYuaoyarlaS52zssB3n34uiYHVlB9lMuMZQ3nC4ZU7h5ZFACW5Yqo7iJcR0/paZZtOLsX601hEFXTwAMCmot52l3ZxDHLRJJMD3HjG0PDc7nradX48HZog+iGgrtBKM/YSknndbjxvGfOKeFjZlbfh/gvxDwgyINsilgctVLTOuR4AZqy9PsciQiFBVXZGeEDOYglGMVWJ2HdrX6P9IfLuMj+Ij1UtYfS/Zg8wamd8B8APLW9l0prxmacRliSg4ExQyOuKhVPW0E5FRizBBHWqCRUvPOw/otb/WYdQtCsKFbVf3F+gwW3NHhBp8eywkBg= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: e039cd1b-67cb-441c-06c0-08d6fea6aa7a X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Jul 2019 04:35:09.5745 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Honnappa.Nagarahalli@arm.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR08MB5181 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > >Reviewed-by: Gavin Hu > >Tested-by: Ruifeng Wang > >--- > > 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 wo= uld be sufficient. However, hash_update case requires store-release on pDat= a. This was the reason to keep store-release for pData in hash_add when the= lock-free algorithm was introduced. > >+ /* 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 poi= nt > of unnecessary ordering I think. > I am afraid it may cause further confusion for future people who read thi= s > 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 l= ookup function. I can add this explanation to the commit message. Please le= t me know if this is ok for you. > >