diff mbox series

[v2] app/test-crypto-perf: add throughput OOP decryption

Message ID 20240319114623.1137757-1-suanmingm@nvidia.com (mailing list archive)
State New
Delegated to: akhil goyal
Headers
Series [v2] app/test-crypto-perf: add throughput OOP decryption |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-sample-apps-testing warning Testing issues
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS

Commit Message

Suanming Mou March 19, 2024, 11:46 a.m. UTC 
  During throughput running, re-filling the test data will
impact the performance test result. So for now, to run
decrypt throughput testing is not supported since the
test data is not filled.

But if user requires OOP(out-of-place) mode, the test
data from source mbuf will never be modified, and if
the test data can be prepared out of the running loop,
the decryption test should be fine.

This commit adds the support of out-of-place decryption
testing for throughput.

[1]:
http://mails.dpdk.org/archives/dev/2023-July/273328.html

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---
 app/test-crypto-perf/cperf_ops.c             |  5 ++-
 app/test-crypto-perf/cperf_options_parsing.c |  8 +++++
 app/test-crypto-perf/cperf_test_throughput.c | 34 +++++++++++++++++---
 3 files changed, 41 insertions(+), 6 deletions(-)

Comments

Power, Ciara March 19, 2024, 3:14 p.m. UTC | #1 
> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Tuesday, March 19, 2024 11:46 AM
> To: gakhil@marvell.com; Power, Ciara <ciara.power@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
> 
> During throughput running, re-filling the test data will impact the performance
> test result. So for now, to run decrypt throughput testing is not supported since
> the test data is not filled.
> 
> But if user requires OOP(out-of-place) mode, the test data from source mbuf will
> never be modified, and if the test data can be prepared out of the running loop,
> the decryption test should be fine.
> 
> This commit adds the support of out-of-place decryption testing for throughput.
> 
> [1]:
> http://mails.dpdk.org/archives/dev/2023-July/273328.html
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> ---
>  app/test-crypto-perf/cperf_ops.c             |  5 ++-
>  app/test-crypto-perf/cperf_options_parsing.c |  8 +++++  app/test-crypto-
> perf/cperf_test_throughput.c | 34 +++++++++++++++++---
>  3 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
> index d3fd115bc0..714616c697 100644
> --- a/app/test-crypto-perf/cperf_ops.c
> +++ b/app/test-crypto-perf/cperf_ops.c
> @@ -644,7 +644,10 @@ cperf_set_ops_aead(struct rte_crypto_op **ops,
>  	}
> 
>  	if ((options->test == CPERF_TEST_TYPE_VERIFY) ||
> -			(options->test == CPERF_TEST_TYPE_LATENCY)) {
> +	    (options->test == CPERF_TEST_TYPE_LATENCY) ||
> +	    (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> +	     (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> +	      options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) {
>  		for (i = 0; i < nb_ops; i++) {
>  			uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i],
>  					uint8_t *, iv_offset);
> diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-
> perf/cperf_options_parsing.c
> index 8c20974273..90526e676f 100644
> --- a/app/test-crypto-perf/cperf_options_parsing.c
> +++ b/app/test-crypto-perf/cperf_options_parsing.c
> @@ -1341,6 +1341,14 @@ cperf_options_check(struct cperf_options
> *options)
>  		}
>  	}
> 
> +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> +	     options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
> +	    !options->out_of_place) {
> +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> throughput decryption.\n");
> +		return -EINVAL;
> +	}

Not totally following some of this, why do we only want to add this for OOP mode?

For example an inplace command I can use before this patch but not after:
./build/app/dpdk-test-crypto-perf -l 2,3 -- --ptest throughput --optype aead --aead-algo aes-gcm --aead-op decrypt --devtype crypto_qat --aead-key-sz 16

I get an error;
USER1: Only out-of-place is allowed in throughput decryption.
USER1: Checking one or more user options failed

Do we want to always force the user to use OOP + test vector file for these throughput decryption tests?
Or should we just add a warning that the throughput may not be reflecting the "success" verify path in PMD if using inplace and the dummy data.

