* RE: [PATCH] test/dma: fix for buffer auto free
2023-11-01 10:18 [PATCH] test/dma: fix for buffer auto free Amit Prakash Shukla
@ 2023-11-01 14:08 ` Hemant Agrawal
2023-11-02 2:06 ` fengchengwen
2023-11-03 18:30 ` [PATCH v2] " Amit Prakash Shukla
2 siblings, 0 replies; 7+ messages in thread
From: Hemant Agrawal @ 2023-11-01 14:08 UTC (permalink / raw)
To: Amit Prakash Shukla, Chengwen Feng, Kevin Laatz, Bruce Richardson
Cc: dev, jerinj, conor.walsh, vattunuru, Gagandeep Singh,
Sachin Saxena (OSS),
cheng1.jiang, ndabilpuram, anoobj, mb
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] test/dma: fix for buffer auto free
2023-11-01 10:18 [PATCH] test/dma: fix for buffer auto free Amit Prakash Shukla
2023-11-01 14:08 ` Hemant Agrawal
@ 2023-11-02 2:06 ` fengchengwen
2023-11-02 9:40 ` [EXT] " Amit Prakash Shukla
2023-11-03 18:30 ` [PATCH v2] " Amit Prakash Shukla
2 siblings, 1 reply; 7+ messages in thread
From: fengchengwen @ 2023-11-02 2:06 UTC (permalink / raw)
To: Amit Prakash Shukla, Kevin Laatz, Bruce Richardson
Cc: dev, jerinj, conor.walsh, vattunuru, g.singh, sachin.saxena,
hemant.agrawal, cheng1.jiang, ndabilpuram, anoobj, mb
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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [EXT] Re: [PATCH] test/dma: fix for buffer auto free
2023-11-02 2:06 ` fengchengwen
@ 2023-11-02 9:40 ` Amit Prakash Shukla
0 siblings, 0 replies; 7+ messages in thread
From: Amit Prakash Shukla @ 2023-11-02 9:40 UTC (permalink / raw)
To: fengchengwen, Kevin Laatz, Bruce Richardson
Cc: dev, Jerin Jacob Kollanukkaran, conor.walsh,
Vamsi Krishna Attunuru, g.singh, sachin.saxena, hemant.agrawal,
cheng1.jiang, Nithin Kumar Dabilpuram, Anoob Joseph, mb
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] test/dma: fix for buffer auto free
2023-11-01 10:18 [PATCH] test/dma: fix for buffer auto free Amit Prakash Shukla
2023-11-01 14:08 ` Hemant Agrawal
2023-11-02 2:06 ` fengchengwen
@ 2023-11-03 18:30 ` Amit Prakash Shukla
2023-11-06 1:08 ` fengchengwen
2 siblings, 1 reply; 7+ messages in thread
From: Amit Prakash Shukla @ 2023-11-03 18:30 UTC (permalink / raw)
To: Chengwen Feng, Kevin Laatz, Bruce Richardson
Cc: dev, jerinj, conor.walsh, vattunuru, g.singh, sachin.saxena,
hemant.agrawal, cheng1.jiang, ndabilpuram, anoobj, mb,
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] test/dma: fix for buffer auto free
2023-11-03 18:30 ` [PATCH v2] " Amit Prakash Shukla
@ 2023-11-06 1:08 ` fengchengwen
2023-11-14 14:58 ` Thomas Monjalon
0 siblings, 1 reply; 7+ messages in thread
From: fengchengwen @ 2023-11-06 1:08 UTC (permalink / raw)
To: Amit Prakash Shukla, Kevin Laatz, Bruce Richardson
Cc: dev, jerinj, conor.walsh, vattunuru, g.singh, sachin.saxena,
hemant.agrawal, cheng1.jiang, ndabilpuram, anoobj, mb
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>
...
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] test/dma: fix for buffer auto free
2023-11-06 1:08 ` fengchengwen
@ 2023-11-14 14:58 ` Thomas Monjalon
0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2023-11-14 14:58 UTC (permalink / raw)
To: Amit Prakash Shukla
Cc: Kevin Laatz, Bruce Richardson, dev, jerinj, conor.walsh,
vattunuru, g.singh, sachin.saxena, hemant.agrawal, cheng1.jiang,
ndabilpuram, anoobj, mb, fengchengwen
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.
^ permalink raw reply [flat|nested] 7+ messages in thread