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

["Wang, Yipeng1" <yipeng1.wang@intel.com>]

Reply inlined:

>-----Original Message-----
>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Honnappa Nagarahalli
>Sent: Thursday, September 6, 2018 10:12 AM
>To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>Cc: dev@dpdk.org; honnappa.nagarahalli@dpdk.org; gavin.hu@arm.com; steve.capper@arm.com; ola.liljedahl@arm.com;
>nd@arm.com; Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>Subject: [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.
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.
>+
> /** 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?

>  *
>  * @param h
>  *   Hash table to add the key to.
>@@ -165,6 +175,11 @@ rte_hash_add_key_data(struct rte_hash *h, const void *key, void *data);
>  * 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'.
>  *
>  * @param h
>  *   Hash table to add the key to.
>@@ -230,6 +245,12 @@ rte_hash_add_key_with_hash(struct rte_hash *h, const void *key, hash_sig_t sig);
>  * 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.
>@@ -241,6 +262,8 @@ rte_hash_add_key_with_hash(struct rte_hash *h, const void *key, hash_sig_t sig);
>  *   - 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(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.