[PATCH] test/dma: fix for buffer auto free

[PATCH] test/dma: fix for buffer auto free

[Amit Prakash Shukla]

Buffer auto free test failed for more than 1 dma device as the device
initialization for the test was been done only for the first dma device.
This changeset fixes the same.

Fixes: 877cb3e37426 ("dmadev: add buffer auto free offload")

Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
---
 app/test/test_dmadev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index 216f84b6bb..3d4cb37ee6 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -49,6 +49,8 @@ struct dma_add_test dma_add_test[] = {
 	[TEST_M2D_AUTO_FREE] = {.name = "m2d_auto_free", .enabled = false},
 };
 
+static bool dev_init;
+
 static void
 __rte_format_printf(3, 4)
 print_err(const char *func, int lineno, const char *format, ...)
@@ -837,7 +839,6 @@ test_m2d_auto_free(int16_t dev_id, uint16_t vchan)
 	};
 	uint32_t buf_cnt1, buf_cnt2;
 	struct rte_mempool_ops *ops;
-	static bool dev_init;
 	uint16_t nb_done = 0;
 	bool dma_err = false;
 	int retry = 100;
@@ -1011,6 +1012,7 @@ test_dmadev_instance(int16_t dev_id)
 
 	if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) &&
 	    dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) {
+		dev_init = false;
 		if (runtest("m2d_auto_free", test_m2d_auto_free, 128, dev_id, vchan,
 			    CHECK_ERRS) < 0)
 			goto err;
-- 
2.25.1

RE: [PATCH] test/dma: fix for buffer auto free

[Hemant Agrawal]

> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c index
> 216f84b6bb..3d4cb37ee6 100644
> --- a/app/test/test_dmadev.c
> +++ b/app/test/test_dmadev.c
> @@ -49,6 +49,8 @@ struct dma_add_test dma_add_test[] = {
>  	[TEST_M2D_AUTO_FREE] = {.name = "m2d_auto_free", .enabled =
> false},  };
> 
> +static bool dev_init;

[Hemant]  should't it be per device id? 

> +
>  static void
>  __rte_format_printf(3, 4)
>  print_err(const char *func, int lineno, const char *format, ...) @@ -837,7
> +839,6 @@ test_m2d_auto_free(int16_t dev_id, uint16_t vchan)
>  	};
>  	uint32_t buf_cnt1, buf_cnt2;
>  	struct rte_mempool_ops *ops;
> -	static bool dev_init;
>  	uint16_t nb_done = 0;
>  	bool dma_err = false;
>  	int retry = 100;
> @@ -1011,6 +1012,7 @@ test_dmadev_instance(int16_t dev_id)
> 
>  	if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) &&
>  	    dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) {
> +		dev_init = false;
>  		if (runtest("m2d_auto_free", test_m2d_auto_free, 128,
> dev_id, vchan,
>  			    CHECK_ERRS) < 0)
>  			goto err;
> --
> 2.25.1

Re: [PATCH] test/dma: fix for buffer auto free

[fengchengwen]

Hi Amit,

  I prefer not use static variable to control it because it introduce many coupling.

  Suggest add one function which prepare the test_m2d_auto_free, like prepare_m2d_auto_free

	if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) &&
	    dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) {
                if (prepare_m2d_auto_free(dev_id) != 0)
                    goto err;
		if (runtest("m2d_auto_free", test_m2d_auto_free, 128, dev_id, vchan,
			    CHECK_ERRS) < 0)
			goto err;
	}	

In the new function, could do reinit vchan just like the beginging test_m2d_auto_free.
static int prepare_m2d_auto_free(int dev_id) {
	const struct rte_dma_vchan_conf qconf = {
		.direction = RTE_DMA_DIR_MEM_TO_DEV,
		.nb_desc = TEST_RINGSIZE,
		.auto_free.m2d.pool = pool,
		.dst_port.port_type = RTE_DMA_PORT_PCIE,
		.dst_port.pcie.coreid = 0,
	};
	/* Stop the device to reconfigure vchan because should use Mem-to-Dev mode. */
	if (rte_dma_stop(dev_id) < 0)
		ERR_RETURN("Error stopping device %u\n", dev_id);
	if (rte_dma_vchan_setup(dev_id, vchan, &qconf) < 0)
		ERR_RETURN("Error with queue configuration\n");
	if (rte_dma_start(dev_id) != 0)
		ERR_RETURN("Error with rte_dma_start()\n");
	return 0;
}