I am not sure.
If we do want to add the limitation on the throughput tests, these changes I think are ok for that.

Thanks,
Ciara

> +
>  	if (options->op_type == CPERF_CIPHER_ONLY ||
>  			options->op_type == CPERF_CIPHER_THEN_AUTH ||
>  			options->op_type == CPERF_AUTH_THEN_CIPHER) { diff
> --git a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-
> perf/cperf_test_throughput.c
> index e3d266d7a4..b347baa913 100644
> --- a/app/test-crypto-perf/cperf_test_throughput.c
> +++ b/app/test-crypto-perf/cperf_test_throughput.c
> @@ -99,6 +99,26 @@ cperf_throughput_test_constructor(struct rte_mempool
> *sess_mp,
>  	return NULL;
>  }
> 
> +static void
> +cperf_verify_init_ops(struct rte_mempool *mp __rte_unused,
> +		      void *opaque_arg,
> +		      void *obj,
> +		      __rte_unused unsigned int i)
> +{
> +	uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> +		sizeof(struct rte_crypto_sym_op);
> +	uint32_t imix_idx = 0;
> +	struct cperf_throughput_ctx *ctx = opaque_arg;
> +	struct rte_crypto_op *op = obj;
> +
> +	(ctx->populate_ops)(&op, ctx->src_buf_offset,
> +			ctx->dst_buf_offset,
> +			1, ctx->sess, ctx->options,
> +			ctx->test_vector, iv_offset, &imix_idx, NULL);
> +
> +	cperf_mbuf_set(op->sym->m_src, ctx->options, ctx->test_vector); }
> +
>  int
>  cperf_throughput_test_runner(void *test_ctx)  { @@ -144,6 +164,9 @@
> cperf_throughput_test_runner(void *test_ctx)
>  	uint16_t iv_offset = sizeof(struct rte_crypto_op) +
>  		sizeof(struct rte_crypto_sym_op);
> 
> +	if (ctx->options->out_of_place)
> +		rte_mempool_obj_iter(ctx->pool, cperf_verify_init_ops, (void
> *)ctx);
> +
>  	while (test_burst_size <= ctx->options->max_burst_size) {
>  		uint64_t ops_enqd = 0, ops_enqd_total = 0, ops_enqd_failed =
> 0;
>  		uint64_t ops_deqd = 0, ops_deqd_total = 0, ops_deqd_failed =
> 0; @@ -176,11 +199,12 @@ cperf_throughput_test_runner(void *test_ctx)
>  			}
> 
>  			/* Setup crypto op, attach mbuf etc */
> -			(ctx->populate_ops)(ops, ctx->src_buf_offset,
> -					ctx->dst_buf_offset,
> -					ops_needed, ctx->sess,
> -					ctx->options, ctx->test_vector,
> -					iv_offset, &imix_idx, &tsc_start);
> +			if (!ctx->options->out_of_place)
> +				(ctx->populate_ops)(ops, ctx->src_buf_offset,
> +						ctx->dst_buf_offset,
> +						ops_needed, ctx->sess,
> +						ctx->options, ctx->test_vector,
> +						iv_offset, &imix_idx,
> &tsc_start);
> 
>  			/**
>  			 * When ops_needed is smaller than ops_enqd, the
> --
> 2.34.1
Suanming Mou March 20, 2024, 12:14 a.m. UTC | #2 
> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Tuesday, March 19, 2024 11:15 PM
> To: Suanming Mou <suanmingm@nvidia.com>; gakhil@marvell.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
> 
> 
> 
> > -----Original Message-----
> > From: Suanming Mou <suanmingm@nvidia.com>
> > Sent: Tuesday, March 19, 2024 11:46 AM
> > To: gakhil@marvell.com; Power, Ciara <ciara.power@intel.com>
> > Cc: dev@dpdk.org
> > Subject: [PATCH v2] app/test-crypto-perf: add throughput OOP
> > decryption
> >
> > During throughput running, re-filling the test data will impact the
> > performance test result. So for now, to run decrypt throughput testing
> > is not supported since the test data is not filled.
> >
> > But if user requires OOP(out-of-place) mode, the test data from source
> > mbuf will never be modified, and if the test data can be prepared out
> > of the running loop, the decryption test should be fine.
> >
> > This commit adds the support of out-of-place decryption testing for throughput.
> >
> > [1]:
> > http://mails.dpdk.org/archives/dev/2023-July/273328.html
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > ---
> >  app/test-crypto-perf/cperf_ops.c             |  5 ++-
> >  app/test-crypto-perf/cperf_options_parsing.c |  8 +++++
> > app/test-crypto- perf/cperf_test_throughput.c | 34
> > +++++++++++++++++---
> >  3 files changed, 41 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test-crypto-perf/cperf_ops.c
> > b/app/test-crypto-perf/cperf_ops.c
> > index d3fd115bc0..714616c697 100644
> > --- a/app/test-crypto-perf/cperf_ops.c
> > +++ b/app/test-crypto-perf/cperf_ops.c
> > @@ -644,7 +644,10 @@ cperf_set_ops_aead(struct rte_crypto_op **ops,
> >  	}
> >
> >  	if ((options->test == CPERF_TEST_TYPE_VERIFY) ||
> > -			(options->test == CPERF_TEST_TYPE_LATENCY)) {
> > +	    (options->test == CPERF_TEST_TYPE_LATENCY) ||
> > +	    (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > +	     (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > +	      options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) {
> >  		for (i = 0; i < nb_ops; i++) {
> >  			uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i],
> >  					uint8_t *, iv_offset);
> > diff --git a/app/test-crypto-perf/cperf_options_parsing.c
> > b/app/test-crypto- perf/cperf_options_parsing.c index
> > 8c20974273..90526e676f 100644
> > --- a/app/test-crypto-perf/cperf_options_parsing.c
> > +++ b/app/test-crypto-perf/cperf_options_parsing.c
> > @@ -1341,6 +1341,14 @@ cperf_options_check(struct cperf_options
> > *options)
> >  		}
> >  	}
> >
> > +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > +	     options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
> > +	    !options->out_of_place) {
> > +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > throughput decryption.\n");
> > +		return -EINVAL;
> > +	}
> 
> Not totally following some of this, why do we only want to add this for OOP
> mode?
> 
> For example an inplace command I can use before this patch but not after:
> ./build/app/dpdk-test-crypto-perf -l 2,3 -- --ptest throughput --optype aead --
> aead-algo aes-gcm --aead-op decrypt --devtype crypto_qat --aead-key-sz 16
> 
> I get an error;
> USER1: Only out-of-place is allowed in throughput decryption.
> USER1: Checking one or more user options failed
> 
> Do we want to always force the user to use OOP + test vector file for these
> throughput decryption tests?
> Or should we just add a warning that the throughput may not be reflecting the
> "success" verify path in PMD if using inplace and the dummy data.
> 
> I am not sure.
> If we do want to add the limitation on the throughput tests, these changes I think
> are ok for that.

Yes, think about that, in throughput mode, we will not fill the test data time to time, otherwise the testing is useless.
So that means the test data should not be overwritten, otherwise decryption will be with invalid data after the first round of decryption. Since the 1st round decryption overwritten the data to the original buf. In that case, test decryption throughput in non-oop mode is meaningless. 
That's the reason we add that limit to avoid the invalid data issue.

> 
> Thanks,
> Ciara
> 
> > +
> >  	if (options->op_type == CPERF_CIPHER_ONLY ||
> >  			options->op_type == CPERF_CIPHER_THEN_AUTH ||
> >  			options->op_type == CPERF_AUTH_THEN_CIPHER) { diff
> --git
> > a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-
> > perf/cperf_test_throughput.c index e3d266d7a4..b347baa913 100644
> > --- a/app/test-crypto-perf/cperf_test_throughput.c
> > +++ b/app/test-crypto-perf/cperf_test_throughput.c
> > @@ -99,6 +99,26 @@ cperf_throughput_test_constructor(struct
> > rte_mempool *sess_mp,
> >  	return NULL;
> >  }
> >
> > +static void
> > +cperf_verify_init_ops(struct rte_mempool *mp __rte_unused,
> > +		      void *opaque_arg,
> > +		      void *obj,
> > +		      __rte_unused unsigned int i)
> > +{
> > +	uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> > +		sizeof(struct rte_crypto_sym_op);
> > +	uint32_t imix_idx = 0;
> > +	struct cperf_throughput_ctx *ctx = opaque_arg;
> > +	struct rte_crypto_op *op = obj;
> > +
> > +	(ctx->populate_ops)(&op, ctx->src_buf_offset,
> > +			ctx->dst_buf_offset,
> > +			1, ctx->sess, ctx->options,
> > +			ctx->test_vector, iv_offset, &imix_idx, NULL);
> > +
> > +	cperf_mbuf_set(op->sym->m_src, ctx->options, ctx->test_vector); }
> > +
> >  int
> >  cperf_throughput_test_runner(void *test_ctx)  { @@ -144,6 +164,9 @@
> > cperf_throughput_test_runner(void *test_ctx)
> >  	uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> >  		sizeof(struct rte_crypto_sym_op);
> >
> > +	if (ctx->options->out_of_place)
> > +		rte_mempool_obj_iter(ctx->pool, cperf_verify_init_ops, (void
> > *)ctx);
> > +
> >  	while (test_burst_size <= ctx->options->max_burst_size) {
> >  		uint64_t ops_enqd = 0, ops_enqd_total = 0, ops_enqd_failed = 0;
> >  		uint64_t ops_deqd = 0, ops_deqd_total = 0, ops_deqd_failed = 0;
> @@
> > -176,11 +199,12 @@ cperf_throughput_test_runner(void *test_ctx)
> >  			}
> >
> >  			/* Setup crypto op, attach mbuf etc */
> > -			(ctx->populate_ops)(ops, ctx->src_buf_offset,
> > -					ctx->dst_buf_offset,
> > -					ops_needed, ctx->sess,
> > -					ctx->options, ctx->test_vector,
> > -					iv_offset, &imix_idx, &tsc_start);
> > +			if (!ctx->options->out_of_place)
> > +				(ctx->populate_ops)(ops, ctx->src_buf_offset,
> > +						ctx->dst_buf_offset,
> > +						ops_needed, ctx->sess,
> > +						ctx->options, ctx->test_vector,
> > +						iv_offset, &imix_idx,
> > &tsc_start);
> >
> >  			/**
> >  			 * When ops_needed is smaller than ops_enqd, the
> > --
> > 2.34.1
Suanming Mou April 1, 2024, 12:30 a.m. UTC | #3 
Hi guys,

Just want to make sure if anything still need to be checked with that patch?

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Wednesday, March 20, 2024 8:15 AM
> To: Power, Ciara <ciara.power@intel.com>; gakhil@marvell.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
> >
> > Not totally following some of this, why do we only want to add this
> > for OOP mode?
> >
> > For example an inplace command I can use before this patch but not after:
> > ./build/app/dpdk-test-crypto-perf -l 2,3 -- --ptest throughput
> > --optype aead -- aead-algo aes-gcm --aead-op decrypt --devtype
> > crypto_qat --aead-key-sz 16
> >
> > I get an error;
> > USER1: Only out-of-place is allowed in throughput decryption.
> > USER1: Checking one or more user options failed
> >
> > Do we want to always force the user to use OOP + test vector file for
> > these throughput decryption tests?
> > Or should we just add a warning that the throughput may not be
> > reflecting the "success" verify path in PMD if using inplace and the dummy data.
> >
> > I am not sure.
> > If we do want to add the limitation on the throughput tests, these
> > changes I think are ok for that.
> 
> Yes, think about that, in throughput mode, we will not fill the test data time to
> time, otherwise the testing is useless.
> So that means the test data should not be overwritten, otherwise decryption will
> be with invalid data after the first round of decryption. Since the 1st round
> decryption overwritten the data to the original buf. In that case, test decryption
> throughput in non-oop mode is meaningless.
> That's the reason we add that limit to avoid the invalid data issue.
> 
>
diff mbox series

Patch

diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
index d3fd115bc0..714616c697 100644
--- a/app/test-crypto-perf/cperf_ops.c
+++ b/app/test-crypto-perf/cperf_ops.c
@@ -644,7 +644,10 @@  cperf_set_ops_aead(struct rte_crypto_op **ops,
 	}
 
 	if ((options->test == CPERF_TEST_TYPE_VERIFY) ||
-			(options->test == CPERF_TEST_TYPE_LATENCY)) {
+	    (options->test == CPERF_TEST_TYPE_LATENCY) ||
+	    (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
+	     (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
+	      options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) {
 		for (i = 0; i < nb_ops; i++) {
 			uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i],
 					uint8_t *, iv_offset);
diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-perf/cperf_options_parsing.c
index 8c20974273..90526e676f 100644
--- a/app/test-crypto-perf/cperf_options_parsing.c
+++ b/app/test-crypto-perf/cperf_options_parsing.c
@@ -1341,6 +1341,14 @@  cperf_options_check(struct cperf_options *options)
 		}
 	}
 
+	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
+	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
+	     options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) &&
+	    !options->out_of_place) {
+		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in throughput decryption.\n");
+		return -EINVAL;
+	}
+
 	if (options->op_type == CPERF_CIPHER_ONLY ||
 			options->op_type == CPERF_CIPHER_THEN_AUTH ||
 			options->op_type == CPERF_AUTH_THEN_CIPHER) {
diff --git a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-perf/cperf_test_throughput.c
index e3d266d7a4..b347baa913 100644
--- a/app/test-crypto-perf/cperf_test_throughput.c
+++ b/app/test-crypto-perf/cperf_test_throughput.c
@@ -99,6 +99,26 @@  cperf_throughput_test_constructor(struct rte_mempool *sess_mp,
 	return NULL;
 }
 
+static void
+cperf_verify_init_ops(struct rte_mempool *mp __rte_unused,
+		      void *opaque_arg,
+		      void *obj,
+		      __rte_unused unsigned int i)
+{
+	uint16_t iv_offset = sizeof(struct rte_crypto_op) +
+		sizeof(struct rte_crypto_sym_op);
+	uint32_t imix_idx = 0;
+	struct cperf_throughput_ctx *ctx = opaque_arg;
+	struct rte_crypto_op *op = obj;
+
+	(ctx->populate_ops)(&op, ctx->src_buf_offset,
+			ctx->dst_buf_offset,
+			1, ctx->sess, ctx->options,
+			ctx->test_vector, iv_offset, &imix_idx, NULL);
+
+	cperf_mbuf_set(op->sym->m_src, ctx->options, ctx->test_vector);
+}
+
 int
 cperf_throughput_test_runner(void *test_ctx)
 {
@@ -144,6 +164,9 @@  cperf_throughput_test_runner(void *test_ctx)
 	uint16_t iv_offset = sizeof(struct rte_crypto_op) +
 		sizeof(struct rte_crypto_sym_op);
 
+	if (ctx->options->out_of_place)
+		rte_mempool_obj_iter(ctx->pool, cperf_verify_init_ops, (void *)ctx);
+
 	while (test_burst_size <= ctx->options->max_burst_size) {
 		uint64_t ops_enqd = 0, ops_enqd_total = 0, ops_enqd_failed = 0;
 		uint64_t ops_deqd = 0, ops_deqd_total = 0, ops_deqd_failed = 0;
@@ -176,11 +199,12 @@  cperf_throughput_test_runner(void *test_ctx)
 			}
 
 			/* Setup crypto op, attach mbuf etc */
-			(ctx->populate_ops)(ops, ctx->src_buf_offset,
-					ctx->dst_buf_offset,
-					ops_needed, ctx->sess,
-					ctx->options, ctx->test_vector,
-					iv_offset, &imix_idx, &tsc_start);
+			if (!ctx->options->out_of_place)
+				(ctx->populate_ops)(ops, ctx->src_buf_offset,
+						ctx->dst_buf_offset,
+						ops_needed, ctx->sess,
+						ctx->options, ctx->test_vector,
+						iv_offset, &imix_idx, &tsc_start);
 
 			/**
 			 * When ops_needed is smaller than ops_enqd, the