DPDK patches and discussions
 help / color / mirror / Atom feed
From: Akhil Goyal <gakhil@marvell.com>
To: Ciara Power <ciara.power@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "declan.doherty@intel.com" <declan.doherty@intel.com>,
	"aconole@redhat.com" <aconole@redhat.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	Anoob Joseph <anoobj@marvell.com>,
	"ruifeng.wang@arm.com" <ruifeng.wang@arm.com>,
	"asomalap@amd.com" <asomalap@amd.com>,
	"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
	"g.singh@nxp.com" <g.singh@nxp.com>,
	Matan Azrad <matan@nvidia.com>,
	NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [EXT] [PATCH v2 3/6] test/crypto: refactor to use sub-testsuites
Date: Tue, 13 Apr 2021 17:51:24 +0000
Message-ID: <MW2PR18MB2284D57782BBCBDCADE39E42D84F9@MW2PR18MB2284.namprd18.prod.outlook.com> (raw)
In-Reply-To: <20210402142424.1353789-4-ciara.power@intel.com>

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


  reply	other threads:[~2021-04-13 17:51 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-02 14:24 [dpdk-dev] [PATCH v2 0/6] test: refactor crypto unit test framework Ciara Power
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 1/6] app/test: refactor of unit test suite runner Ciara Power
2021-04-05 12:10   ` Aaron Conole
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 2/6] test: introduce parent testsuite format Ciara Power
2021-04-05 12:30   ` Aaron Conole
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 3/6] test/crypto: refactor to use sub-testsuites Ciara Power
2021-04-13 17:51   ` Akhil Goyal [this message]
2021-04-13 18:17     ` [dpdk-dev] [EXT] " Thomas Monjalon
2021-04-14 11:12     ` Doherty, Declan
2021-04-14 11:18       ` Akhil Goyal
2021-04-14 11:20         ` Thomas Monjalon
2021-04-14 11:22           ` Akhil Goyal
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 4/6] test/crypto: move testsuite params to header file Ciara Power
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 5/6] test/crypto: fix return value on test skipped Ciara Power
2021-04-13 17:07   ` [dpdk-dev] [EXT] " Akhil Goyal
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 6/6] test/crypto: dynamically build blockcipher suite Ciara Power
2021-04-06  1:34 ` [dpdk-dev] [PATCH v2 0/6] test: refactor crypto unit test framework Ruifeng Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MW2PR18MB2284D57782BBCBDCADE39E42D84F9@MW2PR18MB2284.namprd18.prod.outlook.com \
    --to=gakhil@marvell.com \
    --cc=aconole@redhat.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=anoobj@marvell.com \
    --cc=asomalap@amd.com \
    --cc=ciara.power@intel.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=g.singh@nxp.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=matan@nvidia.com \
    --cc=ruifeng.wang@arm.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git