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

["Doherty, Declan" <declan.doherty@intel.com>]

On 13/04/2021 6:51 PM, Akhil Goyal wrote:
> 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

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?

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