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 CD64AA0562; Wed, 14 Apr 2021 13:13:04 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B25A61619B5; Wed, 14 Apr 2021 13:13:04 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id 5B2971619A3 for ; Wed, 14 Apr 2021 13:13:02 +0200 (CEST) IronPort-SDR: u+D6fk4IcsdX1YI0EsUGb6u0lRX/7PmfiRDlFDd25jrjf0/QM1Tb8VAfUZ3KamKb/sqsIcix+m G0iJ97L0Xw9g== X-IronPort-AV: E=McAfee;i="6200,9189,9953"; a="215109276" X-IronPort-AV: E=Sophos;i="5.82,221,1613462400"; d="scan'208";a="215109276" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Apr 2021 04:12:58 -0700 IronPort-SDR: mCkj54Y9lnt+JNQhtyuthLjqMJgFIManNjuD6iZ3D1FiDrw1b38tZnrf14xGhOn99i6G85QfY9 zmyVMRhjSSYg== X-IronPort-AV: E=Sophos;i="5.82,221,1613462400"; d="scan'208";a="424692551" Received: from dwdohert-mobl1.ger.corp.intel.com (HELO [10.213.234.180]) ([10.213.234.180]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Apr 2021 04:12:55 -0700 To: Akhil Goyal , Ciara Power , "dev@dpdk.org" Cc: "aconole@redhat.com" , "hemant.agrawal@nxp.com" , Anoob Joseph , "ruifeng.wang@arm.com" , "asomalap@amd.com" , "ajit.khaparde@broadcom.com" , "g.singh@nxp.com" , Matan Azrad , NBU-Contact-Thomas Monjalon References: <20210402142424.1353789-1-ciara.power@intel.com> <20210402142424.1353789-4-ciara.power@intel.com> From: "Doherty, Declan" Message-ID: Date: Wed, 14 Apr 2021 12:12:50 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [EXT] [PATCH v2 3/6] test/crypto: refactor to use sub-testsuites 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 Sender: "dev" 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?