| Message ID | 1547720125-32101-1-git-send-email-bernard.iremonger@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | akhil goyal |
| Headers | show |
| Series |
|
| Related | show |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/intel-Performance-Testing | success | Performance Testing PASS |
| ci/mellanox-Performance-Testing | success | Performance Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
> -----Original Message----- > From: Iremonger, Bernard > Sent: Thursday, January 17, 2019 10:15 AM > To: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com> > Cc: Iremonger, Bernard <bernard.iremonger@intel.com> > Subject: [PATCH v2] test/ipsec: fix test suite setup function > > Check for valid crypto_null device before continuing. > Use valid_dev instead of valid_devs[]. > Call create_crypto_session for one driver only. > > Fixes: 05fe65eb66b2 ("test/ipsec: introduce functional test") > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com> > --- > test/test/test_ipsec.c | 91 +++++++++++++++++++++++++------------------------- > 1 file changed, 45 insertions(+), 46 deletions(-) > > diff --git a/test/test/test_ipsec.c b/test/test/test_ipsec.c > index ff1a1c4..a5c1c4b 100644 > --- a/test/test/test_ipsec.c > +++ b/test/test/test_ipsec.c > @@ -62,7 +62,7 @@ struct ipsec_testsuite_params { > struct rte_cryptodev_config conf; > struct rte_cryptodev_qp_conf qp_conf; > > - uint8_t valid_devs[RTE_CRYPTO_MAX_DEVS]; > + uint8_t valid_dev; > uint8_t valid_dev_count; > }; > > @@ -218,8 +218,9 @@ testsuite_setup(void) > { > struct ipsec_testsuite_params *ts_params = &testsuite_params; > struct rte_cryptodev_info info; > - uint32_t nb_devs, dev_id; > + uint32_t i, nb_devs, dev_id; > size_t sess_sz; > + int driver_id; > > memset(ts_params, 0, sizeof(*ts_params)); > > @@ -251,10 +252,24 @@ testsuite_setup(void) > return TEST_FAILED; > } > > - ts_params->valid_devs[ts_params->valid_dev_count++] = 0; > + driver_id = rte_cryptodev_driver_id_get( > + RTE_STR(CRYPTODEV_NAME_NULL_PMD)); I still think that guessing device capabilities by driver name is not a right approach. Please use check_device_capabilites() here. > + > + /* Find first valid crypto device */ > + for (i = 0; i < nb_devs; i++) { > + rte_cryptodev_info_get(i, &info); > + if (info.driver_id == driver_id) { > + ts_params->valid_dev = i; > + ts_params->valid_dev_count++; As you use only one device now, I don't think you need valid_dev_count any more. > + break; > + } > + } > + > + if (ts_params->valid_dev_count < 1) > + return TEST_FAILED; > > /* Set up all the qps on the first of the valid devices found */ > - dev_id = ts_params->valid_devs[0]; > + dev_id = ts_params->valid_dev; > > rte_cryptodev_info_get(dev_id, &info); > > @@ -353,9 +368,9 @@ ut_setup(void) > ts_params->conf.socket_id = SOCKET_ID_ANY; > > /* Start the device */ > - TEST_ASSERT_SUCCESS(rte_cryptodev_start(ts_params->valid_devs[0]), > + TEST_ASSERT_SUCCESS(rte_cryptodev_start(ts_params->valid_dev), > "Failed to start cryptodev %u", > - ts_params->valid_devs[0]); > + ts_params->valid_dev); > > return TEST_SUCCESS; > } > @@ -399,7 +414,7 @@ ut_teardown(void) > rte_mempool_avail_count(ts_params->mbuf_pool)); > > /* Stop the device */ > - rte_cryptodev_stop(ts_params->valid_devs[0]); > + rte_cryptodev_stop(ts_params->valid_dev); > } >
Hi Konstantin, <snip> > > Subject: [PATCH v2] test/ipsec: fix test suite setup function > > > > Check for valid crypto_null device before continuing. > > Use valid_dev instead of valid_devs[]. > > Call create_crypto_session for one driver only. > > > > Fixes: 05fe65eb66b2 ("test/ipsec: introduce functional test") > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com> > > --- > > test/test/test_ipsec.c | 91 > > +++++++++++++++++++++++++------------------------- > > 1 file changed, 45 insertions(+), 46 deletions(-) > > > > diff --git a/test/test/test_ipsec.c b/test/test/test_ipsec.c index > > ff1a1c4..a5c1c4b 100644 > > --- a/test/test/test_ipsec.c > > +++ b/test/test/test_ipsec.c > > @@ -62,7 +62,7 @@ struct ipsec_testsuite_params { > > struct rte_cryptodev_config conf; > > struct rte_cryptodev_qp_conf qp_conf; > > > > - uint8_t valid_devs[RTE_CRYPTO_MAX_DEVS]; > > + uint8_t valid_dev; > > uint8_t valid_dev_count; > > }; > > > > @@ -218,8 +218,9 @@ testsuite_setup(void) { > > struct ipsec_testsuite_params *ts_params = &testsuite_params; > > struct rte_cryptodev_info info; > > - uint32_t nb_devs, dev_id; > > + uint32_t i, nb_devs, dev_id; > > size_t sess_sz; > > + int driver_id; > > > > memset(ts_params, 0, sizeof(*ts_params)); > > > > @@ -251,10 +252,24 @@ testsuite_setup(void) > > return TEST_FAILED; > > } > > > > - ts_params->valid_devs[ts_params->valid_dev_count++] = 0; > > + driver_id = rte_cryptodev_driver_id_get( > > + RTE_STR(CRYPTODEV_NAME_NULL_PMD)); > > > I still think that guessing device capabilities by driver name is not a right > approach. > Please use check_device_capabilites() here. Ok, this will require some refactoring of the tests. > > + > > + /* Find first valid crypto device */ > > + for (i = 0; i < nb_devs; i++) { > > + rte_cryptodev_info_get(i, &info); > > + if (info.driver_id == driver_id) { > > + ts_params->valid_dev = i; > > + ts_params->valid_dev_count++; > > As you use only one device now, I don't think you need valid_dev_count any > more. I will check if it is still needed. > > + break; > > + } > > + } > > + > > + if (ts_params->valid_dev_count < 1) > > + return TEST_FAILED; > > <snip> Regards, Bernard.
Hi Konstantin, <snip> > > > Subject: [PATCH v2] test/ipsec: fix test suite setup function > > > > > > Check for valid crypto_null device before continuing. > > > Use valid_dev instead of valid_devs[]. > > > Call create_crypto_session for one driver only. > > > > > > Fixes: 05fe65eb66b2 ("test/ipsec: introduce functional test") > > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com> > > > --- > > > test/test/test_ipsec.c | 91 > > > +++++++++++++++++++++++++------------------------- > > > 1 file changed, 45 insertions(+), 46 deletions(-) > > > > > > diff --git a/test/test/test_ipsec.c b/test/test/test_ipsec.c index > > > ff1a1c4..a5c1c4b 100644 > > > --- a/test/test/test_ipsec.c > > > +++ b/test/test/test_ipsec.c > > > @@ -62,7 +62,7 @@ struct ipsec_testsuite_params { > > > struct rte_cryptodev_config conf; > > > struct rte_cryptodev_qp_conf qp_conf; > > > > > > - uint8_t valid_devs[RTE_CRYPTO_MAX_DEVS]; > > > + uint8_t valid_dev; > > > uint8_t valid_dev_count; > > > }; > > > > > > @@ -218,8 +218,9 @@ testsuite_setup(void) { > > > struct ipsec_testsuite_params *ts_params = &testsuite_params; > > > struct rte_cryptodev_info info; > > > - uint32_t nb_devs, dev_id; > > > + uint32_t i, nb_devs, dev_id; > > > size_t sess_sz; > > > + int driver_id; > > > > > > memset(ts_params, 0, sizeof(*ts_params)); > > > > > > @@ -251,10 +252,24 @@ testsuite_setup(void) > > > return TEST_FAILED; > > > } > > > > > > - ts_params->valid_devs[ts_params->valid_dev_count++] = 0; > > > + driver_id = rte_cryptodev_driver_id_get( > > > + RTE_STR(CRYPTODEV_NAME_NULL_PMD)); > > > > > > I still think that guessing device capabilities by driver name is not > > a right approach. > > Please use check_device_capabilites() here. > > Ok, this will require some refactoring of the tests. I will refactor test_ipsec.c > > > + > > > + /* Find first valid crypto device */ > > > + for (i = 0; i < nb_devs; i++) { > > > + rte_cryptodev_info_get(i, &info); > > > + if (info.driver_id == driver_id) { > > > + ts_params->valid_dev = i; > > > + ts_params->valid_dev_count++; > > > > As you use only one device now, I don't think you need valid_dev_count > > any more. > > I will check if it is still needed. Still need to know that a valid device has been found. I will rename valid_dev_count to valid_dev_found. > > > > + break; > > > + } > > > + } > > > + > > > + if (ts_params->valid_dev_count < 1) > > > + return TEST_FAILED; > > > > > <snip> I will send a v3 patch. Regards, Bernard.
diff --git a/test/test/test_ipsec.c b/test/test/test_ipsec.c index ff1a1c4..a5c1c4b 100644 --- a/test/test/test_ipsec.c +++ b/test/test/test_ipsec.c @@ -62,7 +62,7 @@ struct ipsec_testsuite_params { struct rte_cryptodev_config conf; struct rte_cryptodev_qp_conf qp_conf; - uint8_t valid_devs[RTE_CRYPTO_MAX_DEVS]; + uint8_t valid_dev; uint8_t valid_dev_count; }; @@ -218,8 +218,9 @@ testsuite_setup(void) { struct ipsec_testsuite_params *ts_params = &testsuite_params; struct rte_cryptodev_info info; - uint32_t nb_devs, dev_id; + uint32_t i, nb_devs, dev_id; size_t sess_sz; + int driver_id; memset(ts_params, 0, sizeof(*ts_params)); @@ -251,10 +252,24 @@ testsuite_setup(void) return TEST_FAILED; } - ts_params->valid_devs[ts_params->valid_dev_count++] = 0; + driver_id = rte_cryptodev_driver_id_get( + RTE_STR(CRYPTODEV_NAME_NULL_PMD)); + + /* Find first valid crypto device */ + for (i = 0; i < nb_devs; i++) { + rte_cryptodev_info_get(i, &info); + if (info.driver_id == driver_id) { + ts_params->valid_dev = i; + ts_params->valid_dev_count++; + break; + } + } + + if (ts_params->valid_dev_count < 1) + return TEST_FAILED; /* Set up all the qps on the first of the valid devices found */ - dev_id = ts_params->valid_devs[0]; + dev_id = ts_params->valid_dev; rte_cryptodev_info_get(dev_id, &info); @@ -353,9 +368,9 @@ ut_setup(void) ts_params->conf.socket_id = SOCKET_ID_ANY; /* Start the device */ - TEST_ASSERT_SUCCESS(rte_cryptodev_start(ts_params->valid_devs[0]), + TEST_ASSERT_SUCCESS(rte_cryptodev_start(ts_params->valid_dev), "Failed to start cryptodev %u", - ts_params->valid_devs[0]); + ts_params->valid_dev); return TEST_SUCCESS; } @@ -399,7 +414,7 @@ ut_teardown(void) rte_mempool_avail_count(ts_params->mbuf_pool)); /* Stop the device */ - rte_cryptodev_stop(ts_params->valid_devs[0]); + rte_cryptodev_stop(ts_params->valid_dev); } #define IPSEC_MAX_PAD_SIZE UINT8_MAX @@ -589,57 +604,42 @@ create_dummy_sec_session(struct ipsec_unitest_params *ut, static int create_crypto_session(struct ipsec_unitest_params *ut, - struct rte_cryptodev_qp_conf *qp, const uint8_t crypto_dev[], - uint32_t crypto_dev_num, uint32_t j) + struct rte_cryptodev_qp_conf *qp, uint8_t crypto_dev, uint32_t j) { int32_t rc; - uint32_t devnum, i; struct rte_cryptodev_sym_session *s; - uint8_t devid[RTE_CRYPTO_MAX_DEVS]; - - /* check which cryptodevs support SA */ - devnum = 0; - for (i = 0; i < crypto_dev_num; i++) { - if (check_cryptodev_capablity(ut, crypto_dev[i]) == 0) - devid[devnum++] = crypto_dev[i]; - } + uint8_t devid; - if (devnum == 0) + /* check if crypto device supports SA */ + if (check_cryptodev_capablity(ut, crypto_dev) == 0) + devid = crypto_dev; + else return -ENODEV; s = rte_cryptodev_sym_session_create(qp->mp_session); if (s == NULL) return -ENOMEM; - /* initiliaze SA crypto session for all supported devices */ - for (i = 0; i != devnum; i++) { - rc = rte_cryptodev_sym_session_init(devid[i], s, + /* initiliaze SA crypto session for device */ + rc = rte_cryptodev_sym_session_init(devid, s, ut->crypto_xforms, qp->mp_session_private); - if (rc != 0) - break; - } - - if (i == devnum) { + if (rc == 0) { ut->ss[j].crypto.ses = s; return 0; + } else { + /* failure, do cleanup */ + rte_cryptodev_sym_session_clear(devid, s); + rte_cryptodev_sym_session_free(s); + return rc; } - - /* failure, do cleanup */ - while (i-- != 0) - rte_cryptodev_sym_session_clear(devid[i], s); - - rte_cryptodev_sym_session_free(s); - return rc; } static int create_session(struct ipsec_unitest_params *ut, - struct rte_cryptodev_qp_conf *qp, const uint8_t crypto_dev[], - uint32_t crypto_dev_num, uint32_t j) + struct rte_cryptodev_qp_conf *qp, uint8_t crypto_dev, uint32_t j) { if (ut->ss[j].type == RTE_SECURITY_ACTION_TYPE_NONE) - return create_crypto_session(ut, qp, crypto_dev, - crypto_dev_num, j); + return create_crypto_session(ut, qp, crypto_dev, j); else return create_dummy_sec_session(ut, qp, j); } @@ -740,8 +740,7 @@ create_sa(enum rte_security_session_action_type action_type, "failed to allocate memory for rte_ipsec_sa\n"); ut->ss[j].type = action_type; - rc = create_session(ut, &ts->qp_conf, ts->valid_devs, - ts->valid_dev_count, j); + rc = create_session(ut, &ts->qp_conf, ts->valid_dev, j); if (rc != 0) return TEST_FAILED; @@ -768,14 +767,14 @@ crypto_ipsec(uint16_t num_pkts) RTE_LOG(ERR, USER1, "rte_ipsec_pkt_crypto_prepare fail\n"); return TEST_FAILED; } - k = rte_cryptodev_enqueue_burst(ts_params->valid_devs[0], 0, + k = rte_cryptodev_enqueue_burst(ts_params->valid_dev, 0, ut_params->cop, num_pkts); if (k != num_pkts) { RTE_LOG(ERR, USER1, "rte_cryptodev_enqueue_burst fail\n"); return TEST_FAILED; } - k = rte_cryptodev_dequeue_burst(ts_params->valid_devs[0], 0, + k = rte_cryptodev_dequeue_burst(ts_params->valid_dev, 0, ut_params->cop, num_pkts); if (k != num_pkts) { RTE_LOG(ERR, USER1, "rte_cryptodev_dequeue_burst fail\n"); @@ -882,7 +881,7 @@ crypto_ipsec_2sa(void) "rte_ipsec_pkt_crypto_prepare fail\n"); return TEST_FAILED; } - k = rte_cryptodev_enqueue_burst(ts_params->valid_devs[0], 0, + k = rte_cryptodev_enqueue_burst(ts_params->valid_dev, 0, ut_params->cop + i, 1); if (k != 1) { RTE_LOG(ERR, USER1, @@ -891,7 +890,7 @@ crypto_ipsec_2sa(void) } } - k = rte_cryptodev_dequeue_burst(ts_params->valid_devs[0], 0, + k = rte_cryptodev_dequeue_burst(ts_params->valid_dev, 0, ut_params->cop, BURST_SIZE); if (k != BURST_SIZE) { RTE_LOG(ERR, USER1, "rte_cryptodev_dequeue_burst fail\n"); @@ -1021,7 +1020,7 @@ crypto_ipsec_2sa_4grp(void) "rte_ipsec_pkt_crypto_prepare fail\n"); return TEST_FAILED; } - k = rte_cryptodev_enqueue_burst(ts_params->valid_devs[0], 0, + k = rte_cryptodev_enqueue_burst(ts_params->valid_dev, 0, ut_params->cop + i, 1); if (k != 1) { RTE_LOG(ERR, USER1, @@ -1030,7 +1029,7 @@ crypto_ipsec_2sa_4grp(void) } } - k = rte_cryptodev_dequeue_burst(ts_params->valid_devs[0], 0, + k = rte_cryptodev_dequeue_burst(ts_params->valid_dev, 0, ut_params->cop, BURST_SIZE); if (k != BURST_SIZE) { RTE_LOG(ERR, USER1, "rte_cryptodev_dequeue_burst fail\n");
Check for valid crypto_null device before continuing. Use valid_dev instead of valid_devs[]. Call create_crypto_session for one driver only. Fixes: 05fe65eb66b2 ("test/ipsec: introduce functional test") Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com> --- test/test/test_ipsec.c | 91 +++++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 46 deletions(-)