From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 96F9F374E for ; Tue, 2 Oct 2018 01:54:53 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Oct 2018 16:54:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,329,1534834800"; d="scan'208";a="88348487" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 01 Oct 2018 16:54:42 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 1 Oct 2018 16:54:41 -0700 Received: from fmsmsx151.amr.corp.intel.com ([169.254.7.87]) by fmsmsx156.amr.corp.intel.com ([169.254.13.194]) with mapi id 14.03.0319.002; Mon, 1 Oct 2018 16:54:41 -0700 From: "Wang, Yipeng1" To: Honnappa Nagarahalli , "Richardson, Bruce" , "De Lara Guarch, Pablo" CC: "dev@dpdk.org" , "Gavin Hu (Arm Technology China)" , Steve Capper , Ola Liljedahl , nd , "Gobriel, Sameh" Thread-Topic: [dpdk-dev] [PATCH 4/4] hash: enable lock-free reader-writer concurrency Thread-Index: AQHURgUYvD/59ef/50aa9CWNa3bnXaUFAShwgAVhzQCAAMU34A== Date: Mon, 1 Oct 2018 23:54:40 +0000 Message-ID: References: <1536253938-192391-1-git-send-email-honnappa.nagarahalli@arm.com> <1536253938-192391-5-git-send-email-honnappa.nagarahalli@arm.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjYwMzRlN2MtYWFhZi00ZGI1LWI2NjAtZjdjOTljYjg5YTk4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWXpKNndRWGlSMjIzT0hCWkdWNXZBZjdZaUpjM203dUlMOExzS0llbVB4XC81YWgycFkrOE0wekF4XC9KelN3dFRhIn0= x-originating-ip: [10.1.200.107] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 4/4] hash: enable lock-free reader-writer concurrency 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: , X-List-Received-Date: Mon, 01 Oct 2018 23:54:54 -0000 >-----Original Message----- >From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] >Sent: Sunday, September 30, 2018 9:11 PM >To: Wang, Yipeng1 ; Richardson, Bruce ; De Lara Guarch, Pablo > >Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) ; Stev= e Capper ; Ola Liljedahl >; nd ; Gobriel, Sameh >Subject: RE: [dpdk-dev] [PATCH 4/4] hash: enable lock-free reader-writer c= oncurrency > >> > >> >Add the flag to enable reader-writer concurrency during run time. The >> >rte_hash_del_xxx APIs do not free the keystore element when this flag >> >is enabled. Hence a new API, rte_hash_free_key_with_position, to free >> >the key store element is added. >> > >> >+/** Flag to support lock free reader writer concurrency */ #define >> >+RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF 0x08 >> [Wang, Yipeng] It would be good to indicate that the lockless implementa= tion >> works for single writer multiple readers. >Multi-writers are supported by using the rw-lock or transactional memory. = Essentially, we still have single writer. This patch works fine >with multi-writer as defined by ' MULTI_WRITER_ADD' flag. I have tested it= as well. I will enable this test case in V2. > >> Also, if people use a mix of the flags for example set both multiwriter = and LF >> flags, then I guess either we need to return an error or maybe multiwrit= er >> should have higher priority. Currently the RW_CONCURRENCY will assume >> MULTI_WRITER_ADD I think. >As mentioned above, multi-writer and LF combination is supported. Yes, RW_= CONCURRENCY currently assumes MULTI_WRITER_ADD. >I think we should separate them. [Wang, Yipeng] It would be great if you could just add a little bit more co= mments to both of the flags to be more specific on what Read write concurrency mean in both cases, just in case users got confused. You may also want to update the documentation later (https://doc.dpdk.org/g= uides/prog_guide/hash_lib.html). =20 > >> >+ >> > /** Signature of key that is stored internally. */ typedef uint32_t >> > hash_sig_t; >> > >> >@@ -143,6 +148,11 @@ rte_hash_count(const struct rte_hash *h); >> > * and should only be called from one thread by default. >> > * Thread safety can be enabled by setting flag during >> > * table creation. >> >+ * When lock free reader writer concurrency is enabled, >> >+ * if this API is called to update an existing entry, >> >+ * the application should free any memory allocated for >> >+ * previous 'data' only after all the readers have stopped >> >+ * using previous 'data'. >> [Wang, Yipeng] Could you be more specific on this description? >> When add_key API is called, the users do not know if it will update an e= xisting >> entry or inserting a new one, do they? >I think, it will depend on the application. The applications I have worked= on so far, added a hash entry as a result of receiving an event >and updated it on receiving another event. I can change the comments to in= dicate that the applications need to be aware of >add/update operations. [Wang, Yipeng] Even if for current rte_hash, after update, the application = may still use the old data. It is the upper level application's=20 Responsibility. How is it specific to lock free implementation? > >> > rte_hash_del_key(const struct rte_hash *h, const void *key); @@ -251,6 >> >+274,12 @@ rte_hash_del_key(const struct rte_hash *h, const void *key); >> > * and should only be called from one thread by default. >> > * Thread safety can be enabled by setting flag during >> > * table creation. >> >+ * If lock free reader writer concurrency is enabled, >> >+ * the hash library's internal memory for the deleted >> >+ * key is not freed. It should be freed by calling >> >+ * rte_hash_free_key_with_position API after all >> >+ * the readers have stopped using the hash entry >> >+ * corresponding to this key. >> > * >> > * @param h >> > * Hash table to remove the key from. >> >@@ -264,6 +293,8 @@ rte_hash_del_key(const struct rte_hash *h, const >> void *key); >> > * - A positive value that can be used by the caller as an offset in= to an >> > * array of user data. This value is unique for this key, and is t= he same >> > * value that was returned when the key was added. >> >+ * When lock free concurrency is enabled, this value should be use= d >> >+ * while calling the rte_hash_free_key_with_position API. >> > */ >> > int32_t >> > rte_hash_del_key_with_hash(const struct rte_hash *h, const void *key, >> >hash_sig_t sig); @@ -290,6 +321,30 @@ >> rte_hash_get_key_with_position(const struct rte_hash *h, const int32_t >> position, >> > void **key); >> > >> [Wang, Yipeng] If possible, how about having a new delete function inste= ad of >> modifying the current one? >> I think it does not need to be tied with the lockless implementation, it= is >> orthogonal to multi-threading implementation. >> people using locks may still want this new deletion behavior. >> If people want old behavior, they can call current API, otherwise they c= an call >> the new deletion function, followed by Rte_hash_free_key_with_position l= ater. >I like the terms 'delete' and 'free'. I am finding it hard to come up with= a good name for the API. It will be on the lines of >'rte_hash_del_key_with_hash_no_free' - I do not like the name much. >Instead, we could have a configuration flag for the hash table, 'RTE_HASH_= EXTRA_FLAGS_FREE_MEM_ON_DEL'. If this is enabled, >'rte_hash_del_...' APIs will free the key store index and any internal mem= ory. Enabling lock-free RW concurrency will enable this flag. >User can enable this flag explicitly while not using lock-free RW concurre= ncy as well. [Wang, Yipeng] I am OK with either way. For flag, maybe we should call it R= TE_HASH_EXTRA_FLAGS_RECYCLE _ON_DEL, since The key-data pair index is recycled to be more specific. User should know t= hat the index might be re-used by another write. BTW, current flag is only 8 bit, as we specify more and more flags, maybe w= e should announce an API change to change it to 32bit for next release.