DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] app/test-crypto-perf: add throughput OOP decryption
@ 2024-01-05 10:01 Suanming Mou
  2024-03-14 18:44 ` [EXT] " Akhil Goyal
  2024-03-19 11:46 ` [PATCH v2] " Suanming Mou
  0 siblings, 2 replies; 11+ messages in thread
From: Suanming Mou @ 2024-01-05 10:01 UTC (permalink / raw)
  To: anoobj, ciara.power; +Cc: dev

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 | 37 +++++++++++++++++---
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
index 84945d1313..1d57b78c2b 100644
--- a/app/test-crypto-perf/cperf_ops.c
+++ b/app/test-crypto-perf/cperf_ops.c
@@ -608,7 +608,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 75afedc7fd..6caca44371 100644
--- a/app/test-crypto-perf/cperf_options_parsing.c
+++ b/app/test-crypto-perf/cperf_options_parsing.c
@@ -1291,6 +1291,14 @@ cperf_options_check(struct cperf_options *options)
 		}
 	}
 
+	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
+	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
+	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
+			!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 f8f8bd717f..eab25ec863 100644
--- a/app/test-crypto-perf/cperf_test_throughput.c
+++ b/app/test-crypto-perf/cperf_test_throughput.c
@@ -98,6 +98,29 @@ 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)
 {
@@ -143,6 +166,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;
@@ -175,11 +201,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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
  2024-01-05 10:01 [PATCH] app/test-crypto-perf: add throughput OOP decryption Suanming Mou
@ 2024-03-14 18:44 ` Akhil Goyal
  2024-03-19  1:57   ` Suanming Mou
  2024-03-19 11:46 ` [PATCH v2] " Suanming Mou
  1 sibling, 1 reply; 11+ messages in thread
From: Akhil Goyal @ 2024-03-14 18:44 UTC (permalink / raw)
  To: Suanming Mou, Anoob Joseph, ciara.power; +Cc: dev

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


> 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 | 37 +++++++++++++++++---
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
> index 84945d1313..1d57b78c2b 100644
> --- a/app/test-crypto-perf/cperf_ops.c
> +++ b/app/test-crypto-perf/cperf_ops.c
> @@ -608,7 +608,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 75afedc7fd..6caca44371 100644
> --- a/app/test-crypto-perf/cperf_options_parsing.c
> +++ b/app/test-crypto-perf/cperf_options_parsing.c
> @@ -1291,6 +1291,14 @@ cperf_options_check(struct cperf_options *options)
>  		}
>  	}
> 
> +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> +	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> +			!options->out_of_place) {
> +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> throughput decryption.\n");
> +		return -EINVAL;
> +	}

This check is blocking cipher_only decryption which should pass irrespective of inplace/oop and
Data correct/incorrect.

> +
>  	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 f8f8bd717f..eab25ec863 100644
> --- a/app/test-crypto-perf/cperf_test_throughput.c
> +++ b/app/test-crypto-perf/cperf_test_throughput.c
> @@ -98,6 +98,29 @@ 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);
Unnecessary line break.

> +
Extra line

> +}
> +
>  int
>  cperf_throughput_test_runner(void *test_ctx)
>  {
> @@ -143,6 +166,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;
> @@ -175,11 +201,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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
  2024-03-14 18:44 ` [EXT] " Akhil Goyal
@ 2024-03-19  1:57   ` Suanming Mou
  2024-03-19  8:23     ` Akhil Goyal
  0 siblings, 1 reply; 11+ messages in thread
From: Suanming Mou @ 2024-03-19  1:57 UTC (permalink / raw)
  To: Akhil Goyal, Anoob Joseph, ciara.power; +Cc: dev

Hi Akhil

Sorry for the late reply.

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, March 15, 2024 2:45 AM
> To: Suanming Mou <suanmingm@nvidia.com>; Anoob Joseph
> <anoobj@marvell.com>; ciara.power@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [EXT] [PATCH] 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.
> >
> 
> 
> > 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 | 37 +++++++++++++++++---
> >  3 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test-crypto-perf/cperf_ops.c
> > b/app/test-crypto-perf/cperf_ops.c
> > index 84945d1313..1d57b78c2b 100644
> > --- a/app/test-crypto-perf/cperf_ops.c
> > +++ b/app/test-crypto-perf/cperf_ops.c
> > @@ -608,7 +608,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
> > 75afedc7fd..6caca44371 100644
> > --- a/app/test-crypto-perf/cperf_options_parsing.c
> > +++ b/app/test-crypto-perf/cperf_options_parsing.c
> > @@ -1291,6 +1291,14 @@ cperf_options_check(struct cperf_options *options)
> >  		}
> >  	}
> >
> > +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > +	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> > +			!options->out_of_place) {
> > +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > throughput decryption.\n");
> > +		return -EINVAL;
> > +	}
> 
> This check is blocking cipher_only decryption which should pass irrespective of
> inplace/oop and Data correct/incorrect.

