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

["Dooley, Brian" <brian.dooley@intel.com>]

Hi Suanming,

This change looks ok to me.

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, July 18, 2024 8:57 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Dooley, Brian
> <brian.dooley@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; Ji,
> Kai <kai.ji@intel.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] app/test-crypto-perf: add throughput OOP
> decryption
> 
> > Hi Brian,
> >
> > Since Ciara is no longer available and you are the new maintainer, can
> > you investigate this patch?
> > There were some concerns which Ciara highlighted. Can you check?
> >
> 
> Any update on this patch?
> 
> 
> > > > 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]:
> > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > 3A__mails.dpdk.org_archives_dev_2023-
> > >
> >
> 2DJuly_273328.html&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DnL7Si2
> wl_P
> > > RwpZ9TWey3eu68gBzn7DkPwuqhd6WNyo&m=eTj0O7iYH-
> > > xiTQ6dNUZpsOXPqnyC1O_-
> > >
> _IKt0j_yQ_N__vy0wIBLb_QyMQtodUrr&s=eDz_NLjqkUH2cYMilKEtdWImOPj
> 5f-
> > > CVKV5UW8P9frk&e=
> > > >
> > > > 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

Acked-by: Brian Dooley <brian.dooley@intel.com>