Re: [dpdk-dev] [PATCH v3 2/7] hash: support do not recycle on delete

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

>-----Original Message-----
>From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
>Sent: Thursday, October 11, 2018 11:32 PM
>To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>Cc: dev@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>; honnappa.nagarahalli@arm.com; dharmik.thakkar@arm.com;
>gavin.hu@arm.com; nd@arm.com
>Subject: [PATCH v3 2/7] hash: support do not recycle on delete
>
>rte_hash_lookup_xxx APIs return the index of the element in
>the key store. Application(reader) can use that index to reference
>other data structures in its scope. Because of this, the
>index should not be recycled till the application completes
>using the index.
>RTE_HASH_EXTRA_FLAGS_RECYCLE_ON_DEL is introduced to support this.
>When this flag is enabled rte_hash_del_xxx APIs do not free the
>key-store index/internal memory associated with the deleted
>entry. The new API rte_hash_free_key_with_position should be called
>to free the key-store index/internal memory after calling
>rte_hash_del_xxx APIs.
>
> 	uint8_t ext_table_support;     /**< Enable extendable bucket table */
>+	uint8_t recycle_on_del;
>+	/**< If internal memory/key-store entry should be
>+	 * freed on calling the rte_hash_del_xxx APIs.
>+	 * If this is set, rte_hash_free_key_with_position must be
[Wang, Yipeng] If this is *not* set?

>+/** Flag to disable freeing of internal memory/indices on hash delete.
>+ * Refer to rte_hash_del_xxx APIs for more details.
>+ */
>+#define RTE_HASH_EXTRA_FLAGS_RECYCLE_ON_DEL 0x10
>+
[Wang, Yipeng] Maybe call it FREE_AFTER_DEL or NO_FREE_ON_DEL?  Recycle_on_del
Sounds like we do the recycle at the delete time, which is opposite to the meaning.

Change *recycle* to *free* to be consistent with the function API name.
I guess I suggested to use *recycle* at beginning, but as a second thought,
I think *free* is more user friendly than recycle. Recycle makes more sense to developers.
And you already use *free* for the function name.

> /**
>  * The type of hash value of a key.
>  * It should be a value of at least 32bit with fully random pattern.
>@@ -236,6 +243,10 @@ rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key, hash_sig_t
>  * and should only be called from one thread by default.
>  * Thread safety can be enabled by setting flag during
>  * table creation.
>+ * If RTE_HASH_EXTRA_FLAGS_RECYCLE_ON_DEL is enabled,
>+ * the hash library's internal memory/index will not be freed by this
>+ * API. rte_hash_free_key_with_position API must be called additionally
>+ * to free the internal memory/index associated with the key.
[Wang, Yipeng] Maybe more explicit on the use case for this flag: This behavior is useful
for multi-threading applications which may still have threads referencing the position after deletion
(or other words of which you think more clear).

Otherwise
Reviewed-by: Yipeng Wang <yipeng1.wang@intel.com>