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

[Akhil Goyal]

Hi Ciara,

//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
> 
> -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