Also I found a bug in test_m2d_auto_free function, if above path taken:
	if (rte_pktmbuf_alloc_bulk(pool, src, NR_MBUF) != 0) {
		printf("alloc src mbufs failed.\n");
		ret = -1;
		goto done;
	}

done:
	rte_pktmbuf_free_bulk(dst, NR_MBUF);
	/* If the test passes source buffer will be freed in hardware. */
	if (ret < 0)
		rte_pktmbuf_free_bulk(&src[nb_done], (NR_MBUF - nb_done));
		----- then it will free invalid mbuf to pool because src was not success init


On 2023/11/1 18:18, Amit Prakash Shukla wrote:
> Buffer auto free test failed for more than 1 dma device as the device
> initialization for the test was been done only for the first dma device.
> This changeset fixes the same.
> 
> Fixes: 877cb3e37426 ("dmadev: add buffer auto free offload")
> 
> Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> ---
>  app/test/test_dmadev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> index 216f84b6bb..3d4cb37ee6 100644
> --- a/app/test/test_dmadev.c
> +++ b/app/test/test_dmadev.c
> @@ -49,6 +49,8 @@ struct dma_add_test dma_add_test[] = {
>  	[TEST_M2D_AUTO_FREE] = {.name = "m2d_auto_free", .enabled = false},
>  };
>  
> +static bool dev_init;
> +
>  static void
>  __rte_format_printf(3, 4)
>  print_err(const char *func, int lineno, const char *format, ...)
> @@ -837,7 +839,6 @@ test_m2d_auto_free(int16_t dev_id, uint16_t vchan)
>  	};
>  	uint32_t buf_cnt1, buf_cnt2;
>  	struct rte_mempool_ops *ops;
> -	static bool dev_init;
>  	uint16_t nb_done = 0;
>  	bool dma_err = false;
>  	int retry = 100;
> @@ -1011,6 +1012,7 @@ test_dmadev_instance(int16_t dev_id)
>  
>  	if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) &&
>  	    dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) {
> +		dev_init = false;
>  		if (runtest("m2d_auto_free", test_m2d_auto_free, 128, dev_id, vchan,
>  			    CHECK_ERRS) < 0)
>  			goto err;
> 

RE: [EXT] Re: [PATCH] test/dma: fix for buffer auto free

[Amit Prakash Shukla]

Hi Chengwen,

Thanks for the review and feedback. I will send v2 with suggested changes.

Thanks,
Amit Shukla

> -----Original Message-----
> From: fengchengwen <fengchengwen@huawei.com>
> Sent: Thursday, November 2, 2023 7:37 AM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> conor.walsh@intel.com; Vamsi Krishna Attunuru <vattunuru@marvell.com>;
> g.singh@nxp.com; sachin.saxena@oss.nxp.com; hemant.agrawal@nxp.com;
> cheng1.jiang@intel.com; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Anoob Joseph <anoobj@marvell.com>;
> mb@smartsharesystems.com
> Subject: [EXT] Re: [PATCH] test/dma: fix for buffer auto free
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Amit,
> 
>   I prefer not use static variable to control it because it introduce many
> coupling.
> 
>   Suggest add one function which prepare the test_m2d_auto_free, like
> prepare_m2d_auto_free
> 
> 	if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) &&
> 	    dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) {
>                 if (prepare_m2d_auto_free(dev_id) != 0)
>                     goto err;
> 		if (runtest("m2d_auto_free", test_m2d_auto_free, 128,
> dev_id, vchan,
> 			    CHECK_ERRS) < 0)
> 			goto err;
> 	}
> 
> In the new function, could do reinit vchan just like the beginging
> test_m2d_auto_free.
> static int prepare_m2d_auto_free(int dev_id) {
> 	const struct rte_dma_vchan_conf qconf = {
> 		.direction = RTE_DMA_DIR_MEM_TO_DEV,
> 		.nb_desc = TEST_RINGSIZE,
> 		.auto_free.m2d.pool = pool,
> 		.dst_port.port_type = RTE_DMA_PORT_PCIE,
> 		.dst_port.pcie.coreid = 0,
> 	};
> 	/* Stop the device to reconfigure vchan because should use Mem-to-
> Dev mode. */
> 	if (rte_dma_stop(dev_id) < 0)
> 		ERR_RETURN("Error stopping device %u\n", dev_id);
> 	if (rte_dma_vchan_setup(dev_id, vchan, &qconf) < 0)
> 		ERR_RETURN("Error with queue configuration\n");
> 	if (rte_dma_start(dev_id) != 0)
> 		ERR_RETURN("Error with rte_dma_start()\n");
> 	return 0;
> }
> 
> 
> 
> Also I found a bug in test_m2d_auto_free function, if above path taken:
> 	if (rte_pktmbuf_alloc_bulk(pool, src, NR_MBUF) != 0) {
> 		printf("alloc src mbufs failed.\n");
> 		ret = -1;
> 		goto done;
> 	}
> 
> done:
> 	rte_pktmbuf_free_bulk(dst, NR_MBUF);
> 	/* If the test passes source buffer will be freed in hardware. */
> 	if (ret < 0)
> 		rte_pktmbuf_free_bulk(&src[nb_done], (NR_MBUF -
> nb_done));
> 		----- then it will free invalid mbuf to pool because src was not
> success init
> 
> 

