diff mbox series

test/crypto: remove tests for unsupported descriptors

Message ID 1589267544-18134-1-git-send-email-anoobj@marvell.com (mailing list archive)
State Accepted, archived
Delegated to: akhil goyal
Headers
Series test/crypto: remove tests for unsupported descriptors |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues
ci/travis-robot success Travis build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues

Commit Message

Anoob Joseph May 12, 2020, 7:12 a.m. UTC 
  Cryptodev doesn't limit the number of descriptors that can be supported
by the PMD.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 app/test/test_cryptodev.c | 45 ---------------------------------------------
 1 file changed, 45 deletions(-)

Comments

Dybkowski, AdamX May 12, 2020, 1:07 p.m. UTC | #1 
Hi Anoob.

What's wrong with this test? Is this unit test failing on any PMD now?
I've checked on several (QAT, OpenSSL, SW ZUC, SW KASUMI, SW SNOW3G, SCHEDULER) and it passes everywhere. Then why should we remove it completely?

Adam

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> Sent: Tuesday, 12 May, 2020 09:12
> To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Cc: Anoob Joseph <anoobj@marvell.com>; Narayana Prasad
> <pathreya@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] test/crypto: remove tests for unsupported
> descriptors
> 
> Cryptodev doesn't limit the number of descriptors that can be supported by
> the PMD.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
>  app/test/test_cryptodev.c | 45 ---------------------------------------------
>  1 file changed, 45 deletions(-)
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> c624018..1ed2df8 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -891,36 +891,6 @@ test_queue_pair_descriptor_setup(void)
>  				ts_params->valid_devs[0]);
>  	}
> 
> -	/* invalid number of descriptors - max supported + 2 */
> -	qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT + 2;
> -
> -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> -				ts_params->valid_devs[0], qp_id, &qp_conf,
> -				rte_cryptodev_socket_id(
> -						ts_params->valid_devs[0])),
> -				"Unexpectedly passed test for "
> -				"rte_cryptodev_queue_pair_setup:"
> -				"num_inflights %u on qp %u on cryptodev
> %u",
> -				qp_conf.nb_descriptors, qp_id,
> -				ts_params->valid_devs[0]);
> -	}
> -
> -	/* invalid number of descriptors - max value of parameter */
> -	qp_conf.nb_descriptors = UINT32_MAX-1;
> -
> -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> -				ts_params->valid_devs[0], qp_id, &qp_conf,
> -				rte_cryptodev_socket_id(
> -						ts_params->valid_devs[0])),
> -				"Unexpectedly passed test for "
> -				"rte_cryptodev_queue_pair_setup:"
> -				"num_inflights %u on qp %u on cryptodev
> %u",
> -				qp_conf.nb_descriptors, qp_id,
> -				ts_params->valid_devs[0]);
> -	}
> -
>  	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
> 
>  	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> @@ -935,21 +905,6 @@ test_queue_pair_descriptor_setup(void)
>  				ts_params->valid_devs[0]);
>  	}
> 
> -	/* invalid number of descriptors - max supported + 1 */
> -	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT + 1;
> -
> -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> -				ts_params->valid_devs[0], qp_id, &qp_conf,
> -				rte_cryptodev_socket_id(
> -						ts_params->valid_devs[0])),
> -				"Unexpectedly passed test for "
> -				"rte_cryptodev_queue_pair_setup:"
> -				"num_inflights %u on qp %u on cryptodev
> %u",
> -				qp_conf.nb_descriptors, qp_id,
> -				ts_params->valid_devs[0]);
> -	}
> -
>  	/* test invalid queue pair id */
>  	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
> 	/*valid */
> 
> --
> 2.7.4
Akhil Goyal May 12, 2020, 1:19 p.m. UTC | #2 
Hi Adam,
> 
> Hi Anoob.
> 
> What's wrong with this test? Is this unit test failing on any PMD now?
> I've checked on several (QAT, OpenSSL, SW ZUC, SW KASUMI, SW SNOW3G,
> SCHEDULER) and it passes everywhere. Then why should we remove it
> completely?
> 

The problem is not with the valid cases. It is the negative cases which will cause issue
On most of the hardware PMDs which do not use qp_conf and simply ignore the values set in qp_conf.
So it actually does not matter whether you set valid or invalid value for those PMDs

