[dpdk-dev] [RFC] hash/lpm: return NULL if the object exists

[dpdk-dev] [RFC] hash/lpm: return NULL if the object exists

[Olivier Matz]

Seen by trying to fix the func_reentrancy autotest. The test
was doing the following on several cores in parallel:

  name = "common_name";
  do several times {
      obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
      if (obj == NULL && lookup(name) == NULL)
          return TEST_FAIL;
  }

Issues:

1/ rings, mempools, hashs API are not coherent
   rings and mempool return NULL if the object does not exist
   hash and lpm return an object that was allocated allocated if
   it already was allocated

2/ The hash/lpm API looks dangerous: when an object is returned,
   the user does not know if it should be freed or not (no refcnt)

3/ There are some possible race conditions in cuckoo_hash as the
   lock is not held in rte_hash_create(). We could find some cases
   where NULL is returned when the object already exists (ex: when
   rte_ring_create() fails).

This patch tries to rationalize the APIs of lpm and hash.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_func_reentrancy.c   | 31 +++++++++++++++++++++----------
 app/test/test_lpm6.c              |  2 +-
 lib/librte_hash/rte_cuckoo_hash.c |  2 +-
 lib/librte_hash/rte_fbk_hash.c    |  4 +++-
 lib/librte_lpm/rte_lpm.c          |  8 ++++++--
 lib/librte_lpm/rte_lpm6.c         |  4 +++-
 6 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/app/test/test_func_reentrancy.c b/app/test/test_func_reentrancy.c
index 5d09296..300a3bc 100644
--- a/app/test/test_func_reentrancy.c
+++ b/app/test/test_func_reentrancy.c
@@ -83,6 +83,7 @@ typedef void (*case_clean_t)(unsigned lcore_id);
 
 #define MAX_LCORES	RTE_MAX_MEMZONE / (MAX_ITER_TIMES * 4U)
 
+static rte_atomic32_t obj_count = RTE_ATOMIC32_INIT(0);
 static rte_atomic32_t synchro = RTE_ATOMIC32_INIT(0);
 
 #define WAIT_SYNCHRO_FOR_SLAVES()   do{ \
@@ -100,6 +101,7 @@ test_eal_init_once(__attribute__((unused)) void *arg)
 
 	WAIT_SYNCHRO_FOR_SLAVES();
 
+	rte_atomic32_set(&obj_count, 1); /* silent the check in the caller */
 	if (rte_eal_init(0, NULL) != -1)
 		return -1;
 
@@ -122,8 +124,8 @@ ring_create_lookup(__attribute__((unused)) void *arg)
 	/* create the same ring simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		rp = rte_ring_create("fr_test_once", 4096, SOCKET_ID_ANY, 0);
-		if ((NULL == rp) && (rte_ring_lookup("fr_test_once") == NULL))
-			return -1;
+		if (rp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -172,8 +174,8 @@ mempool_create_lookup(__attribute__((unused)) void *arg)
 					NULL, NULL,
 					my_obj_init, NULL,
 					SOCKET_ID_ANY, 0);
-		if ((NULL == mp) && (rte_mempool_lookup("fr_test_once") == NULL))
-			return -1;
+		if (mp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -238,8 +240,8 @@ hash_create_free(__attribute__((unused)) void *arg)
 	hash_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_hash_create(&hash_params);
-		if ((NULL == handle) && (rte_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple times simultaneously */
@@ -306,8 +308,8 @@ fbk_create_free(__attribute__((unused)) void *arg)
 	fbk_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_fbk_hash_create(&fbk_params);
-		if ((NULL == handle) && (rte_fbk_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -372,8 +374,8 @@ lpm_create_free(__attribute__((unused)) void *arg)
 	/* create the same lpm simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		lpm = rte_lpm_create("fr_test_once",  SOCKET_ID_ANY, &config);
-		if ((NULL == lpm) && (rte_lpm_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (lpm != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -432,10 +434,12 @@ launch_test(struct test_case *pt_case)
 	unsigned lcore_id;
 	unsigned cores_save = rte_lcore_count();
 	unsigned cores = RTE_MIN(cores_save, MAX_LCORES);
+	unsigned count;
 
 	if (pt_case->func == NULL)
 		return -1;
 
+	rte_atomic32_set(&obj_count, 0);
 	rte_atomic32_set(&synchro, 0);
 
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
@@ -462,6 +466,13 @@ launch_test(struct test_case *pt_case)
 			pt_case->clean(lcore_id);
 	}
 
+	count = rte_atomic32_read(&obj_count);
+	if (count != 1) {
+		printf("%s: common object allocated %d times (should be 1)\n",
+			pt_case->name, count);
+		ret = -1;
+	}
+
 	return ret;
 }
 
diff --git a/app/test/test_lpm6.c b/app/test/test_lpm6.c
index 1f88d7a..b464342 100644
--- a/app/test/test_lpm6.c
+++ b/app/test/test_lpm6.c
@@ -222,7 +222,7 @@ test1(void)
 
 	/* rte_lpm6_create: lpm name == LPM2 */
 	lpm3 = rte_lpm6_create("LPM1", SOCKET_ID_ANY, &config);
-	TEST_LPM_ASSERT(lpm3 == lpm1);
+	TEST_LPM_ASSERT(lpm3 == NULL);
 
 	rte_lpm6_free(lpm1);
 	rte_lpm6_free(lpm2);
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 71b5b76..857e152 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -231,7 +231,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	/* Guarantee there's no existing */
 	h = rte_hash_find_existing(params->name);
 	if (h != NULL)
-		return h;
+		return NULL;
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
diff --git a/lib/librte_hash/rte_fbk_hash.c b/lib/librte_hash/rte_fbk_hash.c
index 8752a47..ea854f1 100644
--- a/lib/librte_hash/rte_fbk_hash.c
+++ b/lib/librte_hash/rte_fbk_hash.c
@@ -140,8 +140,10 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
 		if (strncmp(params->name, ht->name, RTE_FBK_HASH_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		ht = NULL;
 		goto exit;
+	}
 
 	te = rte_zmalloc("FBK_HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index ccaaa2a..dd62f9b 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -209,8 +209,10 @@ rte_lpm_create_v20(const char *name, int socket_id, int max_rules,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -280,8 +282,10 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 6c2b293..4e9f2d0 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -182,8 +182,10 @@ rte_lpm6_create(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM6_TAILQ_ENTRY", sizeof(*te), 0);
-- 
2.1.4

Re: [dpdk-dev] [RFC] hash/lpm: return NULL if the object exists

[Olivier Matz]

Hi Bruce,

On 03/15/2016 01:25 PM, Olivier Matz wrote:
> Seen by trying to fix the func_reentrancy autotest. The test
> was doing the following on several cores in parallel:
> 
>   name = "common_name";
>   do several times {
>       obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
>       if (obj == NULL && lookup(name) == NULL)
>           return TEST_FAIL;
>   }
> 
> Issues:
> 
> 1/ rings, mempools, hashs API are not coherent
>    rings and mempool return NULL if the object does not exist
>    hash and lpm return an object that was allocated allocated if
>    it already was allocated
> 
> 2/ The hash/lpm API looks dangerous: when an object is returned,
>    the user does not know if it should be freed or not (no refcnt)
> 
> 3/ There are some possible race conditions in cuckoo_hash as the
>    lock is not held in rte_hash_create(). We could find some cases
>    where NULL is returned when the object already exists (ex: when
>    rte_ring_create() fails).
> 
> This patch tries to rationalize the APIs of lpm and hash.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Sorry, I forgot to CC you in the first mail. Do you have any opinion
about this rfc patch?

Thanks,
Olivier

Re: [dpdk-dev] [RFC] hash/lpm: return NULL if the object exists

[Bruce Richardson]

On Fri, Mar 25, 2016 at 11:32:47AM +0100, Olivier Matz wrote:
> Hi Bruce,
> 
> On 03/15/2016 01:25 PM, Olivier Matz wrote:
> > Seen by trying to fix the func_reentrancy autotest. The test
> > was doing the following on several cores in parallel:
> > 
> >   name = "common_name";
> >   do several times {
> >       obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
> >       if (obj == NULL && lookup(name) == NULL)
> >           return TEST_FAIL;
> >   }
> > 
> > Issues:
> > 
> > 1/ rings, mempools, hashs API are not coherent
> >    rings and mempool return NULL if the object does not exist
> >    hash and lpm return an object that was allocated allocated if
> >    it already was allocated
> > 
> > 2/ The hash/lpm API looks dangerous: when an object is returned,
> >    the user does not know if it should be freed or not (no refcnt)
> > 
> > 3/ There are some possible race conditions in cuckoo_hash as the
> >    lock is not held in rte_hash_create(). We could find some cases
> >    where NULL is returned when the object already exists (ex: when
> >    rte_ring_create() fails).
> > 
> > This patch tries to rationalize the APIs of lpm and hash.
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Sorry, I forgot to CC you in the first mail. Do you have any opinion
> about this rfc patch?
> 
> Thanks,
> Olivier
Hi Olivier,

the idea looks good, since an object already existing is an error condition on
create. One small change to the libs I'd suggest is to set rte_errno to 
EEXIST before exit, so that the error reason is known to the app.

Regards,
/Bruce

[dpdk-dev] [PATCH 0/4] fix lpm and hash creation

[Olivier Matz]

Seen while trying to fix the func_reentrancy autotest. The
series addresses several issues:

1/ Hash and lpm return a pointer to an existing object if the user requests the
   creation with an already existing name. This look dangerous: when an object
   is returned, the user does not know if it should be freed or not.

2/ There is a race condition in cuckoo_hash as the lock is not held in
   rte_hash_create(). We could find some cases where NULL is returned when the
   object already exists (ex: when rte_ring_create() fails).

3/ There is a race condition func_reentrancy that can fail even if the tested
   API behaves correctly.


Changes since RFC:

- split the patch in 4 patches
- on error, set rte_errno to EEXIST when relevant
- fix locking in cuckoo_hash creation

Olivier Matz (4):
  lpm: allocation of an existing object should fail
  hash: allocation of an existing object should fail
  hash: keep the list locked at creation
  autotest: fix func reentrancy

 app/test/test_func_reentrancy.c   | 31 +++++++++++++++++++++----------
 app/test/test_lpm6.c              |  2 +-
 lib/librte_hash/rte_cuckoo_hash.c | 18 +++++++++++++-----
 lib/librte_hash/rte_fbk_hash.c    |  5 ++++-
 lib/librte_lpm/rte_lpm.c          | 10 ++++++++--
 lib/librte_lpm/rte_lpm6.c         |  5 ++++-
 6 files changed, 51 insertions(+), 20 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH 1/4] lpm: allocation of an existing object should fail

[Olivier Matz]

Change the API of rte_lpm*_create() functions to return NULL and set
rte_errno to EEXIST when the object name already exists.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the lpm API more consistent with the other
APIs (mempool, rings, ...).

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_lpm6.c      |  2 +-
 lib/librte_lpm/rte_lpm.c  | 10 ++++++++--
 lib/librte_lpm/rte_lpm6.c |  5 ++++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/app/test/test_lpm6.c b/app/test/test_lpm6.c
index 1f88d7a..b464342 100644
--- a/app/test/test_lpm6.c
+++ b/app/test/test_lpm6.c
@@ -222,7 +222,7 @@ test1(void)
 
 	/* rte_lpm6_create: lpm name == LPM2 */
 	lpm3 = rte_lpm6_create("LPM1", SOCKET_ID_ANY, &config);
-	TEST_LPM_ASSERT(lpm3 == lpm1);
+	TEST_LPM_ASSERT(lpm3 == NULL);
 
 	rte_lpm6_free(lpm1);
 	rte_lpm6_free(lpm2);
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index af5811c..62e74bb 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -209,8 +209,11 @@ rte_lpm_create_v20(const char *name, int socket_id, int max_rules,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -280,8 +283,11 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 4c44cd7..9877a30 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM6_TAILQ_ENTRY", sizeof(*te), 0);
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/4] lpm: allocation of an existing object should fail

[Stephen Hemminger]

On Wed, 30 Mar 2016 17:30:24 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:

> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> index 4c44cd7..9877a30 100644
> --- a/lib/librte_lpm/rte_lpm6.c
> +++ b/lib/librte_lpm/rte_lpm6.c
> @@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
>  		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
>  			break;
>  	}
> -	if (te != NULL)
> +	if (te != NULL) {
> +		lpm = NULL;
> +		rte_errno = EEXIST;
>  		goto exit;
> +	}
>  
>  	/* allocate tailq entry */
>  

with older memzone model, objects in huge memory area were never freed.
That means when application restarts it finds the old LPM and works.
With your change it would break such an application.

Re: [dpdk-dev] [PATCH 1/4] lpm: allocation of an existing object should fail

[Olivier Matz]

Hi Stephen,

On 03/30/2016 11:46 PM, Stephen Hemminger wrote:
> On Wed, 30 Mar 2016 17:30:24 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
> 
>> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
>> index 4c44cd7..9877a30 100644
>> --- a/lib/librte_lpm/rte_lpm6.c
>> +++ b/lib/librte_lpm/rte_lpm6.c
>> @@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
>>  		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
>>  			break;
>>  	}
>> -	if (te != NULL)
>> +	if (te != NULL) {
>> +		lpm = NULL;
>> +		rte_errno = EEXIST;
>>  		goto exit;
>> +	}
>>  
>>  	/* allocate tailq entry */
>>  
> 
> with older memzone model, objects in huge memory area were never freed.
> That means when application restarts it finds the old LPM and works.
> With your change it would break such an application.
> 

