Re: [dpdk-dev] [PATCH v1 3/3] test/hash: add readwrite test for ext table

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

> > > On Wed, Oct 10, 2018 at 02:48:05PM -0700, Yipeng Wang wrote:
> > > > This commit improves the readwrite test to consider extendable
> > > > table feature and add more functional tests to cover more corner cases.
> > > >
> > > > Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com> ---
> > > > test/test/test_hash_readwrite.c | 70
> > > > ++++++++++++++++++++++++++++++++++------- 1 file changed, 58
> > > > insertions(+), 12 deletions(-)
> > > >
> > > With the extension of this test case, and the addition of other test
> > > cases by Honnappa in the other patch sets in this release, we are
> > > building up quite a large set of hash table autotests, some of whose
> > > meaning and use is a bit obscure. Are there any hash tests that you
> > > feel could be removed at this point, to simplify things?
> > >
> > (this comment does not apply to this patch) Looks like your concern is
> > about maintenance of the test code.
> > IMO, we need to reduce the number of configuration flags in this library
> which should reduce the number of test cases.
> > The flags I think that are not necessary are:
> > RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT - The tests prove that
> this gives significant performance boost. IMO, if the platform supports it, it
> should be enabled without user consent (I am not an expert on TSX).
> > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY - Most use cases require this
> support. Only use case where this is not required is a single thread doing both
> inserts and lookups. Even if such a use case is valid, the lock over head should
> be small.
> >
> I agree with the idea. What I suggest is that only a single flag should be
> needed, and that only for the uncommon case, i.e. where we do not need any
> locking of the hash-table. Otherwise the hash should be thread safe by default
> and using the most effective locking mechanism for the platform.
> 
RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF - should take care of this.

> Unfortunately, doing this requires an ABI change, but since it only should
> affect the create function, it should be doable with function versioning to
> keep backward compatibility.
> 
Looks simple enough. Version the rte_hash_create function and map the existing function to 18.08. The new version of the function will always enable hw_trans_mem_support and rw_concurrency. Should we check to see if these flags are set by the user and print a warning message about deprecation of these flags in the newer version of the function?

> /Bruce