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

[Suanming Mou <suanmingm@nvidia.com>]

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