Hi Suanming, This change looks ok to me.
> -----Original Message----- > From: Akhil Goyal <gak...@marvell.com> > Sent: Thursday, July 18, 2024 8:57 PM > To: Suanming Mou <suanmi...@nvidia.com>; Dooley, Brian > <brian.doo...@intel.com>; Mcnamara, John <john.mcnam...@intel.com>; Ji, > Kai <kai...@intel.com>; Kusztal, ArkadiuszX <arkadiuszx.kusz...@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 <suanmi...@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.doo...@intel.com>
