[dpdk-dev,1/2] test/test_mbuf: Remove mempool global var

Message ID 20170511103044.14298-1-santosh.shukla@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Santosh Shukla May 11, 2017, 10:30 a.m. UTC
  Cc: stable@dpdk.org
Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
---
 test/test/test_mbuf.c | 100 +++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 49 deletions(-)
  

Comments

Santosh Shukla June 5, 2017, 5:04 a.m. UTC | #1
On Thursday 11 May 2017 04:00 PM, Santosh Shukla wrote:

> Cc: stable@dpdk.org
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> ---

Ping?
  
Olivier Matz June 8, 2017, 10:08 a.m. UTC | #2
Hi Santosh,

On Thu, 11 May 2017 16:00:43 +0530, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:
> Cc: stable@dpdk.org
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> ---
>  test/test/test_mbuf.c | 100 +++++++++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 49 deletions(-)
> 

Thank you for doing this fix.

My only comment is that commit introduces a memory leak, which
is fixed by your second patch.

The behavior before your patch was to allocate the pools once
but never free them.
The behavior after this patch is to allocate the pools each time
the test is started and never free them.

After your 2 patches, the behavior is good.
So I suggest to merge them into one commit. What do you think?


Olivier
  
Santosh Shukla June 8, 2017, 11:21 a.m. UTC | #3
Hi Olivier,

On Thursday 08 June 2017 03:38 PM, Olivier Matz wrote:

> Hi Santosh,
>
> On Thu, 11 May 2017 16:00:43 +0530, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:
>> Cc: stable@dpdk.org
>> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>> ---
>>  test/test/test_mbuf.c | 100 +++++++++++++++++++++++++-------------------------
>>  1 file changed, 51 insertions(+), 49 deletions(-)
>>
> Thank you for doing this fix.
>
> My only comment is that commit introduces a memory leak, which
> is fixed by your second patch.
>
> The behavior before your patch was to allocate the pools once
> but never free them.
> The behavior after this patch is to allocate the pools each time
> the test is started and never free them.
>
> After your 2 patches, the behavior is good.
> So I suggest to merge them into one commit. What do you think?
>
Yes. Will send v2.
Thanks.

> Olivier
>
  

Patch

diff --git a/test/test/test_mbuf.c b/test/test/test_mbuf.c
index d3ea812e5..ba93ac783 100644
--- a/test/test/test_mbuf.c
+++ b/test/test/test_mbuf.c
@@ -82,13 +82,8 @@ 
 
 #define MAKE_STRING(x)          # x
 
-static struct rte_mempool *pktmbuf_pool = NULL;
-static struct rte_mempool *pktmbuf_pool2 = NULL;
-
 #ifdef RTE_MBUF_REFCNT_ATOMIC
 
-static struct rte_mempool *refcnt_pool = NULL;
-static struct rte_ring *refcnt_mbuf_ring = NULL;
 static volatile uint32_t refcnt_stop_slaves;
 static unsigned refcnt_lcore[RTE_MAX_LCORE];
 
@@ -144,7 +139,7 @@  static unsigned refcnt_lcore[RTE_MAX_LCORE];
  * test data manipulation in mbuf with non-ascii data
  */
 static int