Sorry, in that case I will remove "options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?

> 
> > +
> >  	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 f8f8bd717f..eab25ec863 100644
> > --- a/app/test-crypto-perf/cperf_test_throughput.c
> > +++ b/app/test-crypto-perf/cperf_test_throughput.c
> > @@ -98,6 +98,29 @@ 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);
> Unnecessary line break.
> 
> > +
> Extra line

Will update.

> 
> > +}
> > +
> >  int
> >  cperf_throughput_test_runner(void *test_ctx)  { @@ -143,6 +166,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;
> @@
> > -175,11 +201,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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
  2024-03-19  1:57   ` Suanming Mou
@ 2024-03-19  8:23     ` Akhil Goyal
  2024-03-19  9:06       ` Suanming Mou
  0 siblings, 1 reply; 11+ messages in thread
From: Akhil Goyal @ 2024-03-19  8:23 UTC (permalink / raw)
  To: Suanming Mou, Anoob Joseph, ciara.power; +Cc: dev

> > > +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > +	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> > > +			!options->out_of_place) {
> > > +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > throughput decryption.\n");
> > > +		return -EINVAL;
> > > +	}
> >
> > This check is blocking cipher_only decryption which should pass irrespective of
> > inplace/oop and Data correct/incorrect.
> 
> Sorry, in that case I will remove "options->cipher_op ==
> RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?

I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
Instead of cipher_op.

Ciara, What do you suggest? You were also seeing some issues in this patch.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
  2024-03-19  8:23     ` Akhil Goyal
@ 2024-03-19  9:06       ` Suanming Mou
  2024-03-19  9:32         ` Akhil Goyal
  0 siblings, 1 reply; 11+ messages in thread
From: Suanming Mou @ 2024-03-19  9:06 UTC (permalink / raw)
  To: Akhil Goyal, Anoob Joseph, ciara.power; +Cc: dev



> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, March 19, 2024 4:23 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Anoob Joseph
> <anoobj@marvell.com>; ciara.power@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
> 
> > > > +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > > +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > > +	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> > > > +			!options->out_of_place) {
> > > > +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > > throughput decryption.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > >
> > > This check is blocking cipher_only decryption which should pass
> > > irrespective of inplace/oop and Data correct/incorrect.
> >
> > Sorry, in that case I will remove "options->cipher_op ==
> > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
> 
> I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
> Instead of cipher_op.

I'm not sure. Since in AEAD OP, auth_op will always be RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be rejected.
If the combination here is too complicated, what about just remove that limits and let user to decide? If the input is not correct, PMD will reject it as well.

> 
> Ciara, What do you suggest? You were also seeing some issues in this patch.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
  2024-03-19  9:06       ` Suanming Mou
@ 2024-03-19  9:32         ` Akhil Goyal
  2024-03-19 11:43           ` Suanming Mou
  0 siblings, 1 reply; 11+ messages in thread
From: Akhil Goyal @ 2024-03-19  9:32 UTC (permalink / raw)
  To: Suanming Mou, Anoob Joseph, ciara.power; +Cc: dev

> > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP
> decryption
> >
> > > > > +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > > > +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > > > +	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT)
> &&
> > > > > +			!options->out_of_place) {
> > > > > +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > > > throughput decryption.\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > >
> > > > This check is blocking cipher_only decryption which should pass
> > > > irrespective of inplace/oop and Data correct/incorrect.
> > >
> > > Sorry, in that case I will remove "options->cipher_op ==
> > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
> >
> > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
> > Instead of cipher_op.
> 
> I'm not sure. Since in AEAD OP, auth_op will always be
> RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be
> rejected.
> If the combination here is too complicated, what about just remove that limits and
> let user to decide? If the input is not correct, PMD will reject it as well.

The problematic cases are where auth data (ICV) is not correct.
i.e. AEAD, AUTH_ONLY and CIPHER_AUTH.

Hence following check should be ok.
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) {

Yes PMD will report error if the input data is not correct,
but we cannot just fail in that case just because the app is intentionally not filling the data.
It should report unsupported case. 
> 
> >
> > Ciara, What do you suggest? You were also seeing some issues in this patch.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
  2024-03-19  9:32         ` Akhil Goyal
@ 2024-03-19 11:43           ` Suanming Mou
  0 siblings, 0 replies; 11+ messages in thread
From: Suanming Mou @ 2024-03-19 11:43 UTC (permalink / raw)
  To: Akhil Goyal, Anoob Joseph, ciara.power; +Cc: dev



> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, March 19, 2024 5:32 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Anoob Joseph
> <anoobj@marvell.com>; ciara.power@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption
> 
> > > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP
> > decryption
> > >
> > > > > > +	if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > > > > +	    (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > > > > +	     options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT)
> > &&
> > > > > > +			!options->out_of_place) {
> > > > > > +		RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > > > > throughput decryption.\n");
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > >
> > > > > This check is blocking cipher_only decryption which should pass
> > > > > irrespective of inplace/oop and Data correct/incorrect.
> > > >
> > > > Sorry, in that case I will remove "options->cipher_op ==
> > > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> > > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
> > >
> > > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
> > > Instead of cipher_op.
> >
> > I'm not sure. Since in AEAD OP, auth_op will always be
> > RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be
> > rejected.
> > If the combination here is too complicated, what about just remove
> > that limits and let user to decide? If the input is not correct, PMD will reject it as
> well.
> 
> The problematic cases are where auth data (ICV) is not correct.
> i.e. AEAD, AUTH_ONLY and CIPHER_AUTH.
> 
> Hence following check should be ok.
> 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) {

OK, that make sense. Will update, thanks.

> 
> Yes PMD will report error if the input data is not correct, but we cannot just fail in
> that case just because the app is intentionally not filling the data.
> It should report unsupported case.
> >
> > >
> > > Ciara, What do you suggest? You were also seeing some issues in this patch.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
  2024-01-05 10:01 [PATCH] app/test-crypto-perf: add throughput OOP decryption Suanming Mou
  2024-03-14 18:44 ` [EXT] " Akhil Goyal
@ 2024-03-19 11:46 ` Suanming Mou
  2024-03-19 15:14   ` Power, Ciara
  1 sibling, 1 reply; 11+ messages in thread
From: Suanming Mou @ 2024-03-19 11:46 UTC (permalink / raw)
  To: gakhil, Ciara Power; +Cc: dev

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;
+	}
+
 	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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
  2024-03-19 11:46 ` [PATCH v2] " Suanming Mou
@ 2024-03-19 15:14   ` Power, Ciara
  2024-03-20  0:14     ` Suanming Mou
  0 siblings, 1 reply; 11+ messages in thread
From: Power, Ciara @ 2024-03-19 15:14 UTC (permalink / raw)
  To: Suanming Mou, gakhil; +Cc: dev



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
  2024-03-19 15:14   ` Power, Ciara
