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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> >
> >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?
Agree.

> 
> >+/** 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.
Will change it to 'NO_FREE_ON_DEL'. It will be disabled by default. Will be enabled by default for RW-Concurrency-LF.

> 
> > /**
> >  * 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).
I don't think we should address any particular use case here as there can be many which we might not have imagined. IMO, it is better to do this in documentation where we can talk about use cases (that we know and want to address) and how to use these APIs.

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