Re: [dpdk-dev] [EXT] [PATCH v2 3/6] test/crypto: refactor to use sub-testsuites

[Akhil Goyal <gakhil@marvell.com>]

Hi Declan,

> > //snip
> >
> >>   	nb_devs = rte_cryptodev_count();
> >>   	if (nb_devs < 1) {
> >>   		RTE_LOG(WARNING, USER1, "No crypto devices found?\n");
> >> @@ -838,6 +630,228 @@ testsuite_setup(void)
> >>   	return TEST_SUCCESS;
> >>   }
> >>
> >> +static int
> >> +qat_testsuite_setup(void)
> >> +{
> >> +	return testsuite_params_setup();
> >> +}
> >> +
> >> +static int
> >> +aesni_mb_testsuite_setup(void)
> >> +{
> >> +	int32_t nb_devs, ret;
> >> +	nb_devs = rte_cryptodev_device_count_by_driver(
> >> +			rte_cryptodev_driver_id_get(
> >> +			RTE_STR(CRYPTODEV_NAME_AESNI_MB_PMD)));
> >> +	if (nb_devs < 1) {
> >> +		ret =
> >> rte_vdev_init(RTE_STR(CRYPTODEV_NAME_AESNI_MB_PMD), NULL);
> >> +
> >> +		TEST_ASSERT(ret == 0,
> >> +			"Failed to create instance of pmd : %s",
> >> +			RTE_STR(CRYPTODEV_NAME_AESNI_MB_PMD));
> >> +	}
> >> +	return testsuite_params_setup();
> >> +}
> >
> > Why is it required to have a separate function for each of the PMD?
> > I believe a single function with an argument should be sufficient.
> > And in that single function a simple switch case may process the vdevs
> differently.
> >
> > As of now, it looks that moving the unnecessary duplicate code from
> > one place to another.
> >
> > This was one cleanup we have been looking forward to from quite some
> time.
> > Every time a new PMD comes, a separate function is formed and the length
> of
> > the code increases.
> >
> >
> > //snip
> 
> The virtual device creation code could be dropped completely if user is
> guaranteed to create the required crypto PMD from EAL devargs and we
> could assume that the first crypto device of the test suite type is
> always used?

Agreed.
 
> 
> >>
> >> -static struct unit_test_suite cryptodev_virtio_testsuite = {
> >> +static struct unit_test_suite cryptodev_virtio_sub_testsuite = {
> >>   	.suite_name = "Crypto VIRTIO Unit Test Suite",
> >> -	.setup = testsuite_setup,
> >> -	.teardown = testsuite_teardown,
> >>   	.unit_test_cases = {
> >>   		TEST_CASE_ST(ut_setup, ut_teardown,
> >> test_AES_cipheronly_all),
> >>
> >> @@ -13935,15 +14107,12 @@ static struct unit_test_suite
> >> cryptodev_virtio_testsuite = {
> >>   	}
> >>   };
> >>
> >> -static struct unit_test_suite cryptodev_caam_jr_testsuite  = {
> >> -	.suite_name = "Crypto CAAM JR Unit Test Suite",
> >> -	.setup = testsuite_setup,
> >> -	.teardown = testsuite_teardown,
> >> +static struct unit_test_suite cryptodev_caam_jr_sub_testsuite = {
> >> +	.suite_name = "Crypto CAAM JR Sub Unit Test Suite",
> >>   	.unit_test_cases = {
> >>   		TEST_CASE_ST(ut_setup, ut_teardown,
> >> -			     test_device_configure_invalid_dev_id),
> >> -		TEST_CASE_ST(ut_setup, ut_teardown,
> >> -			     test_multi_session),
> >> +				test_device_configure_invalid_dev_id),
> >> +		TEST_CASE_ST(ut_setup, ut_teardown, test_multi_session),
> >>
> >>   		TEST_CASE_ST(ut_setup, ut_teardown, test_AES_chain_all),
> >>   		TEST_CASE_ST(ut_setup, ut_teardown, test_3DES_chain_all),
> >> @@ -13955,58 +14124,28 @@ static struct unit_test_suite
> >> cryptodev_caam_jr_testsuite  = {
> >>   	}
> >>   };
> >
> > Splitting the complete testsuite into logical generic algo based sub testsuite
> > Is a good idea. I appreciate that.
> >
> > But introducing PMD based test suite is not recommended. We have been
> > trying from past few releases to clean this up. And this patch is again
> introducing
> > the same. When I first saw this series, I saw only the algo based splitting
> and
> > when it was run on the board, it was showing results in an organized way.
> > But this was not expected that, PMD based test suites are reintroduced by
> > Intel who helped in removing them in last few releases.
> >
> > This will make an unnecessary addition of duplicate code whenever a new
> PMD
> > is introduced.
> >
> > I recommend to use a single parent suite - cryptodev_testsuite and there
> > Can be multiple sub testsuites based on Algos etc. but not on the basis of
> PMD.
> >
> > Regards,
> > Akhil
> >
> 
> Hey Akhil, I understand the sentiment of this, we were just trying to
> avoid necessary failures by executing testsuites which aren't supported
> by the PMD under test, and we're confident that all testsuites/tests are
> correctly verifying their capabilities requirements. If we add some code
> into the testsuite setup functions to test capabilities required for the
> testsuites vs those required by the PMD then we could do as you are
> suggesting. If we can make this change quickly would you consider this
> patchset for inclusion in RC2?

I can take these patches upto RC2.