@ 2024-03-20  0:14     ` Suanming Mou
  2024-04-01  0:30       ` Suanming Mou
  0 siblings, 1 reply; 11+ messages in thread
From: Suanming Mou @ 2024-03-20  0:14 UTC (permalink / raw)
  To: Power, Ciara, gakhil; +Cc: dev



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
  2024-03-20  0:14     ` Suanming Mou
@ 2024-04-01  0:30       ` Suanming Mou
  0 siblings, 0 replies; 11+ messages in thread
From: Suanming Mou @ 2024-04-01  0:30 UTC (permalink / raw)
  To: Suanming Mou, Power, Ciara, gakhil; +Cc: dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-04-01  0:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-05 10:01 [PATCH] app/test-crypto-perf: add throughput OOP decryption Suanming Mou
2024-03-14 18:44 ` [EXT] " Akhil Goyal
2024-03-19  1:57   ` Suanming Mou
2024-03-19  8:23     ` Akhil Goyal
2024-03-19  9:06       ` Suanming Mou
2024-03-19  9:32         ` Akhil Goyal
2024-03-19 11:43           ` Suanming Mou
2024-03-19 11:46 ` [PATCH v2] " Suanming Mou
2024-03-19 15:14   ` Power, Ciara
2024-03-20  0:14     ` Suanming Mou
2024-04-01  0:30       ` Suanming Mou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).