Could you be more precise about the use case you are
describing? Are you talking about a secondary process?

The API description of lpm and hash says since the first
release that EEXIST should be returned if a memzone with
the same name already exists:

 * @return
 *   Handle to LPM object on success, NULL otherwise with rte_errno set
 *   to an appropriate values. Possible rte_errno values include:
 *    - E_RTE_NO_CONFIG - function could not get pointer to rte_config
structure
 *    - E_RTE_SECONDARY - function was called from a secondary process
instance
 *    - EINVAL - invalid parameter passed to function
 *    - ENOSPC - the maximum number of memzones has already been allocated
 *    - EEXIST - a memzone with the same name already exists
 *    - ENOMEM - no appropriate memory area found in which to create memzone
 */
struct rte_lpm *
rte_lpm_create(const char *name, int socket_id,
		const struct rte_lpm_config *config);

 * @return
 *   Pointer to hash table structure that is used in future hash table
 *   operations, or NULL on error, with error code set in rte_errno.
 *   Possible rte_errno errors include:
 *    - E_RTE_NO_CONFIG - function could not get pointer to rte_config
structure
 *    - E_RTE_SECONDARY - function was called from a secondary process
instance
 *    - ENOENT - missing entry
 *    - EINVAL - invalid parameter passed to function
 *    - ENOSPC - the maximum number of memzones has already been allocated
 *    - EEXIST - a memzone with the same name already exists
 *    - ENOMEM - no appropriate memory area found in which to create memzone
 */
struct rte_hash *
rte_hash_create(const struct rte_hash_parameters *params);


>From my point of view, the behavior I'm fixing is more a bug
fix than an API change. But if required, I can send a deprecation
notice for 16.04 and have the fix integrated for 16.07.

Re: [dpdk-dev] [PATCH 1/4] lpm: allocation of an existing object should fail

[Olivier Matz]

On 03/31/2016 09:35 AM, Olivier Matz wrote:
> On 03/30/2016 11:46 PM, Stephen Hemminger wrote:
>> with older memzone model, objects in huge memory area were never freed.
>> That means when application restarts it finds the old LPM and works.
>> With your change it would break such an application.
>>
> 
> Could you be more precise about the use case you are
> describing? Are you talking about a secondary process?
> 
> The API description of lpm and hash says since the first
> release that EEXIST should be returned if a memzone with
> the same name already exists:
> 
>  * @return
>  *   Handle to LPM object on success, NULL otherwise with rte_errno set
>  *   to an appropriate values. Possible rte_errno values include:
>  *    - E_RTE_NO_CONFIG - function could not get pointer to rte_config
> structure
>  *    - E_RTE_SECONDARY - function was called from a secondary process
> instance
>  *    - EINVAL - invalid parameter passed to function
>  *    - ENOSPC - the maximum number of memzones has already been allocated
>  *    - EEXIST - a memzone with the same name already exists
>  *    - ENOMEM - no appropriate memory area found in which to create memzone
>  */
> struct rte_lpm *
> rte_lpm_create(const char *name, int socket_id,
> 		const struct rte_lpm_config *config);
> 
>  * @return
>  *   Pointer to hash table structure that is used in future hash table
>  *   operations, or NULL on error, with error code set in rte_errno.
>  *   Possible rte_errno errors include:
>  *    - E_RTE_NO_CONFIG - function could not get pointer to rte_config
> structure
>  *    - E_RTE_SECONDARY - function was called from a secondary process
> instance
>  *    - ENOENT - missing entry
>  *    - EINVAL - invalid parameter passed to function
>  *    - ENOSPC - the maximum number of memzones has already been allocated
>  *    - EEXIST - a memzone with the same name already exists
>  *    - ENOMEM - no appropriate memory area found in which to create memzone
>  */
> struct rte_hash *
> rte_hash_create(const struct rte_hash_parameters *params);
> 
> 
> From my point of view, the behavior I'm fixing is more a bug
> fix than an API change. But if required, I can send a deprecation
> notice for 16.04 and have the fix integrated for 16.07.
> 

Stephen, any comment on this please?

The problem is today some unit tests are not passing correctly.


Thanks

Re: [dpdk-dev] [PATCH 1/4] lpm: allocation of an existing object should fail

[Bruce Richardson]

On Wed, Mar 30, 2016 at 02:46:49PM -0700, Stephen Hemminger wrote:
> On Wed, 30 Mar 2016 17:30:24 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> > diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> > index 4c44cd7..9877a30 100644
> > --- a/lib/librte_lpm/rte_lpm6.c
> > +++ b/lib/librte_lpm/rte_lpm6.c
> > @@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
> >  		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
> >  			break;
> >  	}
> > -	if (te != NULL)
> > +	if (te != NULL) {
> > +		lpm = NULL;
> > +		rte_errno = EEXIST;
> >  		goto exit;
> > +	}
> >  
> >  	/* allocate tailq entry */
> >  
> 
> with older memzone model, objects in huge memory area were never freed.
> That means when application restarts it finds the old LPM and works.
> With your change it would break such an application.

Is all the memory not zeroed on initialization?

/Bruce

[dpdk-dev] [PATCH 2/4] hash: allocation of an existing object should fail

[Olivier Matz]

Change the API of rte_hash*_create() functions to return NULL and set
rte_errno to EEXIST when the object name already exists.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the hash API more consistent with the other
APIs (mempool, rings, ...).

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 6 ++++--
 lib/librte_hash/rte_fbk_hash.c    | 5 ++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 71b5b76..ccec2db 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -230,8 +230,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	/* Guarantee there's no existing */
 	h = rte_hash_find_existing(params->name);