I believe negative test should not be there for parameters which are optional to be used.
And IMO the patch is fine.

Regards,
Akhil
Anoob Joseph May 12, 2020, 1:26 p.m. UTC | #3 
Hi Adam,

The crypto library doesn't specify an upper limit for the nb_descriptors to be passed for queue_pair_setup. So technically all the values that you have passed as invalid is not correct.

I did check few PMDs when I saw this issue with OCTEON TX2 crypto PMD. QAT support max 4096 descriptors and so the invalids that is present is exactly following that. I checked DPAA drivers, but in that the entire conf is left unused. So I'm not sure whether DPAA passes these cases. I couldn't check the entire list that you have mentioned. But it doesn't much sense for s/w PMDs to limit max_nb_descriptors. 

If we need to have an invalid check, we should probably add max_nb_descriptors in dev_info. And then we can have these tests. I'm open for that idea also. But I don't know whether that is allowed now or not. I leave that to Akhil.

Thanks,
Anoob

> -----Original Message-----
> From: Dybkowski, AdamX <adamx.dybkowski@intel.com>
> Sent: Tuesday, May 12, 2020 6:37 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Doherty, Declan <declan.doherty@intel.com>
> Cc: Narayana Prasad Raju Athreya <pathreya@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Subject: [EXT] RE: [dpdk-dev] [PATCH] test/crypto: remove tests for
> unsupported descriptors
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob.
> 
> What's wrong with this test? Is this unit test failing on any PMD now?
> I've checked on several (QAT, OpenSSL, SW ZUC, SW KASUMI, SW SNOW3G,
> SCHEDULER) and it passes everywhere. Then why should we remove it
> completely?
> 
> Adam
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> > Sent: Tuesday, 12 May, 2020 09:12
> > To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > <declan.doherty@intel.com>
> > Cc: Anoob Joseph <anoobj@marvell.com>; Narayana Prasad
> > <pathreya@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; De Lara
> > Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] test/crypto: remove tests for unsupported
> > descriptors
> >
> > Cryptodev doesn't limit the number of descriptors that can be
> > supported by the PMD.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > ---
> >  app/test/test_cryptodev.c | 45
> > ---------------------------------------------
> >  1 file changed, 45 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index
> > c624018..1ed2df8 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -891,36 +891,6 @@ test_queue_pair_descriptor_setup(void)
> >  				ts_params->valid_devs[0]);
> >  	}
> >
> > -	/* invalid number of descriptors - max supported + 2 */
> > -	qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT + 2;
> > -
> > -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> > -				ts_params->valid_devs[0], qp_id, &qp_conf,
> > -				rte_cryptodev_socket_id(
> > -						ts_params->valid_devs[0])),
> > -				"Unexpectedly passed test for "
> > -				"rte_cryptodev_queue_pair_setup:"
> > -				"num_inflights %u on qp %u on cryptodev
> > %u",
> > -				qp_conf.nb_descriptors, qp_id,
> > -				ts_params->valid_devs[0]);
> > -	}
> > -
> > -	/* invalid number of descriptors - max value of parameter */
> > -	qp_conf.nb_descriptors = UINT32_MAX-1;
> > -
> > -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> > -				ts_params->valid_devs[0], qp_id, &qp_conf,
> > -				rte_cryptodev_socket_id(
> > -						ts_params->valid_devs[0])),
> > -				"Unexpectedly passed test for "
> > -				"rte_cryptodev_queue_pair_setup:"
> > -				"num_inflights %u on qp %u on cryptodev
> > %u",
> > -				qp_conf.nb_descriptors, qp_id,
> > -				ts_params->valid_devs[0]);
> > -	}
> > -
> >  	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
> >
> >  	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > @@ -935,21 +905,6 @@ test_queue_pair_descriptor_setup(void)
> >  				ts_params->valid_devs[0]);
> >  	}
> >
> > -	/* invalid number of descriptors - max supported + 1 */
> > -	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT + 1;
> > -
> > -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> > -				ts_params->valid_devs[0], qp_id, &qp_conf,
> > -				rte_cryptodev_socket_id(
> > -						ts_params->valid_devs[0])),
> > -				"Unexpectedly passed test for "
> > -				"rte_cryptodev_queue_pair_setup:"
> > -				"num_inflights %u on qp %u on cryptodev
> > %u",
> > -				qp_conf.nb_descriptors, qp_id,
> > -				ts_params->valid_devs[0]);
> > -	}
> > -
> >  	/* test invalid queue pair id */
> >  	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
> > 	/*valid */
> >
> > --
> > 2.7.4
Dybkowski, AdamX May 13, 2020, 8:33 a.m. UTC | #4 
Hi Anoob.