-test_pktmbuf_with_non_ascii_data(void)
+test_pktmbuf_with_non_ascii_data(struct rte_mempool *pktmbuf_pool)
 {
 	struct rte_mbuf *m = NULL;
 	char *data;
@@ -182,7 +177,7 @@  test_pktmbuf_with_non_ascii_data(void)
  * test data manipulation in mbuf
  */
 static int
-test_one_pktmbuf(void)
+test_one_pktmbuf(struct rte_mempool *pktmbuf_pool)
 {
 	struct rte_mbuf *m = NULL;
 	char *data, *data2, *hdr;
@@ -328,7 +323,7 @@  test_one_pktmbuf(void)
 }
 
 static int
-testclone_testupdate_testdetach(void)
+testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 {
 	struct rte_mbuf *m = NULL;
 	struct rte_mbuf *clone = NULL;
@@ -432,7 +427,8 @@  testclone_testupdate_testdetach(void)
 }
 
 static int
-test_attach_from_different_pool(void)
+test_attach_from_different_pool(struct rte_mempool *pktmbuf_pool,
+				struct rte_mempool *pktmbuf_pool2)
 {
 	struct rte_mbuf *m = NULL;
 	struct rte_mbuf *clone = NULL;
@@ -542,7 +538,7 @@  test_attach_from_different_pool(void)
  * test allocation and free of mbufs
  */
 static int
-test_pktmbuf_pool(void)
+test_pktmbuf_pool(struct rte_mempool *pktmbuf_pool)
 {
 	unsigned i;
 	struct rte_mbuf *m[NB_MBUF];
@@ -583,7 +579,7 @@  test_pktmbuf_pool(void)
  * test that the pointer to the data on a packet mbuf is set properly
  */
 static int
-test_pktmbuf_pool_ptr(void)
+test_pktmbuf_pool_ptr(struct rte_mempool *pktmbuf_pool)
 {
 	unsigned i;
 	struct rte_mbuf *m[NB_MBUF];
@@ -636,7 +632,7 @@  test_pktmbuf_pool_ptr(void)
 }
 
 static int
-test_pktmbuf_free_segment(void)
+test_pktmbuf_free_segment(struct rte_mempool *pktmbuf_pool)
 {
 	unsigned i;
 	struct rte_mbuf *m[NB_MBUF];
@@ -680,10 +676,11 @@  test_pktmbuf_free_segment(void)
 #ifdef RTE_MBUF_REFCNT_ATOMIC
 
 static int
-test_refcnt_slave(__attribute__((unused)) void *arg)
+test_refcnt_slave(void *arg)
 {
 	unsigned lcore, free;
 	void *mp = 0;
+	struct rte_ring *refcnt_mbuf_ring = arg;
 
 	lcore = rte_lcore_id();
 	printf("%s started at lcore %u\n", __func__, lcore);
@@ -704,7 +701,9 @@  test_refcnt_slave(__attribute__((unused)) void *arg)
 }
 
 static void
-test_refcnt_iter(unsigned lcore, unsigned iter)
+test_refcnt_iter(unsigned int lcore, unsigned int iter,
+		 struct rte_mempool *refcnt_pool,
+		 struct rte_ring *refcnt_mbuf_ring)
 {
 	uint16_t ref;
 	unsigned i, n, tref, wn;
@@ -760,7 +759,8 @@  test_refcnt_iter(unsigned lcore, unsigned iter)
 }
 
 static int
-test_refcnt_master(void)
+test_refcnt_master(struct rte_mempool *refcnt_pool,
+		   struct rte_ring *refcnt_mbuf_ring)
 {
 	unsigned i, lcore;
 
@@ -768,7 +768,7 @@  test_refcnt_master(void)
 	printf("%s started at lcore %u\n", __func__, lcore);
 
 	for (i = 0; i != REFCNT_MAX_ITER; i++)
-		test_refcnt_iter(lcore, i);
+		test_refcnt_iter(lcore, i, refcnt_pool, refcnt_mbuf_ring);
 
 	refcnt_stop_slaves = 1;
 	rte_wmb();
@@ -783,9 +783,9 @@  static int
 test_refcnt_mbuf(void)
 {
 #ifdef RTE_MBUF_REFCNT_ATOMIC
-
 	unsigned lnum, master, slave, tref;
-
+	struct rte_mempool *refcnt_pool = NULL;
+	struct rte_ring *refcnt_mbuf_ring = NULL;
 
 	if ((lnum = rte_lcore_count()) == 1) {
 		printf("skipping %s, number of lcores: %u is not enough\n",
@@ -797,20 +797,19 @@  test_refcnt_mbuf(void)
 
 	/* create refcnt pool & ring if they don't exist */
 
-	if (refcnt_pool == NULL &&
-			(refcnt_pool = rte_pktmbuf_pool_create(
-				MAKE_STRING(refcnt_pool),
-				REFCNT_MBUF_NUM, 0, 0, 0,
-				SOCKET_ID_ANY)) == NULL) {
+	refcnt_pool = rte_pktmbuf_pool_create(MAKE_STRING(refcnt_pool),
+					      REFCNT_MBUF_NUM, 0, 0, 0,
+					      SOCKET_ID_ANY);
+	if (refcnt_pool == NULL) {
 		printf("%s: cannot allocate " MAKE_STRING(refcnt_pool) "\n",
 		    __func__);
 		return -1;
 	}
 
-	if (refcnt_mbuf_ring == NULL &&
-			(refcnt_mbuf_ring = rte_ring_create("refcnt_mbuf_ring",
+	refcnt_mbuf_ring = rte_ring_create("refcnt_mbuf_ring",
 			rte_align32pow2(REFCNT_RING_SIZE), SOCKET_ID_ANY,
-			RING_F_SP_ENQ)) == NULL) {
+					RING_F_SP_ENQ);
+	if (refcnt_mbuf_ring == NULL) {
 		printf("%s: cannot allocate " MAKE_STRING(refcnt_mbuf_ring)
 		    "\n", __func__);
 		return -1;
@@ -819,9 +818,10 @@  test_refcnt_mbuf(void)
 	refcnt_stop_slaves = 0;
 	memset(refcnt_lcore, 0, sizeof (refcnt_lcore));
 
-	rte_eal_mp_remote_launch(test_refcnt_slave, NULL, SKIP_MASTER);
+	rte_eal_mp_remote_launch(test_refcnt_slave, refcnt_mbuf_ring,
+				 SKIP_MASTER);
 
-	test_refcnt_master();
+	test_refcnt_master(refcnt_pool, refcnt_mbuf_ring);
 
 	rte_eal_mp_wait_lcore();
 
@@ -870,7 +870,7 @@  verify_mbuf_check_panics(struct rte_mbuf *buf)
 }
 
 static int
-test_failing_mbuf_sanity_check(void)
+test_failing_mbuf_sanity_check(struct rte_mempool *pktmbuf_pool)
 {
 	struct rte_mbuf *buf;
 	struct rte_mbuf badbuf;
@@ -931,7 +931,9 @@  test_failing_mbuf_sanity_check(void)
 }
 
 static int
-test_mbuf_linearize(int pkt_len, int nb_segs) {
+test_mbuf_linearize(struct rte_mempool *pktmbuf_pool, int pkt_len,
+		    int nb_segs)
+{
 
 	struct rte_mbuf *m = NULL, *mbuf = NULL;
 	uint8_t *data;
@@ -1022,7 +1024,7 @@  test_mbuf_linearize(int pkt_len, int nb_segs) {
 }
 
 static int
-test_mbuf_linearize_check(void)
+test_mbuf_linearize_check(struct rte_mempool *pktmbuf_pool)
 {
 	struct test_mbuf_array {
 		int size;
@@ -1039,7 +1041,7 @@  test_mbuf_linearize_check(void)
 	printf("Test mbuf linearize API\n");
 
 	for (i = 0; i < RTE_DIM(mbuf_array); i++)
-		if (test_mbuf_linearize(mbuf_array[i].size,
+		if (test_mbuf_linearize(pktmbuf_pool, mbuf_array[i].size,
 				mbuf_array[i].nb_segs)) {
 			printf("Test failed for %d, %d\n", mbuf_array[i].size,
 					mbuf_array[i].nb_segs);
@@ -1052,13 +1054,15 @@  test_mbuf_linearize_check(void)
 static int
 test_mbuf(void)
 {
+	struct rte_mempool *pktmbuf_pool = NULL;
+	struct rte_mempool *pktmbuf_pool2 = NULL;
+
+
 	RTE_BUILD_BUG_ON(sizeof(struct rte_mbuf) != RTE_CACHE_LINE_MIN_SIZE * 2);
 
 	/* create pktmbuf pool if it does not exist */
-	if (pktmbuf_pool == NULL) {
-		pktmbuf_pool = rte_pktmbuf_pool_create("test_pktmbuf_pool",
+	pktmbuf_pool = rte_pktmbuf_pool_create("test_pktmbuf_pool",
 			NB_MBUF, 32, 0, MBUF_DATA_SIZE, SOCKET_ID_ANY);
-	}
 
 	if (pktmbuf_pool == NULL) {
 		printf("cannot allocate mbuf pool\n");
@@ -1067,10 +1071,8 @@  test_mbuf(void)
 
 	/* create a specific pktmbuf pool with a priv_size != 0 and no data
 	 * room size */
-	if (pktmbuf_pool2 == NULL) {
-		pktmbuf_pool2 = rte_pktmbuf_pool_create("test_pktmbuf_pool2",
+	pktmbuf_pool2 = rte_pktmbuf_pool_create("test_pktmbuf_pool2",
 			NB_MBUF, 32, MBUF2_PRIV_SIZE, 0, SOCKET_ID_ANY);
-	}
 
 	if (pktmbuf_pool2 == NULL) {
 		printf("cannot allocate mbuf pool\n");
@@ -1078,25 +1080,25 @@  test_mbuf(void)
 	}
 
 	/* test multiple mbuf alloc */
-	if (test_pktmbuf_pool() < 0) {
+	if (test_pktmbuf_pool(pktmbuf_pool) < 0) {
 		printf("test_mbuf_pool() failed\n");
 		return -1;
 	}
 
 	/* do it another time to check that all mbufs were freed */
-	if (test_pktmbuf_pool() < 0) {
+	if (test_pktmbuf_pool(pktmbuf_pool) < 0) {
 		printf("test_mbuf_pool() failed (2)\n");
 		return -1;
 	}
 
 	/* test that the pointer to the data on a packet mbuf is set properly */
-	if (test_pktmbuf_pool_ptr() < 0) {
+	if (test_pktmbuf_pool_ptr(pktmbuf_pool) < 0) {
 		printf("test_pktmbuf_pool_ptr() failed\n");
 		return -1;
 	}
 
 	/* test data manipulation in mbuf */
-	if (test_one_pktmbuf() < 0) {
+	if (test_one_pktmbuf(pktmbuf_pool) < 0) {
 		printf("test_one_mbuf() failed\n");
 		return -1;
 	}
@@ -1106,28 +1108,28 @@  test_mbuf(void)
 	 * do it another time, to check that allocation reinitialize
 	 * the mbuf correctly
 	 */
-	if (test_one_pktmbuf() < 0) {
+	if (test_one_pktmbuf(pktmbuf_pool) < 0) {
 		printf("test_one_mbuf() failed (2)\n");
 		return -1;
 	}
 
-	if (test_pktmbuf_with_non_ascii_data() < 0) {
+	if (test_pktmbuf_with_non_ascii_data(pktmbuf_pool) < 0) {
 		printf("test_pktmbuf_with_non_ascii_data() failed\n");
 		return -1;
 	}
 
 	/* test free pktmbuf segment one by one */
-	if (test_pktmbuf_free_segment() < 0) {
+	if (test_pktmbuf_free_segment(pktmbuf_pool) < 0) {
 		printf("test_pktmbuf_free_segment() failed.\n");
 		return -1;
 	}
 
-	if (testclone_testupdate_testdetach()<0){
+	if (testclone_testupdate_testdetach(pktmbuf_pool) < 0) {
 		printf("testclone_and_testupdate() failed \n");
 		return -1;
 	}
 
-	if (test_attach_from_different_pool() < 0) {
+	if (test_attach_from_different_pool(pktmbuf_pool, pktmbuf_pool2) < 0) {
 		printf("test_attach_from_different_pool() failed\n");
 		return -1;
 	}
@@ -1137,12 +1139,12 @@  test_mbuf(void)
 		return -1;
 	}
 
-	if (test_failing_mbuf_sanity_check() < 0) {
+	if (test_failing_mbuf_sanity_check(pktmbuf_pool) < 0) {
 		printf("test_failing_mbuf_sanity_check() failed\n");
 		return -1;
 	}
 
-	if (test_mbuf_linearize_check() < 0) {
+	if (test_mbuf_linearize_check(pktmbuf_pool) < 0) {
 		printf("test_mbuf_linearize_check() failed\n");
 		return -1;
 	}