Re: [dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Nov 01, 2018 at 07:43:29PM +0000, Honnappa Nagarahalli wrote:
> > >
> > > diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> > > b/lib/librte_hash/rte_cuckoo_hash.c
> > > index 5ddcccd87..a11de22be 100644
> > > --- a/lib/librte_hash/rte_cuckoo_hash.c
> > > +++ b/lib/librte_hash/rte_cuckoo_hash.c
> > > @@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h,  }
> > >
> > >  struct rte_hash *
> > > -rte_hash_create(const struct rte_hash_parameters *params)
> > > +rte_hash_create_v20(const struct rte_hash_parameters *params)
> > >  {
> > >  	struct rte_hash *h = NULL;
> > >  	struct rte_tailq_entry *te = NULL;
> > > @@ -446,6 +446,323 @@ rte_hash_create(const struct
> > rte_hash_parameters *params)
> > >  	rte_free(tbl_chng_cnt);
> > >  	return NULL;
> > >  }
> > > +VERSION_SYMBOL(rte_hash_create, _v20, 2.0);
> > > +
> > 
> > To make reviewing this easier, can I ask if in V2 you can put the v18.11
> > function first, before the existing one. Hopefully that means that the "new"
> > function from git's point of view is the existing one, showing it as a block
> > add that we can pretty much skip reviewing. The benefit of this is that the
> > changes in the v1811 should then show as line-by-line diffs in the patch, so
> > we can easily review the changes made in the new case. It's hard to spot
> > them in the whole function below.
> > 
> I agree it is painful to review. I tried what you suggested here, it still shows v18.11 function as a complete new block of code. I will create another commit in the series. The first commit will have just the exact duplication of the function (but renamed to v18.11). The next commit will have all the required changes for the v18.11 function and deprecation related changes. We can squash both of them once the review completes.
> Let me know if there is a better solution.

Thanks. I'll leave the squashing up to Thomas to decide, because even on
later review e.g. if we ever need to bisect, having the commits separate
could help.

> 
> > > +struct rte_hash *
> > > +rte_hash_create_v1811(const struct rte_hash_parameters *params) {
> > > +	struct rte_hash *h = NULL;
> > > +	struct rte_tailq_entry *te = NULL;
> > > +	struct rte_hash_list *hash_list;
> > > +	struct rte_ring *r = NULL;
> > > +	struct rte_ring *r_ext = NULL;
> > > +	char hash_name[RTE_HASH_NAMESIZE];
> > > +	void *k = NULL;
> > > +	void *buckets = NULL;
> > > +	void *buckets_ext = NULL;
> > > +	char ring_name[RTE_RING_NAMESIZE];
> > > +	char ext_ring_name[RTE_RING_NAMESIZE];
> > > +	unsigned num_key_slots;
> > > +	unsigned i;
> > > +
> > > +	/* Validate correct usage of extra options */
> > > +	if ((params->extra_flag &
> > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) &&
> > > +	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) {
> > > +		rte_errno = EINVAL;
> > > +		RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket "
> > > +			"feature not supported with rw concurrency "
> > > +			"lock free\n");
> > 
> > Please don't split the error text across lines. If you put it on a line by itself,
> > hopefully checkpatch should not complain. If checkpatch does complain
> > about the long line, just ignore it!
> > [Yes, I know this is copied from original code, but since it appears as new
> > code in this patch, we should fix it]
> > 
> Ok, I can do that. There are other check patch issues which I did not fix (thinking it is old code), will fix those as well.
> 
> > > +		return NULL;
> > > +	}
> > > +
> > > +	/* Check extra flags field to check extra options. */
> > > +	if (params->extra_flag &
> > RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD)
> > > +		use_local_cache = 1;
> > > +
> > >
> > >  void
> > >  rte_hash_free(struct rte_hash *h)
> > > diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
> > > index c93d1a137..93c7019ec 100644
> > > --- a/lib/librte_hash/rte_hash.h
> > > +++ b/lib/librte_hash/rte_hash.h
> > > @@ -30,13 +30,27 @@ extern "C" {
> > >  #define RTE_HASH_LOOKUP_BULK_MAX		64
> > >  #define RTE_HASH_LOOKUP_MULTI_MAX
> > 	RTE_HASH_LOOKUP_BULK_MAX
> > >
> > > -/** Enable Hardware transactional memory support. */
> > > +/**
> > > + * @deprecated
> > > + * This define will be removed in the next release.
> > > + * If the target platform supports hardware transactional memory
> > > + * it will be used without user consent as it provides the best
> > > +possible
> > > + * performance.
> > > + *
> > > + * Enable Hardware transactional memory support.
> > > + */
> > >  #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT	0x01
> > >
> > >  /** Default behavior of insertion, single writer/multi writer */
> > > #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02
> > >
> > > -/** Flag to support reader writer concurrency */
> > > +/**
> > > + * @deprecated
> > > + * This define will be removed in the next release.
> > > + * This library should be thread-safe by default.
> > > + *
> > > + * Flag to support reader writer concurrency  */
> > >  #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04
> > >
> > 
> > Do we not need/want to add some new flags to disable these features, in
> > case there are cases where either RW concurrency, or transaction memory is
> > unwanted?
> > 
> I cannot think of a case where RW concurrency is not required, except for single thread use case. In the single thread use case, the existing flags provide the most optimal solution. Hopefully, it will be clear from the changes.
> I am not sure on if there is a case for no wanting transactional memory while using lock based RW concurrency. We have a flag for non-lock based RW concurrency in which case transactional memory will not be used.
> IMO, I do not see a need for additional flags.
>  

I think for testing purposes at least, we need an option to disable TSX.
When assessing performance impacting changes to the hash, the developer
needs to evaluate the impact in both TSX and non-TSX scenarios.

/Bruce