Thanks for this longer explanation.
I think can remove the test as intended.

Acked-by: Adam Dybkowski <adamx.dybkowski@intel.com>

Adam 

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Tuesday, 12 May, 2020 15:27
> To: Dybkowski, AdamX <adamx.dybkowski@intel.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>
> Cc: Narayana Prasad Raju Athreya <pathreya@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] test/crypto: remove tests for unsupported
> descriptors
> 
> Hi Adam,
> 
> The crypto library doesn't specify an upper limit for the nb_descriptors to be
> passed for queue_pair_setup. So technically all the values that you have
> passed as invalid is not correct.
> 
> I did check few PMDs when I saw this issue with OCTEON TX2 crypto PMD.
> QAT support max 4096 descriptors and so the invalids that is present is
> exactly following that. I checked DPAA drivers, but in that the entire conf is
> left unused. So I'm not sure whether DPAA passes these cases. I couldn't
> check the entire list that you have mentioned. But it doesn't much sense for
> s/w PMDs to limit max_nb_descriptors.
> 
> If we need to have an invalid check, we should probably add
> max_nb_descriptors in dev_info. And then we can have these tests. I'm open
> for that idea also. But I don't know whether that is allowed now or not. I
> leave that to Akhil.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Dybkowski, AdamX <adamx.dybkowski@intel.com>
> > Sent: Tuesday, May 12, 2020 6:37 PM
> > To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> > <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>
> > Cc: Narayana Prasad Raju Athreya <pathreya@marvell.com>; Ankur
> Dwivedi
> > <adwivedi@marvell.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> > Subject: [EXT] RE: [dpdk-dev] [PATCH] test/crypto: remove tests for
> > unsupported descriptors
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > Hi Anoob.
> >
> > What's wrong with this test? Is this unit test failing on any PMD now?
> > I've checked on several (QAT, OpenSSL, SW ZUC, SW KASUMI, SW SNOW3G,
> > SCHEDULER) and it passes everywhere. Then why should we remove it
> > completely?
> >
> > Adam
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> > > Sent: Tuesday, 12 May, 2020 09:12
> > > To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > > <declan.doherty@intel.com>
> > > Cc: Anoob Joseph <anoobj@marvell.com>; Narayana Prasad
> > > <pathreya@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; De
> > > Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH] test/crypto: remove tests for
> > > unsupported descriptors
> > >
> > > Cryptodev doesn't limit the number of descriptors that can be
> > > supported by the PMD.
> > >
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > ---
> > >  app/test/test_cryptodev.c | 45
> > > ---------------------------------------------
> > >  1 file changed, 45 deletions(-)
> > >
> > > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > > index
> > > c624018..1ed2df8 100644
> > > --- a/app/test/test_cryptodev.c
> > > +++ b/app/test/test_cryptodev.c
> > > @@ -891,36 +891,6 @@ test_queue_pair_descriptor_setup(void)
> > >  				ts_params->valid_devs[0]);
> > >  	}
> > >
> > > -	/* invalid number of descriptors - max supported + 2 */
> > > -	qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT + 2;
> > > -
> > > -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > > -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> > > -				ts_params->valid_devs[0], qp_id, &qp_conf,
> > > -				rte_cryptodev_socket_id(
> > > -						ts_params->valid_devs[0])),
> > > -				"Unexpectedly passed test for "
> > > -				"rte_cryptodev_queue_pair_setup:"
> > > -				"num_inflights %u on qp %u on cryptodev
> > > %u",
> > > -				qp_conf.nb_descriptors, qp_id,
> > > -				ts_params->valid_devs[0]);
> > > -	}
> > > -
> > > -	/* invalid number of descriptors - max value of parameter */
> > > -	qp_conf.nb_descriptors = UINT32_MAX-1;
> > > -
> > > -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > > -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> > > -				ts_params->valid_devs[0], qp_id, &qp_conf,
> > > -				rte_cryptodev_socket_id(
> > > -						ts_params->valid_devs[0])),
> > > -				"Unexpectedly passed test for "
> > > -				"rte_cryptodev_queue_pair_setup:"
> > > -				"num_inflights %u on qp %u on cryptodev
> > > %u",
> > > -				qp_conf.nb_descriptors, qp_id,
> > > -				ts_params->valid_devs[0]);
> > > -	}
> > > -
> > >  	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
> > >
> > >  	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > > @@ -935,21 +905,6 @@ test_queue_pair_descriptor_setup(void)
> > >  				ts_params->valid_devs[0]);
> > >  	}
> > >
> > > -	/* invalid number of descriptors - max supported + 1 */
> > > -	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT + 1;
> > > -
> > > -	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
> > > -		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
> > > -				ts_params->valid_devs[0], qp_id, &qp_conf,
> > > -				rte_cryptodev_socket_id(
> > > -						ts_params->valid_devs[0])),
> > > -				"Unexpectedly passed test for "
> > > -				"rte_cryptodev_queue_pair_setup:"
> > > -				"num_inflights %u on qp %u on cryptodev
> > > %u",
> > > -				qp_conf.nb_descriptors, qp_id,
> > > -				ts_params->valid_devs[0]);
> > > -	}
> > > -
> > >  	/* test invalid queue pair id */
> > >  	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
> > > 	/*valid */
> > >
> > > --
> > > 2.7.4
Akhil Goyal May 15, 2020, 6:09 p.m. UTC | #5 
> 
> Hi Anoob.
> 
> Thanks for this longer explanation.
> I think can remove the test as intended.
> 
> Acked-by: Adam Dybkowski <adamx.dybkowski@intel.com>
> 
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Applied to dpdk-next-crypto

