Hi Jack,
> -----Original Message-----
> From: Jack Bond-Preston <jack.bond-pres...@foss.arm.com>
> Sent: Tuesday, June 4, 2024 3:55 PM
> To: Ciara Power <ciara.po...@intel.com>
> Cc: dev@dpdk.org; Wathsala Vithanage <wathsala.vithan...@arm.com>; Paul
> Szczepanek <paul.szczepa...@arm.com>
> Subject: [PATCH] app/test-crypto-perf: add shared session option
>
> Add the option to create one session for the PMD, and share it across all of
> the
> queue pairs. This may help to discover/debug concurrency issues (both
> correctness and performance) that can occur when using this configuration.
>
> Signed-off-by: Jack Bond-Preston <jack.bond-pres...@foss.arm.com>
> Reviewed-by: Wathsala Vithanage <wathsala.vithan...@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepa...@arm.com>
> ---
> app/test-crypto-perf/cperf.h | 3 ++-
> app/test-crypto-perf/cperf_options.h | 2 ++
> app/test-crypto-perf/cperf_options_parsing.c | 12 ++++++++++
> app/test-crypto-perf/cperf_test_latency.c | 23 +++++++++++++------
> app/test-crypto-perf/cperf_test_latency.h | 3 ++-
> .../cperf_test_pmd_cyclecount.c | 21 ++++++++++++-----
> .../cperf_test_pmd_cyclecount.h | 3 ++-
> app/test-crypto-perf/cperf_test_throughput.c | 21 ++++++++++++-----
> app/test-crypto-perf/cperf_test_throughput.h | 3 ++-
> app/test-crypto-perf/cperf_test_verify.c | 21 ++++++++++++-----
> app/test-crypto-perf/cperf_test_verify.h | 3 ++-
> app/test-crypto-perf/main.c | 23 +++++++++++++++++--
> 12 files changed, 106 insertions(+), 32 deletions(-)
>
> diff --git a/app/test-crypto-perf/cperf.h b/app/test-crypto-perf/cperf.h index
> db58228dce..06df5d88ad 100644
> --- a/app/test-crypto-perf/cperf.h
> +++ b/app/test-crypto-perf/cperf.h
> @@ -19,7 +19,8 @@ typedef void *(*cperf_constructor_t)(
> uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *t_vec,
> - const struct cperf_op_fns *op_fns);
> + const struct cperf_op_fns *op_fns,
> + void **sess);
>
> typedef int (*cperf_runner_t)(void *test_ctx); typedef void
> (*cperf_destructor_t)(void *test_ctx); diff --git a/app/test-crypto-
> perf/cperf_options.h b/app/test-crypto-perf/cperf_options.h
> index be36c70be1..69312a8f7f 100644
> --- a/app/test-crypto-perf/cperf_options.h
> +++ b/app/test-crypto-perf/cperf_options.h
> @@ -27,6 +27,7 @@
> #define CPERF_DEVTYPE ("devtype")
> #define CPERF_OPTYPE ("optype")
> #define CPERF_SESSIONLESS ("sessionless")
> +#define CPERF_SHARED_SESSION ("shared-session")
> #define CPERF_OUT_OF_PLACE ("out-of-place")
> #define CPERF_TEST_FILE ("test-file")
> #define CPERF_TEST_NAME ("test-name")
> @@ -104,6 +105,7 @@ struct cperf_options {
> uint16_t nb_qps;
>
> uint32_t sessionless:1;
> + uint32_t shared_session:1;
> uint32_t out_of_place:1;
> uint32_t silent:1;
> uint32_t csv:1;
> diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-
> perf/cperf_options_parsing.c
> index 8c20974273..55f858fa1b 100644
> --- a/app/test-crypto-perf/cperf_options_parsing.c
> +++ b/app/test-crypto-perf/cperf_options_parsing.c
> @@ -39,6 +39,7 @@ usage(char *progname)
> " --optype cipher-only / auth-only / cipher-then-auth / auth-
> then-cipher /\n"
> " aead / pdcp / docsis / ipsec / modex / tls-record : set
> operation type\n"
> " --sessionless: enable session-less crypto operations\n"
> + " --shared-session: share 1 session across all queue pairs on
> crypto device\n"
> " --out-of-place: enable out-of-place crypto operations\n"
> " --test-file NAME: set the test vector file path\n"
> " --test-name NAME: set specific test name section in test
> file\n"
> @@ -508,6 +509,14 @@ parse_sessionless(struct cperf_options *opts,
> return 0;
> }
>
> +static int
> +parse_shared_session(struct cperf_options *opts,
> + const char *arg __rte_unused)
> +{
> + opts->shared_session = 1;
> + return 0;
> +}
> +
> static int
> parse_out_of_place(struct cperf_options *opts,
> const char *arg __rte_unused)
> @@ -910,6 +919,7 @@ static struct option lgopts[] = {
>
> { CPERF_SILENT, no_argument, 0, 0 },
> { CPERF_SESSIONLESS, no_argument, 0, 0 },
> + { CPERF_SHARED_SESSION, no_argument, 0, 0 },
> { CPERF_OUT_OF_PLACE, no_argument, 0, 0 },
> { CPERF_TEST_FILE, required_argument, 0, 0 },
> { CPERF_TEST_NAME, required_argument, 0, 0 }, @@ -1035,6
> +1045,7 @@ cperf_opts_parse_long(int opt_idx, struct cperf_options *opts)
> { CPERF_DEVTYPE, parse_device_type },
> { CPERF_OPTYPE, parse_op_type },
> { CPERF_SESSIONLESS, parse_sessionless },
> + { CPERF_SHARED_SESSION, parse_shared_session },
> { CPERF_OUT_OF_PLACE, parse_out_of_place },
> { CPERF_IMIX, parse_imix },
> { CPERF_TEST_FILE, parse_test_file },
> @@ -1439,6 +1450,7 @@ cperf_options_dump(struct cperf_options *opts)
> printf("# number of queue pairs per device: %u\n", opts->nb_qps);
> printf("# crypto operation: %s\n", cperf_op_type_strs[opts-
> >op_type]);
> printf("# sessionless: %s\n", opts->sessionless ? "yes" : "no");
> + printf("# shared session: %s\n", opts->shared_session ? "yes" : "no");
> printf("# out of place: %s\n", opts->out_of_place ? "yes" : "no");
> if (opts->test == CPERF_TEST_TYPE_PMDCC)
> printf("# inter-burst delay: %u ms\n", opts->pmdcc_delay);
> diff --git a/app/test-crypto-perf/cperf_test_latency.c b/app/test-crypto-
> perf/cperf_test_latency.c
> index b8ad6bf4d4..bbf33fa03d 100644
> --- a/app/test-crypto-perf/cperf_test_latency.c
> +++ b/app/test-crypto-perf/cperf_test_latency.c
> @@ -1,7 +1,6 @@
> /* SPDX-License-Identifier: BSD-3-Clause
> * Copyright(c) 2016-2017 Intel Corporation
> */
> -
> #include <rte_malloc.h>
> #include <rte_cycles.h>
> #include <rte_crypto.h>
> @@ -25,6 +24,7 @@ struct cperf_latency_ctx {
> struct rte_mempool *pool;
>
> void *sess;
> + uint8_t sess_owner;
>
> cperf_populate_ops_t populate_ops;
>
> @@ -46,7 +46,7 @@ cperf_latency_test_free(struct cperf_latency_ctx *ctx)
> if (ctx == NULL)
> return;
>
> - if (ctx->sess != NULL) {
> + if (ctx->sess != NULL && ctx->sess_owner) {
> if (ctx->options->op_type == CPERF_ASYM_MODEX)
> rte_cryptodev_asym_session_free(ctx->dev_id, ctx-
> >sess); #ifdef RTE_LIB_SECURITY @@ -72,7 +72,8 @@
> cperf_latency_test_constructor(struct rte_mempool *sess_mp,
> uint8_t dev_id, uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *op_fns)
> + const struct cperf_op_fns *op_fns,
> + void **sess)
> {
> struct cperf_latency_ctx *ctx = NULL;
> size_t extra_op_priv_size = sizeof(struct priv_op_data); @@ -93,10
> +94,18 @@ cperf_latency_test_constructor(struct rte_mempool *sess_mp,
> sizeof(struct rte_crypto_sym_op) +
> sizeof(struct cperf_op_result *);
>
> - ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> - test_vector, iv_offset);
> - if (ctx->sess == NULL)
> - goto err;
> +
> + if (*sess != NULL) {
> + ctx->sess = *sess;
> + ctx->sess_owner = false;
> + } else {
> + ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> test_vector,
> + iv_offset);
> + if (ctx->sess == NULL)
> + goto err;
> + *sess = ctx->sess;
> + ctx->sess_owner = true;
> + }
>
> if (cperf_alloc_common_memory(options, test_vector, dev_id, qp_id,
> extra_op_priv_size,
> diff --git a/app/test-crypto-perf/cperf_test_latency.h b/app/test-crypto-
> perf/cperf_test_latency.h
> index d3fc3218d7..0f2b61e21f 100644
> --- a/app/test-crypto-perf/cperf_test_latency.h
> +++ b/app/test-crypto-perf/cperf_test_latency.h
> @@ -21,7 +21,8 @@ cperf_latency_test_constructor(
> uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *ops_fn);
> + const struct cperf_op_fns *ops_fn,
> + void **sess);
>
> int
> cperf_latency_test_runner(void *test_ctx); diff --git a/app/test-crypto-
> perf/cperf_test_pmd_cyclecount.c b/app/test-crypto-
> perf/cperf_test_pmd_cyclecount.c
> index 7191d99ea4..07a842f40a 100644
> --- a/app/test-crypto-perf/cperf_test_pmd_cyclecount.c
> +++ b/app/test-crypto-perf/cperf_test_pmd_cyclecount.c
> @@ -29,6 +29,7 @@ struct cperf_pmd_cyclecount_ctx {
> struct rte_crypto_op **ops_processed;
>
> void *sess;
> + uint8_t sess_owner;
>
> cperf_populate_ops_t populate_ops;
>
> @@ -63,7 +64,7 @@ cperf_pmd_cyclecount_test_free(struct
> cperf_pmd_cyclecount_ctx *ctx)
> if (!ctx)
> return;
>
> - if (ctx->sess) {
> + if (ctx->sess != NULL && ctx->sess_owner) {
> #ifdef RTE_LIB_SECURITY
> if (ctx->options->op_type == CPERF_PDCP ||
> ctx->options->op_type == CPERF_DOCSIS) {
> @@ -89,7 +90,8 @@ cperf_pmd_cyclecount_test_constructor(struct
> rte_mempool *sess_mp,
> uint8_t dev_id, uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *op_fns)
> + const struct cperf_op_fns *op_fns,
> + void **sess)
> {
> struct cperf_pmd_cyclecount_ctx *ctx = NULL;
>
> @@ -112,10 +114,17 @@ cperf_pmd_cyclecount_test_constructor(struct
> rte_mempool *sess_mp,
> uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> sizeof(struct rte_crypto_sym_op);
>
> - ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> - test_vector, iv_offset);
> - if (ctx->sess == NULL)
> - goto err;
> + if (*sess != NULL) {
> + ctx->sess = *sess;
> + ctx->sess_owner = false;
> + } else {
> + ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> test_vector,
> + iv_offset);
> + if (ctx->sess == NULL)
> + goto err;
> + *sess = ctx->sess;
> + ctx->sess_owner = true;
> + }
>
> if (cperf_alloc_common_memory(options, test_vector, dev_id, qp_id,
> 0,
> &ctx->src_buf_offset, &ctx->dst_buf_offset, diff --git
> a/app/test-crypto-perf/cperf_test_pmd_cyclecount.h b/app/test-crypto-
> perf/cperf_test_pmd_cyclecount.h
> index beb4419910..417fdbbed4 100644
> --- a/app/test-crypto-perf/cperf_test_pmd_cyclecount.h
> +++ b/app/test-crypto-perf/cperf_test_pmd_cyclecount.h
> @@ -22,7 +22,8 @@ cperf_pmd_cyclecount_test_constructor(
> uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *ops_fn);
> + const struct cperf_op_fns *ops_fn,
> + void **sess);
>
> int
> cperf_pmd_cyclecount_test_runner(void *test_ctx); diff --git a/app/test-
> crypto-perf/cperf_test_throughput.c b/app/test-crypto-
> perf/cperf_test_throughput.c
> index c0891e7c99..54f2f6a060 100644
> --- a/app/test-crypto-perf/cperf_test_throughput.c
> +++ b/app/test-crypto-perf/cperf_test_throughput.c
> @@ -21,6 +21,7 @@ struct cperf_throughput_ctx {
> struct rte_mempool *pool;
>
> void *sess;
> + uint8_t sess_owner;
>
> cperf_populate_ops_t populate_ops;
>
> @@ -36,7 +37,7 @@ cperf_throughput_test_free(struct
> cperf_throughput_ctx *ctx) {
> if (!ctx)
> return;
> - if (ctx->sess) {
> + if (ctx->sess != NULL && ctx->sess_owner) {
> if (ctx->options->op_type == CPERF_ASYM_MODEX)
> rte_cryptodev_asym_session_free(ctx->dev_id,
> (void *)ctx->sess);
> @@ -63,7 +64,8 @@ cperf_throughput_test_constructor(struct
> rte_mempool *sess_mp,
> uint8_t dev_id, uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *op_fns)
> + const struct cperf_op_fns *op_fns,
> + void **sess)
> {
> struct cperf_throughput_ctx *ctx = NULL;
>
> @@ -82,10 +84,17 @@ cperf_throughput_test_constructor(struct
> rte_mempool *sess_mp,
> uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> sizeof(struct rte_crypto_sym_op);
>
> - ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> - test_vector, iv_offset);
> - if (ctx->sess == NULL)
> - goto err;
> + if (*sess != NULL) {
> + ctx->sess = *sess;
> + ctx->sess_owner = false;
> + } else {
> + ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> test_vector,
> + iv_offset);
> + if (ctx->sess == NULL)
> + goto err;
> + *sess = ctx->sess;
> + ctx->sess_owner = true;
> + }
>
> if (cperf_alloc_common_memory(options, test_vector, dev_id, qp_id,
> 0,
> &ctx->src_buf_offset, &ctx->dst_buf_offset, diff --git
> a/app/test-crypto-perf/cperf_test_throughput.h b/app/test-crypto-
> perf/cperf_test_throughput.h
> index 439ec8e559..d9011b6a51 100644
> --- a/app/test-crypto-perf/cperf_test_throughput.h
> +++ b/app/test-crypto-perf/cperf_test_throughput.h
> @@ -22,7 +22,8 @@ cperf_throughput_test_constructor(
> uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *ops_fn);
> + const struct cperf_op_fns *ops_fn,
> + void **sess);
>
> int
> cperf_throughput_test_runner(void *test_ctx); diff --git a/app/test-crypto-
> perf/cperf_test_verify.c b/app/test-crypto-perf/cperf_test_verify.c
> index 222c7a1cd8..7f6a244794 100644
> --- a/app/test-crypto-perf/cperf_test_verify.c
> +++ b/app/test-crypto-perf/cperf_test_verify.c
> @@ -21,6 +21,7 @@ struct cperf_verify_ctx {
> struct rte_mempool *pool;
>
> void *sess;
> + uint8_t sess_owner;
>
> cperf_populate_ops_t populate_ops;
>
> @@ -41,7 +42,7 @@ cperf_verify_test_free(struct cperf_verify_ctx *ctx)
> if (ctx == NULL)
> return;
>
> - if (ctx->sess != NULL) {
> + if (ctx->sess != NULL && ctx->sess_owner) {
> if (ctx->options->op_type == CPERF_ASYM_MODEX)
> rte_cryptodev_asym_session_free(ctx->dev_id, ctx-
> >sess); #ifdef RTE_LIB_SECURITY @@ -67,7 +68,8 @@
> cperf_verify_test_constructor(struct rte_mempool *sess_mp,
> uint8_t dev_id, uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *op_fns)
> + const struct cperf_op_fns *op_fns,
> + void **sess)
> {
> struct cperf_verify_ctx *ctx = NULL;
>
> @@ -86,10 +88,17 @@ cperf_verify_test_constructor(struct rte_mempool
> *sess_mp,
> uint16_t iv_offset = sizeof(struct rte_crypto_op) +
> sizeof(struct rte_crypto_sym_op);
>
> - ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> - test_vector, iv_offset);
> - if (ctx->sess == NULL)
> - goto err;
> + if (*sess != NULL) {
> + ctx->sess = *sess;
> + ctx->sess_owner = false;
> + } else {
> + ctx->sess = op_fns->sess_create(sess_mp, dev_id, options,
> + test_vector, iv_offset);
> + if (ctx->sess == NULL)
> + goto err;
> + *sess = ctx->sess;
> + ctx->sess_owner = true;
> + }
>
> if (cperf_alloc_common_memory(options, test_vector, dev_id, qp_id,
> 0,
> &ctx->src_buf_offset, &ctx->dst_buf_offset, diff --git
> a/app/test-crypto-perf/cperf_test_verify.h b/app/test-crypto-
> perf/cperf_test_verify.h
> index 9f70ad87ba..dcc10e6e98 100644
> --- a/app/test-crypto-perf/cperf_test_verify.h
> +++ b/app/test-crypto-perf/cperf_test_verify.h
> @@ -22,7 +22,8 @@ cperf_verify_test_constructor(
> uint16_t qp_id,
> const struct cperf_options *options,
> const struct cperf_test_vector *test_vector,
> - const struct cperf_op_fns *ops_fn);
> + const struct cperf_op_fns *ops_fn,
> + void **sess);
>
> int
> cperf_verify_test_runner(void *test_ctx); diff --git a/app/test-crypto-
> perf/main.c b/app/test-crypto-perf/main.c index 40c0b4b54f..312c9dcc96
> 100644
> --- a/app/test-crypto-perf/main.c
> +++ b/app/test-crypto-perf/main.c
> @@ -647,6 +647,9 @@ main(int argc, char **argv)
>
> i = 0;
> uint8_t qp_id = 0, cdev_index = 0;
> +
> + void *sess = NULL;
> +
> RTE_LCORE_FOREACH_WORKER(lcore_id) {
>
> if (i == total_nb_qps)
> @@ -663,14 +666,30 @@ main(int argc, char **argv)
> ctx[i] = cperf_testmap[opts.test].constructor(
> session_pool_socket[socket_id].sess_mp,
> cdev_id, qp_id,
> - &opts, t_vec, &op_fns);
> + &opts, t_vec, &op_fns, &sess);
> +
> + /*
> + * If sess was NULL, the constructor will have set it to a newly
> + * created session. This means future calls to constructors will
> + * provide this session, sharing it across all qps. If session
> + * sharing is not enabled, re-set sess to NULL, to prevent this.
> + */
> + if (!opts.shared_session)
> + sess = NULL;
> +
> if (ctx[i] == NULL) {
> RTE_LOG(ERR, USER1, "Test run constructor
> failed\n");
> goto err;
> }
> +
> qp_id = (qp_id + 1) % opts.nb_qps;
> - if (qp_id == 0)
> + if (qp_id == 0) {
> cdev_index++;
> + /* If next qp is on a new cdev, don't share the session
> + * - it shouldn't be shared across different cdevs.
> + */
> + sess = NULL;
> + }
> i++;
> }
>
> --
> 2.34.1
Would be good to add a doc update too. Thanks.
Acked-by: Brian Dooley <brian.doo...@intel.com>
