From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 6D13C2C1A for ; Tue, 15 Mar 2016 13:25:57 +0100 (CET) Received: from glumotte.dev.6wind.com (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id 7B8CB24ACF for ; Tue, 15 Mar 2016 13:25:14 +0100 (CET) From: Olivier Matz To: dev@dpdk.org Date: Tue, 15 Mar 2016 13:25:45 +0100 Message-Id: <1458044745-32764-1-git-send-email-olivier.matz@6wind.com> X-Mailer: git-send-email 2.1.4 Subject: [dpdk-dev] [RFC] hash/lpm: return NULL if the object exists X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Mar 2016 12:25:57 -0000 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 --- 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