Thanks.
diff mbox series

Patch

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index c624018..1ed2df8 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -891,36 +891,6 @@  test_queue_pair_descriptor_setup(void)
 				ts_params->valid_devs[0]);
 	}
 
-	/* invalid number of descriptors - max supported + 2 */
-	qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT + 2;
-
-	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
-		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
-				ts_params->valid_devs[0], qp_id, &qp_conf,
-				rte_cryptodev_socket_id(
-						ts_params->valid_devs[0])),
-				"Unexpectedly passed test for "
-				"rte_cryptodev_queue_pair_setup:"
-				"num_inflights %u on qp %u on cryptodev %u",
-				qp_conf.nb_descriptors, qp_id,
-				ts_params->valid_devs[0]);
-	}
-
-	/* invalid number of descriptors - max value of parameter */
-	qp_conf.nb_descriptors = UINT32_MAX-1;
-
-	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
-		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
-				ts_params->valid_devs[0], qp_id, &qp_conf,
-				rte_cryptodev_socket_id(
-						ts_params->valid_devs[0])),
-				"Unexpectedly passed test for "
-				"rte_cryptodev_queue_pair_setup:"
-				"num_inflights %u on qp %u on cryptodev %u",
-				qp_conf.nb_descriptors, qp_id,
-				ts_params->valid_devs[0]);
-	}
-
 	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
 
 	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
@@ -935,21 +905,6 @@  test_queue_pair_descriptor_setup(void)
 				ts_params->valid_devs[0]);
 	}
 
-	/* invalid number of descriptors - max supported + 1 */
-	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT + 1;
-
-	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs; qp_id++) {
-		TEST_ASSERT_FAIL(rte_cryptodev_queue_pair_setup(
-				ts_params->valid_devs[0], qp_id, &qp_conf,
-				rte_cryptodev_socket_id(
-						ts_params->valid_devs[0])),
-				"Unexpectedly passed test for "
-				"rte_cryptodev_queue_pair_setup:"
-				"num_inflights %u on qp %u on cryptodev %u",
-				qp_conf.nb_descriptors, qp_id,
-				ts_params->valid_devs[0]);
-	}
-
 	/* test invalid queue pair id */
 	qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;	/*valid */