From: Akhil Goyal <gakhil@marvell.com>
To: "Power, Ciara" <ciara.power@intel.com>,
Suanming Mou <suanmingm@nvidia.com>,
Brian Dooley <brian.dooley@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v2] app/test-crypto-perf: add throughput OOP decryption
Date: Thu, 20 Jun 2024 06:44:43 +0000 [thread overview]
Message-ID: <CO6PR18MB4484C5B8047EA002AE3EC636D8C82@CO6PR18MB4484.namprd18.prod.outlook.com> (raw)
In-Reply-To: <SN7PR11MB7639BD7F02F1DF9DD4B0309DE62C2@SN7PR11MB7639.namprd11.prod.outlook.com>
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?
Regards,
Akhil
> > 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=DnL7Si2wl_P
> RwpZ9TWey3eu68gBzn7DkPwuqhd6WNyo&m=eTj0O7iYH-
> xiTQ6dNUZpsOXPqnyC1O_-
> _IKt0j_yQ_N__vy0wIBLb_QyMQtodUrr&s=eDz_NLjqkUH2cYMilKEtdWImOPj5f-
> 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
prev parent reply other threads:[~2024-06-20 6:44 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-05 10:01 [PATCH] " 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
2024-06-14 9:38 ` Suanming Mou
2024-06-20 6:44 ` Akhil Goyal [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO6PR18MB4484C5B8047EA002AE3EC636D8C82@CO6PR18MB4484.namprd18.prod.outlook.com \
--to=gakhil@marvell.com \
--cc=brian.dooley@intel.com \
--cc=ciara.power@intel.com \
--cc=dev@dpdk.org \
--cc=suanmingm@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).