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 08EF1A046B for ; Fri, 28 Jun 2019 20:40:31 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C5E5C1B9CD; Fri, 28 Jun 2019 20:40:30 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id AB4951B9B9; Fri, 28 Jun 2019 20:40:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Jun 2019 11:40:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,428,1557212400"; d="scan'208";a="170820942" Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by FMSMGA003.fm.intel.com with ESMTP; 28 Jun 2019 11:40:27 -0700 Received: from orsmsx157.amr.corp.intel.com (10.22.240.23) by ORSMSX105.amr.corp.intel.com (10.22.225.132) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 28 Jun 2019 11:40:26 -0700 Received: from orsmsx104.amr.corp.intel.com ([169.254.4.70]) by ORSMSX157.amr.corp.intel.com ([169.254.9.246]) with mapi id 14.03.0439.000; Fri, 28 Jun 2019 11:40:26 -0700 From: "Wang, Yipeng1" To: Honnappa Nagarahalli , "Gobriel, Sameh" , "Richardson, Bruce" , "De Lara Guarch, Pablo" CC: "gavin.hu@arm.com" , "ruifeng.wang@arm.com" , "dev@dpdk.org" , "nd@arm.com" , "stable@dpdk.org" Thread-Topic: [PATCH 2/3] lib/hash: load pData after full key compare Thread-Index: AQHVK5syNdL2m0zXvEuHE2Sc2GqGyKaxYNTw Date: Fri, 28 Jun 2019 18:40:26 +0000 Message-ID: References: <20190625211520.43181-1-honnappa.nagarahalli@arm.com> <20190625211520.43181-3-honnappa.nagarahalli@arm.com> In-Reply-To: <20190625211520.43181-3-honnappa.nagarahalli@arm.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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTgyOTE3MGQtOGFhYi00NGIzLWJkOWYtYjk3MzJhOGMwMDA0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiZUtCUnUrY1FWRGlLTHdvXC9ubkJGMlNxbjRZZWN4MnloK0J1cFppTmhJTVNPcTdwbTRSSmdvbzZmYVNCcVwvdjlOIn0= 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >-----Original Message----- >From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com] >Sent: Tuesday, June 25, 2019 2:15 PM >To: Wang, Yipeng1 ; Gobriel, Sameh ; Richardson, Bruce >; De Lara Guarch, Pablo ; honnappa.nagarahalli@arm.com >Cc: gavin.hu@arm.com; ruifeng.wang@arm.com; dev@dpdk.org; nd@arm.com; stab= le@dpdk.org >Subject: [PATCH 2/3] lib/hash: load pData after full key compare > >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_cucko= o_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 *dat= a, 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 ke= y? I thought the guard of key_idx already can make sure The order for the application to read data.=20 >+ /* 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 point= of unnecessary ordering I think. I am afraid it may cause further confusion for future people who read this = change, especially it is not in the commit Message (and it is a bug fix).=20 >