[dpdk-dev] [PATCH 4/4] hash: enable lock-free reader-writer concurrency

Wang, Yipeng1 yipeng1.wang at intel.com
Tue Oct 2 01:54:40 CEST 2018


>-----Original Message-----
>From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli at arm.com]
>Sent: Sunday, September 30, 2018 9:11 PM
>To: Wang, Yipeng1 <yipeng1.wang at intel.com>; Richardson, Bruce <bruce.richardson at intel.com>; De Lara Guarch, Pablo
><pablo.de.lara.guarch at intel.com>
>Cc: dev at dpdk.org; Gavin Hu (Arm Technology China) <Gavin.Hu at arm.com>; Steve Capper <Steve.Capper at arm.com>; Ola Liljedahl
><Ola.Liljedahl at arm.com>; nd <nd at arm.com>; Gobriel, Sameh <sameh.gobriel at intel.com>
>Subject: RE: [dpdk-dev] [PATCH 4/4] hash: enable lock-free reader-writer concurrency
>
>> >
>> >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 implementation
>> 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 multiwriter
>> 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 comments 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/guides/prog_guide/hash_lib.html).
 
>
>> >+
>> > /** 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 existing
>> 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 indicate 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 
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 into an
>> >  *     array of user data. This value is unique for this key, and is the same
>> >  *     value that was returned when the key was added.
>> >+ *     When lock free concurrency is enabled, this value should be used
>> >+ *     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 instead 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 can call
>> the new deletion function, followed by Rte_hash_free_key_with_position later.
>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 memory. Enabling lock-free RW concurrency will enable this flag.
>User can enable this flag explicitly while not using lock-free RW concurrency as well.
[Wang, Yipeng] I am OK with either way. For flag, maybe we should call it RTE_HASH_EXTRA_FLAGS_RECYCLE _ON_DEL, since
The key-data pair index is recycled to be more specific. User should know that the index might be re-used by another write.
BTW, current flag is only 8 bit, as we specify more and more flags, maybe we should announce an API change to change it to 32bit
for next release.




More information about the dev mailing list