<snip>

[PATCH v2] test/dma: fix for buffer auto free

[Amit Prakash Shukla]

Buffer auto free test failed for more than 1 dma device as the device
initialization for the test was been done only for the first dma
device. This changeset fixes the same and also fixes the freeing of
the uninitialised source buffer in error condition.

Fixes: 877cb3e37426 ("dmadev: add buffer auto free offload")

Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
---
v2:
- Incorporated review suggestion from Chengwen.

 app/test/test_dmadev.c | 81 ++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 43 deletions(-)

diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
index 216f84b6bb..7581fc2b4c 100644
--- a/app/test/test_dmadev.c
+++ b/app/test/test_dmadev.c
@@ -827,65 +827,35 @@ static int
 test_m2d_auto_free(int16_t dev_id, uint16_t vchan)
 {
 #define NR_MBUF 256
-	struct rte_mbuf *src[NR_MBUF], *dst[NR_MBUF];
-	const struct rte_dma_vchan_conf qconf = {
-		.direction = RTE_DMA_DIR_MEM_TO_DEV,
-		.nb_desc = TEST_RINGSIZE,
-		.auto_free.m2d.pool = pool,
-		.dst_port.port_type = RTE_DMA_PORT_PCIE,
-		.dst_port.pcie.coreid = 0,
-	};
+	struct rte_mempool_cache *cache;
+	struct rte_mbuf *src[NR_MBUF];
 	uint32_t buf_cnt1, buf_cnt2;
 	struct rte_mempool_ops *ops;
-	static bool dev_init;
 	uint16_t nb_done = 0;
 	bool dma_err = false;
 	int retry = 100;
 	int i, ret = 0;
+	rte_iova_t dst;
 
-	if (!dev_init) {
-		/* Stop the device to reconfigure vchan. */
-		if (rte_dma_stop(dev_id) < 0)
-			ERR_RETURN("Error stopping device %u\n", dev_id);
-
-		if (rte_dma_vchan_setup(dev_id, vchan, &qconf) < 0)
-			ERR_RETURN("Error with queue configuration\n");
-
-		if (rte_dma_start(dev_id) != 0)
-			ERR_RETURN("Error with rte_dma_start()\n");
-
-		dev_init = true;
-	}
-
-	if (rte_pktmbuf_alloc_bulk(pool, dst, NR_MBUF) != 0)
-		ERR_RETURN("alloc dst mbufs failed.\n");
-
-	for (i = 0; i < NR_MBUF; i++) {
-		/* Using mbuf structure to hold remote iova address. */
-		rte_mbuf_iova_set(dst[i], (rte_iova_t)env_test_param[TEST_PARAM_REMOTE_ADDR]);
-		dst[i]->data_off = 0;
-	}
+	dst = (rte_iova_t)env_test_param[TEST_PARAM_REMOTE_ADDR];
 
 	/* Capture buffer count before allocating source buffer. */
+	cache = rte_mempool_default_cache(pool, rte_lcore_id());
 	ops = rte_mempool_get_ops(pool->ops_index);
-	buf_cnt1 = ops->get_count(pool);
+	buf_cnt1 = ops->get_count(pool) + cache->len;
 
-	if (rte_pktmbuf_alloc_bulk(pool, src, NR_MBUF) != 0) {
-		printf("alloc src mbufs failed.\n");
-		ret = -1;
-		goto done;
-	}
+	if (rte_pktmbuf_alloc_bulk(pool, src, NR_MBUF) != 0)
+		ERR_RETURN("alloc src mbufs failed.\n");
 
-	if ((buf_cnt1 - NR_MBUF) != ops->get_count(pool)) {
+	if ((buf_cnt1 - NR_MBUF) != (ops->get_count(pool) + cache->len)) {
 		printf("Buffer count check failed.\n");
 		ret = -1;
 		goto done;
 	}
 
 	for (i = 0; i < NR_MBUF; i++) {
-		ret = rte_dma_copy(dev_id, vchan, rte_mbuf_data_iova(src[i]),
-				   rte_mbuf_data_iova(dst[i]), COPY_LEN,
-				   RTE_DMA_OP_FLAG_AUTO_FREE);
+		ret = rte_dma_copy(dev_id, vchan, rte_mbuf_data_iova(src[i]), dst,
+				   COPY_LEN, RTE_DMA_OP_FLAG_AUTO_FREE);
 
 		if (ret < 0) {
 			printf("rte_dma_copy returned error.\n");
@@ -902,14 +872,13 @@ test_m2d_auto_free(int16_t dev_id, uint16_t vchan)
 		rte_delay_us_sleep(1000);
 	} while (retry-- && (nb_done < NR_MBUF));
 
-	buf_cnt2 = ops->get_count(pool);
+	buf_cnt2 = ops->get_count(pool) + cache->len;
 	if ((buf_cnt1 != buf_cnt2) || dma_err) {
 		printf("Free mem to dev buffer test failed.\n");
 		ret = -1;
 	}
 
 done:
-	rte_pktmbuf_free_bulk(dst, NR_MBUF);
 	/* If the test passes source buffer will be freed in hardware. */
 	if (ret < 0)
 		rte_pktmbuf_free_bulk(&src[nb_done], (NR_MBUF - nb_done));
@@ -917,6 +886,30 @@ test_m2d_auto_free(int16_t dev_id, uint16_t vchan)
 	return ret;
 }
 
+static int
+prepare_m2d_auto_free(int16_t dev_id, uint16_t vchan)
+{
+	const struct rte_dma_vchan_conf qconf = {
+		.direction = RTE_DMA_DIR_MEM_TO_DEV,
+		.nb_desc = TEST_RINGSIZE,
+		.auto_free.m2d.pool = pool,
+		.dst_port.port_type = RTE_DMA_PORT_PCIE,
+		.dst_port.pcie.coreid = 0,
+	};
+
+	/* Stop the device to reconfigure vchan. */
+	if (rte_dma_stop(dev_id) < 0)
+		ERR_RETURN("Error stopping device %u\n", dev_id);
+
+	if (rte_dma_vchan_setup(dev_id, vchan, &qconf) < 0)
+		ERR_RETURN("Error with queue configuration\n");
+
+	if (rte_dma_start(dev_id) != 0)
+		ERR_RETURN("Error with rte_dma_start()\n");
+
+	return 0;
+}
+
 static int
 test_dmadev_instance(int16_t dev_id)
 {
@@ -1011,6 +1004,8 @@ test_dmadev_instance(int16_t dev_id)
 
 	if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) &&
 	    dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) {
+		if (prepare_m2d_auto_free(dev_id, vchan) != 0)
+			goto err;
 		if (runtest("m2d_auto_free", test_m2d_auto_free, 128, dev_id, vchan,
 			    CHECK_ERRS) < 0)
 			goto err;
-- 
2.25.1

Re: [PATCH v2] test/dma: fix for buffer auto free

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2023/11/4 2:30, Amit Prakash Shukla wrote:
> Buffer auto free test failed for more than 1 dma device as the device
> initialization for the test was been done only for the first dma
> device. This changeset fixes the same and also fixes the freeing of
> the uninitialised source buffer in error condition.
> 
> Fixes: 877cb3e37426 ("dmadev: add buffer auto free offload")
> 
> Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>

...

Re: [PATCH v2] test/dma: fix for buffer auto free

[Thomas Monjalon]

06/11/2023 02:08, fengchengwen:
> On 2023/11/4 2:30, Amit Prakash Shukla wrote:
> > Buffer auto free test failed for more than 1 dma device as the device
> > initialization for the test was been done only for the first dma
> > device. This changeset fixes the same and also fixes the freeing of
> > the uninitialised source buffer in error condition.
> > 
> > Fixes: 877cb3e37426 ("dmadev: add buffer auto free offload")
> > 
> > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

Applied, thanks.