From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1F25A432B4; Mon, 6 Nov 2023 04:47:10 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A2D05402B5; Mon, 6 Nov 2023 04:47:09 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id B3F864027F for ; Mon, 6 Nov 2023 04:47:06 +0100 (CET) Received: from dggpeml100024.china.huawei.com (unknown [172.30.72.54]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4SNxzJ3bXTzrT6L; Mon, 6 Nov 2023 11:43:55 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by dggpeml100024.china.huawei.com (7.185.36.115) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Mon, 6 Nov 2023 11:47:02 +0800 Subject: Re: [PATCH v3 1/3] test/dma: use unit test framework To: Gowrishankar Muthukrishnan , CC: , Vamsi Attunuru , Amit Prakash Shukla , Vidya Sagar Velumuri , Kevin Laatz , Bruce Richardson References: <67a9b287681794a30ef68ee34782a59d6d3e009e.1699025423.git.gmuthukrishn@marvell.com> From: fengchengwen Message-ID: Date: Mon, 6 Nov 2023 11:47:02 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <67a9b287681794a30ef68ee34782a59d6d3e009e.1699025423.git.gmuthukrishn@marvell.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggpeml100024.china.huawei.com (7.185.36.115) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Gowrishankar, Thanks for your works. On 2023/11/3 23:38, Gowrishankar Muthukrishnan wrote: > Use unit test framework to execute DMA tests. > > Signed-off-by: Gowrishankar Muthukrishnan > Suggested-by: Chengwen Feng > --- > app/test/test_dmadev.c | 240 ++++++++++++++++++++++++++++--------- > app/test/test_dmadev_api.c | 89 ++++---------- > 2 files changed, 212 insertions(+), 117 deletions(-) > > diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c > index 216f84b6bb..780941fc1e 100644 > --- a/app/test/test_dmadev.c > +++ b/app/test/test_dmadev.c > @@ -22,7 +22,9 @@ > #define COPY_LEN 1024 > > static struct rte_mempool *pool; > +static int16_t test_dev_id; > static uint16_t id_count; > +static int vchan; > > enum { > TEST_PARAM_REMOTE_ADDR = 0, > @@ -61,13 +63,35 @@ print_err(const char *func, int lineno, const char *format, ...) > va_end(ap); > } > > +struct runtest_param { > + const char *printable; > + int (*test_fn)(int16_t dev_id, uint16_t vchan); > + int iterations; > + int16_t dev_id; > + uint16_t vchan; > + bool check_err_stats; > +}; > + > static int > -runtest(const char *printable, int (*test_fn)(int16_t dev_id, uint16_t vchan), int iterations, > - int16_t dev_id, uint16_t vchan, bool check_err_stats) > +runtest(const void *args) > { > + int (*test_fn)(int16_t dev_id, uint16_t vchan); > + const struct runtest_param *param = args; > struct rte_dma_stats stats; > + const char *printable; > + bool check_err_stats; > + int iterations; > + int16_t dev_id; > + uint16_t vchan; > int i; > > + printable = param->printable; > + iterations = param->iterations; > + dev_id = param->dev_id; > + vchan = param->vchan; > + check_err_stats = param->check_err_stats; > + test_fn = param->test_fn; > + > rte_dma_stats_reset(dev_id, vchan); > printf("DMA Dev %d: Running %s Tests %s\n", dev_id, printable, > check_err_stats ? " " : "(errors expected)"); > @@ -918,9 +942,21 @@ test_m2d_auto_free(int16_t dev_id, uint16_t vchan) > } > > static int > -test_dmadev_instance(int16_t dev_id) > +test_dmadev_burst_setup(void) > { > -#define CHECK_ERRS true > + if (rte_dma_burst_capacity(test_dev_id, vchan) < 64) { > + printf("DMA Dev %u: insufficient burst capacity (64 required), skipping tests\n", > + test_dev_id); > + return TEST_SKIPPED; > + } > + > + return TEST_SUCCESS; > +} > + > +static int > +test_dmadev_setup(void) > +{ > + int16_t dev_id = test_dev_id; > struct rte_dma_stats stats; > struct rte_dma_info info; > const struct rte_dma_conf conf = { .nb_vchans = 1}; > @@ -928,16 +964,12 @@ test_dmadev_instance(int16_t dev_id) > .direction = RTE_DMA_DIR_MEM_TO_MEM, > .nb_desc = TEST_RINGSIZE, > }; > - const int vchan = 0; > int ret; > > ret = rte_dma_info_get(dev_id, &info); > if (ret != 0) > ERR_RETURN("Error with rte_dma_info_get()\n"); > > - printf("\n### Test dmadev instance %u [%s]\n", > - dev_id, info.dev_name); > - > if (info.max_vchans < 1) > ERR_RETURN("Error, no channels available on device id %u\n", dev_id); > > @@ -976,20 +1008,123 @@ test_dmadev_instance(int16_t dev_id) > if (pool == NULL) > ERR_RETURN("Error with mempool creation\n"); > > + return 0; > +} > + > +static void > +test_dmadev_teardown(void) > +{ > + rte_mempool_free(pool); > + rte_dma_stop(test_dev_id); > + rte_dma_stats_reset(test_dev_id, vchan); > + test_dev_id = -EINVAL; > +} > + > +static int > +test_dmadev_instance(int16_t dev_id) > +{ > +#define CHECK_ERRS true suggest rm this define > + enum { > + TEST_COPY = 0, > + TEST_START, > + TEST_BURST, > + TEST_ERR, > + TEST_FILL, > + TEST_M2D, > + TEST_END > + }; > + > + struct runtest_param param[TEST_END]; > + struct rte_dma_info dev_info; > + struct unit_test_suite *ts; > + struct unit_test_case *tc; > + int ret; > + > + if (rte_dma_info_get(dev_id, &dev_info) < 0) > + return TEST_SKIPPED; > + > + test_dev_id = dev_id; > + vchan = 0; > + > + ts = calloc(1, sizeof(struct unit_test_suite) > + + sizeof(struct unit_test_case) * (TEST_END + 1)); > + > + ts->suite_name = "DMA dev instance testsuite"; > + ts->setup = test_dmadev_setup; > + ts->teardown = test_dmadev_teardown; > + > + param[TEST_COPY].printable = "copy"; > + param[TEST_COPY].test_fn = test_enqueue_copies; > + param[TEST_COPY].iterations = 640; > + param[TEST_COPY].dev_id = dev_id; > + param[TEST_COPY].vchan = vchan; > + param[TEST_COPY].check_err_stats = CHECK_ERRS; > + > + param[TEST_START].printable = "stop-start"; > + param[TEST_START].test_fn = test_stop_start; > + param[TEST_START].iterations = 1; > + param[TEST_START].dev_id = dev_id; > + param[TEST_START].vchan = vchan; > + param[TEST_START].check_err_stats = CHECK_ERRS; > + > + param[TEST_BURST].printable = "burst capacity"; > + param[TEST_BURST].test_fn = test_burst_capacity; > + param[TEST_BURST].iterations = 1; > + param[TEST_BURST].dev_id = dev_id; > + param[TEST_BURST].vchan = vchan; > + param[TEST_BURST].check_err_stats = CHECK_ERRS; > + > + param[TEST_ERR].printable = "error handling"; > + param[TEST_ERR].test_fn = test_completion_handling; > + param[TEST_ERR].iterations = 1; > + param[TEST_ERR].dev_id = dev_id; > + param[TEST_ERR].vchan = vchan; > + param[TEST_ERR].check_err_stats = CHECK_ERRS; > + > + param[TEST_FILL].printable = "fill"; > + param[TEST_FILL].test_fn = test_enqueue_fill; > + param[TEST_FILL].iterations = 1; > + param[TEST_FILL].dev_id = dev_id; > + param[TEST_FILL].vchan = vchan; > + param[TEST_FILL].check_err_stats = CHECK_ERRS; > + > + param[TEST_M2D].printable = "m2d_auto_free"; > + param[TEST_M2D].test_fn = test_m2d_auto_free; > + param[TEST_M2D].iterations = 128; > + param[TEST_M2D].dev_id = dev_id; > + param[TEST_M2D].vchan = vchan; > + param[TEST_M2D].check_err_stats = CHECK_ERRS; The above param was constant for all dmadev, suggest make them as global variable. > + > + for (int i = 0; i < TEST_END; i++) { > + tc = &ts->unit_test_cases[i]; > + tc->setup = NULL; > + tc->teardown = NULL; > + tc->testcase = NULL; > + tc->testcase_with_data = runtest; > + tc->data = ¶m[i]; suggest set name, it could be param[i].printable > + tc->enabled = 0; > + } > + > + tc = &ts->unit_test_cases[TEST_END]; > + tc->setup = NULL; > + tc->teardown = NULL; > + tc->testcase = NULL; > + tc->testcase_with_data = NULL; > + tc->data = NULL; > + tc->enabled = 0; With above global variable, could use TEST_CASE_WITH_DATA here which pass TEST_XXX as data. > + > + printf("\n### Test dmadev instance %u [%s]\n", > + dev_id, dev_info.dev_name); > + > /* run the test cases, use many iterations to ensure UINT16_MAX id wraparound */ The comment could consider del or place it to proper context. the same with other comments > - if (runtest("copy", test_enqueue_copies, 640, dev_id, vchan, CHECK_ERRS) < 0) > - goto err; > + ts->unit_test_cases[TEST_COPY].enabled = 1; > > /* run tests stopping/starting devices and check jobs still work after restart */ > - if (runtest("stop-start", test_stop_start, 1, dev_id, vchan, CHECK_ERRS) < 0) > - goto err; > + ts->unit_test_cases[TEST_START].enabled = 1; > > /* run some burst capacity tests */ > - if (rte_dma_burst_capacity(dev_id, vchan) < 64) > - printf("DMA Dev %u: insufficient burst capacity (64 required), skipping tests\n", > - dev_id); > - else if (runtest("burst capacity", test_burst_capacity, 1, dev_id, vchan, CHECK_ERRS) < 0) > - goto err; > + ts->unit_test_cases[TEST_BURST].setup = test_dmadev_burst_setup; > + ts->unit_test_cases[TEST_BURST].enabled = 1; > > /* to test error handling we can provide null pointers for source or dest in copies. This > * requires VA mode in DPDK, since NULL(0) is a valid physical address. > @@ -997,55 +1132,40 @@ test_dmadev_instance(int16_t dev_id) > */ > if (rte_eal_iova_mode() != RTE_IOVA_VA) > printf("DMA Dev %u: DPDK not in VA mode, skipping error handling tests\n", dev_id); > - else if ((info.dev_capa & RTE_DMA_CAPA_HANDLES_ERRORS) == 0) > + else if ((dev_info.dev_capa & RTE_DMA_CAPA_HANDLES_ERRORS) == 0) > printf("DMA Dev %u: device does not report errors, skipping error handling tests\n", > dev_id); > - else if (runtest("error handling", test_completion_handling, 1, > - dev_id, vchan, !CHECK_ERRS) < 0) > - goto err; > + else > + ts->unit_test_cases[TEST_ERR].enabled = 1; > > - if ((info.dev_capa & RTE_DMA_CAPA_OPS_FILL) == 0) > + if ((dev_info.dev_capa & RTE_DMA_CAPA_OPS_FILL) == 0) > printf("DMA Dev %u: No device fill support, skipping fill tests\n", dev_id); > - else if (runtest("fill", test_enqueue_fill, 1, dev_id, vchan, CHECK_ERRS) < 0) > - goto err; > + else > + ts->unit_test_cases[TEST_FILL].enabled = 1; > > - if ((info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) && > + if ((dev_info.dev_capa & RTE_DMA_CAPA_M2D_AUTO_FREE) && > dma_add_test[TEST_M2D_AUTO_FREE].enabled == true) { > - if (runtest("m2d_auto_free", test_m2d_auto_free, 128, dev_id, vchan, > - CHECK_ERRS) < 0) > - goto err; > + ts->unit_test_cases[TEST_M2D].enabled = 1; > } > > - rte_mempool_free(pool); > - > - if (rte_dma_stop(dev_id) < 0) > - ERR_RETURN("Error stopping device %u\n", dev_id); > - > - rte_dma_stats_reset(dev_id, vchan); > - return 0; > + ret = unit_test_suite_runner(ts); > + test_dev_id = -EINVAL; > + free(ts); > > -err: > - rte_mempool_free(pool); > - rte_dma_stop(dev_id); > - return -1; > + return ret; > } > > static int > -test_apis(void) > +test_apis(const void *args) > { > - const char *pmd = "dma_skeleton"; > - int id; > - int ret; > + const int16_t dev_id = *(const int16_t *)args; > + struct rte_dma_info dev_info; > > - /* attempt to create skeleton instance - ignore errors due to one being already present */ > - rte_vdev_init(pmd, NULL); > - id = rte_dma_get_dev_id_by_name(pmd); > - if (id < 0) > + if (rte_dma_info_get(dev_id, &dev_info) < 0) > return TEST_SKIPPED; > - printf("\n### Test dmadev infrastructure using skeleton driver\n"); > - ret = test_dma_api(id); > > - return ret; > + printf("\n### Test dmadev infrastructure using %s\n", dev_info.dev_name); suggest place above printf in the beginning of test_dma_api() > + return test_dma_api(dev_id); > } > > static void > @@ -1088,16 +1208,28 @@ parse_dma_env_var(void) > static int > test_dma(void) > { > - int i; > + const char *pmd = "dma_skeleton"; > + int i, avail; > > parse_dma_env_var(); > > + /* attempt to create skeleton instance - ignore errors due to one being already present*/ > + rte_vdev_init(pmd, NULL); > + i = rte_dma_get_dev_id_by_name(pmd); > + if (i < 0) > + return TEST_SKIPPED; +1 for place create skeleton invoking here. > + > /* basic sanity on dmadev infrastructure */ > - if (test_apis() < 0) > + if (test_apis(&i) < 0) > ERR_RETURN("Error performing API tests\n"); > > - if (rte_dma_count_avail() == 0) > - return TEST_SKIPPED; > + avail = rte_dma_count_avail(); > + if (avail == 0) > + return 0; > + > + i = rte_dma_next_dev(0); > + if (avail > 1 && test_apis(&i) < 0) > + ERR_RETURN("Error performing API tests\n"); Why only test later dmadev? Maybe it's ok for one of the channels, but should consider multi-vendor DMA devices exist. So suggest test all or just test skeleton for the API test. Also, suggest move API related code to test_dmadev_api.c, e.g.: void test_api(void) ---place this function in test_dmadev_api.c { RTE_DMA_FOREACH_DEV(i) { // get device info // check whether support memory-to-memory, because API test will setup memory-to-memory vchan // if not should skip it if (test_dma_api(i) < 0) ERR_RETURN("Error, test failure for device %d\n", i); } } > > RTE_DMA_FOREACH_DEV(i) > if (test_dmadev_instance(i) < 0) > diff --git a/app/test/test_dmadev_api.c b/app/test/test_dmadev_api.c > index 4a181af90a..da8fddb3ca 100644 > --- a/app/test/test_dmadev_api.c > +++ b/app/test/test_dmadev_api.c > @@ -9,31 +9,22 @@ > #include > #include > > -extern int test_dma_api(uint16_t dev_id); > +#include "test.h" > > -#define DMA_TEST_API_RUN(test) \ > - testsuite_run_test(test, #test) > +extern int test_dma_api(uint16_t dev_id); > > #define TEST_MEMCPY_SIZE 1024 > #define TEST_WAIT_US_VAL 50000 > > -#define TEST_SUCCESS 0 > -#define TEST_FAILED -1 > - > static int16_t test_dev_id; > static int16_t invalid_dev_id; > > static char *src; > static char *dst; > > -static int total; > -static int passed; > -static int failed; > - > static int > -testsuite_setup(int16_t dev_id) > +testsuite_setup(void) > { > - test_dev_id = dev_id; > invalid_dev_id = -1; > > src = rte_malloc("dmadev_test_src", TEST_MEMCPY_SIZE, 0); > @@ -46,10 +37,6 @@ testsuite_setup(int16_t dev_id) > return -ENOMEM; > } > > - total = 0; > - passed = 0; > - failed = 0; > - > /* Set dmadev log level to critical to suppress unnecessary output > * during API tests. > */ > @@ -71,25 +58,6 @@ testsuite_teardown(void) > rte_log_set_level_pattern("lib.dmadev", RTE_LOG_INFO); > } > > -static void > -testsuite_run_test(int (*test)(void), const char *name) > -{ > - int ret = 0; > - > - if (test) { > - ret = test(); > - if (ret < 0) { > - failed++; > - printf("%s Failed\n", name); > - } else { > - passed++; > - printf("%s Passed\n", name); > - } > - } > - > - total++; > -} > - > static int > test_dma_get_dev_id_by_name(void) > { > @@ -301,7 +269,7 @@ setup_one_vchan(void) > > ret = rte_dma_info_get(test_dev_id, &dev_info); > RTE_TEST_ASSERT_SUCCESS(ret, "Failed to obtain device info, %d", ret); > - dev_conf.nb_vchans = dev_info.max_vchans; > + dev_conf.nb_vchans = 1; > ret = rte_dma_configure(test_dev_id, &dev_conf); > RTE_TEST_ASSERT_SUCCESS(ret, "Failed to configure, %d", ret); > vchan_conf.direction = RTE_DMA_DIR_MEM_TO_MEM; > @@ -537,38 +505,33 @@ test_dma_completed_status(void) > return TEST_SUCCESS; > } > > +static struct unit_test_suite dma_api_testsuite = { > + .suite_name = "DMA API Test Suite", > + .setup = testsuite_setup, > + .teardown = testsuite_teardown, > + .unit_test_cases = { > + TEST_CASE(test_dma_get_dev_id_by_name), > + TEST_CASE(test_dma_is_valid_dev), > + TEST_CASE(test_dma_count), > + TEST_CASE(test_dma_info_get), > + TEST_CASE(test_dma_configure), > + TEST_CASE(test_dma_vchan_setup), > + TEST_CASE(test_dma_start_stop), > + TEST_CASE(test_dma_stats), > + TEST_CASE(test_dma_dump), > + TEST_CASE(test_dma_completed), > + TEST_CASE(test_dma_completed_status), > + TEST_CASES_END() > + } > +}; > + > int > test_dma_api(uint16_t dev_id) > { > - int ret = testsuite_setup(dev_id); > - if (ret) { > - printf("testsuite setup fail!\n"); > - return -1; > - } > + test_dev_id = dev_id; > > /* If the testcase exit successfully, ensure that the test dmadev exist > * and the dmadev is in the stopped state. > */ I think the above comments could delete or place just before 'static struct unit_test_suite dma_api_testsuite = {' > - DMA_TEST_API_RUN(test_dma_get_dev_id_by_name); > - DMA_TEST_API_RUN(test_dma_is_valid_dev); > - DMA_TEST_API_RUN(test_dma_count); > - DMA_TEST_API_RUN(test_dma_info_get); > - DMA_TEST_API_RUN(test_dma_configure); > - DMA_TEST_API_RUN(test_dma_vchan_setup); > - DMA_TEST_API_RUN(test_dma_start_stop); > - DMA_TEST_API_RUN(test_dma_stats); > - DMA_TEST_API_RUN(test_dma_dump); > - DMA_TEST_API_RUN(test_dma_completed); > - DMA_TEST_API_RUN(test_dma_completed_status); > - > - testsuite_teardown(); > - > - printf("Total tests : %d\n", total); > - printf("Passed : %d\n", passed); > - printf("Failed : %d\n", failed); > - > - if (failed) > - return -1; > - > - return 0; > + return unit_test_suite_runner(&dma_api_testsuite); > }; > For the dmadev, which have API test, memory-to-memory test, memory-to-device test, currently packed into two test-suites, But I think its better use sub testsuite (could refer test_pdcp.c). Anyway, this is further more, we could do it later. As the first stage, I think current impl is OK with above comments addressed. Thanks Chengwen