-	if (h != NULL)
-		return h;
+	if (h != NULL) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
diff --git a/lib/librte_hash/rte_fbk_hash.c b/lib/librte_hash/rte_fbk_hash.c
index 8752a47..ba1e475 100644
--- a/lib/librte_hash/rte_fbk_hash.c
+++ b/lib/librte_hash/rte_fbk_hash.c
@@ -140,8 +140,11 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
 		if (strncmp(params->name, ht->name, RTE_FBK_HASH_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		ht = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	te = rte_zmalloc("FBK_HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
-- 
2.1.4

[dpdk-dev] [PATCH 3/4] hash: keep the list locked at creation

[Olivier Matz]

To avoid a race condition while creating a new hash object, the
list has to be locked before the lookup, and released only once the
new object is added in the list.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index ccec2db..18351fa 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -228,11 +228,17 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
 
-	/* Guarantee there's no existing */
-	h = rte_hash_find_existing(params->name);
-	if (h != NULL) {
+	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+
+	/* guarantee there's no existing */
+	TAILQ_FOREACH(te, hash_list, next) {
+		h = (struct rte_hash *) te->data;
+		if (strncmp(name, h->name, RTE_HASH_NAMESIZE) == 0)
+			break;
+	}
+	if (te != NULL) {
 		rte_errno = EEXIST;
-		return NULL;
+		goto err;
 	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
@@ -359,13 +365,13 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	for (i = 1; i < params->entries + 1; i++)
 		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
 
-	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
 	te->data = (void *) h;
 	TAILQ_INSERT_TAIL(hash_list, te, next);
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
 	return h;
 err:
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 	rte_free(te);
 	rte_free(h);
 	rte_free(buckets);
-- 
2.1.4

[dpdk-dev] [PATCH 4/4] autotest: fix func reentrancy

[Olivier Matz]

The previous code in func_reentrancy autotest was doing in parallel
something close to:

  name = "common_name";
  do several times {
      obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
      if (obj == NULL && lookup(name) == NULL)
          return TEST_FAIL;
  }

This code is not safe. For instance:

   mempool_create() is called on core 0, it creates a ring. At the same
   time on core 1, mempool_create() is called too and the creation of the
   ring fails (EEXIST). But the mempool lookup can fail on core 1 if
   the mempool is not added in the list by core 0.

This commit fixes the func_reentrancy autotest that now works with all
tested class of objects.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_func_reentrancy.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/app/test/test_func_reentrancy.c b/app/test/test_func_reentrancy.c
index 5d09296..300a3bc 100644
--- a/app/test/test_func_reentrancy.c
+++ b/app/test/test_func_reentrancy.c
@@ -83,6 +83,7 @@ typedef void (*case_clean_t)(unsigned lcore_id);
 
 #define MAX_LCORES	RTE_MAX_MEMZONE / (MAX_ITER_TIMES * 4U)
 
+static rte_atomic32_t obj_count = RTE_ATOMIC32_INIT(0);
 static rte_atomic32_t synchro = RTE_ATOMIC32_INIT(0);
 
 #define WAIT_SYNCHRO_FOR_SLAVES()   do{ \
@@ -100,6 +101,7 @@ test_eal_init_once(__attribute__((unused)) void *arg)
 
 	WAIT_SYNCHRO_FOR_SLAVES();
 
+	rte_atomic32_set(&obj_count, 1); /* silent the check in the caller */
 	if (rte_eal_init(0, NULL) != -1)
 		return -1;
 
@@ -122,8 +124,8 @@ ring_create_lookup(__attribute__((unused)) void *arg)
 	/* create the same ring simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		rp = rte_ring_create("fr_test_once", 4096, SOCKET_ID_ANY, 0);
-		if ((NULL == rp) && (rte_ring_lookup("fr_test_once") == NULL))
-			return -1;
+		if (rp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -172,8 +174,8 @@ mempool_create_lookup(__attribute__((unused)) void *arg)
 					NULL, NULL,
 					my_obj_init, NULL,
 					SOCKET_ID_ANY, 0);
-		if ((NULL == mp) && (rte_mempool_lookup("fr_test_once") == NULL))
-			return -1;
+		if (mp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -238,8 +240,8 @@ hash_create_free(__attribute__((unused)) void *arg)
 	hash_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_hash_create(&hash_params);
-		if ((NULL == handle) && (rte_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple times simultaneously */
@@ -306,8 +308,8 @@ fbk_create_free(__attribute__((unused)) void *arg)
 	fbk_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_fbk_hash_create(&fbk_params);
-		if ((NULL == handle) && (rte_fbk_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -372,8 +374,8 @@ lpm_create_free(__attribute__((unused)) void *arg)
 	/* create the same lpm simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		lpm = rte_lpm_create("fr_test_once",  SOCKET_ID_ANY, &config);
-		if ((NULL == lpm) && (rte_lpm_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (lpm != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -432,10 +434,12 @@ launch_test(struct test_case *pt_case)
 	unsigned lcore_id;
 	unsigned cores_save = rte_lcore_count();
 	unsigned cores = RTE_MIN(cores_save, MAX_LCORES);
+	unsigned count;
 
 	if (pt_case->func == NULL)
 		return -1;
 
+	rte_atomic32_set(&obj_count, 0);
 	rte_atomic32_set(&synchro, 0);
 
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
@@ -462,6 +466,13 @@ launch_test(struct test_case *pt_case)
 			pt_case->clean(lcore_id);
 	}
 
+	count = rte_atomic32_read(&obj_count);
+	if (count != 1) {
+		printf("%s: common object allocated %d times (should be 1)\n",
+			pt_case->name, count);
+		ret = -1;
+	}
+
 	return ret;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH 0/4] fix lpm and hash creation

[Olivier Matz]

On 03/30/2016 05:30 PM, Olivier Matz wrote:
> Seen while trying to fix the func_reentrancy autotest. The
> series addresses several issues:
> 
> 1/ Hash and lpm return a pointer to an existing object if the user requests the
>    creation with an already existing name. This look dangerous: when an object
>    is returned, the user does not know if it should be freed or not.
> 
> 2/ There is a race condition in cuckoo_hash as the lock is not held in
>    rte_hash_create(). We could find some cases where NULL is returned when the
>    object already exists (ex: when rte_ring_create() fails).
> 
> 3/ There is a race condition func_reentrancy that can fail even if the tested
>    API behaves correctly.
> 
> 
> Changes since RFC:
> 
> - split the patch in 4 patches
> - on error, set rte_errno to EEXIST when relevant
> - fix locking in cuckoo_hash creation
> 
> Olivier Matz (4):
>   lpm: allocation of an existing object should fail
>   hash: allocation of an existing object should fail
>   hash: keep the list locked at creation
>   autotest: fix func reentrancy
> 
>  app/test/test_func_reentrancy.c   | 31 +++++++++++++++++++++----------
>  app/test/test_lpm6.c              |  2 +-
>  lib/librte_hash/rte_cuckoo_hash.c | 18 +++++++++++++-----
>  lib/librte_hash/rte_fbk_hash.c    |  5 ++++-
>  lib/librte_lpm/rte_lpm.c          | 10 ++++++++--
>  lib/librte_lpm/rte_lpm6.c         |  5 ++++-
>  6 files changed, 51 insertions(+), 20 deletions(-)
> 

Self-nack, there is a typo in rte_cuckoo_hash.c breaking the
compilation.

[dpdk-dev] [PATCH v2 0/4] fix creation of duplicate lpm and hash

[Olivier Matz]

Seen while trying to fix the func_reentrancy autotest. The
series addresses several issues:

1/ Hash and lpm return a pointer to an existing object if the user requests the
   creation with an already existing name. This look dangerous: when an object
   is returned, the user does not know if it should be freed or not.

2/ There is a race condition in cuckoo_hash as the lock is not held in
   rte_hash_create(). We could find some cases where NULL is returned when the
   object already exists (ex: when rte_ring_create() fails).

3/ There is a race condition func_reentrancy that can fail even if the tested
   API behaves correctly.


RFC -> v1:

- split the patch in 4 patches
- on error, set rte_errno to EEXIST when relevant
- fix locking in cuckoo_hash creation

v1 -> v2:

- fix compilation issue in cuckoo hash
- update the hash test to conform to the new behavior
- rework locking modification in cuckoo_hash
- passed autotests: hash, lpm, lpm6, func_reentrancy

Olivier Matz (4):
  lpm: allocation of an existing object should fail
  hash: allocation of an existing object should fail
  hash: keep the list locked at creation
  autotest: fix func reentrancy

 app/test/test_func_reentrancy.c   | 31 +++++++++++------
 app/test/test_hash.c              | 65 +++++++++++++-----------------------
 app/test/test_lpm6.c              |  2 +-
 lib/librte_hash/rte_cuckoo_hash.c | 70 ++++++++++++++++++++++-----------------
 lib/librte_hash/rte_fbk_hash.c    |  5 ++-
 lib/librte_lpm/rte_lpm.c          | 10 ++++--
 lib/librte_lpm/rte_lpm6.c         |  5 ++-
 7 files changed, 101 insertions(+), 87 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v2 1/4] lpm: allocation of an existing object should fail

[Olivier Matz]

Change rte_lpm*_create() functions to return NULL and set rte_errno to
EEXIST when the object name already exists. This is the behavior
described in the API documentation in the header file.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the lpm API more consistent with the other
APIs (mempool, rings, ...).

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_lpm6.c      |  2 +-
 lib/librte_lpm/rte_lpm.c  | 10 ++++++++--
 lib/librte_lpm/rte_lpm6.c |  5 ++++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/app/test/test_lpm6.c b/app/test/test_lpm6.c
index 1f88d7a..b464342 100644
--- a/app/test/test_lpm6.c
+++ b/app/test/test_lpm6.c
@@ -222,7 +222,7 @@ test1(void)
 
 	/* rte_lpm6_create: lpm name == LPM2 */
 	lpm3 = rte_lpm6_create("LPM1", SOCKET_ID_ANY, &config);
-	TEST_LPM_ASSERT(lpm3 == lpm1);
+	TEST_LPM_ASSERT(lpm3 == NULL);
 
 	rte_lpm6_free(lpm1);
 	rte_lpm6_free(lpm2);
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index bd3563f..73c9ec3 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -209,8 +209,11 @@ rte_lpm_create_v20(const char *name, int socket_id, int max_rules,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -280,8 +283,11 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 4c44cd7..9877a30 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		lpm = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM6_TAILQ_ENTRY", sizeof(*te), 0);
-- 
2.1.4

[dpdk-dev] [PATCH v2 2/4] hash: allocation of an existing object should fail

[Olivier Matz]

Change rte_hash*_create() functions to return NULL and set rte_errno to
EEXIST when the object name already exists. This is the behavior
described in the API documentation in the header file.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the hash API more consistent with the other
APIs (mempool, rings, ...).

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_hash.c              | 65 ++++++++++++++-------------------------
 lib/librte_hash/rte_cuckoo_hash.c |  6 ++--
 lib/librte_hash/rte_fbk_hash.c    |  5 ++-
 3 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/app/test/test_hash.c b/app/test/test_hash.c
index 2f3d884..adbdb4a 100644
--- a/app/test/test_hash.c
+++ b/app/test/test_hash.c
@@ -805,15 +805,11 @@ fbk_hash_unit_test(void)
 	RETURN_IF_ERROR_FBK(handle == NULL, "fbk hash creation should have succeeded");
 
 	tmp = rte_fbk_hash_create(&invalid_params_same_name_2);
-	RETURN_IF_ERROR_FBK(tmp == NULL, "fbk hash creation should have succeeded");
-	if (tmp != handle) {
-			printf("ERROR line %d: hashes should have been the same\n", __LINE__);
-			rte_fbk_hash_free(handle);
-			rte_fbk_hash_free(tmp);
-			return -1;
-	}
+	if (tmp != NULL)
+		rte_fbk_hash_free(tmp);
+	RETURN_IF_ERROR_FBK(tmp != NULL, "fbk hash creation should have failed");
 
-	/* we are not freeing tmp or handle here because we need a hash list
+	/* we are not freeing  handle here because we need a hash list
 	 * to be not empty for the next test */
 
 	/* create a hash in non-empty list - good for coverage */
@@ -988,7 +984,7 @@ static int test_fbk_hash_find_existing(void)
  */
 static int test_hash_creation_with_bad_parameters(void)
 {
-	struct rte_hash *handle;
+	struct rte_hash *handle, *tmp;
 	struct rte_hash_parameters params;
 
 	handle = rte_hash_create(NULL);
@@ -1038,7 +1034,23 @@ static int test_hash_creation_with_bad_parameters(void)
 		return -1;
 	}
 
+	/* test with same name should fail */
+	memcpy(&params, &ut_params, sizeof(params));
+	params.name = "same_name";
+	handle = rte_hash_create(&params);
+	if (handle == NULL) {
+		printf("Cannot create first hash table with 'same_name'\n");
+		return -1;
+	}
+	tmp = rte_hash_create(&params);
+	if (tmp != NULL) {
+		printf("Creation of hash table with same name should fail\n");
+		rte_hash_free(handle);
+		rte_hash_free(tmp);
+		return -1;
+	}
 	rte_hash_free(handle);
+
 	printf("# Test successful. No more errors expected\n");
 
 	return 0;
@@ -1051,12 +1063,12 @@ static int test_hash_creation_with_bad_parameters(void)
 static int
 test_hash_creation_with_good_parameters(void)
 {
-	struct rte_hash *handle, *tmp;
+	struct rte_hash *handle;
 	struct rte_hash_parameters params;
 
 	/* create with null hash function - should choose DEFAULT_HASH_FUNC */
 	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "same_name";
+	params.name = "name";
 	params.hash_func = NULL;
 	handle = rte_hash_create(&params);
 	if (handle == NULL) {
@@ -1064,37 +1076,6 @@ test_hash_creation_with_good_parameters(void)
 		return -1;
 	}
 
-	/* this test is trying to create a hash with the same name as previous one.
-	 * this should return a pointer to the hash we previously created.
-	 * the previous hash isn't freed exactly for the purpose of it being in
-	 * the hash list.
-	 */
-	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "same_name";
-	tmp = rte_hash_create(&params);
-
-	/* check if the returned handle is actually equal to the previous hash */
-	if (handle != tmp) {
-		rte_hash_free(handle);
-		rte_hash_free(tmp);
-		printf("Creating hash with existing name was successful\n");
-		return -1;
-	}
-
-	/* try creating hash when there already are hashes in the list.
-	 * the previous hash is not freed to have a non-empty hash list.
-	 * the other hash that's in the list is still pointed to by "handle" var.
-	 */
-	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "different_name";
-	tmp = rte_hash_create(&params);
-	if (tmp == NULL) {
-		rte_hash_free(handle);
-		printf("Creating hash with valid parameters failed\n");
-		return -1;
-	}
-
-	rte_hash_free(tmp);
 	rte_hash_free(handle);
 
 	return 0;
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 71b5b76..ccec2db 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -230,8 +230,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	/* Guarantee there's no existing */
 	h = rte_hash_find_existing(params->name);
-	if (h != NULL)
-		return h;
+	if (h != NULL) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
diff --git a/lib/librte_hash/rte_fbk_hash.c b/lib/librte_hash/rte_fbk_hash.c
index 8752a47..ba1e475 100644
--- a/lib/librte_hash/rte_fbk_hash.c
+++ b/lib/librte_hash/rte_fbk_hash.c
@@ -140,8 +140,11 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
 		if (strncmp(params->name, ht->name, RTE_FBK_HASH_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	if (te != NULL) {
+		ht = NULL;
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	te = rte_zmalloc("FBK_HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
-- 
2.1.4

[dpdk-dev] [PATCH v2 3/4] hash: keep the list locked at creation

[Olivier Matz]

To avoid a race condition while creating a new hash object, the
list has to be locked before the lookup, and released only once the
new object is added in the list.

As the lock is held by the rte_ring_create(), move its creation at the
beginning of the function and only take the lock after the ring is
created to avoid a deadlock.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 68 ++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index ccec2db..63a74fd 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -226,19 +226,46 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT)
 		hw_trans_mem_support = 1;
 
+	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
+	if (hw_trans_mem_support)
+		/*
+		 * Increase number of slots by total number of indices
+		 * that can be stored in the lcore caches
+		 * except for the first cache
+		 */
+		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
+					LCORE_CACHE_SIZE + 1;
+	else
+		num_key_slots = params->entries + 1;
+
+	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
+	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
+			params->socket_id, 0);
+	if (r == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err;
+	}
+
 	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
 
-	/* Guarantee there's no existing */
-	h = rte_hash_find_existing(params->name);
-	if (h != NULL) {
+	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+
+	/* guarantee there's no existing: this is normally already checked
+	 * by ring creation above */
+	TAILQ_FOREACH(te, hash_list, next) {
+		h = (struct rte_hash *) te->data;
+		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
+			break;
+	}
+	if (te != NULL) {
 		rte_errno = EEXIST;
-		return NULL;
+		goto err_unlock;
 	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
 		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct rte_hash),
@@ -246,7 +273,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (h == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	const uint32_t num_buckets = rte_align32pow2(params->entries)
@@ -258,23 +285,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (buckets == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	const uint32_t key_entry_size = sizeof(struct rte_hash_key) + params->key_len;
-
-	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
-	if (hw_trans_mem_support)
-		/*
-		 * Increase number of slots by total number of indices
-		 * that can be stored in the lcore caches
-		 * except for the first cache
-		 */
-		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
-					LCORE_CACHE_SIZE + 1;
-	else
-		num_key_slots = params->entries + 1;
-
 	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
 
 	k = rte_zmalloc_socket(NULL, key_tbl_size,
@@ -282,7 +296,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (k == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 /*
@@ -325,14 +339,6 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	h->rte_hash_cmp_eq = memcmp;
 #endif
 
-	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
-	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
-			params->socket_id, 0);
-	if (r == NULL) {
-		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
-	}
-
 	if (hw_trans_mem_support) {
 		h->local_free_slots = rte_zmalloc_socket(NULL,
 				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
@@ -359,13 +365,15 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	for (i = 1; i < params->entries + 1; i++)
 		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
 
-	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
 	te->data = (void *) h;
 	TAILQ_INSERT_TAIL(hash_list, te, next);
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
 	return h;
+err_unlock:
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 err:
+	rte_ring_free(r);
 	rte_free(te);
 	rte_free(h);
 	rte_free(buckets);
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2 3/4] hash: keep the list locked at creation

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, April 05, 2016 8:36 AM
> To: dev@dpdk.org
> Cc: Richardson, Bruce
> Subject: [dpdk-dev] [PATCH v2 3/4] hash: keep the list locked at creation
> 
> To avoid a race condition while creating a new hash object, the
> list has to be locked before the lookup, and released only once the
> new object is added in the list.
> 
> As the lock is held by the rte_ring_create(), move its creation at the
> beginning of the function and only take the lock after the ring is
> created to avoid a deadlock.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  lib/librte_hash/rte_cuckoo_hash.c | 68 ++++++++++++++++++++++-------------
> ----
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> b/lib/librte_hash/rte_cuckoo_hash.c
> index ccec2db..63a74fd 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.c
> +++ b/lib/librte_hash/rte_cuckoo_hash.c
> @@ -226,19 +226,46 @@ rte_hash_create(const struct rte_hash_parameters
> *params)
>  	if (params->extra_flag &
> RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT)
>  		hw_trans_mem_support = 1;
> 
> +	/* Store all keys and leave the first entry as a dummy entry for
> lookup_bulk */
> +	if (hw_trans_mem_support)
> +		/*
> +		 * Increase number of slots by total number of indices
> +		 * that can be stored in the lcore caches
> +		 * except for the first cache
> +		 */
> +		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
> +					LCORE_CACHE_SIZE + 1;
> +	else
> +		num_key_slots = params->entries + 1;
> +
> +	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
> +	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
> +			params->socket_id, 0);
> +	if (r == NULL) {
> +		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> +		goto err;
> +	}
> +
>  	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
> 
> -	/* Guarantee there's no existing */
> -	h = rte_hash_find_existing(params->name);
> -	if (h != NULL) {
> +	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> +
> +	/* guarantee there's no existing: this is normally already checked
> +	 * by ring creation above */
> +	TAILQ_FOREACH(te, hash_list, next) {
> +		h = (struct rte_hash *) te->data;
> +		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE)
> == 0)
> +			break;
> +	}
> +	if (te != NULL) {
>  		rte_errno = EEXIST;
> -		return NULL;
> +		goto err_unlock;
>  	}
> 
>  	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
>  	if (te == NULL) {
>  		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
> -		goto err;
> +		goto err_unlock;
>  	}
> 
>  	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct
> rte_hash),
> @@ -246,7 +273,7 @@ rte_hash_create(const struct rte_hash_parameters
> *params)
> 
>  	if (h == NULL) {
>  		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> -		goto err;
> +		goto err_unlock;
>  	}
> 
>  	const uint32_t num_buckets = rte_align32pow2(params->entries)
> @@ -258,23 +285,10 @@ rte_hash_create(const struct rte_hash_parameters
> *params)
> 
>  	if (buckets == NULL) {
>  		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> -		goto err;
> +		goto err_unlock;
>  	}
> 
>  	const uint32_t key_entry_size = sizeof(struct rte_hash_key) +
> params->key_len;
> -
> -	/* Store all keys and leave the first entry as a dummy entry for
> lookup_bulk */
> -	if (hw_trans_mem_support)
> -		/*
> -		 * Increase number of slots by total number of indices
> -		 * that can be stored in the lcore caches
> -		 * except for the first cache
> -		 */
> -		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
> -					LCORE_CACHE_SIZE + 1;
> -	else
> -		num_key_slots = params->entries + 1;
> -
>  	const uint64_t key_tbl_size = (uint64_t) key_entry_size *
> num_key_slots;
> 
>  	k = rte_zmalloc_socket(NULL, key_tbl_size,
> @@ -282,7 +296,7 @@ rte_hash_create(const struct rte_hash_parameters
> *params)
> 
>  	if (k == NULL) {
>  		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> -		goto err;
> +		goto err_unlock;
>  	}
> 
>  /*
> @@ -325,14 +339,6 @@ rte_hash_create(const struct rte_hash_parameters
> *params)
>  	h->rte_hash_cmp_eq = memcmp;
>  #endif
> 
> -	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
> -	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
> -			params->socket_id, 0);
> -	if (r == NULL) {
> -		RTE_LOG(ERR, HASH, "memory allocation failed\n");
> -		goto err;
> -	}
> -
>  	if (hw_trans_mem_support) {
>  		h->local_free_slots = rte_zmalloc_socket(NULL,
>  				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
> @@ -359,13 +365,15 @@ rte_hash_create(const struct rte_hash_parameters
> *params)
>  	for (i = 1; i < params->entries + 1; i++)
>  		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
> 
> -	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
>  	te->data = (void *) h;
>  	TAILQ_INSERT_TAIL(hash_list, te, next);
>  	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>
>  	return h;
> +err_unlock:
> +	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>  err:
> +	rte_ring_free(r);

This patch does not apply cleanly, since another patch for this file last week was applied
(http://dpdk.org/dev/patchwork/patch/11896/).

Could you rebase this patchset against mainline?

Thanks,
Pablo

[dpdk-dev] [PATCH v2 4/4] autotest: fix func reentrancy

[Olivier Matz]

The previous code in func_reentrancy autotest was doing in parallel
something close to:

  name = "common_name";
  do several times {
      obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
      if (obj == NULL && lookup(name) == NULL)
          return TEST_FAIL;
  }

This code is not safe. For instance:

   mempool_create() is called on core 0, it creates a ring. At the same
   time on core 1, mempool_create() is called too and the creation of the
   ring fails (EEXIST). But the mempool lookup can fail on core 1 if
   the mempool is not added in the list by core 0.

This commit fixes the func_reentrancy autotest that now works with all
tested class of objects.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_func_reentrancy.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/app/test/test_func_reentrancy.c b/app/test/test_func_reentrancy.c
index 5d09296..300a3bc 100644
--- a/app/test/test_func_reentrancy.c
+++ b/app/test/test_func_reentrancy.c
@@ -83,6 +83,7 @@ typedef void (*case_clean_t)(unsigned lcore_id);
 
 #define MAX_LCORES	RTE_MAX_MEMZONE / (MAX_ITER_TIMES * 4U)
 
+static rte_atomic32_t obj_count = RTE_ATOMIC32_INIT(0);
 static rte_atomic32_t synchro = RTE_ATOMIC32_INIT(0);
 
 #define WAIT_SYNCHRO_FOR_SLAVES()   do{ \
@@ -100,6 +101,7 @@ test_eal_init_once(__attribute__((unused)) void *arg)
 
 	WAIT_SYNCHRO_FOR_SLAVES();
 
+	rte_atomic32_set(&obj_count, 1); /* silent the check in the caller */
 	if (rte_eal_init(0, NULL) != -1)
 		return -1;
 
@@ -122,8 +124,8 @@ ring_create_lookup(__attribute__((unused)) void *arg)
 	/* create the same ring simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		rp = rte_ring_create("fr_test_once", 4096, SOCKET_ID_ANY, 0);
-		if ((NULL == rp) && (rte_ring_lookup("fr_test_once") == NULL))
-			return -1;
+		if (rp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -172,8 +174,8 @@ mempool_create_lookup(__attribute__((unused)) void *arg)
 					NULL, NULL,
 					my_obj_init, NULL,
 					SOCKET_ID_ANY, 0);
-		if ((NULL == mp) && (rte_mempool_lookup("fr_test_once") == NULL))
-			return -1;
+		if (mp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -238,8 +240,8 @@ hash_create_free(__attribute__((unused)) void *arg)
 	hash_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_hash_create(&hash_params);
-		if ((NULL == handle) && (rte_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple times simultaneously */
@@ -306,8 +308,8 @@ fbk_create_free(__attribute__((unused)) void *arg)
 	fbk_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_fbk_hash_create(&fbk_params);
-		if ((NULL == handle) && (rte_fbk_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -372,8 +374,8 @@ lpm_create_free(__attribute__((unused)) void *arg)
 	/* create the same lpm simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		lpm = rte_lpm_create("fr_test_once",  SOCKET_ID_ANY, &config);
-		if ((NULL == lpm) && (rte_lpm_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (lpm != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -432,10 +434,12 @@ launch_test(struct test_case *pt_case)
 	unsigned lcore_id;
 	unsigned cores_save = rte_lcore_count();
 	unsigned cores = RTE_MIN(cores_save, MAX_LCORES);
+	unsigned count;
 
 	if (pt_case->func == NULL)
 		return -1;
 
+	rte_atomic32_set(&obj_count, 0);
 	rte_atomic32_set(&synchro, 0);
 
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
@@ -462,6 +466,13 @@ launch_test(struct test_case *pt_case)
 			pt_case->clean(lcore_id);
 	}
 
+	count = rte_atomic32_read(&obj_count);
+	if (count != 1) {
+		printf("%s: common object allocated %d times (should be 1)\n",
+			pt_case->name, count);
+		ret = -1;
+	}
+
 	return ret;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2 4/4] autotest: fix func reentrancy

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, April 05, 2016 8:36 AM
> To: dev@dpdk.org
> Cc: Richardson, Bruce
> Subject: [dpdk-dev] [PATCH v2 4/4] autotest: fix func reentrancy
> 
> The previous code in func_reentrancy autotest was doing in parallel
> something close to:
> 
>   name = "common_name";
>   do several times {
>       obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
>       if (obj == NULL && lookup(name) == NULL)
>           return TEST_FAIL;
>   }
> 
> This code is not safe. For instance:
> 
>    mempool_create() is called on core 0, it creates a ring. At the same
>    time on core 1, mempool_create() is called too and the creation of the
>    ring fails (EEXIST). But the mempool lookup can fail on core 1 if
>    the mempool is not added in the list by core 0.
> 
> This commit fixes the func_reentrancy autotest that now works with all
> tested class of objects.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Hi Olivier,

Could you include a "Fixes" line here?

Thanks,
Pablo

[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[Olivier Matz]

Seen while trying to fix the func_reentrancy autotest. The
series addresses several issues:

1/ Hash and lpm return a pointer to an existing object if the user requests the
   creation with an already existing name. This look dangerous: when an object
   is returned, the user does not know if it should be freed or not.

2/ There is a race condition in cuckoo_hash as the lock is not held in
   rte_hash_create(). We could find some cases where NULL is returned when the
   object already exists (ex: when rte_ring_create() fails).

3/ There is a race condition func_reentrancy that can fail even if the tested
   API behaves correctly.


RFC -> v1:

- split the patch in 4 patches
- on error, set rte_errno to EEXIST when relevant
- fix locking in cuckoo_hash creation

v1 -> v2:

- fix compilation issue in cuckoo hash
- update the hash test to conform to the new behavior
- rework locking modification in cuckoo_hash
- passed autotests: hash, lpm, lpm6, func_reentrancy

v2 -> v3:

- rebase against head
- add "Fixes:" in commit messages
- properly set lpm or hash pointers to NULL on error before returning

Olivier Matz (4):
  lpm: allocation of an existing object should fail
  hash: allocation of an existing object should fail
  hash: keep the list locked at creation
  autotest: fix func reentrancy

 app/test/test_func_reentrancy.c   | 31 +++++++++++------
 app/test/test_hash.c              | 65 +++++++++++++----------------------
 app/test/test_lpm6.c              |  2 +-
 lib/librte_hash/rte_cuckoo_hash.c | 72 +++++++++++++++++++++++----------------
 lib/librte_hash/rte_fbk_hash.c    |  5 ++-
 lib/librte_lpm/rte_lpm.c          | 10 ++++--
 lib/librte_lpm/rte_lpm6.c         |  5 ++-
 7 files changed, 103 insertions(+), 87 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v3 1/4] lpm: allocation of an existing object should fail

[Olivier Matz]

Change rte_lpm*_create() functions to return NULL and set rte_errno to
EEXIST when the object name already exists. This is the behavior
described in the API documentation in the header file.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the lpm API more consistent with the other
APIs (mempool, rings, ...).

Fixes: 899d8bc9b3 ("lpm: make tailq fully local")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_lpm6.c      |  2 +-
 lib/librte_lpm/rte_lpm.c  | 10 ++++++++--
 lib/librte_lpm/rte_lpm6.c |  5 ++++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/app/test/test_lpm6.c b/app/test/test_lpm6.c
index 1f88d7a..b464342 100644
--- a/app/test/test_lpm6.c
+++ b/app/test/test_lpm6.c
@@ -222,7 +222,7 @@ test1(void)
 
 	/* rte_lpm6_create: lpm name == LPM2 */
 	lpm3 = rte_lpm6_create("LPM1", SOCKET_ID_ANY, &config);
-	TEST_LPM_ASSERT(lpm3 == lpm1);
+	TEST_LPM_ASSERT(lpm3 == NULL);
 
 	rte_lpm6_free(lpm1);
 	rte_lpm6_free(lpm2);
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index bd3563f..8bdf606 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -209,8 +209,11 @@ rte_lpm_create_v20(const char *name, int socket_id, int max_rules,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	lpm = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -280,8 +283,11 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	lpm = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 4c44cd7..ba4353c 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	lpm = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM6_TAILQ_ENTRY", sizeof(*te), 0);
-- 
2.1.4

[dpdk-dev] [PATCH v3 2/4] hash: allocation of an existing object should fail

[Olivier Matz]

Change rte_hash*_create() functions to return NULL and set rte_errno to
EEXIST when the object name already exists. This is the behavior
described in the API documentation in the header file.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the hash API more consistent with the other
APIs (mempool, rings, ...).

Fixes: 4542f89397 ("hash: make tailq fully local")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_hash.c              | 65 ++++++++++++++-------------------------
 lib/librte_hash/rte_cuckoo_hash.c |  6 ++--
 lib/librte_hash/rte_fbk_hash.c    |  5 ++-
 3 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/app/test/test_hash.c b/app/test/test_hash.c
index 2f3d884..adbdb4a 100644
--- a/app/test/test_hash.c
+++ b/app/test/test_hash.c
@@ -805,15 +805,11 @@ fbk_hash_unit_test(void)
 	RETURN_IF_ERROR_FBK(handle == NULL, "fbk hash creation should have succeeded");
 
 	tmp = rte_fbk_hash_create(&invalid_params_same_name_2);
-	RETURN_IF_ERROR_FBK(tmp == NULL, "fbk hash creation should have succeeded");
-	if (tmp != handle) {
-			printf("ERROR line %d: hashes should have been the same\n", __LINE__);
-			rte_fbk_hash_free(handle);
-			rte_fbk_hash_free(tmp);
-			return -1;
-	}
+	if (tmp != NULL)
+		rte_fbk_hash_free(tmp);
+	RETURN_IF_ERROR_FBK(tmp != NULL, "fbk hash creation should have failed");
 
-	/* we are not freeing tmp or handle here because we need a hash list
+	/* we are not freeing  handle here because we need a hash list
 	 * to be not empty for the next test */
 
 	/* create a hash in non-empty list - good for coverage */
@@ -988,7 +984,7 @@ static int test_fbk_hash_find_existing(void)
  */
 static int test_hash_creation_with_bad_parameters(void)
 {
-	struct rte_hash *handle;
+	struct rte_hash *handle, *tmp;
 	struct rte_hash_parameters params;
 
 	handle = rte_hash_create(NULL);
@@ -1038,7 +1034,23 @@ static int test_hash_creation_with_bad_parameters(void)
 		return -1;
 	}
 
+	/* test with same name should fail */
+	memcpy(&params, &ut_params, sizeof(params));
+	params.name = "same_name";
+	handle = rte_hash_create(&params);
+	if (handle == NULL) {
+		printf("Cannot create first hash table with 'same_name'\n");
+		return -1;
+	}
+	tmp = rte_hash_create(&params);
+	if (tmp != NULL) {
+		printf("Creation of hash table with same name should fail\n");
+		rte_hash_free(handle);
+		rte_hash_free(tmp);
+		return -1;
+	}
 	rte_hash_free(handle);
+
 	printf("# Test successful. No more errors expected\n");
 
 	return 0;
@@ -1051,12 +1063,12 @@ static int test_hash_creation_with_bad_parameters(void)
 static int
 test_hash_creation_with_good_parameters(void)
 {
-	struct rte_hash *handle, *tmp;
+	struct rte_hash *handle;
 	struct rte_hash_parameters params;
 
 	/* create with null hash function - should choose DEFAULT_HASH_FUNC */
 	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "same_name";
+	params.name = "name";
 	params.hash_func = NULL;
 	handle = rte_hash_create(&params);
 	if (handle == NULL) {
@@ -1064,37 +1076,6 @@ test_hash_creation_with_good_parameters(void)
 		return -1;
 	}
 
-	/* this test is trying to create a hash with the same name as previous one.
-	 * this should return a pointer to the hash we previously created.
-	 * the previous hash isn't freed exactly for the purpose of it being in
-	 * the hash list.
-	 */
-	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "same_name";
-	tmp = rte_hash_create(&params);
-
-	/* check if the returned handle is actually equal to the previous hash */
-	if (handle != tmp) {
-		rte_hash_free(handle);
-		rte_hash_free(tmp);
-		printf("Creating hash with existing name was successful\n");
-		return -1;
-	}
-
-	/* try creating hash when there already are hashes in the list.
-	 * the previous hash is not freed to have a non-empty hash list.
-	 * the other hash that's in the list is still pointed to by "handle" var.
-	 */
-	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "different_name";
-	tmp = rte_hash_create(&params);
-	if (tmp == NULL) {
-		rte_hash_free(handle);
-		printf("Creating hash with valid parameters failed\n");
-		return -1;
-	}
-
-	rte_hash_free(tmp);
 	rte_hash_free(handle);
 
 	return 0;
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 04e95b7..b00cc12 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -299,8 +299,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	/* Guarantee there's no existing */
 	h = rte_hash_find_existing(params->name);
-	if (h != NULL)
-		return h;
+	if (h != NULL) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
diff --git a/lib/librte_hash/rte_fbk_hash.c b/lib/librte_hash/rte_fbk_hash.c
index 8752a47..55c9f35 100644
--- a/lib/librte_hash/rte_fbk_hash.c
+++ b/lib/librte_hash/rte_fbk_hash.c
@@ -140,8 +140,11 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
 		if (strncmp(params->name, ht->name, RTE_FBK_HASH_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	ht = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	te = rte_zmalloc("FBK_HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
-- 
2.1.4

[dpdk-dev] [PATCH v3 3/4] hash: keep the list locked at creation

[Olivier Matz]

To avoid a race condition while creating a new hash object, the
list has to be locked before the lookup, and released only once the
new object is added in the list.

As the lock is held by the rte_ring_create(), move its creation at the
beginning of the function and only take the lock after the ring is
created to avoid a deadlock.

Fixes: 48a3991196 ("hash: replace with cuckoo hash implementation")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 70 ++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index b00cc12..7b7d1f8 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -295,19 +295,48 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT)
 		hw_trans_mem_support = 1;
 
+	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
+	if (hw_trans_mem_support)
+		/*
+		 * Increase number of slots by total number of indices
+		 * that can be stored in the lcore caches
+		 * except for the first cache
+		 */
+		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
+					LCORE_CACHE_SIZE + 1;
+	else
+		num_key_slots = params->entries + 1;
+
+	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
+	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
+			params->socket_id, 0);
+	if (r == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err;
+	}
+
 	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
 
-	/* Guarantee there's no existing */
-	h = rte_hash_find_existing(params->name);
-	if (h != NULL) {
+	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+
+	/* guarantee there's no existing: this is normally already checked
+	 * by ring creation above */
+	TAILQ_FOREACH(te, hash_list, next) {
+		h = (struct rte_hash *) te->data;
+		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
+			break;
+	}
+	h = NULL;
+	if (te != NULL) {
 		rte_errno = EEXIST;
-		return NULL;
+		te = NULL;
+		goto err_unlock;
 	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
 		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct rte_hash),
@@ -315,7 +344,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (h == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	const uint32_t num_buckets = rte_align32pow2(params->entries)
@@ -327,23 +356,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (buckets == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	const uint32_t key_entry_size = sizeof(struct rte_hash_key) + params->key_len;
-
-	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
-	if (hw_trans_mem_support)
-		/*
-		 * Increase number of slots by total number of indices
-		 * that can be stored in the lcore caches
-		 * except for the first cache
-		 */
-		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
-					LCORE_CACHE_SIZE + 1;
-	else
-		num_key_slots = params->entries + 1;
-
 	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
 
 	k = rte_zmalloc_socket(NULL, key_tbl_size,
@@ -351,7 +367,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (k == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 /*
@@ -393,14 +409,6 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 #endif
 
-	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
-	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
-			params->socket_id, 0);
-	if (r == NULL) {
-		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
-	}
-
 	if (hw_trans_mem_support) {
 		h->local_free_slots = rte_zmalloc_socket(NULL,
 				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
@@ -427,13 +435,15 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	for (i = 1; i < params->entries + 1; i++)
 		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
 
-	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
 	te->data = (void *) h;
 	TAILQ_INSERT_TAIL(hash_list, te, next);
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
 	return h;
+err_unlock:
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 err:
+	rte_ring_free(r);
 	rte_free(te);
 	rte_free(h);
 	rte_free(buckets);
-- 
2.1.4

[dpdk-dev] [PATCH v3 4/4] autotest: fix func reentrancy

[Olivier Matz]

The previous code in func_reentrancy autotest was doing in parallel
something close to:

  name = "common_name";
  do several times {
      obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
      if (obj == NULL && lookup(name) == NULL)
          return TEST_FAIL;
  }

This code is not safe. For instance:

   mempool_create() is called on core 0, it creates a ring. At the same
   time on core 1, mempool_create() is called too and the creation of the
   ring fails (EEXIST). But the mempool lookup can fail on core 1 if
   the mempool is not added in the list by core 0.

This commit fixes the func_reentrancy autotest that now works with all
tested class of objects.

Fixes: 104a92bd02 ("app: add reentrancy tests")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_func_reentrancy.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/app/test/test_func_reentrancy.c b/app/test/test_func_reentrancy.c
index 5d09296..300a3bc 100644
--- a/app/test/test_func_reentrancy.c
+++ b/app/test/test_func_reentrancy.c
@@ -83,6 +83,7 @@ typedef void (*case_clean_t)(unsigned lcore_id);
 
 #define MAX_LCORES	RTE_MAX_MEMZONE / (MAX_ITER_TIMES * 4U)
 
+static rte_atomic32_t obj_count = RTE_ATOMIC32_INIT(0);
 static rte_atomic32_t synchro = RTE_ATOMIC32_INIT(0);
 
 #define WAIT_SYNCHRO_FOR_SLAVES()   do{ \
@@ -100,6 +101,7 @@ test_eal_init_once(__attribute__((unused)) void *arg)
 
 	WAIT_SYNCHRO_FOR_SLAVES();
 
+	rte_atomic32_set(&obj_count, 1); /* silent the check in the caller */
 	if (rte_eal_init(0, NULL) != -1)
 		return -1;
 
@@ -122,8 +124,8 @@ ring_create_lookup(__attribute__((unused)) void *arg)
 	/* create the same ring simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		rp = rte_ring_create("fr_test_once", 4096, SOCKET_ID_ANY, 0);
-		if ((NULL == rp) && (rte_ring_lookup("fr_test_once") == NULL))
-			return -1;
+		if (rp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -172,8 +174,8 @@ mempool_create_lookup(__attribute__((unused)) void *arg)
 					NULL, NULL,
 					my_obj_init, NULL,
 					SOCKET_ID_ANY, 0);
-		if ((NULL == mp) && (rte_mempool_lookup("fr_test_once") == NULL))
-			return -1;
+		if (mp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -238,8 +240,8 @@ hash_create_free(__attribute__((unused)) void *arg)
 	hash_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_hash_create(&hash_params);
-		if ((NULL == handle) && (rte_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple times simultaneously */
@@ -306,8 +308,8 @@ fbk_create_free(__attribute__((unused)) void *arg)
 	fbk_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_fbk_hash_create(&fbk_params);
-		if ((NULL == handle) && (rte_fbk_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -372,8 +374,8 @@ lpm_create_free(__attribute__((unused)) void *arg)
 	/* create the same lpm simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		lpm = rte_lpm_create("fr_test_once",  SOCKET_ID_ANY, &config);
-		if ((NULL == lpm) && (rte_lpm_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (lpm != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -432,10 +434,12 @@ launch_test(struct test_case *pt_case)
 	unsigned lcore_id;
 	unsigned cores_save = rte_lcore_count();
 	unsigned cores = RTE_MIN(cores_save, MAX_LCORES);
+	unsigned count;
 
 	if (pt_case->func == NULL)
 		return -1;
 
+	rte_atomic32_set(&obj_count, 0);
 	rte_atomic32_set(&synchro, 0);
 
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
@@ -462,6 +466,13 @@ launch_test(struct test_case *pt_case)
 			pt_case->clean(lcore_id);
 	}
 
+	count = rte_atomic32_read(&obj_count);
+	if (count != 1) {
+		printf("%s: common object allocated %d times (should be 1)\n",
+			pt_case->name, count);
+		ret = -1;
+	}
+
 	return ret;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[Thomas Monjalon]

2016-04-05 13:53, Olivier Matz:
> Seen while trying to fix the func_reentrancy autotest. The
> series addresses several issues:
> 
> 1/ Hash and lpm return a pointer to an existing object if the user requests the
>    creation with an already existing name. This look dangerous: when an object
>    is returned, the user does not know if it should be freed or not.
> 
> 2/ There is a race condition in cuckoo_hash as the lock is not held in
>    rte_hash_create(). We could find some cases where NULL is returned when the
>    object already exists (ex: when rte_ring_create() fails).
> 
> 3/ There is a race condition func_reentrancy that can fail even if the tested
>    API behaves correctly.

Pablo, Bruce,
What do you think of these fixes for 16.04?

Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, April 05, 2016 4:52 PM
> To: Richardson, Bruce; De Lara Guarch, Pablo
> Cc: dev@dpdk.org; Olivier Matz
> Subject: Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
> 
> 2016-04-05 13:53, Olivier Matz:
> > Seen while trying to fix the func_reentrancy autotest. The
> > series addresses several issues:
> >
> > 1/ Hash and lpm return a pointer to an existing object if the user requests
> the
> >    creation with an already existing name. This look dangerous: when an
> object
> >    is returned, the user does not know if it should be freed or not.
> >
> > 2/ There is a race condition in cuckoo_hash as the lock is not held in
> >    rte_hash_create(). We could find some cases where NULL is returned
> when the
> >    object already exists (ex: when rte_ring_create() fails).
> >
> > 3/ There is a race condition func_reentrancy that can fail even if the tested
> >    API behaves correctly.
> 
> Pablo, Bruce,
> What do you think of these fixes for 16.04?

I was reviewing them yesterday, but couldn't finish in time. I will do now.

Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, April 05, 2016 12:54 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce; De Lara Guarch, Pablo
> Subject: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
> 
> Seen while trying to fix the func_reentrancy autotest. The
> series addresses several issues:
> 
> 1/ Hash and lpm return a pointer to an existing object if the user requests the
>    creation with an already existing name. This look dangerous: when an
> object
>    is returned, the user does not know if it should be freed or not.
> 
> 2/ There is a race condition in cuckoo_hash as the lock is not held in
>    rte_hash_create(). We could find some cases where NULL is returned when
> the
>    object already exists (ex: when rte_ring_create() fails).
> 
> 3/ There is a race condition func_reentrancy that can fail even if the tested
>    API behaves correctly.
> 
> 
> RFC -> v1:
> 
> - split the patch in 4 patches
> - on error, set rte_errno to EEXIST when relevant
> - fix locking in cuckoo_hash creation
> 
> v1 -> v2:
> 
> - fix compilation issue in cuckoo hash
> - update the hash test to conform to the new behavior
> - rework locking modification in cuckoo_hash
> - passed autotests: hash, lpm, lpm6, func_reentrancy
> 
> v2 -> v3:
> 
> - rebase against head
> - add "Fixes:" in commit messages
> - properly set lpm or hash pointers to NULL on error before returning
> 
> Olivier Matz (4):
>   lpm: allocation of an existing object should fail
>   hash: allocation of an existing object should fail
>   hash: keep the list locked at creation
>   autotest: fix func reentrancy
> 
>  app/test/test_func_reentrancy.c   | 31 +++++++++++------
>  app/test/test_hash.c              | 65 +++++++++++++----------------------
>  app/test/test_lpm6.c              |  2 +-
>  lib/librte_hash/rte_cuckoo_hash.c | 72 +++++++++++++++++++++++------------
> ----
>  lib/librte_hash/rte_fbk_hash.c    |  5 ++-
>  lib/librte_lpm/rte_lpm.c          | 10 ++++--
>  lib/librte_lpm/rte_lpm6.c         |  5 ++-
>  7 files changed, 103 insertions(+), 87 deletions(-)
> 
> --
> 2.1.4

I wonder if you should include something in release notes.
We are fixing the API, so I guess we don't need to follow the deprecation process, but at least a note in the documentation?

Apart from that,

Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Thanks!

Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[Olivier Matz]

On 04/06/2016 12:32 PM, De Lara Guarch, Pablo wrote:
> 
> I wonder if you should include something in release notes.
> We are fixing the API, so I guess we don't need to follow the deprecation process, but at least a note in the documentation?

Good idea, I'll send a v4 with the deprecation notice.

> Apart from that,
> 
> Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Thank you for the review.

Olivier

Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, April 06, 2016 12:15 PM
> To: De Lara Guarch, Pablo; dev@dpdk.org
> Cc: Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
> 
> 
> 
> On 04/06/2016 12:32 PM, De Lara Guarch, Pablo wrote:
> >
> > I wonder if you should include something in release notes.
> > We are fixing the API, so I guess we don't need to follow the deprecation
> process, but at least a note in the documentation?
> 
> Good idea, I'll send a v4 with the deprecation notice.

Well, not sure if this needs a deprecation notice.
I mean, it is an API fix: yes, this is changing what the function returns
in a particular situation (when the hash/lpm already exists) ,
but it was going against the API documentation, so a deprecation notice should not be necessary.
(just my opinion, I could be quite wrong here :P).

I was thinking more on adding a note in Resolved issues.

Thanks,
Pablo
> 
> > Apart from that,
> >
> > Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 
> Thank you for the review.
> 
> Olivier

Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash

[Olivier Matz]

On 04/06/2016 01:20 PM, De Lara Guarch, Pablo wrote:
>> On 04/06/2016 12:32 PM, De Lara Guarch, Pablo wrote:
>>>
>>> I wonder if you should include something in release notes.
>>> We are fixing the API, so I guess we don't need to follow the deprecation
>> process, but at least a note in the documentation?
>>
>> Good idea, I'll send a v4 with the deprecation notice.
> 
> Well, not sure if this needs a deprecation notice.
> I mean, it is an API fix: yes, this is changing what the function returns
> in a particular situation (when the hash/lpm already exists) ,
> but it was going against the API documentation, so a deprecation notice should not be necessary.
> (just my opinion, I could be quite wrong here :P).
> 
> I was thinking more on adding a note in Resolved issues.

Yes, agree, it's a bug fix.

Another argument to not follow the API change process is that
the initial behavior was to return EEXIST, but it was changed
by this commit:

  http://dpdk.org/browse/dpdk/commit/?id=916e4f4f4e

By the way, the "Fixes:" line was not referencing this commit
in the v3, I'll also change that in v4.

Thanks,
Olivier

[dpdk-dev] [PATCH v4 0/4] fix creation of duplicate lpm and hash

[Olivier Matz]

Seen while trying to fix the func_reentrancy autotest. The
series addresses several issues:

1/ Hash and lpm return a pointer to an existing object if the user requests the
   creation with an already existing name. This look dangerous: when an object
   is returned, the user does not know if it should be freed or not.

2/ There is a race condition in cuckoo_hash as the lock is not held in
   rte_hash_create(). We could find some cases where NULL is returned when the
   object already exists (ex: when rte_ring_create() fails).

3/ There is a race condition func_reentrancy that can fail even if the tested
   API behaves correctly.


RFC -> v1:

- split the patch in 4 patches
- on error, set rte_errno to EEXIST when relevant
- fix locking in cuckoo_hash creation

v1 -> v2:

- fix compilation issue in cuckoo hash
- update the hash test to conform to the new behavior
- rework locking modification in cuckoo_hash
- passed autotests: hash, lpm, lpm6, func_reentrancy

v2 -> v3:

- rebase against head
- add "Fixes:" in commit messages
- properly set lpm or hash pointers to NULL on error before returning

v3 -> v4:

- add entries in the release note to explain the API changes
  in lpm and hash
- correct the "Fixes:" line in the first 2 patches

Olivier Matz (4):
  lpm: allocation of an existing object should fail
  hash: allocation of an existing object should fail
  hash: keep the list locked at creation
  autotest: fix func reentrancy

 app/test/test_func_reentrancy.c        | 31 ++++++++++-----
 app/test/test_hash.c                   | 65 +++++++++++-------------------
 app/test/test_lpm6.c                   |  2 +-
 doc/guides/rel_notes/release_16_04.rst | 17 ++++++++
 lib/librte_hash/rte_cuckoo_hash.c      | 72 ++++++++++++++++++++--------------
 lib/librte_hash/rte_fbk_hash.c         |  5 ++-
 lib/librte_lpm/rte_lpm.c               | 10 ++++-
 lib/librte_lpm/rte_lpm6.c              |  5 ++-
 8 files changed, 120 insertions(+), 87 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v4 1/4] lpm: allocation of an existing object should fail

[Olivier Matz]

Change rte_lpm*_create() functions to return NULL and set rte_errno to
EEXIST when the object name already exists. This is the behavior
described in the API documentation in the header file.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the lpm API more consistent with the other
APIs (mempool, rings, ...).

Fixes: 916e4f4f4e ("memory: fix for multi process support")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 app/test/test_lpm6.c                   |  2 +-
 doc/guides/rel_notes/release_16_04.rst |  9 +++++++++
 lib/librte_lpm/rte_lpm.c               | 10 ++++++++--
 lib/librte_lpm/rte_lpm6.c              |  5 ++++-
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/app/test/test_lpm6.c b/app/test/test_lpm6.c
index 1f88d7a..b464342 100644
--- a/app/test/test_lpm6.c
+++ b/app/test/test_lpm6.c
@@ -222,7 +222,7 @@ test1(void)
 
 	/* rte_lpm6_create: lpm name == LPM2 */
 	lpm3 = rte_lpm6_create("LPM1", SOCKET_ID_ANY, &config);
-	TEST_LPM_ASSERT(lpm3 == lpm1);
+	TEST_LPM_ASSERT(lpm3 == NULL);
 
 	rte_lpm6_free(lpm1);
 	rte_lpm6_free(lpm2);
diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index d6e358f..9a6fd4a 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -432,6 +432,15 @@ Libraries
 
   Fixed core dump issue on txq and swq when dropless is set to yes.
 
+* **lpm: Fixed return value when allocating an existing object.**
+
+  Changed the ``rte_lpm*_create()`` functions to return ``NULL`` and set
+  ``rte_errno`` to ``EEXIST` when the object name already exists. This is
+  the behavior described in the API documentation in the header file.
+  The previous behavior was to return a pointer to the existing object in
+  that case, preventing the caller to know if the object had to be freed
+  or not.
+
 
 Examples
 ~~~~~~~~
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index bd3563f..8bdf606 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -209,8 +209,11 @@ rte_lpm_create_v20(const char *name, int socket_id, int max_rules,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	lpm = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -280,8 +283,11 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	lpm = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 4c44cd7..ba4353c 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
 		if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	lpm = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	/* allocate tailq entry */
 	te = rte_zmalloc("LPM6_TAILQ_ENTRY", sizeof(*te), 0);
-- 
2.1.4

[dpdk-dev] [PATCH v4 2/4] hash: allocation of an existing object should fail

[Olivier Matz]

Change rte_hash*_create() functions to return NULL and set rte_errno to
EEXIST when the object name already exists. This is the behavior
described in the API documentation in the header file.

These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.

Doing this change also makes the hash API more consistent with the other
APIs (mempool, rings, ...).

Fixes: 916e4f4f4e ("memory: fix for multi process support")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 app/test/test_hash.c                   | 65 ++++++++++++----------------------
 doc/guides/rel_notes/release_16_04.rst |  8 +++++
 lib/librte_hash/rte_cuckoo_hash.c      |  6 ++--
 lib/librte_hash/rte_fbk_hash.c         |  5 ++-
 4 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/app/test/test_hash.c b/app/test/test_hash.c
index 2f3d884..adbdb4a 100644
--- a/app/test/test_hash.c
+++ b/app/test/test_hash.c
@@ -805,15 +805,11 @@ fbk_hash_unit_test(void)
 	RETURN_IF_ERROR_FBK(handle == NULL, "fbk hash creation should have succeeded");
 
 	tmp = rte_fbk_hash_create(&invalid_params_same_name_2);
-	RETURN_IF_ERROR_FBK(tmp == NULL, "fbk hash creation should have succeeded");
-	if (tmp != handle) {
-			printf("ERROR line %d: hashes should have been the same\n", __LINE__);
-			rte_fbk_hash_free(handle);
-			rte_fbk_hash_free(tmp);
-			return -1;
-	}
+	if (tmp != NULL)
+		rte_fbk_hash_free(tmp);
+	RETURN_IF_ERROR_FBK(tmp != NULL, "fbk hash creation should have failed");
 
-	/* we are not freeing tmp or handle here because we need a hash list
+	/* we are not freeing  handle here because we need a hash list
 	 * to be not empty for the next test */
 
 	/* create a hash in non-empty list - good for coverage */
@@ -988,7 +984,7 @@ static int test_fbk_hash_find_existing(void)
  */
 static int test_hash_creation_with_bad_parameters(void)
 {
-	struct rte_hash *handle;
+	struct rte_hash *handle, *tmp;
 	struct rte_hash_parameters params;
 
 	handle = rte_hash_create(NULL);
@@ -1038,7 +1034,23 @@ static int test_hash_creation_with_bad_parameters(void)
 		return -1;
 	}
 
+	/* test with same name should fail */
+	memcpy(&params, &ut_params, sizeof(params));
+	params.name = "same_name";
+	handle = rte_hash_create(&params);
+	if (handle == NULL) {
+		printf("Cannot create first hash table with 'same_name'\n");
+		return -1;
+	}
+	tmp = rte_hash_create(&params);
+	if (tmp != NULL) {
+		printf("Creation of hash table with same name should fail\n");
+		rte_hash_free(handle);
+		rte_hash_free(tmp);
+		return -1;
+	}
 	rte_hash_free(handle);
+
 	printf("# Test successful. No more errors expected\n");
 
 	return 0;
@@ -1051,12 +1063,12 @@ static int test_hash_creation_with_bad_parameters(void)
 static int
 test_hash_creation_with_good_parameters(void)
 {
-	struct rte_hash *handle, *tmp;
+	struct rte_hash *handle;
 	struct rte_hash_parameters params;
 
 	/* create with null hash function - should choose DEFAULT_HASH_FUNC */
 	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "same_name";
+	params.name = "name";
 	params.hash_func = NULL;
 	handle = rte_hash_create(&params);
 	if (handle == NULL) {
@@ -1064,37 +1076,6 @@ test_hash_creation_with_good_parameters(void)
 		return -1;
 	}
 
-	/* this test is trying to create a hash with the same name as previous one.
-	 * this should return a pointer to the hash we previously created.
-	 * the previous hash isn't freed exactly for the purpose of it being in
-	 * the hash list.
-	 */
-	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "same_name";
-	tmp = rte_hash_create(&params);
-
-	/* check if the returned handle is actually equal to the previous hash */
-	if (handle != tmp) {
-		rte_hash_free(handle);
-		rte_hash_free(tmp);
-		printf("Creating hash with existing name was successful\n");
-		return -1;
-	}
-
-	/* try creating hash when there already are hashes in the list.
-	 * the previous hash is not freed to have a non-empty hash list.
-	 * the other hash that's in the list is still pointed to by "handle" var.
-	 */
-	memcpy(&params, &ut_params, sizeof(params));
-	params.name = "different_name";
-	tmp = rte_hash_create(&params);
-	if (tmp == NULL) {
-		rte_hash_free(handle);
-		printf("Creating hash with valid parameters failed\n");
-		return -1;
-	}
-
-	rte_hash_free(tmp);
 	rte_hash_free(handle);
 
 	return 0;
diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index 9a6fd4a..bc5c533 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -441,6 +441,14 @@ Libraries
   that case, preventing the caller to know if the object had to be freed
   or not.
 
+* **hash: Fixed return value when allocating an existing hash table.**
+
+  Changed the ``rte_hash*_create()`` functions to return ``NULL`` and set
+  ``rte_errno`` to ``EEXIST` when the object name already exists. This is
+  the behavior described in the API documentation in the header file.
+  The previous behavior was to return a pointer to the existing object in
+  that case, preventing the caller to know if the object had to be freed
+  or not.
 
 Examples
 ~~~~~~~~
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 04e95b7..b00cc12 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -299,8 +299,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	/* Guarantee there's no existing */
 	h = rte_hash_find_existing(params->name);
-	if (h != NULL)
-		return h;
+	if (h != NULL) {
+		rte_errno = EEXIST;
+		return NULL;
+	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
diff --git a/lib/librte_hash/rte_fbk_hash.c b/lib/librte_hash/rte_fbk_hash.c
index 8752a47..55c9f35 100644
--- a/lib/librte_hash/rte_fbk_hash.c
+++ b/lib/librte_hash/rte_fbk_hash.c
@@ -140,8 +140,11 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
 		if (strncmp(params->name, ht->name, RTE_FBK_HASH_NAMESIZE) == 0)
 			break;
 	}
-	if (te != NULL)
+	ht = NULL;
+	if (te != NULL) {
+		rte_errno = EEXIST;
 		goto exit;
+	}
 
 	te = rte_zmalloc("FBK_HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
-- 
2.1.4

[dpdk-dev] [PATCH v4 3/4] hash: keep the list locked at creation

[Olivier Matz]

To avoid a race condition while creating a new hash object, the
list has to be locked before the lookup, and released only once the
new object is added in the list.

As the lock is held by the rte_ring_create(), move its creation at the
beginning of the function and only take the lock after the ring is
created to avoid a deadlock.

Fixes: 48a3991196 ("hash: replace with cuckoo hash implementation")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 70 ++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index b00cc12..7b7d1f8 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -295,19 +295,48 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT)
 		hw_trans_mem_support = 1;
 
+	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
+	if (hw_trans_mem_support)
+		/*
+		 * Increase number of slots by total number of indices
+		 * that can be stored in the lcore caches
+		 * except for the first cache
+		 */
+		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
+					LCORE_CACHE_SIZE + 1;
+	else
+		num_key_slots = params->entries + 1;
+
+	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
+	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
+			params->socket_id, 0);
+	if (r == NULL) {
+		RTE_LOG(ERR, HASH, "memory allocation failed\n");
+		goto err;
+	}
+
 	snprintf(hash_name, sizeof(hash_name), "HT_%s", params->name);
 
-	/* Guarantee there's no existing */
-	h = rte_hash_find_existing(params->name);
-	if (h != NULL) {
+	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+
+	/* guarantee there's no existing: this is normally already checked
+	 * by ring creation above */
+	TAILQ_FOREACH(te, hash_list, next) {
+		h = (struct rte_hash *) te->data;
+		if (strncmp(params->name, h->name, RTE_HASH_NAMESIZE) == 0)
+			break;
+	}
+	h = NULL;
+	if (te != NULL) {
 		rte_errno = EEXIST;
-		return NULL;
+		te = NULL;
+		goto err_unlock;
 	}
 
 	te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
 		RTE_LOG(ERR, HASH, "tailq entry allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	h = (struct rte_hash *)rte_zmalloc_socket(hash_name, sizeof(struct rte_hash),
@@ -315,7 +344,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (h == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	const uint32_t num_buckets = rte_align32pow2(params->entries)
@@ -327,23 +356,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (buckets == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 	const uint32_t key_entry_size = sizeof(struct rte_hash_key) + params->key_len;
-
-	/* Store all keys and leave the first entry as a dummy entry for lookup_bulk */
-	if (hw_trans_mem_support)
-		/*
-		 * Increase number of slots by total number of indices
-		 * that can be stored in the lcore caches
-		 * except for the first cache
-		 */
-		num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
-					LCORE_CACHE_SIZE + 1;
-	else
-		num_key_slots = params->entries + 1;
-
 	const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
 
 	k = rte_zmalloc_socket(NULL, key_tbl_size,
@@ -351,7 +367,7 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
 	if (k == NULL) {
 		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
+		goto err_unlock;
 	}
 
 /*
@@ -393,14 +409,6 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
 #endif
 
-	snprintf(ring_name, sizeof(ring_name), "HT_%s", params->name);
-	r = rte_ring_create(ring_name, rte_align32pow2(num_key_slots),
-			params->socket_id, 0);
-	if (r == NULL) {
-		RTE_LOG(ERR, HASH, "memory allocation failed\n");
-		goto err;
-	}
-
 	if (hw_trans_mem_support) {
 		h->local_free_slots = rte_zmalloc_socket(NULL,
 				sizeof(struct lcore_cache) * RTE_MAX_LCORE,
@@ -427,13 +435,15 @@ rte_hash_create(const struct rte_hash_parameters *params)
 	for (i = 1; i < params->entries + 1; i++)
 		rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
 
-	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
 	te->data = (void *) h;
 	TAILQ_INSERT_TAIL(hash_list, te, next);
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
 	return h;
+err_unlock:
+	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 err:
+	rte_ring_free(r);
 	rte_free(te);
 	rte_free(h);
 	rte_free(buckets);
-- 
2.1.4

[dpdk-dev] [PATCH v4 4/4] autotest: fix func reentrancy

[Olivier Matz]

The previous code in func_reentrancy autotest was doing in parallel
something close to:

  name = "common_name";
  do several times {
      obj = allocate_an_object(name)   // obj = ring, mempool, hash, lpm, ...
      if (obj == NULL && lookup(name) == NULL)
          return TEST_FAIL;
  }

This code is not safe. For instance:

   mempool_create() is called on core 0, it creates a ring. At the same
   time on core 1, mempool_create() is called too and the creation of the
   ring fails (EEXIST). But the mempool lookup can fail on core 1 if
   the mempool is not added in the list by core 0.

This commit fixes the func_reentrancy autotest that now works with all
tested class of objects.

Fixes: 104a92bd02 ("app: add reentrancy tests")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 app/test/test_func_reentrancy.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/app/test/test_func_reentrancy.c b/app/test/test_func_reentrancy.c
index 5d09296..300a3bc 100644
--- a/app/test/test_func_reentrancy.c
+++ b/app/test/test_func_reentrancy.c
@@ -83,6 +83,7 @@ typedef void (*case_clean_t)(unsigned lcore_id);
 
 #define MAX_LCORES	RTE_MAX_MEMZONE / (MAX_ITER_TIMES * 4U)
 
+static rte_atomic32_t obj_count = RTE_ATOMIC32_INIT(0);
 static rte_atomic32_t synchro = RTE_ATOMIC32_INIT(0);
 
 #define WAIT_SYNCHRO_FOR_SLAVES()   do{ \
@@ -100,6 +101,7 @@ test_eal_init_once(__attribute__((unused)) void *arg)
 
 	WAIT_SYNCHRO_FOR_SLAVES();
 
+	rte_atomic32_set(&obj_count, 1); /* silent the check in the caller */
 	if (rte_eal_init(0, NULL) != -1)
 		return -1;
 
@@ -122,8 +124,8 @@ ring_create_lookup(__attribute__((unused)) void *arg)
 	/* create the same ring simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		rp = rte_ring_create("fr_test_once", 4096, SOCKET_ID_ANY, 0);
-		if ((NULL == rp) && (rte_ring_lookup("fr_test_once") == NULL))
-			return -1;
+		if (rp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -172,8 +174,8 @@ mempool_create_lookup(__attribute__((unused)) void *arg)
 					NULL, NULL,
 					my_obj_init, NULL,
 					SOCKET_ID_ANY, 0);
-		if ((NULL == mp) && (rte_mempool_lookup("fr_test_once") == NULL))
-			return -1;
+		if (mp != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create/lookup new ring several times */
@@ -238,8 +240,8 @@ hash_create_free(__attribute__((unused)) void *arg)
 	hash_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_hash_create(&hash_params);
-		if ((NULL == handle) && (rte_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple times simultaneously */
@@ -306,8 +308,8 @@ fbk_create_free(__attribute__((unused)) void *arg)
 	fbk_params.name = "fr_test_once";
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		handle = rte_fbk_hash_create(&fbk_params);
-		if ((NULL == handle) && (rte_fbk_hash_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (handle != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -372,8 +374,8 @@ lpm_create_free(__attribute__((unused)) void *arg)
 	/* create the same lpm simultaneously on all threads */
 	for (i = 0; i < MAX_ITER_TIMES; i++) {
 		lpm = rte_lpm_create("fr_test_once",  SOCKET_ID_ANY, &config);
-		if ((NULL == lpm) && (rte_lpm_find_existing("fr_test_once") == NULL))
-			return -1;
+		if (lpm != NULL)
+			rte_atomic32_inc(&obj_count);
 	}
 
 	/* create mutiple fbk tables simultaneously */
@@ -432,10 +434,12 @@ launch_test(struct test_case *pt_case)
 	unsigned lcore_id;
 	unsigned cores_save = rte_lcore_count();
 	unsigned cores = RTE_MIN(cores_save, MAX_LCORES);
+	unsigned count;
 
 	if (pt_case->func == NULL)
 		return -1;
 
+	rte_atomic32_set(&obj_count, 0);
 	rte_atomic32_set(&synchro, 0);
 
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
@@ -462,6 +466,13 @@ launch_test(struct test_case *pt_case)
 			pt_case->clean(lcore_id);
 	}
 
+	count = rte_atomic32_read(&obj_count);
+	if (count != 1) {
+		printf("%s: common object allocated %d times (should be 1)\n",
+			pt_case->name, count);
+		ret = -1;
+	}
+
 	return ret;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v4 0/4] fix creation of duplicate lpm and hash

[Thomas Monjalon]

2016-04-06 15:27, Olivier Matz:
> Seen while trying to fix the func_reentrancy autotest. The
> series addresses several issues:
> 
> 1/ Hash and lpm return a pointer to an existing object if the user requests the
>    creation with an already existing name. This look dangerous: when an object
>    is returned, the user does not know if it should be freed or not.
> 
> 2/ There is a race condition in cuckoo_hash as the lock is not held in
>    rte_hash_create(). We could find some cases where NULL is returned when the
>    object already exists (ex: when rte_ring_create() fails).
> 
> 3/ There is a race condition func_reentrancy that can fail even if the tested
>    API behaves correctly.

Applied, thanks