* [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
@ 2018-08-24 17:48 Konstantin Ananyev
2018-10-05 11:05 ` Ananyev, Konstantin
2018-11-12 21:01 ` Trahe, Fiona
0 siblings, 2 replies; 10+ messages in thread
From: Konstantin Ananyev @ 2018-08-24 17:48 UTC (permalink / raw)
To: dev
Cc: Konstantin Ananyev, Pablo de Lara, Akhil Goyal, Declan Doherty,
Ravi Kumar, Jerin Jacob, Fan Zhang, Fiona Trahe,
Tomasz Duszynski, Hemant Agrawal, Natalie Samsonov,
Dmitri Epshtein, Jay Zhou
This RFC for proposes several changes inside rte_cryptodev_sym_session.
Note that this is just RFC not a complete patch, so for now
I modified only the librte_cryptodev itself,
some cryptodev PMD, test-crypto-perf and ipsec-secgw example.
Proposed changes means ABI/API breakage inside cryptodev,
so looking for feedback from crypto-dev lib and crypto-PMD maintainiers.
Below are details and reasoning for proposed changes.
1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear()
operate based on cytpodev device id, though inside
rte_cryptodev_sym_session device specific data is addressed
by driver id (not device id).
That creates a problem with current implementation when we have
two or more devices with the same driver used by the same session.
Consider the following example:
struct rte_cryptodev_sym_session *sess;
rte_cryptodev_sym_session_init(dev_id=X, sess, ...);
rte_cryptodev_sym_session_init(dev_id=Y, sess, ...);
rte_cryptodev_sym_session_clear(dev_id=X, sess);
After that point if X and Y uses the same driver,
then sess can't be used by device Y any more.
The reason for that - driver specific (not device specific)
data per session, plus there is no information
how many device instances use that data.
Probably the simplest way to deal with that issue -
add a reference counter per each driver data.
2.rte_cryptodev_sym_session_set_user_data() and
rte_cryptodev_sym_session_get_user_data() -
with current implementation there is no defined way for the user to
determine what is the max allowed size of the private data.
Even within rte_cryptodev_sym_session_set_user_data() we just blindly
copying user provided data without checking memory boundaries violation.
To overcome that issue I added 'uint16_t priv_size' into
rte_cryptodev_sym_session structure.
3.rte_cryptodev_sym_session contains an array of variable size for
driver specific data.
Though number of elements in that array is determined by static
variable nb_drivers, that could be modified by
rte_cryptodev_allocate_driver().
That construction seems to work ok so far, as right now users register
all their PMDs at startup, though it doesn't mean that it would always
remain like that.
To make it less error prone I added 'uint16_t nb_drivers' into the
rte_cryptodev_sym_session structure.
At least that allows related functions to check that provided
driver id wouldn't overrun variable array boundaries,
again it allows to determine size of already allocated session
without accessing global variable.
4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
would have sort of readonly type data (init once at allocation time,
keep unmodified through session life-time).
That requires more changes in current cryptodev implementation:
Right now inside cryptodev framework both rte_cryptodev_sym_session
and driver specific session data are two completely different sctrucures
(e.g. struct struct null_crypto_session and struct null_crypto_session).
Though current cryptodev implementation implicitly assumes that driver
will allocate both of them from within the same mempool.
Plus this is done in a manner that they override each other fields
(reuse the same space - sort of implicit C union).
That's probably not the best programming practice,
plus make impossible to have readonly fields inside both of them.
So to overcome that situation I changed an API a bit, to allow
to use two different mempools for these two distinct data structures.
5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
I suppose that self-explanatory, and might be used in a lot of places
(would be quite useful for ipsec library we develop).
So the new proposed layout for rte_cryptodev_sym_session:
struct rte_cryptodev_sym_session {
uint64_t userdata;
/**< Can be used for external metadata */
uint16_t nb_drivers;
/**< number of elements in sess_data array */
uint16_t priv_size;
/**< session private data will be placed after sess_data */
__extension__ struct {
void *data;
uint16_t refcnt;
} sess_data[0];
/**< Driver specific session material, variable size */
};
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
app/test-crypto-perf/cperf.h | 1 +
app/test-crypto-perf/cperf_ops.c | 11 +-
app/test-crypto-perf/cperf_ops.h | 2 +-
app/test-crypto-perf/cperf_test_latency.c | 5 +-
app/test-crypto-perf/cperf_test_latency.h | 1 +
app/test-crypto-perf/cperf_test_pmd_cyclecount.c | 5 +-
app/test-crypto-perf/cperf_test_pmd_cyclecount.h | 1 +
app/test-crypto-perf/cperf_test_throughput.c | 5 +-
app/test-crypto-perf/cperf_test_throughput.h | 1 +
app/test-crypto-perf/cperf_test_verify.c | 5 +-
app/test-crypto-perf/cperf_test_verify.h | 1 +
app/test-crypto-perf/main.c | 111 +++++++++++------
drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 10 +-
drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c | 5 +-
drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 4 +-
drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 10 +-
drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 5 +-
drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h | 4 +-
drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 3 +-
drivers/crypto/dpaa_sec/dpaa_sec.c | 3 +-
drivers/crypto/null/null_crypto_pmd.c | 14 ++-
drivers/crypto/null/null_crypto_pmd_ops.c | 5 +-
drivers/crypto/null/null_crypto_pmd_private.h | 4 +-
drivers/crypto/scheduler/scheduler_pmd_ops.c | 5 +-
drivers/crypto/virtio/virtio_cryptodev.c | 6 +-
examples/ipsec-secgw/ipsec-secgw.c | 116 ++++++++++++------
examples/ipsec-secgw/ipsec.h | 2 +
lib/librte_cryptodev/rte_cryptodev.c | 134 ++++++++++++---------
lib/librte_cryptodev/rte_cryptodev.h | 53 ++++++--
lib/librte_cryptodev/rte_cryptodev_pmd.h | 16 ++-
30 files changed, 356 insertions(+), 192 deletions(-)
diff --git a/app/test-crypto-perf/cperf.h b/app/test-crypto-perf/cperf.h
index db58228dc..2e0acac62 100644
--- a/app/test-crypto-perf/cperf.h
+++ b/app/test-crypto-perf/cperf.h
@@ -15,6 +15,7 @@ struct cperf_op_fns;
typedef void *(*cperf_constructor_t)(
struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id,
uint16_t qp_id,
const struct cperf_options *options,
diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
index 8f320099d..2a202f90a 100644
--- a/app/test-crypto-perf/cperf_ops.c
+++ b/app/test-crypto-perf/cperf_ops.c
@@ -469,6 +469,7 @@ cperf_set_ops_aead(struct rte_crypto_op **ops,
static struct rte_cryptodev_sym_session *
cperf_create_session(struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id,
const struct cperf_options *options,
const struct cperf_test_vector *test_vector,
@@ -505,7 +506,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
}
/* create crypto session */
rte_cryptodev_sym_session_init(dev_id, sess, &cipher_xform,
- sess_mp);
+ priv_mp);
/*
* auth only
*/
@@ -532,7 +533,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
}
/* create crypto session */
rte_cryptodev_sym_session_init(dev_id, sess, &auth_xform,
- sess_mp);
+ priv_mp);
/*
* cipher and auth
*/
@@ -589,12 +590,12 @@ cperf_create_session(struct rte_mempool *sess_mp,
cipher_xform.next = &auth_xform;
/* create crypto session */
rte_cryptodev_sym_session_init(dev_id,
- sess, &cipher_xform, sess_mp);
+ sess, &cipher_xform, priv_mp);
} else { /* auth then cipher */
auth_xform.next = &cipher_xform;
/* create crypto session */
rte_cryptodev_sym_session_init(dev_id,
- sess, &auth_xform, sess_mp);
+ sess, &auth_xform, priv_mp);
}
} else { /* options->op_type == CPERF_AEAD */
aead_xform.type = RTE_CRYPTO_SYM_XFORM_AEAD;
@@ -615,7 +616,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
/* Create crypto session */
rte_cryptodev_sym_session_init(dev_id,
- sess, &aead_xform, sess_mp);
+ sess, &aead_xform, priv_mp);
}
return sess;
diff --git a/app/test-crypto-perf/cperf_ops.h b/app/test-crypto-perf/cperf_ops.h
index 29e109f2a..80b38d537 100644
--- a/app/test-crypto-perf/cperf_ops.h
+++ b/app/test-crypto-perf/cperf_ops.h
@@ -13,7 +13,7 @@
typedef struct rte_cryptodev_sym_session *(*cperf_sessions_create_t)(
- struct rte_mempool *sess_mp,
+ struct rte_mempool *sess_mp, struct rte_mempool *priv_mp,
uint8_t dev_id, const struct cperf_options *options,
const struct cperf_test_vector *test_vector,
uint16_t iv_offset);
diff --git a/app/test-crypto-perf/cperf_test_latency.c b/app/test-crypto-perf/cperf_test_latency.c
index c9c98dc50..8c8f759eb 100644
--- a/app/test-crypto-perf/cperf_test_latency.c
+++ b/app/test-crypto-perf/cperf_test_latency.c
@@ -62,6 +62,7 @@ cperf_latency_test_free(struct cperf_latency_ctx *ctx)
void *
cperf_latency_test_constructor(struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id, uint16_t qp_id,
const struct cperf_options *options,
const struct cperf_test_vector *test_vector,
@@ -86,8 +87,8 @@ 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);
+ ctx->sess = op_fns->sess_create(sess_mp, priv_mp, dev_id, options,
+ test_vector, iv_offset);
if (ctx->sess == NULL)
goto err;
diff --git a/app/test-crypto-perf/cperf_test_latency.h b/app/test-crypto-perf/cperf_test_latency.h
index d3fc3218d..85c61586f 100644
--- a/app/test-crypto-perf/cperf_test_latency.h
+++ b/app/test-crypto-perf/cperf_test_latency.h
@@ -17,6 +17,7 @@
void *
cperf_latency_test_constructor(
struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id,
uint16_t qp_id,
const struct cperf_options *options,
diff --git a/app/test-crypto-perf/cperf_test_pmd_cyclecount.c b/app/test-crypto-perf/cperf_test_pmd_cyclecount.c
index c8d16db6d..b3a06f810 100644
--- a/app/test-crypto-perf/cperf_test_pmd_cyclecount.c
+++ b/app/test-crypto-perf/cperf_test_pmd_cyclecount.c
@@ -80,6 +80,7 @@ cperf_pmd_cyclecount_test_free(struct cperf_pmd_cyclecount_ctx *ctx)
void *
cperf_pmd_cyclecount_test_constructor(struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id, uint16_t qp_id,
const struct cperf_options *options,
const struct cperf_test_vector *test_vector,
@@ -106,8 +107,8 @@ 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);
+ ctx->sess = op_fns->sess_create(sess_mp, priv_mp, dev_id, options,
+ test_vector, iv_offset);
if (ctx->sess == NULL)
goto err;
diff --git a/app/test-crypto-perf/cperf_test_pmd_cyclecount.h b/app/test-crypto-perf/cperf_test_pmd_cyclecount.h
index beb441991..1b22508dd 100644
--- a/app/test-crypto-perf/cperf_test_pmd_cyclecount.h
+++ b/app/test-crypto-perf/cperf_test_pmd_cyclecount.h
@@ -18,6 +18,7 @@
void *
cperf_pmd_cyclecount_test_constructor(
struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id,
uint16_t qp_id,
const struct cperf_options *options,
diff --git a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-perf/cperf_test_throughput.c
index 8766d6e9b..abd04a332 100644
--- a/app/test-crypto-perf/cperf_test_throughput.c
+++ b/app/test-crypto-perf/cperf_test_throughput.c
@@ -47,6 +47,7 @@ cperf_throughput_test_free(struct cperf_throughput_ctx *ctx)
void *
cperf_throughput_test_constructor(struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id, uint16_t qp_id,
const struct cperf_options *options,
const struct cperf_test_vector *test_vector,
@@ -69,8 +70,8 @@ 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);
+ ctx->sess = op_fns->sess_create(sess_mp, priv_mp, dev_id, options,
+ test_vector, iv_offset);
if (ctx->sess == NULL)
goto err;
diff --git a/app/test-crypto-perf/cperf_test_throughput.h b/app/test-crypto-perf/cperf_test_throughput.h
index 439ec8e55..b7bb2e749 100644
--- a/app/test-crypto-perf/cperf_test_throughput.h
+++ b/app/test-crypto-perf/cperf_test_throughput.h
@@ -18,6 +18,7 @@
void *
cperf_throughput_test_constructor(
struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id,
uint16_t qp_id,
const struct cperf_options *options,
diff --git a/app/test-crypto-perf/cperf_test_verify.c b/app/test-crypto-perf/cperf_test_verify.c
index 9134b921e..e645fc5e8 100644
--- a/app/test-crypto-perf/cperf_test_verify.c
+++ b/app/test-crypto-perf/cperf_test_verify.c
@@ -51,6 +51,7 @@ cperf_verify_test_free(struct cperf_verify_ctx *ctx)
void *
cperf_verify_test_constructor(struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id, uint16_t qp_id,
const struct cperf_options *options,
const struct cperf_test_vector *test_vector,
@@ -73,8 +74,8 @@ 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);
+ ctx->sess = op_fns->sess_create(sess_mp, priv_mp, dev_id, options,
+ test_vector, iv_offset);
if (ctx->sess == NULL)
goto err;
diff --git a/app/test-crypto-perf/cperf_test_verify.h b/app/test-crypto-perf/cperf_test_verify.h
index 9f70ad87b..2484af697 100644
--- a/app/test-crypto-perf/cperf_test_verify.h
+++ b/app/test-crypto-perf/cperf_test_verify.h
@@ -18,6 +18,7 @@
void *
cperf_verify_test_constructor(
struct rte_mempool *sess_mp,
+ struct rte_mempool *priv_mp,
uint8_t dev_id,
uint16_t qp_id,
const struct cperf_options *options,
diff --git a/app/test-crypto-perf/main.c b/app/test-crypto-perf/main.c
index 5c7dadb60..42a34c74f 100644
--- a/app/test-crypto-perf/main.c
+++ b/app/test-crypto-perf/main.c
@@ -22,6 +22,11 @@
#include "cperf_test_pmd_cyclecount.h"
+static struct {
+ struct rte_mempool *sess_mp;
+ struct rte_mempool *priv_mp;
+} session_pool_socket[RTE_MAX_NUMA_NODES];
+
const char *cperf_test_type_strs[] = {
[CPERF_TEST_TYPE_THROUGHPUT] = "throughput",
[CPERF_TEST_TYPE_LATENCY] = "latency",
@@ -61,8 +66,59 @@ const struct cperf_test cperf_testmap[] = {
};
static int
-cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
- struct rte_mempool *session_pool_socket[])
+fill_session_pool_socket(int32_t socket_id, uint32_t max_sess_size,
+ uint32_t nb_sessions)
+{
+ char mp_name[RTE_MEMPOOL_NAMESIZE];
+ struct rte_mempool *sess_mp;
+
+ if (session_pool_socket[socket_id].priv_mp == NULL) {
+
+ snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
+ "priv_sess_mp_%u", socket_id);
+
+ sess_mp = rte_mempool_create(mp_name,
+ nb_sessions,
+ max_sess_size,
+ 0, 0, NULL, NULL, NULL,
+ NULL, socket_id,
+ 0);
+
+ if (sess_mp == NULL) {
+ printf("Cannot create pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ return -ENOMEM;
+ }
+
+ printf("Allocated pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ session_pool_socket[socket_id].priv_mp = sess_mp;
+ }
+
+ if (session_pool_socket[socket_id].sess_mp == NULL) {
+
+ snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
+ "sess_mp_%u", socket_id);
+
+ sess_mp = rte_cryptodev_sym_session_pool_create(mp_name,
+ nb_sessions, 0, 0, socket_id);
+
+ if (sess_mp == NULL) {
+ printf("Cannot create pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ return -ENOMEM;
+ }
+
+ printf("Allocated pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ session_pool_socket[socket_id].sess_mp = sess_mp;
+ }
+
+ return 0;
+}
+
+static int
+cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs)
{
uint8_t enabled_cdev_count = 0, nb_lcores, cdev_id;
uint32_t sessions_needed = 0;
@@ -96,7 +152,7 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
uint32_t max_sess_size = 0, sess_size;
for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
- sess_size = rte_cryptodev_sym_get_private_session_size(cdev_id);
+ sess_size = rte_cryptodev_sym_private_session_size(cdev_id);
if (sess_size > max_sess_size)
max_sess_size = sess_size;
}
@@ -144,10 +200,6 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
.socket_id = socket_id
};
- struct rte_cryptodev_qp_conf qp_conf = {
- .nb_descriptors = opts->nb_descriptors
- };
-
/**
* Device info specifies the min headroom and tailroom
* requirement for the crypto PMD. This need to be honoured
@@ -194,29 +246,19 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
"%u sessions\n", opts->nb_qps);
return -ENOTSUP;
}
- if (session_pool_socket[socket_id] == NULL) {
- char mp_name[RTE_MEMPOOL_NAMESIZE];
- struct rte_mempool *sess_mp;
-
- snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
- "sess_mp_%u", socket_id);
- sess_mp = rte_mempool_create(mp_name,
- sessions_needed,
- max_sess_size,
- 0,
- 0, NULL, NULL, NULL,
- NULL, socket_id,
- 0);
-
- if (sess_mp == NULL) {
- printf("Cannot create session pool on socket %d\n",
- socket_id);
- return -ENOMEM;
- }
- printf("Allocated session pool on socket %d\n", socket_id);
- session_pool_socket[socket_id] = sess_mp;
- }
+ ret = fill_session_pool_socket(socket_id, max_sess_size,
+ sessions_needed);
+ if (ret < 0)
+ return ret;
+
+ struct rte_cryptodev_qp_conf qp_conf = {
+ .nb_descriptors = opts->nb_descriptors,
+ .sess_pool = session_pool_socket[socket_id].sess_mp,
+ .priv_sess_pool =
+ session_pool_socket[socket_id].priv_mp,
+ };
+
ret = rte_cryptodev_configure(cdev_id, &conf);
if (ret < 0) {
@@ -226,8 +268,7 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
for (j = 0; j < opts->nb_qps; j++) {
ret = rte_cryptodev_queue_pair_setup(cdev_id, j,
- &qp_conf, socket_id,
- session_pool_socket[socket_id]);
+ &qp_conf, socket_id);
if (ret < 0) {
printf("Failed to setup queue pair %u on "
"cryptodev %u", j, cdev_id);
@@ -445,7 +486,6 @@ main(int argc, char **argv)
struct cperf_op_fns op_fns;
void *ctx[RTE_MAX_LCORE] = { };
- struct rte_mempool *session_pool_socket[RTE_MAX_NUMA_NODES] = { 0 };
int nb_cryptodevs = 0;
uint16_t total_nb_qps = 0;
@@ -479,8 +519,7 @@ main(int argc, char **argv)
goto err;
}
- nb_cryptodevs = cperf_initialize_cryptodev(&opts, enabled_cdevs,
- session_pool_socket);
+ nb_cryptodevs = cperf_initialize_cryptodev(&opts, enabled_cdevs);
if (!opts.silent)
cperf_options_dump(&opts);
@@ -548,7 +587,9 @@ main(int argc, char **argv)
uint8_t socket_id = rte_cryptodev_socket_id(cdev_id);
ctx[i] = cperf_testmap[opts.test].constructor(
- session_pool_socket[socket_id], cdev_id, qp_id,
+ session_pool_socket[socket_id].sess_mp,
+ session_pool_socket[socket_id].priv_mp,
+ cdev_id, qp_id,
&opts, t_vec, &op_fns);
if (ctx[i] == NULL) {
RTE_LOG(ERR, USER1, "Test run constructor failed\n");
diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
index 752e0cd6a..c53705601 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
@@ -137,7 +137,7 @@ aesni_gcm_get_session(struct aesni_gcm_qp *qp, struct rte_crypto_op *op)
if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
return NULL;
- if (rte_mempool_get(qp->sess_mp, (void **)&_sess_private_data))
+ if (rte_mempool_get(qp->priv_mp, (void **)&_sess_private_data))
return NULL;
sess = (struct aesni_gcm_session *)_sess_private_data;
@@ -145,7 +145,7 @@ aesni_gcm_get_session(struct aesni_gcm_qp *qp, struct rte_crypto_op *op)
if (unlikely(aesni_gcm_set_session_parameters(qp->ops,
sess, sym_op->xform) != 0)) {
rte_mempool_put(qp->sess_mp, _sess);
- rte_mempool_put(qp->sess_mp, _sess_private_data);
+ rte_mempool_put(qp->priv_mp, _sess_private_data);
sess = NULL;
}
sym_op->session = (struct rte_cryptodev_sym_session *)_sess;
@@ -391,9 +391,9 @@ handle_completed_gcm_crypto_op(struct aesni_gcm_qp *qp,
/* Free session if a session-less crypto op */
if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
memset(sess, 0, sizeof(struct aesni_gcm_session));
- memset(op->sym->session, 0,
- rte_cryptodev_sym_get_header_session_size());
- rte_mempool_put(qp->sess_mp, sess);
+ set_sym_session_private_data(op->sym->session,
+ cryptodev_driver_id, NULL);
+ rte_mempool_put(qp->priv_mp, sess);
rte_mempool_put(qp->sess_mp, op->sym->session);
op->sym->session = NULL;
}
diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c b/drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c
index b6b4dd028..b02e43387 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c
@@ -201,7 +201,7 @@ aesni_gcm_pmd_qp_create_processed_pkts_ring(struct aesni_gcm_qp *qp,
static int
aesni_gcm_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
const struct rte_cryptodev_qp_conf *qp_conf,
- int socket_id, struct rte_mempool *session_pool)
+ int socket_id)
{
struct aesni_gcm_qp *qp = NULL;
struct aesni_gcm_private *internals = dev->data->dev_private;
@@ -229,7 +229,8 @@ aesni_gcm_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
if (qp->processed_pkts == NULL)
goto qp_setup_cleanup;
- qp->sess_mp = session_pool;
+ qp->sess_mp = qp_conf->sess_pool;
+ qp->priv_mp = qp_conf->priv_sess_pool;
memset(&qp->qp_stats, 0, sizeof(qp->qp_stats));
diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h b/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h
index c13a12a57..bd42b3935 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h
@@ -47,7 +47,9 @@ struct aesni_gcm_qp {
struct rte_cryptodev_stats qp_stats; /* 8 * 4 = 32 B */
/**< Queue pair statistics */
struct rte_mempool *sess_mp;
- /**< Session Mempool */
+ /**< crypto session Mempool */
+ struct rte_mempool *priv_mp;
+ /**< private session Mempool */
uint16_t id;
/**< Queue Pair Identifier */
char name[RTE_CRYPTODEV_NAME_MAX_LEN];
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
index 93dc7a443..21e9fade7 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
@@ -516,7 +516,7 @@ get_session(struct aesni_mb_qp *qp, struct rte_crypto_op *op)
if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
return NULL;
- if (rte_mempool_get(qp->sess_mp, (void **)&_sess_private_data))
+ if (rte_mempool_get(qp->priv_mp, (void **)&_sess_private_data))
return NULL;
sess = (struct aesni_mb_session *)_sess_private_data;
@@ -524,7 +524,7 @@ get_session(struct aesni_mb_qp *qp, struct rte_crypto_op *op)
if (unlikely(aesni_mb_set_session_parameters(qp->op_fns,
sess, op->sym->xform) != 0)) {
rte_mempool_put(qp->sess_mp, _sess);
- rte_mempool_put(qp->sess_mp, _sess_private_data);
+ rte_mempool_put(qp->priv_mp, _sess_private_data);
sess = NULL;
}
op->sym->session = (struct rte_cryptodev_sym_session *)_sess;
@@ -741,9 +741,9 @@ post_process_mb_job(struct aesni_mb_qp *qp, JOB_AES_HMAC *job)
/* Free session if a session-less crypto op */
if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
memset(sess, 0, sizeof(struct aesni_mb_session));
- memset(op->sym->session, 0,
- rte_cryptodev_sym_get_header_session_size());
- rte_mempool_put(qp->sess_mp, sess);
+ set_sym_session_private_data(op->sym->session,
+ cryptodev_driver_id, NULL);
+ rte_mempool_put(qp->priv_mp, sess);
rte_mempool_put(qp->sess_mp, op->sym->session);
op->sym->session = NULL;
}
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
index ab26e5ae4..63cf82547 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
@@ -482,7 +482,7 @@ aesni_mb_pmd_qp_create_processed_ops_ring(struct aesni_mb_qp *qp,
static int
aesni_mb_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
const struct rte_cryptodev_qp_conf *qp_conf,
- int socket_id, struct rte_mempool *session_pool)
+ int socket_id)
{
struct aesni_mb_qp *qp = NULL;
struct aesni_mb_private *internals = dev->data->dev_private;
@@ -520,7 +520,8 @@ aesni_mb_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
goto qp_setup_cleanup;
}
- qp->sess_mp = session_pool;
+ qp->sess_mp = qp_conf->sess_pool;
+ qp->priv_mp = qp_conf->priv_sess_pool;
memset(&qp->stats, 0, sizeof(qp->stats));
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h
index 70e9d18e5..bca326851 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h
@@ -137,7 +137,9 @@ struct aesni_mb_qp {
struct rte_ring *ingress_queue;
/**< Ring for placing operations ready for processing */
struct rte_mempool *sess_mp;
- /**< Session Mempool */
+ /**< crypto session Mempool */
+ struct rte_mempool *priv_mp;
+ /**< private Session Mempool */
struct rte_cryptodev_stats stats;
/**< Queue pair statistics */
uint8_t digest_idx;
diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index 2a3c61c66..19935ee8e 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -1416,8 +1416,7 @@ dpaa2_sec_queue_pair_release(struct rte_cryptodev *dev, uint16_t queue_pair_id)
static int
dpaa2_sec_queue_pair_setup(struct rte_cryptodev *dev, uint16_t qp_id,
__rte_unused const struct rte_cryptodev_qp_conf *qp_conf,
- __rte_unused int socket_id,
- __rte_unused struct rte_mempool *session_pool)
+ __rte_unused int socket_id)
{
struct dpaa2_sec_dev_private *priv = dev->data->dev_private;
struct dpaa2_sec_qp *qp;
diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index f571050b7..5583d939d 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -1576,8 +1576,7 @@ dpaa_sec_queue_pair_release(struct rte_cryptodev *dev,
static int
dpaa_sec_queue_pair_setup(struct rte_cryptodev *dev, uint16_t qp_id,
__rte_unused const struct rte_cryptodev_qp_conf *qp_conf,
- __rte_unused int socket_id,
- __rte_unused struct rte_mempool *session_pool)
+ __rte_unused int socket_id)
{
struct dpaa_sec_dev_private *internals;
struct dpaa_sec_qp *qp = NULL;
diff --git a/drivers/crypto/null/null_crypto_pmd.c b/drivers/crypto/null/null_crypto_pmd.c
index 6e29a21a6..10096d450 100644
--- a/drivers/crypto/null/null_crypto_pmd.c
+++ b/drivers/crypto/null/null_crypto_pmd.c
@@ -49,16 +49,18 @@ null_crypto_set_session_parameters(
/** Process crypto operation for mbuf */
static int
process_op(const struct null_crypto_qp *qp, struct rte_crypto_op *op,
- struct null_crypto_session *sess __rte_unused)
+ struct null_crypto_session *sess)
{
/* set status as successful by default */
op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
/* Free session if a session-less crypto op. */
if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
- memset(op->sym->session, 0,
- sizeof(struct null_crypto_session));
- rte_cryptodev_sym_session_free(op->sym->session);
+ memset(sess, 0, sizeof(*sess));
+ set_sym_session_private_data(op->sym->session,
+ cryptodev_driver_id, NULL);
+ rte_mempool_put(qp->priv_mp, sess);
+ rte_mempool_put(qp->sess_mp, op->sym->session);
op->sym->session = NULL;
}
@@ -87,7 +89,7 @@ get_session(struct null_crypto_qp *qp, struct rte_crypto_op *op)
if (rte_mempool_get(qp->sess_mp, (void **)&_sess))
return NULL;
- if (rte_mempool_get(qp->sess_mp, (void **)&_sess_private_data))
+ if (rte_mempool_get(qp->priv_mp, (void **)&_sess_private_data))
return NULL;
sess = (struct null_crypto_session *)_sess_private_data;
@@ -95,7 +97,7 @@ get_session(struct null_crypto_qp *qp, struct rte_crypto_op *op)
if (unlikely(null_crypto_set_session_parameters(sess,
sym_op->xform) != 0)) {
rte_mempool_put(qp->sess_mp, _sess);
- rte_mempool_put(qp->sess_mp, _sess_private_data);
+ rte_mempool_put(qp->priv_mp, _sess_private_data);
sess = NULL;
}
sym_op->session = (struct rte_cryptodev_sym_session *)_sess;
diff --git a/drivers/crypto/null/null_crypto_pmd_ops.c b/drivers/crypto/null/null_crypto_pmd_ops.c
index bb2b6e144..d25be42a6 100644
--- a/drivers/crypto/null/null_crypto_pmd_ops.c
+++ b/drivers/crypto/null/null_crypto_pmd_ops.c
@@ -184,7 +184,7 @@ null_crypto_pmd_qp_create_processed_pkts_ring(struct null_crypto_qp *qp,
static int
null_crypto_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
const struct rte_cryptodev_qp_conf *qp_conf,
- int socket_id, struct rte_mempool *session_pool)
+ int socket_id)
{
struct null_crypto_private *internals = dev->data->dev_private;
struct null_crypto_qp *qp;
@@ -228,7 +228,8 @@ null_crypto_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
goto qp_setup_cleanup;
}
- qp->sess_mp = session_pool;
+ qp->sess_mp = qp_conf->sess_pool;
+ qp->priv_mp = qp_conf->priv_sess_pool;
memset(&qp->qp_stats, 0, sizeof(qp->qp_stats));
diff --git a/drivers/crypto/null/null_crypto_pmd_private.h b/drivers/crypto/null/null_crypto_pmd_private.h
index d5905afd8..1853e2153 100644
--- a/drivers/crypto/null/null_crypto_pmd_private.h
+++ b/drivers/crypto/null/null_crypto_pmd_private.h
@@ -30,7 +30,9 @@ struct null_crypto_qp {
struct rte_ring *processed_pkts;
/**< Ring for placing process packets */
struct rte_mempool *sess_mp;
- /**< Session Mempool */
+ /**< crypto session Mempool */
+ struct rte_mempool *priv_mp;
+ /**< private session Mempool */
struct rte_cryptodev_stats qp_stats;
/**< Queue pair statistics */
} __rte_cache_aligned;
diff --git a/drivers/crypto/scheduler/scheduler_pmd_ops.c b/drivers/crypto/scheduler/scheduler_pmd_ops.c
index 778071ca0..e6c431687 100644
--- a/drivers/crypto/scheduler/scheduler_pmd_ops.c
+++ b/drivers/crypto/scheduler/scheduler_pmd_ops.c
@@ -390,8 +390,7 @@ scheduler_pmd_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
/** Setup a queue pair */
static int
scheduler_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
- const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
- struct rte_mempool *session_pool)
+ const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
{
struct scheduler_ctx *sched_ctx = dev->data->dev_private;
struct scheduler_qp_ctx *qp_ctx;
@@ -419,7 +418,7 @@ scheduler_pmd_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
* must be big enough for all the drivers used.
*/
ret = rte_cryptodev_queue_pair_setup(slave_id, qp_id,
- qp_conf, socket_id, session_pool);
+ qp_conf, socket_id);
if (ret < 0)
return ret;
}
diff --git a/drivers/crypto/virtio/virtio_cryptodev.c b/drivers/crypto/virtio/virtio_cryptodev.c
index 568b5a406..4bae3b865 100644
--- a/drivers/crypto/virtio/virtio_cryptodev.c
+++ b/drivers/crypto/virtio/virtio_cryptodev.c
@@ -36,8 +36,7 @@ static void virtio_crypto_dev_stats_reset(struct rte_cryptodev *dev);
static int virtio_crypto_qp_setup(struct rte_cryptodev *dev,
uint16_t queue_pair_id,
const struct rte_cryptodev_qp_conf *qp_conf,
- int socket_id,
- struct rte_mempool *session_pool);
+ int socket_id);
static int virtio_crypto_qp_release(struct rte_cryptodev *dev,
uint16_t queue_pair_id);
static void virtio_crypto_dev_free_mbufs(struct rte_cryptodev *dev);
@@ -585,8 +584,7 @@ virtio_crypto_dev_stats_reset(struct rte_cryptodev *dev)
static int
virtio_crypto_qp_setup(struct rte_cryptodev *dev, uint16_t queue_pair_id,
const struct rte_cryptodev_qp_conf *qp_conf,
- int socket_id,
- struct rte_mempool *session_pool __rte_unused)
+ int socket_id)
{
int ret;
struct virtqueue *vq;
diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
index b45b87bde..cce0789bf 100644
--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c
@@ -821,11 +821,15 @@ main_loop(__attribute__((unused)) void *dummy)
qconf->inbound.sa_ctx = socket_ctx[socket_id].sa_in;
qconf->inbound.cdev_map = cdev_map_in;
qconf->inbound.session_pool = socket_ctx[socket_id].session_pool;
+ qconf->inbound.priv_session_pool =
+ socket_ctx[socket_id].priv_session_pool;
qconf->outbound.sp4_ctx = socket_ctx[socket_id].sp_ip4_out;
qconf->outbound.sp6_ctx = socket_ctx[socket_id].sp_ip6_out;
qconf->outbound.sa_ctx = socket_ctx[socket_id].sa_out;
qconf->outbound.cdev_map = cdev_map_out;
qconf->outbound.session_pool = socket_ctx[socket_id].session_pool;
+ qconf->outbound.priv_session_pool =
+ socket_ctx[socket_id].priv_session_pool;
if (qconf->nb_rx_queue == 0) {
RTE_LOG(INFO, IPSEC, "lcore %u has nothing to do\n", lcore_id);
@@ -972,20 +976,19 @@ print_usage(const char *prgname)
}
static int32_t
-parse_portmask(const char *portmask)
+parse_portmask(const char *portmask, uint32_t *pmv)
{
- char *end = NULL;
+ char *end;
unsigned long pm;
/* parse hexadecimal string */
+ errno = 0;
pm = strtoul(portmask, &end, 16);
- if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0'))
+ if (errno != 0 || *end != '\0' || pm > UINT32_MAX)
return -1;
- if ((pm == 0) && errno)
- return -1;
-
- return pm;
+ *pmv = pm;
+ return 0;
}
static int32_t
@@ -1063,6 +1066,7 @@ parse_args(int32_t argc, char **argv)
int32_t opt, ret;
char **argvopt;
int32_t option_index;
+ uint32_t v;
char *prgname = argv[0];
int32_t f_present = 0;
@@ -1073,8 +1077,8 @@ parse_args(int32_t argc, char **argv)
switch (opt) {
case 'p':
- enabled_port_mask = parse_portmask(optarg);
- if (enabled_port_mask == 0) {
+ ret = parse_portmask(optarg, &enabled_port_mask);
+ if (ret < 0 || enabled_port_mask == 0) {
printf("invalid portmask\n");
print_usage(prgname);
return -1;
@@ -1085,8 +1089,8 @@ parse_args(int32_t argc, char **argv)
promiscuous_on = 1;
break;
case 'u':
- unprotected_port_mask = parse_portmask(optarg);
- if (unprotected_port_mask == 0) {
+ ret = parse_portmask(optarg, &unprotected_port_mask);
+ if (ret < 0) {
printf("invalid unprotected portmask\n");
print_usage(prgname);
return -1;
@@ -1147,15 +1151,16 @@ parse_args(int32_t argc, char **argv)
single_sa_idx);
break;
case CMD_LINE_OPT_CRYPTODEV_MASK_NUM:
- ret = parse_portmask(optarg);
+ ret = parse_portmask(optarg, &v);
if (ret == -1) {
- printf("Invalid argument[portmask]\n");
+ printf("Invalid argument[%s]\n",
+ CMD_LINE_OPT_CRYPTODEV_MASK);
print_usage(prgname);
return -1;
}
/* else */
- enabled_cryptodev_mask = ret;
+ enabled_cryptodev_mask = v;
break;
default:
print_usage(prgname);
@@ -1360,6 +1365,54 @@ check_cryptodev_mask(uint8_t cdev_id)
return -1;
}
+static int
+fill_session_pool(int32_t socket_id, uint32_t max_sess_size)
+{
+ char mp_name[RTE_MEMPOOL_NAMESIZE];
+ struct rte_mempool *sess_mp;
+
+ if (!socket_ctx[socket_id].priv_session_pool) {
+
+ snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
+ "priv_sess_mp_%u", socket_id);
+ sess_mp = rte_mempool_create(mp_name,
+ CDEV_MP_NB_OBJS,
+ max_sess_size,
+ CDEV_MP_CACHE_SZ,
+ 0, NULL, NULL, NULL,
+ NULL, socket_id,
+ 0);
+ if (sess_mp == NULL) {
+ printf("Cannot create pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ return -ENOMEM;
+ }
+
+ printf("Allocated pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ socket_ctx[socket_id].priv_session_pool = sess_mp;
+ }
+
+ if (!socket_ctx[socket_id].session_pool) {
+
+ snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
+ "sess_mp_%u", socket_id);
+
+ sess_mp = rte_cryptodev_sym_session_pool_create(mp_name,
+ CDEV_MP_NB_OBJS, CDEV_MP_CACHE_SZ, 0, socket_id);
+ if (sess_mp == NULL) {
+ printf("Cannot create pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ return -ENOMEM;
+ }
+
+ printf("Allocated pool \"%s\" on socket %d\n",
+ mp_name, socket_id);
+ socket_ctx[socket_id].session_pool = sess_mp;
+ }
+
+ return 0;
+}
static int32_t
cryptodevs_init(void)
@@ -1392,7 +1445,7 @@ cryptodevs_init(void)
uint32_t max_sess_sz = 0, sess_sz;
for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
- sess_sz = rte_cryptodev_sym_get_private_session_size(cdev_id);
+ sess_sz = rte_cryptodev_sym_private_session_size(cdev_id);
if (sess_sz > max_sess_sz)
max_sess_sz = sess_sz;
}
@@ -1448,38 +1501,23 @@ cryptodevs_init(void)
"Device does not support at least %u "
"sessions", CDEV_MP_NB_OBJS / 2);
- if (!socket_ctx[dev_conf.socket_id].session_pool) {
- char mp_name[RTE_MEMPOOL_NAMESIZE];
- struct rte_mempool *sess_mp;
-
- snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
- "sess_mp_%u", dev_conf.socket_id);
- sess_mp = rte_mempool_create(mp_name,
- CDEV_MP_NB_OBJS,
- max_sess_sz,
- CDEV_MP_CACHE_SZ,
- 0, NULL, NULL, NULL,
- NULL, dev_conf.socket_id,
- 0);
- if (sess_mp == NULL)
- rte_exit(EXIT_FAILURE,
- "Cannot create session pool on socket %d\n",
- dev_conf.socket_id);
- else
- printf("Allocated session pool on socket %d\n",
- dev_conf.socket_id);
- socket_ctx[dev_conf.socket_id].session_pool = sess_mp;
- }
+ if (fill_session_pool(dev_conf.socket_id, max_sess_sz) != 0)
+ rte_exit(EXIT_FAILURE,
+ "Cannot create session pools on socket %d\n",
+ dev_conf.socket_id);
if (rte_cryptodev_configure(cdev_id, &dev_conf))
rte_panic("Failed to initialize cryptodev %u\n",
cdev_id);
qp_conf.nb_descriptors = CDEV_QUEUE_DESC;
+ qp_conf.sess_pool = socket_ctx[dev_conf.socket_id].session_pool;
+ qp_conf.priv_sess_pool =
+ socket_ctx[dev_conf.socket_id].priv_session_pool;
+
for (qp = 0; qp < dev_conf.nb_queue_pairs; qp++)
if (rte_cryptodev_queue_pair_setup(cdev_id, qp,
- &qp_conf, dev_conf.socket_id,
- socket_ctx[dev_conf.socket_id].session_pool))
+ &qp_conf, dev_conf.socket_id))
rte_panic("Failed to setup queue %u for "
"cdev_id %u\n", 0, cdev_id);
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index c998c8076..8ddc5d4f6 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -144,6 +144,7 @@ struct ipsec_ctx {
uint16_t last_qp;
struct cdev_qp tbl[MAX_QP_PER_LCORE];
struct rte_mempool *session_pool;
+ struct rte_mempool *priv_session_pool;
struct rte_mbuf *ol_pkts[MAX_PKT_BURST] __rte_aligned(sizeof(void *));
uint16_t ol_pkts_cnt;
};
@@ -166,6 +167,7 @@ struct socket_ctx {
struct rt_ctx *rt_ip6;
struct rte_mempool *mbuf_pool;
struct rte_mempool *session_pool;
+ struct rte_mempool *priv_session_pool;
};
struct cnt_blk {
diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 63ae23f00..e25282445 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -943,8 +943,7 @@ rte_cryptodev_close(uint8_t dev_id)
int
rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
- const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
- struct rte_mempool *session_pool)
+ const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
{
struct rte_cryptodev *dev;
@@ -954,6 +953,12 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
return -EINVAL;
}
+ if (qp_conf == NULL || qp_conf->sess_pool == NULL ||
+ qp_conf->priv_sess_pool == NULL) {
+ CDEV_LOG_ERR("Invalid queue_pair config");
+ return -EINVAL;
+ }
+
dev = &rte_crypto_devices[dev_id];
if (queue_pair_id >= dev->data->nb_queue_pairs) {
CDEV_LOG_ERR("Invalid queue_pair_id=%d", queue_pair_id);
@@ -969,7 +974,7 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_pair_setup, -ENOTSUP);
return (*dev->dev_ops->queue_pair_setup)(dev, queue_pair_id, qp_conf,
- socket_id, session_pool);
+ socket_id);
}
@@ -1146,6 +1151,41 @@ rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
rte_spinlock_unlock(&rte_cryptodev_cb_lock);
}
+static void
+cryptodev_sym_session_init_elem(__rte_unused struct rte_mempool *pool,
+ void *arg, void *obj, __rte_unused uint32_t idx)
+{
+ struct rte_cryptodev_sym_session *ds;
+ const struct rte_cryptodev_sym_session *ss;
+
+ ds = obj;
+ ss = arg;
+
+ *ds = *ss;
+ memset(ds->sess_data, 0, rte_cryptodev_sym_session_data_size(ds));
+}
+
+struct rte_mempool *
+rte_cryptodev_sym_session_pool_create(const char *name,
+ uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
+ int socket_id)
+{
+ struct rte_mempool *mp;
+ uint32_t elt_size;
+ struct rte_cryptodev_sym_session s = {
+ .nb_drivers = nb_drivers,
+ .priv_size = priv_size,
+ };
+
+ elt_size = rte_cryptodev_sym_session_max_size(priv_size);
+ mp = rte_mempool_create(name, nb_elts, elt_size, cache_size, 0,
+ NULL, NULL, cryptodev_sym_session_init_elem, &s,
+ socket_id, 0);
+ if (mp == NULL)
+ CDEV_LOG_ERR("%s(name=%s) failed, rte_errno=%d\n",
+ __func__, name, rte_errno);
+ return mp;
+}
int
rte_cryptodev_sym_session_init(uint8_t dev_id,
@@ -1163,12 +1203,15 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
return -EINVAL;
index = dev->driver_id;
+ if (index > sess->nb_drivers)
+ return -EINVAL;
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_configure, -ENOTSUP);
- if (sess->sess_private_data[index] == NULL) {
+ if (sess->sess_data[index].refcnt == 0) {
ret = dev->dev_ops->sym_session_configure(dev, xforms,
- sess, mp);
+ sess, mp);
+
if (ret < 0) {
CDEV_LOG_ERR(
"dev_id %d failed to configure session details",
@@ -1177,6 +1220,7 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
}
}
+ sess->sess_data[index].refcnt++;
return 0;
}
@@ -1229,8 +1273,7 @@ rte_cryptodev_sym_session_create(struct rte_mempool *mp)
/* Clear device session pointer.
* Include the flag indicating presence of user data
*/
- memset(sess, 0, (sizeof(void *) * nb_drivers) + sizeof(uint8_t));
-
+ memset(sess->sess_data, 0, rte_cryptodev_sym_session_data_size(sess));
return sess;
}
@@ -1258,16 +1301,20 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
struct rte_cryptodev_sym_session *sess)
{
struct rte_cryptodev *dev;
+ uint32_t idx;
dev = rte_cryptodev_pmd_get_dev(dev_id);
- if (dev == NULL || sess == NULL)
+ if (dev == NULL || sess == NULL || dev->driver_id > sess->nb_drivers)
return -EINVAL;
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_clear, -ENOTSUP);
- dev->dev_ops->sym_session_clear(dev, sess);
+ idx = dev->driver_id;
+ if (--sess->sess_data[idx].refcnt != 0)
+ return -EBUSY;
+ dev->dev_ops->sym_session_clear(dev, sess);
return 0;
}
@@ -1285,7 +1332,6 @@ rte_cryptodev_asym_session_clear(uint8_t dev_id,
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->asym_session_clear, -ENOTSUP);
dev->dev_ops->asym_session_clear(dev, sess);
-
return 0;
}
@@ -1293,7 +1339,6 @@ int
rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
{
uint8_t i;
- void *sess_priv;
struct rte_mempool *sess_mp;
if (sess == NULL)
@@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
/* Check that all device private data has been freed */
for (i = 0; i < nb_drivers; i++) {
- sess_priv = get_sym_session_private_data(sess, i);
- if (sess_priv != NULL)
+ if (sess->sess_data[i].refcnt != 0)
return -EBUSY;
}
@@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
return 0;
}
+unsigned int
+rte_cryptodev_sym_session_max_data_size(void)
+{
+ struct rte_cryptodev_sym_session *sess = NULL;
+
+ return (sizeof(sess->sess_data[0]) * nb_drivers);
+}
+
+size_t
+rte_cryptodev_sym_session_max_size(uint16_t priv_size)
+{
+ struct rte_cryptodev_sym_session *sess = NULL;
+
+ return (sizeof(*sess) + priv_size +
+ rte_cryptodev_sym_session_max_data_size());
+}
+
int __rte_experimental
rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
{
@@ -1337,18 +1398,6 @@ rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
return 0;
}
-
-unsigned int
-rte_cryptodev_sym_get_header_session_size(void)
-{
- /*
- * Header contains pointers to the private data
- * of all registered drivers, and a flag which
- * indicates presence of user data
- */
- return ((sizeof(void *) * nb_drivers) + sizeof(uint8_t));
-}
-
unsigned int __rte_experimental
rte_cryptodev_asym_get_header_session_size(void)
{
@@ -1361,11 +1410,9 @@ rte_cryptodev_asym_get_header_session_size(void)
}
unsigned int
-rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
+rte_cryptodev_sym_private_session_size(uint8_t dev_id)
{
struct rte_cryptodev *dev;
- unsigned int header_size = sizeof(void *) * nb_drivers;
- unsigned int priv_sess_size;
if (!rte_cryptodev_pmd_is_valid_dev(dev_id))
return 0;
@@ -1375,18 +1422,7 @@ rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
if (*dev->dev_ops->sym_session_get_size == NULL)
return 0;
- priv_sess_size = (*dev->dev_ops->sym_session_get_size)(dev);
-
- /*
- * If size is less than session header size,
- * return the latter, as this guarantees that
- * sessionless operations will work
- */
- if (priv_sess_size < header_size)
- return header_size;
-
- return priv_sess_size;
-
+ return (*dev->dev_ops->sym_session_get_size)(dev);
}
unsigned int __rte_experimental
@@ -1409,7 +1445,6 @@ rte_cryptodev_asym_get_private_session_size(uint8_t dev_id)
return header_size;
return priv_sess_size;
-
}
int __rte_experimental
@@ -1418,15 +1453,10 @@ rte_cryptodev_sym_session_set_user_data(
void *data,
uint16_t size)
{
- uint16_t off_set = sizeof(void *) * nb_drivers;
- uint8_t *user_data_present = (uint8_t *)sess + off_set;
-
- if (sess == NULL)
+ if (sess == NULL || sess->priv_size < size)
return -EINVAL;
- *user_data_present = 1;
- off_set += sizeof(uint8_t);
- rte_memcpy((uint8_t *)sess + off_set, data, size);
+ rte_memcpy(sess->sess_data + sess->nb_drivers, data, size);
return 0;
}
@@ -1434,14 +1464,10 @@ void * __rte_experimental
rte_cryptodev_sym_session_get_user_data(
struct rte_cryptodev_sym_session *sess)
{
- uint16_t off_set = sizeof(void *) * nb_drivers;
- uint8_t *user_data_present = (uint8_t *)sess + off_set;
-
- if (sess == NULL || !*user_data_present)
+ if (sess == NULL || sess->priv_size == 0)
return NULL;
- off_set += sizeof(uint8_t);
- return (uint8_t *)sess + off_set;
+ return (sess->sess_data + sess->nb_drivers);
}
/** Initialise rte_crypto_op mempool element */
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 4099823f1..d88454f02 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -495,6 +495,14 @@ enum rte_cryptodev_event_type {
/** Crypto device queue pair configuration structure. */
struct rte_cryptodev_qp_conf {
uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
+ struct rte_mempool *sess_pool;
+ /**< Pointer to crypto sessions mempool,
+ * used for session-less operations.
+ */
+ struct rte_mempool *priv_sess_pool;
+ /**< Pointer to device specific sessions mempool,
+ * used for session-less operations.
+ */
};
/**
@@ -680,17 +688,13 @@ rte_cryptodev_close(uint8_t dev_id);
* *SOCKET_ID_ANY* if there is no NUMA constraint
* for the DMA memory allocated for the receive
* queue pair.
- * @param session_pool Pointer to device session mempool, used
- * for session-less operations.
- *
* @return
* - 0: Success, queue pair correctly set up.
* - <0: Queue pair configuration failed
*/
extern int
rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
- const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
- struct rte_mempool *session_pool);
+ const struct rte_cryptodev_qp_conf *qp_conf, int socket_id);
/**
* Get the number of queue pairs on a specific crypto device
@@ -954,10 +958,43 @@ rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t qp_id,
* has a fixed algo, key, op-type, digest_len etc.
*/
struct rte_cryptodev_sym_session {
- __extension__ void *sess_private_data[0];
- /**< Private symmetric session material */
+ uint64_t userdata;
+ /**< Can be used for external metadata */
+ uint16_t nb_drivers;
+ /**< number of elements in sess_data array */
+ uint16_t priv_size;
+ /**< session private data will be placed after sess_data */
+ __extension__ struct {
+ void *data;
+ uint16_t refcnt;
+ } sess_data[0];
+ /**< Driver specific session material, variable size */
};
+static inline size_t
+rte_cryptodev_sym_session_data_size(const struct rte_cryptodev_sym_session *s)
+{
+ return (sizeof(s->sess_data[0]) * s->nb_drivers);
+}
+
+static inline size_t
+rte_cryptodev_sym_session_size(const struct rte_cryptodev_sym_session *s)
+{
+ return (sizeof(*s) + (s)->priv_size +
+ rte_cryptodev_sym_session_data_size(s));
+}
+
+unsigned int
+rte_cryptodev_sym_session_max_data_size(void);
+
+size_t
+rte_cryptodev_sym_session_max_size(uint16_t priv_size);
+
+struct rte_mempool *
+rte_cryptodev_sym_session_pool_create(const char *name,
+ uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
+ int socket_id);
+
/** Cryptodev asymmetric crypto session */
struct rte_cryptodev_asym_session {
__extension__ void *sess_private_data[0];
@@ -1123,7 +1160,7 @@ rte_cryptodev_asym_get_header_session_size(void);
* symmetric session
*/
unsigned int
-rte_cryptodev_sym_get_private_session_size(uint8_t dev_id);
+rte_cryptodev_sym_private_session_size(uint8_t dev_id);
/**
* Get the size of the private data for asymmetric session
diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
index 6ff49d64d..2f98f65d1 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -191,13 +191,12 @@ typedef void (*cryptodev_info_get_t)(struct rte_cryptodev *dev,
* @param qp_id Queue Pair Index
* @param qp_conf Queue configuration structure
* @param socket_id Socket Index
- * @param session_pool Pointer to device session mempool
*
* @return Returns 0 on success.
*/
typedef int (*cryptodev_queue_pair_setup_t)(struct rte_cryptodev *dev,
uint16_t qp_id, const struct rte_cryptodev_qp_conf *qp_conf,
- int socket_id, struct rte_mempool *session_pool);
+ int socket_id);
/**
* Release memory resources allocated by given queue pair.
@@ -478,20 +477,25 @@ RTE_INIT(init_ ##driver_id)\
static inline void *
get_sym_session_private_data(const struct rte_cryptodev_sym_session *sess,
- uint8_t driver_id) {
- return sess->sess_private_data[driver_id];
+ uint8_t driver_id)
+{
+ if (driver_id < sess->nb_drivers)
+ return sess->sess_data[driver_id].data;
+ return NULL;
}
static inline void
set_sym_session_private_data(struct rte_cryptodev_sym_session *sess,
uint8_t driver_id, void *private_data)
{
- sess->sess_private_data[driver_id] = private_data;
+ if (driver_id < sess->nb_drivers)
+ sess->sess_data[driver_id].data = private_data;
}
static inline void *
get_asym_session_private_data(const struct rte_cryptodev_asym_session *sess,
- uint8_t driver_id) {
+ uint8_t driver_id)
+{
return sess->sess_private_data[driver_id];
}
--
2.13.6
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-08-24 17:48 [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session Konstantin Ananyev
@ 2018-10-05 11:05 ` Ananyev, Konstantin
2018-11-12 21:01 ` Trahe, Fiona
1 sibling, 0 replies; 10+ messages in thread
From: Ananyev, Konstantin @ 2018-10-05 11:05 UTC (permalink / raw)
To: dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Trahe, Fiona, Tomasz Duszynski,
Hemant Agrawal, Natalie Samsonov, Dmitri Epshtein, Jay Zhou
Hi everyone,
> This RFC for proposes several changes inside rte_cryptodev_sym_session.
> Note that this is just RFC not a complete patch, so for now
> I modified only the librte_cryptodev itself,
> some cryptodev PMD, test-crypto-perf and ipsec-secgw example.
> Proposed changes means ABI/API breakage inside cryptodev,
> so looking for feedback from crypto-dev lib and crypto-PMD maintainiers.
> Below are details and reasoning for proposed changes.
>
> 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear()
> operate based on cytpodev device id, though inside
> rte_cryptodev_sym_session device specific data is addressed
> by driver id (not device id).
> That creates a problem with current implementation when we have
> two or more devices with the same driver used by the same session.
> Consider the following example:
>
> struct rte_cryptodev_sym_session *sess;
> rte_cryptodev_sym_session_init(dev_id=X, sess, ...);
> rte_cryptodev_sym_session_init(dev_id=Y, sess, ...);
> rte_cryptodev_sym_session_clear(dev_id=X, sess);
>
> After that point if X and Y uses the same driver,
> then sess can't be used by device Y any more.
> The reason for that - driver specific (not device specific)
> data per session, plus there is no information
> how many device instances use that data.
> Probably the simplest way to deal with that issue -
> add a reference counter per each driver data.
>
> 2.rte_cryptodev_sym_session_set_user_data() and
> rte_cryptodev_sym_session_get_user_data() -
> with current implementation there is no defined way for the user to
> determine what is the max allowed size of the private data.
> Even within rte_cryptodev_sym_session_set_user_data() we just blindly
> copying user provided data without checking memory boundaries violation.
> To overcome that issue I added 'uint16_t priv_size' into
> rte_cryptodev_sym_session structure.
>
> 3.rte_cryptodev_sym_session contains an array of variable size for
> driver specific data.
> Though number of elements in that array is determined by static
> variable nb_drivers, that could be modified by
> rte_cryptodev_allocate_driver().
> That construction seems to work ok so far, as right now users register
> all their PMDs at startup, though it doesn't mean that it would always
> remain like that.
> To make it less error prone I added 'uint16_t nb_drivers' into the
> rte_cryptodev_sym_session structure.
> At least that allows related functions to check that provided
> driver id wouldn't overrun variable array boundaries,
> again it allows to determine size of already allocated session
> without accessing global variable.
>
> 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
> would have sort of readonly type data (init once at allocation time,
> keep unmodified through session life-time).
> That requires more changes in current cryptodev implementation:
> Right now inside cryptodev framework both rte_cryptodev_sym_session
> and driver specific session data are two completely different sctrucures
> (e.g. struct struct null_crypto_session and struct null_crypto_session).
> Though current cryptodev implementation implicitly assumes that driver
> will allocate both of them from within the same mempool.
> Plus this is done in a manner that they override each other fields
> (reuse the same space - sort of implicit C union).
> That's probably not the best programming practice,
> plus make impossible to have readonly fields inside both of them.
> So to overcome that situation I changed an API a bit, to allow
> to use two different mempools for these two distinct data structures.
>
> 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
> I suppose that self-explanatory, and might be used in a lot of places
> (would be quite useful for ipsec library we develop).
>
> So the new proposed layout for rte_cryptodev_sym_session:
> struct rte_cryptodev_sym_session {
> uint64_t userdata;
> /**< Can be used for external metadata */
> uint16_t nb_drivers;
> /**< number of elements in sess_data array */
> uint16_t priv_size;
> /**< session private data will be placed after sess_data */
> __extension__ struct {
> void *data;
> uint16_t refcnt;
> } sess_data[0];
> /**< Driver specific session material, variable size */
> };
>
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
Ok, didn't hear any objections, so far,
so I suppose everyone are ok in general with proposed changes.
Will go ahead with deprecation notice for 18.11 then.
Konstantin
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-08-24 17:48 [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session Konstantin Ananyev
2018-10-05 11:05 ` Ananyev, Konstantin
@ 2018-11-12 21:01 ` Trahe, Fiona
2018-11-12 23:16 ` Trahe, Fiona
2018-11-13 18:56 ` Ananyev, Konstantin
1 sibling, 2 replies; 10+ messages in thread
From: Trahe, Fiona @ 2018-11-12 21:01 UTC (permalink / raw)
To: Ananyev, Konstantin, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou, Trahe, Fiona
Hi Konstantin,
Sorry for the delay in reviewing this and thanks for your proposals.
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, August 24, 2018 10:48 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Tomasz Duszynski <tdu@semihalf.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Natalie Samsonov <nsamsono@marvell.com>; Dmitri Epshtein
> <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> Subject: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
>
> This RFC for proposes several changes inside rte_cryptodev_sym_session.
> Note that this is just RFC not a complete patch, so for now
> I modified only the librte_cryptodev itself,
> some cryptodev PMD, test-crypto-perf and ipsec-secgw example.
> Proposed changes means ABI/API breakage inside cryptodev,
> so looking for feedback from crypto-dev lib and crypto-PMD maintainiers.
> Below are details and reasoning for proposed changes.
>
> 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear()
> operate based on cytpodev device id, though inside
> rte_cryptodev_sym_session device specific data is addressed
> by driver id (not device id).
> That creates a problem with current implementation when we have
> two or more devices with the same driver used by the same session.
> Consider the following example:
>
> struct rte_cryptodev_sym_session *sess;
> rte_cryptodev_sym_session_init(dev_id=X, sess, ...);
> rte_cryptodev_sym_session_init(dev_id=Y, sess, ...);
> rte_cryptodev_sym_session_clear(dev_id=X, sess);
>
> After that point if X and Y uses the same driver,
> then sess can't be used by device Y any more.
> The reason for that - driver specific (not device specific)
> data per session, plus there is no information
> how many device instances use that data.
> Probably the simplest way to deal with that issue -
> add a reference counter per each driver data.
[Fiona] Ok, I agree with this issue and proposed fix.
We need to also document that it's user's responsibility
not to call either init() or clear() twice on same device, as
that would break the ref count.
The same should be added to asym_session - though I accept
it'sbe outside of the scope of this patch.
> 2.rte_cryptodev_sym_session_set_user_data() and
> rte_cryptodev_sym_session_get_user_data() -
> with current implementation there is no defined way for the user to
> determine what is the max allowed size of the private data.
> Even within rte_cryptodev_sym_session_set_user_data() we just blindly
> copying user provided data without checking memory boundaries violation.
> To overcome that issue I added 'uint16_t priv_size' into
> rte_cryptodev_sym_session structure.
[Fiona] I agree, this is needed.
But I propose to call it user_data_sz NOT priv_size.
See https://patches.dpdk.org/patch/42515/ to understand why.
> 3.rte_cryptodev_sym_session contains an array of variable size for
> driver specific data.
> Though number of elements in that array is determined by static
> variable nb_drivers, that could be modified by
> rte_cryptodev_allocate_driver().
> That construction seems to work ok so far, as right now users register
> all their PMDs at startup, though it doesn't mean that it would always
> remain like that.
> To make it less error prone I added 'uint16_t nb_drivers' into the
> rte_cryptodev_sym_session structure.
> At least that allows related functions to check that provided
> driver id wouldn't overrun variable array boundaries,
> again it allows to determine size of already allocated session
> without accessing global variable.
[Fiona] I agree with both issue and solution.
The same should be added to asym_session - though again
it's outside of the scope of this patch.
> 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
> would have sort of readonly type data (init once at allocation time,
> keep unmodified through session life-time).
> That requires more changes in current cryptodev implementation:
> Right now inside cryptodev framework both rte_cryptodev_sym_session
> and driver specific session data are two completely different sctrucures
> (e.g. struct struct null_crypto_session and struct null_crypto_session).
> Though current cryptodev implementation implicitly assumes that driver
> will allocate both of them from within the same mempool.
> Plus this is done in a manner that they override each other fields
> (reuse the same space - sort of implicit C union).
> That's probably not the best programming practice,
> plus make impossible to have readonly fields inside both of them.
> So to overcome that situation I changed an API a bit, to allow
> to use two different mempools for these two distinct data structures.
>
> 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
> I suppose that self-explanatory, and might be used in a lot of places
> (would be quite useful for ipsec library we develop).
[Fiona] Seems unnecessary - the set_user_data can be used. Why have 2
separate user data spaces in the session? - it's confusing.
If these is some good reason, then a different name should be used for clarity.
Not private. And not user. Possibly opaque data. Though afaik we had this in the op
and removed it as it was felt appending user_data was enough.
> So the new proposed layout for rte_cryptodev_sym_session:
> struct rte_cryptodev_sym_session {
> uint64_t userdata;
> /**< Can be used for external metadata */
> uint16_t nb_drivers;
> /**< number of elements in sess_data array */
> uint16_t priv_size;
> /**< session private data will be placed after sess_data */
> __extension__ struct {
> void *data;
> uint16_t refcnt;
> } sess_data[0];
> /**< Driver specific session material, variable size */
> };
>
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> app/test-crypto-perf/cperf.h | 1 +
> app/test-crypto-perf/cperf_ops.c | 11 +-
> app/test-crypto-perf/cperf_ops.h | 2 +-
> app/test-crypto-perf/cperf_test_latency.c | 5 +-
> app/test-crypto-perf/cperf_test_latency.h | 1 +
> app/test-crypto-perf/cperf_test_pmd_cyclecount.c | 5 +-
> app/test-crypto-perf/cperf_test_pmd_cyclecount.h | 1 +
> app/test-crypto-perf/cperf_test_throughput.c | 5 +-
> app/test-crypto-perf/cperf_test_throughput.h | 1 +
> app/test-crypto-perf/cperf_test_verify.c | 5 +-
> app/test-crypto-perf/cperf_test_verify.h | 1 +
> app/test-crypto-perf/main.c | 111 +++++++++++------
> drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 10 +-
> drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c | 5 +-
> drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 4 +-
> drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 10 +-
> drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 5 +-
> drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h | 4 +-
> drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 3 +-
> drivers/crypto/dpaa_sec/dpaa_sec.c | 3 +-
> drivers/crypto/null/null_crypto_pmd.c | 14 ++-
> drivers/crypto/null/null_crypto_pmd_ops.c | 5 +-
> drivers/crypto/null/null_crypto_pmd_private.h | 4 +-
> drivers/crypto/scheduler/scheduler_pmd_ops.c | 5 +-
> drivers/crypto/virtio/virtio_cryptodev.c | 6 +-
> examples/ipsec-secgw/ipsec-secgw.c | 116 ++++++++++++------
> examples/ipsec-secgw/ipsec.h | 2 +
> lib/librte_cryptodev/rte_cryptodev.c | 134 ++++++++++++---------
> lib/librte_cryptodev/rte_cryptodev.h | 53 ++++++--
> lib/librte_cryptodev/rte_cryptodev_pmd.h | 16 ++-
> 30 files changed, 356 insertions(+), 192 deletions(-)
>
///snip///
> struct cnt_blk {
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 63ae23f00..e25282445 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -943,8 +943,7 @@ rte_cryptodev_close(uint8_t dev_id)
>
> int
> rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> - const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
> - struct rte_mempool *session_pool)
> + const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
>
> {
> struct rte_cryptodev *dev;
> @@ -954,6 +953,12 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> return -EINVAL;
> }
>
> + if (qp_conf == NULL || qp_conf->sess_pool == NULL ||
> + qp_conf->priv_sess_pool == NULL) {
> + CDEV_LOG_ERR("Invalid queue_pair config");
> + return -EINVAL;
> + }
> +
> dev = &rte_crypto_devices[dev_id];
> if (queue_pair_id >= dev->data->nb_queue_pairs) {
> CDEV_LOG_ERR("Invalid queue_pair_id=%d", queue_pair_id);
> @@ -969,7 +974,7 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_pair_setup, -ENOTSUP);
>
> return (*dev->dev_ops->queue_pair_setup)(dev, queue_pair_id, qp_conf,
> - socket_id, session_pool);
> + socket_id);
> }
>
>
> @@ -1146,6 +1151,41 @@ rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
> rte_spinlock_unlock(&rte_cryptodev_cb_lock);
> }
>
> +static void
> +cryptodev_sym_session_init_elem(__rte_unused struct rte_mempool *pool,
> + void *arg, void *obj, __rte_unused uint32_t idx)
> +{
> + struct rte_cryptodev_sym_session *ds;
> + const struct rte_cryptodev_sym_session *ss;
> +
> + ds = obj;
> + ss = arg;
> +
> + *ds = *ss;
> + memset(ds->sess_data, 0, rte_cryptodev_sym_session_data_size(ds));
> +}
> +
> +struct rte_mempool *
> +rte_cryptodev_sym_session_pool_create(const char *name,
> + uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
> + int socket_id)
> +{
> + struct rte_mempool *mp;
> + uint32_t elt_size;
> + struct rte_cryptodev_sym_session s = {
> + .nb_drivers = nb_drivers,
> + .priv_size = priv_size,
> + };
> +
> + elt_size = rte_cryptodev_sym_session_max_size(priv_size);
> + mp = rte_mempool_create(name, nb_elts, elt_size, cache_size, 0,
> + NULL, NULL, cryptodev_sym_session_init_elem, &s,
> + socket_id, 0);
> + if (mp == NULL)
> + CDEV_LOG_ERR("%s(name=%s) failed, rte_errno=%d\n",
> + __func__, name, rte_errno);
> + return mp;
> +}
>
> int
> rte_cryptodev_sym_session_init(uint8_t dev_id,
> @@ -1163,12 +1203,15 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> return -EINVAL;
>
> index = dev->driver_id;
> + if (index > sess->nb_drivers)
> + return -EINVAL;
>
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_configure, -ENOTSUP);
>
> - if (sess->sess_private_data[index] == NULL) {
> + if (sess->sess_data[index].refcnt == 0) {
> ret = dev->dev_ops->sym_session_configure(dev, xforms,
> - sess, mp);
> + sess, mp);
> +
> if (ret < 0) {
> CDEV_LOG_ERR(
> "dev_id %d failed to configure session details",
> @@ -1177,6 +1220,7 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> }
> }
>
> + sess->sess_data[index].refcnt++;
> return 0;
> }
>
> @@ -1229,8 +1273,7 @@ rte_cryptodev_sym_session_create(struct rte_mempool *mp)
> /* Clear device session pointer.
> * Include the flag indicating presence of user data
> */
> - memset(sess, 0, (sizeof(void *) * nb_drivers) + sizeof(uint8_t));
> -
> + memset(sess->sess_data, 0, rte_cryptodev_sym_session_data_size(sess));
> return sess;
> }
>
> @@ -1258,16 +1301,20 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
> struct rte_cryptodev_sym_session *sess)
> {
> struct rte_cryptodev *dev;
> + uint32_t idx;
>
> dev = rte_cryptodev_pmd_get_dev(dev_id);
>
> - if (dev == NULL || sess == NULL)
> + if (dev == NULL || sess == NULL || dev->driver_id > sess->nb_drivers)
> return -EINVAL;
>
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_clear, -ENOTSUP);
>
> - dev->dev_ops->sym_session_clear(dev, sess);
> + idx = dev->driver_id;
> + if (--sess->sess_data[idx].refcnt != 0)
> + return -EBUSY;
>
> + dev->dev_ops->sym_session_clear(dev, sess);
> return 0;
> }
>
> @@ -1285,7 +1332,6 @@ rte_cryptodev_asym_session_clear(uint8_t dev_id,
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->asym_session_clear, -ENOTSUP);
>
> dev->dev_ops->asym_session_clear(dev, sess);
> -
> return 0;
> }
>
> @@ -1293,7 +1339,6 @@ int
> rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> {
> uint8_t i;
> - void *sess_priv;
> struct rte_mempool *sess_mp;
>
> if (sess == NULL)
> @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
>
> /* Check that all device private data has been freed */
> for (i = 0; i < nb_drivers; i++) {
[Fiona] Use the sess.nb_drivers rather than the global.
Actually maybe name slightly differently to reduce the chance of that mistake happening, e.g.
rename sess.nb_drivers to sess.num_drivers?
> - sess_priv = get_sym_session_private_data(sess, i);
> - if (sess_priv != NULL)
> + if (sess->sess_data[i].refcnt != 0)
> return -EBUSY;
> }
>
> @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> return 0;
> }
>
> +unsigned int
> +rte_cryptodev_sym_session_max_data_size(void)
[Fiona] Suggest renaming this
rte_cryptodev_sym_session_max_array_size()
> +{
> + struct rte_cryptodev_sym_session *sess = NULL;
> +
> + return (sizeof(sess->sess_data[0]) * nb_drivers);
> +}
> +
> +size_t
> +rte_cryptodev_sym_session_max_size(uint16_t priv_size)
> +{
> + struct rte_cryptodev_sym_session *sess = NULL;
> +
> + return (sizeof(*sess) + priv_size +
> + rte_cryptodev_sym_session_max_data_size());
> +}
> +
> int __rte_experimental
> rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
> {
> @@ -1337,18 +1398,6 @@ rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
> return 0;
> }
>
> -
> -unsigned int
> -rte_cryptodev_sym_get_header_session_size(void)
> -{
> - /*
> - * Header contains pointers to the private data
> - * of all registered drivers, and a flag which
> - * indicates presence of user data
> - */
> - return ((sizeof(void *) * nb_drivers) + sizeof(uint8_t));
> -}
> -
> unsigned int __rte_experimental
> rte_cryptodev_asym_get_header_session_size(void)
> {
> @@ -1361,11 +1410,9 @@ rte_cryptodev_asym_get_header_session_size(void)
> }
>
> unsigned int
> -rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
> +rte_cryptodev_sym_private_session_size(uint8_t dev_id)
> {
> struct rte_cryptodev *dev;
> - unsigned int header_size = sizeof(void *) * nb_drivers;
> - unsigned int priv_sess_size;
>
> if (!rte_cryptodev_pmd_is_valid_dev(dev_id))
> return 0;
> @@ -1375,18 +1422,7 @@ rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
> if (*dev->dev_ops->sym_session_get_size == NULL)
> return 0;
>
> - priv_sess_size = (*dev->dev_ops->sym_session_get_size)(dev);
> -
> - /*
> - * If size is less than session header size,
> - * return the latter, as this guarantees that
> - * sessionless operations will work
> - */
> - if (priv_sess_size < header_size)
> - return header_size;
> -
> - return priv_sess_size;
> -
> + return (*dev->dev_ops->sym_session_get_size)(dev);
> }
>
> unsigned int __rte_experimental
> @@ -1409,7 +1445,6 @@ rte_cryptodev_asym_get_private_session_size(uint8_t dev_id)
> return header_size;
>
> return priv_sess_size;
> -
> }
>
> int __rte_experimental
> @@ -1418,15 +1453,10 @@ rte_cryptodev_sym_session_set_user_data(
> void *data,
> uint16_t size)
> {
> - uint16_t off_set = sizeof(void *) * nb_drivers;
> - uint8_t *user_data_present = (uint8_t *)sess + off_set;
> -
> - if (sess == NULL)
> + if (sess == NULL || sess->priv_size < size)
> return -EINVAL;
>
> - *user_data_present = 1;
> - off_set += sizeof(uint8_t);
> - rte_memcpy((uint8_t *)sess + off_set, data, size);
> + rte_memcpy(sess->sess_data + sess->nb_drivers, data, size);
> return 0;
> }
>
> @@ -1434,14 +1464,10 @@ void * __rte_experimental
> rte_cryptodev_sym_session_get_user_data(
> struct rte_cryptodev_sym_session *sess)
> {
> - uint16_t off_set = sizeof(void *) * nb_drivers;
> - uint8_t *user_data_present = (uint8_t *)sess + off_set;
> -
> - if (sess == NULL || !*user_data_present)
> + if (sess == NULL || sess->priv_size == 0)
> return NULL;
>
> - off_set += sizeof(uint8_t);
> - return (uint8_t *)sess + off_set;
> + return (sess->sess_data + sess->nb_drivers);
> }
>
> /** Initialise rte_crypto_op mempool element */
> diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
> index 4099823f1..d88454f02 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -495,6 +495,14 @@ enum rte_cryptodev_event_type {
> /** Crypto device queue pair configuration structure. */
> struct rte_cryptodev_qp_conf {
> uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
> + struct rte_mempool *sess_pool;
> + /**< Pointer to crypto sessions mempool,
> + * used for session-less operations.
> + */
> + struct rte_mempool *priv_sess_pool;
> + /**< Pointer to device specific sessions mempool,
> + * used for session-less operations.
> + */
> };
>
> /**
> @@ -680,17 +688,13 @@ rte_cryptodev_close(uint8_t dev_id);
> * *SOCKET_ID_ANY* if there is no NUMA constraint
> * for the DMA memory allocated for the receive
> * queue pair.
> - * @param session_pool Pointer to device session mempool, used
> - * for session-less operations.
> - *
> * @return
> * - 0: Success, queue pair correctly set up.
> * - <0: Queue pair configuration failed
> */
> extern int
> rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> - const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
> - struct rte_mempool *session_pool);
> + const struct rte_cryptodev_qp_conf *qp_conf, int socket_id);
>
> /**
> * Get the number of queue pairs on a specific crypto device
> @@ -954,10 +958,43 @@ rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t qp_id,
> * has a fixed algo, key, op-type, digest_len etc.
> */
> struct rte_cryptodev_sym_session {
> - __extension__ void *sess_private_data[0];
> - /**< Private symmetric session material */
> + uint64_t userdata;
> + /**< Can be used for external metadata */
> + uint16_t nb_drivers;
> + /**< number of elements in sess_data array */
> + uint16_t priv_size;
> + /**< session private data will be placed after sess_data */
> + __extension__ struct {
> + void *data;
> + uint16_t refcnt;
> + } sess_data[0];
> + /**< Driver specific session material, variable size */
> };
>
> +static inline size_t
> +rte_cryptodev_sym_session_data_size(const struct rte_cryptodev_sym_session *s)
> +{
> + return (sizeof(s->sess_data[0]) * s->nb_drivers);
> +}
> +
> +static inline size_t
> +rte_cryptodev_sym_session_size(const struct rte_cryptodev_sym_session *s)
> +{
> + return (sizeof(*s) + (s)->priv_size +
> + rte_cryptodev_sym_session_data_size(s));
> +}
> +
[Fiona] Are above 2 fns used?
Look like duplicates of the "max" fns?
> +unsigned int
> +rte_cryptodev_sym_session_max_data_size(void);
> +
> +size_t
> +rte_cryptodev_sym_session_max_size(uint16_t priv_size);
> +
> +struct rte_mempool *
> +rte_cryptodev_sym_session_pool_create(const char *name,
> + uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
> + int socket_id);
> +
> /** Cryptodev asymmetric crypto session */
> struct rte_cryptodev_asym_session {
> __extension__ void *sess_private_data[0];
> @@ -1123,7 +1160,7 @@ rte_cryptodev_asym_get_header_session_size(void);
> * symmetric session
> */
> unsigned int
> -rte_cryptodev_sym_get_private_session_size(uint8_t dev_id);
> +rte_cryptodev_sym_private_session_size(uint8_t dev_id);
>
> /**
> * Get the size of the private data for asymmetric session
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> index 6ff49d64d..2f98f65d1 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> @@ -191,13 +191,12 @@ typedef void (*cryptodev_info_get_t)(struct rte_cryptodev *dev,
> * @param qp_id Queue Pair Index
> * @param qp_conf Queue configuration structure
> * @param socket_id Socket Index
> - * @param session_pool Pointer to device session mempool
> *
> * @return Returns 0 on success.
> */
> typedef int (*cryptodev_queue_pair_setup_t)(struct rte_cryptodev *dev,
> uint16_t qp_id, const struct rte_cryptodev_qp_conf *qp_conf,
> - int socket_id, struct rte_mempool *session_pool);
> + int socket_id);
>
> /**
> * Release memory resources allocated by given queue pair.
> @@ -478,20 +477,25 @@ RTE_INIT(init_ ##driver_id)\
>
> static inline void *
> get_sym_session_private_data(const struct rte_cryptodev_sym_session *sess,
> - uint8_t driver_id) {
> - return sess->sess_private_data[driver_id];
> + uint8_t driver_id)
> +{
> + if (driver_id < sess->nb_drivers)
> + return sess->sess_data[driver_id].data;
> + return NULL;
> }
>
> static inline void
> set_sym_session_private_data(struct rte_cryptodev_sym_session *sess,
> uint8_t driver_id, void *private_data)
> {
> - sess->sess_private_data[driver_id] = private_data;
> + if (driver_id < sess->nb_drivers)
> + sess->sess_data[driver_id].data = private_data;
> }
>
> static inline void *
> get_asym_session_private_data(const struct rte_cryptodev_asym_session *sess,
> - uint8_t driver_id) {
> + uint8_t driver_id)
> +{
> return sess->sess_private_data[driver_id];
> }
>
> --
> 2.13.6
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-12 21:01 ` Trahe, Fiona
@ 2018-11-12 23:16 ` Trahe, Fiona
2018-11-12 23:24 ` Trahe, Fiona
2018-11-13 18:56 ` Ananyev, Konstantin
1 sibling, 1 reply; 10+ messages in thread
From: Trahe, Fiona @ 2018-11-12 23:16 UTC (permalink / raw)
To: Ananyev, Konstantin, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou, Trahe, Fiona
RE item 4: use of session pool in qp setup:
> > 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
> > would have sort of readonly type data (init once at allocation time,
> > keep unmodified through session life-time).
> > That requires more changes in current cryptodev implementation:
> > Right now inside cryptodev framework both rte_cryptodev_sym_session
> > and driver specific session data are two completely different sctrucures
> > (e.g. struct struct null_crypto_session and struct null_crypto_session).
> > Though current cryptodev implementation implicitly assumes that driver
> > will allocate both of them from within the same mempool.
> > Plus this is done in a manner that they override each other fields
> > (reuse the same space - sort of implicit C union).
> > That's probably not the best programming practice,
> > plus make impossible to have readonly fields inside both of them.
> > So to overcome that situation I changed an API a bit, to allow
> > to use two different mempools for these two distinct data structures.
[Fiona] Ok, I can see either way on this.
Seems we could continue to use a single pool for multiple struct types as long as the size is bigger
than the maximum. But it's also reasonable to change to two as they are used so differently
- especially if that allows one to be handled as read-only after init and the other not - but I
didn't see any code which was doing this.
Anyway, if we go with 2 pools, I propose we remove the sess pool from the qp config, rather than pass in both pools.
I just did a trawl trough the PMDs and none use the sess pool - that param is a hangover from earlier.
A few store it in a local var, but never use it for anything.
Now is a good time to remove it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-12 23:16 ` Trahe, Fiona
@ 2018-11-12 23:24 ` Trahe, Fiona
2018-11-13 18:56 ` Ananyev, Konstantin
0 siblings, 1 reply; 10+ messages in thread
From: Trahe, Fiona @ 2018-11-12 23:24 UTC (permalink / raw)
To: Ananyev, Konstantin, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou, Trahe, Fiona
Correction below
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, November 12, 2018 4:17 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Doherty, Declan <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Tomasz Duszynski
> <tdu@semihalf.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Natalie Samsonov
> <nsamsono@marvell.com>; Dmitri Epshtein <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>;
> Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
>
> RE item 4: use of session pool in qp setup:
> > > 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
> > > would have sort of readonly type data (init once at allocation time,
> > > keep unmodified through session life-time).
> > > That requires more changes in current cryptodev implementation:
> > > Right now inside cryptodev framework both rte_cryptodev_sym_session
> > > and driver specific session data are two completely different sctrucures
> > > (e.g. struct struct null_crypto_session and struct null_crypto_session).
> > > Though current cryptodev implementation implicitly assumes that driver
> > > will allocate both of them from within the same mempool.
> > > Plus this is done in a manner that they override each other fields
> > > (reuse the same space - sort of implicit C union).
> > > That's probably not the best programming practice,
> > > plus make impossible to have readonly fields inside both of them.
> > > So to overcome that situation I changed an API a bit, to allow
> > > to use two different mempools for these two distinct data structures.
> [Fiona] Ok, I can see either way on this.
> Seems we could continue to use a single pool for multiple struct types as long as the size is bigger
> than the maximum. But it's also reasonable to change to two as they are used so differently
> - especially if that allows one to be handled as read-only after init and the other not - but I
> didn't see any code which was doing this.
> Anyway, if we go with 2 pools, I propose we remove the sess pool from the qp config, rather than pass in
> both pools.
> I just did a trawl trough the PMDs and none use the sess pool - that param is a hangover from earlier.
> A few store it in a local var, but never use it for anything.
> Now is a good time to remove it.
[Fiona] Correction - I found some PMDs do use it - for the sessionless case. So I suppose we
will need to pass in the two pool params.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-12 23:24 ` Trahe, Fiona
@ 2018-11-13 18:56 ` Ananyev, Konstantin
0 siblings, 0 replies; 10+ messages in thread
From: Ananyev, Konstantin @ 2018-11-13 18:56 UTC (permalink / raw)
To: Trahe, Fiona, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, November 12, 2018 11:25 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Tomasz Duszynski <tdu@semihalf.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Natalie Samsonov
> <nsamsono@marvell.com>; Dmitri Epshtein <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>; Trahe, Fiona
> <fiona.trahe@intel.com>
> Subject: RE: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
>
> Correction below
>
> > -----Original Message-----
> > From: Trahe, Fiona
> > Sent: Monday, November 12, 2018 4:17 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> > Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> > Doherty, Declan <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> > <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Tomasz Duszynski
> > <tdu@semihalf.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Natalie Samsonov
> > <nsamsono@marvell.com>; Dmitri Epshtein <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>;
> > Trahe, Fiona <fiona.trahe@intel.com>
> > Subject: RE: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
> >
> > RE item 4: use of session pool in qp setup:
> > > > 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
> > > > would have sort of readonly type data (init once at allocation time,
> > > > keep unmodified through session life-time).
> > > > That requires more changes in current cryptodev implementation:
> > > > Right now inside cryptodev framework both rte_cryptodev_sym_session
> > > > and driver specific session data are two completely different sctrucures
> > > > (e.g. struct struct null_crypto_session and struct null_crypto_session).
> > > > Though current cryptodev implementation implicitly assumes that driver
> > > > will allocate both of them from within the same mempool.
> > > > Plus this is done in a manner that they override each other fields
> > > > (reuse the same space - sort of implicit C union).
> > > > That's probably not the best programming practice,
> > > > plus make impossible to have readonly fields inside both of them.
> > > > So to overcome that situation I changed an API a bit, to allow
> > > > to use two different mempools for these two distinct data structures.
> > [Fiona] Ok, I can see either way on this.
> > Seems we could continue to use a single pool for multiple struct types as long as the size is bigger
> > than the maximum. But it's also reasonable to change to two as they are used so differently
> > - especially if that allows one to be handled as read-only after init and the other not - but I
> > didn't see any code which was doing this.
> > Anyway, if we go with 2 pools, I propose we remove the sess pool from the qp config, rather than pass in
> > both pools.
> > I just did a trawl trough the PMDs and none use the sess pool - that param is a hangover from earlier.
> > A few store it in a local var, but never use it for anything.
> > Now is a good time to remove it.
> [Fiona] Correction - I found some PMDs do use it - for the sessionless case. So I suppose we
> will need to pass in the two pool params.
Yes, we need it for sessionless op.
Konstantin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-12 21:01 ` Trahe, Fiona
2018-11-12 23:16 ` Trahe, Fiona
@ 2018-11-13 18:56 ` Ananyev, Konstantin
2018-11-14 0:46 ` Trahe, Fiona
1 sibling, 1 reply; 10+ messages in thread
From: Ananyev, Konstantin @ 2018-11-13 18:56 UTC (permalink / raw)
To: Trahe, Fiona, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou
Hi Fiona,
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, November 12, 2018 9:01 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Tomasz Duszynski <tdu@semihalf.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Natalie Samsonov
> <nsamsono@marvell.com>; Dmitri Epshtein <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>; Trahe, Fiona
> <fiona.trahe@intel.com>
> Subject: RE: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
>
> Hi Konstantin,
> Sorry for the delay in reviewing this and thanks for your proposals.
NP, thanks for review.
My comments/answers inline.
Can you also have a look at related deprecation note:
http://patches.dpdk.org/patch/46633/
and provide the feedback?
Konstantin
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Friday, August 24, 2018 10:48 AM
> > To: dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > <declan.doherty@intel.com>; Ravi Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> > <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Trahe, Fiona
> > <fiona.trahe@intel.com>; Tomasz Duszynski <tdu@semihalf.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Natalie Samsonov <nsamsono@marvell.com>; Dmitri Epshtein
> > <dima@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> > Subject: [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
> >
> > This RFC for proposes several changes inside rte_cryptodev_sym_session.
> > Note that this is just RFC not a complete patch, so for now
> > I modified only the librte_cryptodev itself,
> > some cryptodev PMD, test-crypto-perf and ipsec-secgw example.
> > Proposed changes means ABI/API breakage inside cryptodev,
> > so looking for feedback from crypto-dev lib and crypto-PMD maintainiers.
> > Below are details and reasoning for proposed changes.
> >
> > 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear()
> > operate based on cytpodev device id, though inside
> > rte_cryptodev_sym_session device specific data is addressed
> > by driver id (not device id).
> > That creates a problem with current implementation when we have
> > two or more devices with the same driver used by the same session.
> > Consider the following example:
> >
> > struct rte_cryptodev_sym_session *sess;
> > rte_cryptodev_sym_session_init(dev_id=X, sess, ...);
> > rte_cryptodev_sym_session_init(dev_id=Y, sess, ...);
> > rte_cryptodev_sym_session_clear(dev_id=X, sess);
> >
> > After that point if X and Y uses the same driver,
> > then sess can't be used by device Y any more.
> > The reason for that - driver specific (not device specific)
> > data per session, plus there is no information
> > how many device instances use that data.
> > Probably the simplest way to deal with that issue -
> > add a reference counter per each driver data.
> [Fiona] Ok, I agree with this issue and proposed fix.
> We need to also document that it's user's responsibility
> not to call either init() or clear() twice on same device, as
> that would break the ref count.
I suppose it is obvious constrain, but sure, extra wording
can be put into the comments/docs, np with that.
> The same should be added to asym_session - though I accept
> it'sbe outside of the scope of this patch.
Agree on both - yes similar changes need to be done for asym,
and yes that patch targets sym session only.
>
>
> > 2.rte_cryptodev_sym_session_set_user_data() and
> > rte_cryptodev_sym_session_get_user_data() -
> > with current implementation there is no defined way for the user to
> > determine what is the max allowed size of the private data.
> > Even within rte_cryptodev_sym_session_set_user_data() we just blindly
> > copying user provided data without checking memory boundaries violation.
> > To overcome that issue I added 'uint16_t priv_size' into
> > rte_cryptodev_sym_session structure.
> [Fiona] I agree, this is needed.
> But I propose to call it user_data_sz NOT priv_size.
> See https://patches.dpdk.org/patch/42515/ to understand why.
Hmm that differs with mbuf naming scheme
(which I tried to follow here), but ok -
I don't have strong opinion here.
>
>
> > 3.rte_cryptodev_sym_session contains an array of variable size for
> > driver specific data.
> > Though number of elements in that array is determined by static
> > variable nb_drivers, that could be modified by
> > rte_cryptodev_allocate_driver().
> > That construction seems to work ok so far, as right now users register
> > all their PMDs at startup, though it doesn't mean that it would always
> > remain like that.
> > To make it less error prone I added 'uint16_t nb_drivers' into the
> > rte_cryptodev_sym_session structure.
> > At least that allows related functions to check that provided
> > driver id wouldn't overrun variable array boundaries,
> > again it allows to determine size of already allocated session
> > without accessing global variable.
> [Fiona] I agree with both issue and solution.
> The same should be added to asym_session - though again
> it's outside of the scope of this patch.
>
> > 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
> > would have sort of readonly type data (init once at allocation time,
> > keep unmodified through session life-time).
> > That requires more changes in current cryptodev implementation:
> > Right now inside cryptodev framework both rte_cryptodev_sym_session
> > and driver specific session data are two completely different sctrucures
> > (e.g. struct struct null_crypto_session and struct null_crypto_session).
> > Though current cryptodev implementation implicitly assumes that driver
> > will allocate both of them from within the same mempool.
> > Plus this is done in a manner that they override each other fields
> > (reuse the same space - sort of implicit C union).
> > That's probably not the best programming practice,
> > plus make impossible to have readonly fields inside both of them.
> > So to overcome that situation I changed an API a bit, to allow
> > to use two different mempools for these two distinct data structures.
> >
> > 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
> > I suppose that self-explanatory, and might be used in a lot of places
> > (would be quite useful for ipsec library we develop).
> [Fiona] Seems unnecessary - the set_user_data can be used. Why have 2
> separate user data spaces in the session? - it's confusing.
It allows quickly set/get external metadata associated with that session.
As an example - we plan to use it for pointer to ipsec SA associated
with given session.
Storing it inside priv_data section (user_data in your naming convention)
has several limitations:
- extra function call and extra memory dereference.
- each app would have to take into account that field when calculates size
for session mempool element.
Also note that inside one app could exist several session pools,
possibly with different layout for user private data,
unknown for generic libs.
Again, here I just used current mbuf approach:
userdata - (pointer to) some external metadata
(possibly temporally) associated with given mbuf.
priv_size (you suggest to call it user_data_sz) -
size of the application private data for given mbuf.
> If these is some good reason, then a different name should be used for clarity.
> Not private. And not user. Possibly opaque data.
Ok.
> Though afaik we had this in the op
> and removed it as it was felt appending user_data was enough.
>
> > So the new proposed layout for rte_cryptodev_sym_session:
> > struct rte_cryptodev_sym_session {
> > uint64_t userdata;
> > /**< Can be used for external metadata */
> > uint16_t nb_drivers;
> > /**< number of elements in sess_data array */
> > uint16_t priv_size;
> > /**< session private data will be placed after sess_data */
> > __extension__ struct {
> > void *data;
> > uint16_t refcnt;
> > } sess_data[0];
> > /**< Driver specific session material, variable size */
> > };
> >
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> > app/test-crypto-perf/cperf.h | 1 +
> > app/test-crypto-perf/cperf_ops.c | 11 +-
> > app/test-crypto-perf/cperf_ops.h | 2 +-
> > app/test-crypto-perf/cperf_test_latency.c | 5 +-
> > app/test-crypto-perf/cperf_test_latency.h | 1 +
> > app/test-crypto-perf/cperf_test_pmd_cyclecount.c | 5 +-
> > app/test-crypto-perf/cperf_test_pmd_cyclecount.h | 1 +
> > app/test-crypto-perf/cperf_test_throughput.c | 5 +-
> > app/test-crypto-perf/cperf_test_throughput.h | 1 +
> > app/test-crypto-perf/cperf_test_verify.c | 5 +-
> > app/test-crypto-perf/cperf_test_verify.h | 1 +
> > app/test-crypto-perf/main.c | 111 +++++++++++------
> > drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 10 +-
> > drivers/crypto/aesni_gcm/aesni_gcm_pmd_ops.c | 5 +-
> > drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 4 +-
> > drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 10 +-
> > drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 5 +-
> > drivers/crypto/aesni_mb/rte_aesni_mb_pmd_private.h | 4 +-
> > drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 3 +-
> > drivers/crypto/dpaa_sec/dpaa_sec.c | 3 +-
> > drivers/crypto/null/null_crypto_pmd.c | 14 ++-
> > drivers/crypto/null/null_crypto_pmd_ops.c | 5 +-
> > drivers/crypto/null/null_crypto_pmd_private.h | 4 +-
> > drivers/crypto/scheduler/scheduler_pmd_ops.c | 5 +-
> > drivers/crypto/virtio/virtio_cryptodev.c | 6 +-
> > examples/ipsec-secgw/ipsec-secgw.c | 116 ++++++++++++------
> > examples/ipsec-secgw/ipsec.h | 2 +
> > lib/librte_cryptodev/rte_cryptodev.c | 134 ++++++++++++---------
> > lib/librte_cryptodev/rte_cryptodev.h | 53 ++++++--
> > lib/librte_cryptodev/rte_cryptodev_pmd.h | 16 ++-
> > 30 files changed, 356 insertions(+), 192 deletions(-)
> >
> ///snip///
>
> > struct cnt_blk {
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> > index 63ae23f00..e25282445 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -943,8 +943,7 @@ rte_cryptodev_close(uint8_t dev_id)
> >
> > int
> > rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> > - const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
> > - struct rte_mempool *session_pool)
> > + const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
> >
> > {
> > struct rte_cryptodev *dev;
> > @@ -954,6 +953,12 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> > return -EINVAL;
> > }
> >
> > + if (qp_conf == NULL || qp_conf->sess_pool == NULL ||
> > + qp_conf->priv_sess_pool == NULL) {
> > + CDEV_LOG_ERR("Invalid queue_pair config");
> > + return -EINVAL;
> > + }
> > +
> > dev = &rte_crypto_devices[dev_id];
> > if (queue_pair_id >= dev->data->nb_queue_pairs) {
> > CDEV_LOG_ERR("Invalid queue_pair_id=%d", queue_pair_id);
> > @@ -969,7 +974,7 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_pair_setup, -ENOTSUP);
> >
> > return (*dev->dev_ops->queue_pair_setup)(dev, queue_pair_id, qp_conf,
> > - socket_id, session_pool);
> > + socket_id);
> > }
> >
> >
> > @@ -1146,6 +1151,41 @@ rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
> > rte_spinlock_unlock(&rte_cryptodev_cb_lock);
> > }
> >
> > +static void
> > +cryptodev_sym_session_init_elem(__rte_unused struct rte_mempool *pool,
> > + void *arg, void *obj, __rte_unused uint32_t idx)
> > +{
> > + struct rte_cryptodev_sym_session *ds;
> > + const struct rte_cryptodev_sym_session *ss;
> > +
> > + ds = obj;
> > + ss = arg;
> > +
> > + *ds = *ss;
> > + memset(ds->sess_data, 0, rte_cryptodev_sym_session_data_size(ds));
> > +}
> > +
> > +struct rte_mempool *
> > +rte_cryptodev_sym_session_pool_create(const char *name,
> > + uint32_t nb_elts, uint32_t cache_size, uint16_t priv_size,
> > + int socket_id)
> > +{
> > + struct rte_mempool *mp;
> > + uint32_t elt_size;
> > + struct rte_cryptodev_sym_session s = {
> > + .nb_drivers = nb_drivers,
> > + .priv_size = priv_size,
> > + };
> > +
> > + elt_size = rte_cryptodev_sym_session_max_size(priv_size);
> > + mp = rte_mempool_create(name, nb_elts, elt_size, cache_size, 0,
> > + NULL, NULL, cryptodev_sym_session_init_elem, &s,
> > + socket_id, 0);
> > + if (mp == NULL)
> > + CDEV_LOG_ERR("%s(name=%s) failed, rte_errno=%d\n",
> > + __func__, name, rte_errno);
> > + return mp;
> > +}
> >
> > int
> > rte_cryptodev_sym_session_init(uint8_t dev_id,
> > @@ -1163,12 +1203,15 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> > return -EINVAL;
> >
> > index = dev->driver_id;
> > + if (index > sess->nb_drivers)
> > + return -EINVAL;
> >
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_configure, -ENOTSUP);
> >
> > - if (sess->sess_private_data[index] == NULL) {
> > + if (sess->sess_data[index].refcnt == 0) {
> > ret = dev->dev_ops->sym_session_configure(dev, xforms,
> > - sess, mp);
> > + sess, mp);
> > +
> > if (ret < 0) {
> > CDEV_LOG_ERR(
> > "dev_id %d failed to configure session details",
> > @@ -1177,6 +1220,7 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
> > }
> > }
> >
> > + sess->sess_data[index].refcnt++;
> > return 0;
> > }
> >
> > @@ -1229,8 +1273,7 @@ rte_cryptodev_sym_session_create(struct rte_mempool *mp)
> > /* Clear device session pointer.
> > * Include the flag indicating presence of user data
> > */
> > - memset(sess, 0, (sizeof(void *) * nb_drivers) + sizeof(uint8_t));
> > -
> > + memset(sess->sess_data, 0, rte_cryptodev_sym_session_data_size(sess));
> > return sess;
> > }
> >
> > @@ -1258,16 +1301,20 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
> > struct rte_cryptodev_sym_session *sess)
> > {
> > struct rte_cryptodev *dev;
> > + uint32_t idx;
> >
> > dev = rte_cryptodev_pmd_get_dev(dev_id);
> >
> > - if (dev == NULL || sess == NULL)
> > + if (dev == NULL || sess == NULL || dev->driver_id > sess->nb_drivers)
> > return -EINVAL;
> >
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->sym_session_clear, -ENOTSUP);
> >
> > - dev->dev_ops->sym_session_clear(dev, sess);
> > + idx = dev->driver_id;
> > + if (--sess->sess_data[idx].refcnt != 0)
> > + return -EBUSY;
> >
> > + dev->dev_ops->sym_session_clear(dev, sess);
> > return 0;
> > }
> >
> > @@ -1285,7 +1332,6 @@ rte_cryptodev_asym_session_clear(uint8_t dev_id,
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->asym_session_clear, -ENOTSUP);
> >
> > dev->dev_ops->asym_session_clear(dev, sess);
> > -
> > return 0;
> > }
> >
> > @@ -1293,7 +1339,6 @@ int
> > rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > {
> > uint8_t i;
> > - void *sess_priv;
> > struct rte_mempool *sess_mp;
> >
> > if (sess == NULL)
> > @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> >
> > /* Check that all device private data has been freed */
> > for (i = 0; i < nb_drivers; i++) {
> [Fiona] Use the sess.nb_drivers rather than the global.
Ok, though doesn't really matter here.
get_sym_session_private_data() will return NULL for invalid index anyway.
> Actually maybe name slightly differently to reduce the chance of that mistake happening, e.g.
> rename sess.nb_drivers to sess.num_drivers?
>
> > - sess_priv = get_sym_session_private_data(sess, i);
> > - if (sess_priv != NULL)
> > + if (sess->sess_data[i].refcnt != 0)
> > return -EBUSY;
> > }
> >
> > @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > return 0;
> > }
> >
> > +unsigned int
> > +rte_cryptodev_sym_session_max_data_size(void)
> [Fiona] Suggest renaming this
> rte_cryptodev_sym_session_max_array_size()
I usually don't care much about names, but that seems just confusing:
totally unclear which array we are talking about.
Any better naming ideas?
>
> > +{
> > + struct rte_cryptodev_sym_session *sess = NULL;
> > +
> > + return (sizeof(sess->sess_data[0]) * nb_drivers);
> > +}
> > +
> > +size_t
> > +rte_cryptodev_sym_session_max_size(uint16_t priv_size)
> > +{
> > + struct rte_cryptodev_sym_session *sess = NULL;
> > +
> > + return (sizeof(*sess) + priv_size +
> > + rte_cryptodev_sym_session_max_data_size());
> > +}
> > +
> > int __rte_experimental
> > rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
> > {
> > @@ -1337,18 +1398,6 @@ rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess)
> > return 0;
> > }
> >
> > -
> > -unsigned int
> > -rte_cryptodev_sym_get_header_session_size(void)
> > -{
> > - /*
> > - * Header contains pointers to the private data
> > - * of all registered drivers, and a flag which
> > - * indicates presence of user data
> > - */
> > - return ((sizeof(void *) * nb_drivers) + sizeof(uint8_t));
> > -}
> > -
> > unsigned int __rte_experimental
> > rte_cryptodev_asym_get_header_session_size(void)
> > {
> > @@ -1361,11 +1410,9 @@ rte_cryptodev_asym_get_header_session_size(void)
> > }
> >
> > unsigned int
> > -rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
> > +rte_cryptodev_sym_private_session_size(uint8_t dev_id)
> > {
> > struct rte_cryptodev *dev;
> > - unsigned int header_size = sizeof(void *) * nb_drivers;
> > - unsigned int priv_sess_size;
> >
> > if (!rte_cryptodev_pmd_is_valid_dev(dev_id))
> > return 0;
> > @@ -1375,18 +1422,7 @@ rte_cryptodev_sym_get_private_session_size(uint8_t dev_id)
> > if (*dev->dev_ops->sym_session_get_size == NULL)
> > return 0;
> >
> > - priv_sess_size = (*dev->dev_ops->sym_session_get_size)(dev);
> > -
> > - /*
> > - * If size is less than session header size,
> > - * return the latter, as this guarantees that
> > - * sessionless operations will work
> > - */
> > - if (priv_sess_size < header_size)
> > - return header_size;
> > -
> > - return priv_sess_size;
> > -
> > + return (*dev->dev_ops->sym_session_get_size)(dev);
> > }
> >
> > unsigned int __rte_experimental
> > @@ -1409,7 +1445,6 @@ rte_cryptodev_asym_get_private_session_size(uint8_t dev_id)
> > return header_size;
> >
> > return priv_sess_size;
> > -
> > }
> >
> > int __rte_experimental
> > @@ -1418,15 +1453,10 @@ rte_cryptodev_sym_session_set_user_data(
> > void *data,
> > uint16_t size)
> > {
> > - uint16_t off_set = sizeof(void *) * nb_drivers;
> > - uint8_t *user_data_present = (uint8_t *)sess + off_set;
> > -
> > - if (sess == NULL)
> > + if (sess == NULL || sess->priv_size < size)
> > return -EINVAL;
> >
> > - *user_data_present = 1;
> > - off_set += sizeof(uint8_t);
> > - rte_memcpy((uint8_t *)sess + off_set, data, size);
> > + rte_memcpy(sess->sess_data + sess->nb_drivers, data, size);
> > return 0;
> > }
> >
> > @@ -1434,14 +1464,10 @@ void * __rte_experimental
> > rte_cryptodev_sym_session_get_user_data(
> > struct rte_cryptodev_sym_session *sess)
> > {
> > - uint16_t off_set = sizeof(void *) * nb_drivers;
> > - uint8_t *user_data_present = (uint8_t *)sess + off_set;
> > -
> > - if (sess == NULL || !*user_data_present)
> > + if (sess == NULL || sess->priv_size == 0)
> > return NULL;
> >
> > - off_set += sizeof(uint8_t);
> > - return (uint8_t *)sess + off_set;
> > + return (sess->sess_data + sess->nb_drivers);
> > }
> >
> > /** Initialise rte_crypto_op mempool element */
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
> > index 4099823f1..d88454f02 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.h
> > +++ b/lib/librte_cryptodev/rte_cryptodev.h
> > @@ -495,6 +495,14 @@ enum rte_cryptodev_event_type {
> > /** Crypto device queue pair configuration structure. */
> > struct rte_cryptodev_qp_conf {
> > uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
> > + struct rte_mempool *sess_pool;
> > + /**< Pointer to crypto sessions mempool,
> > + * used for session-less operations.
> > + */
> > + struct rte_mempool *priv_sess_pool;
> > + /**< Pointer to device specific sessions mempool,
> > + * used for session-less operations.
> > + */
> > };
> >
> > /**
> > @@ -680,17 +688,13 @@ rte_cryptodev_close(uint8_t dev_id);
> > * *SOCKET_ID_ANY* if there is no NUMA constraint
> > * for the DMA memory allocated for the receive
> > * queue pair.
> > - * @param session_pool Pointer to device session mempool, used
> > - * for session-less operations.
> > - *
> > * @return
> > * - 0: Success, queue pair correctly set up.
> > * - <0: Queue pair configuration failed
> > */
> > extern int
> > rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
> > - const struct rte_cryptodev_qp_conf *qp_conf, int socket_id,
> > - struct rte_mempool *session_pool);
> > + const struct rte_cryptodev_qp_conf *qp_conf, int socket_id);
> >
> > /**
> > * Get the number of queue pairs on a specific crypto device
> > @@ -954,10 +958,43 @@ rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t qp_id,
> > * has a fixed algo, key, op-type, digest_len etc.
> > */
> > struct rte_cryptodev_sym_session {
> > - __extension__ void *sess_private_data[0];
> > - /**< Private symmetric session material */
> > + uint64_t userdata;
> > + /**< Can be used for external metadata */
> > + uint16_t nb_drivers;
> > + /**< number of elements in sess_data array */
> > + uint16_t priv_size;
> > + /**< session private data will be placed after sess_data */
> > + __extension__ struct {
> > + void *data;
> > + uint16_t refcnt;
> > + } sess_data[0];
> > + /**< Driver specific session material, variable size */
> > };
> >
> > +static inline size_t
> > +rte_cryptodev_sym_session_data_size(const struct rte_cryptodev_sym_session *s)
> > +{
> > + return (sizeof(s->sess_data[0]) * s->nb_drivers);
> > +}
> > +
> > +static inline size_t
> > +rte_cryptodev_sym_session_size(const struct rte_cryptodev_sym_session *s)
> > +{
> > + return (sizeof(*s) + (s)->priv_size +
> > + rte_cryptodev_sym_session_data_size(s));
> > +}
> > +
> [Fiona] Are above 2 fns used?
> Look like duplicates of the "max" fns?
Not really: rte_cryptodev_sym_session_max_data_size() and
rte_cryptodev_sym_session_max_size() use global var nb_drivers.
Returns amount of space required to create a session that
can work with all attached at that moment drivers.
Planned to be used by in rte_cryptodev_sym_session_max_size().
While these 2 funcs return size of particular session object.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-13 18:56 ` Ananyev, Konstantin
@ 2018-11-14 0:46 ` Trahe, Fiona
2018-11-14 8:35 ` Ananyev, Konstantin
2018-11-14 10:14 ` Zhang, Roy Fan
0 siblings, 2 replies; 10+ messages in thread
From: Trahe, Fiona @ 2018-11-14 0:46 UTC (permalink / raw)
To: Ananyev, Konstantin, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou, Trahe, Fiona, Zhang,
Roy Fan
Hi Konstantin,
//snip///
> Can you also have a look at related deprecation note:
> http://patches.dpdk.org/patch/46633/
> and provide the feedback?
> Konstantin
[Fiona] will do
> > [Fiona] Ok, I agree with this issue and proposed fix.
> > We need to also document that it's user's responsibility
> > not to call either init() or clear() twice on same device, as
> > that would break the ref count.
>
> I suppose it is obvious constrain, but sure, extra wording
> can be put into the comments/docs, np with that.
[Fiona] ok
> > [Fiona] I agree, this is needed.
> > But I propose to call it user_data_sz NOT priv_size.
> > See https://patches.dpdk.org/patch/42515/ to understand why.
>
> Hmm that differs with mbuf naming scheme
> (which I tried to follow here), but ok -
> I don't have strong opinion here.
[Fiona] ok
> > > 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
> > > I suppose that self-explanatory, and might be used in a lot of places
> > > (would be quite useful for ipsec library we develop).
> > [Fiona] Seems unnecessary - the set_user_data can be used. Why have 2
> > separate user data spaces in the session? - it's confusing.
>
> It allows quickly set/get external metadata associated with that session.
> As an example - we plan to use it for pointer to ipsec SA associated
> with given session.
> Storing it inside priv_data section (user_data in your naming convention)
> has several limitations:
> - extra function call and extra memory dereference.
> - each app would have to take into account that field when calculates size
> for session mempool element.
> Also note that inside one app could exist several session pools,
> possibly with different layout for user private data,
> unknown for generic libs.
>
> Again, here I just used current mbuf approach:
> userdata - (pointer to) some external metadata
> (possibly temporally) associated with given mbuf.
> priv_size (you suggest to call it user_data_sz) -
> size of the application private data for given mbuf.
>
> > If these is some good reason, then a different name should be used for clarity.
> > Not private. And not user. Possibly opaque data.
>
> Ok.
[Fiona] ok, let's go with opaque data so.
The naming problem arises only because the PMD data in the
session is referred to as private data already in various APIs, so when user data got added it
didn't make sense to also call it private data, so we named it user-data - so not consistent
with mbuf.
> > > @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > >
> > > /* Check that all device private data has been freed */
> > > for (i = 0; i < nb_drivers; i++) {
> > [Fiona] Use the sess.nb_drivers rather than the global.
>
> Ok, though doesn't really matter here.
> get_sym_session_private_data() will return NULL for invalid index anyway.
[Fiona] Except that you removed the NULL check below and are using that
invalid index to deref the array.
> > Actually maybe name slightly differently to reduce the chance of that mistake happening, e.g.
> > rename sess.nb_drivers to sess.num_drivers?
> >
> > > - sess_priv = get_sym_session_private_data(sess, i);
> > > - if (sess_priv != NULL)
> > > + if (sess->sess_data[i].refcnt != 0)
> > > return -EBUSY;
> > > }
> > >
> > > @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > > return 0;
> > > }
> > >
> > > +unsigned int
> > > +rte_cryptodev_sym_session_max_data_size(void)
> > [Fiona] Suggest renaming this
> > rte_cryptodev_sym_session_max_array_size()
>
> I usually don't care much about names, but that seems just confusing:
> totally unclear which array we are talking about.
> Any better naming ideas?
[Fiona] Isn't it returning the size of the array of structs in the session hdr?
Seems a bit more informative than "data".
Can't think of anything better, if you find array confusing then I
don't feel that strongly about it, stick with data.
Or just get rid of the function altogether and put inside
rte_cryptodev_sym_session_max_size()
Unless it's called elsewhere.
Same applies to the other _data_size fn below.
>
> >
> > > +{
> > > + struct rte_cryptodev_sym_session *sess = NULL;
> > > +
> > > + return (sizeof(sess->sess_data[0]) * nb_drivers);
> > > +}
> > > +
> > > +size_t
> > > +rte_cryptodev_sym_session_max_size(uint16_t priv_size)
> > > +{
> > > + struct rte_cryptodev_sym_session *sess = NULL;
> > > +
> > > + return (sizeof(*sess) + priv_size +
> > > + rte_cryptodev_sym_session_max_data_size());
> > > +}
> > > +
> > >
> > > +static inline size_t
> > > +rte_cryptodev_sym_session_data_size(const struct rte_cryptodev_sym_session *s)
> > > +{
> > > + return (sizeof(s->sess_data[0]) * s->nb_drivers);
> > > +}
> > > +
> > > +static inline size_t
> > > +rte_cryptodev_sym_session_size(const struct rte_cryptodev_sym_session *s)
> > > +{
> > > + return (sizeof(*s) + (s)->priv_size +
> > > + rte_cryptodev_sym_session_data_size(s));
> > > +}
> > > +
> > [Fiona] Are above 2 fns used?
> > Look like duplicates of the "max" fns?
>
> Not really: rte_cryptodev_sym_session_max_data_size() and
> rte_cryptodev_sym_session_max_size() use global var nb_drivers.
> Returns amount of space required to create a session that
> can work with all attached at that moment drivers.
> Planned to be used by in rte_cryptodev_sym_session_max_size().
> While these 2 funcs return size of particular session object.
[Fiona] ok
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-14 0:46 ` Trahe, Fiona
@ 2018-11-14 8:35 ` Ananyev, Konstantin
2018-11-14 10:14 ` Zhang, Roy Fan
1 sibling, 0 replies; 10+ messages in thread
From: Ananyev, Konstantin @ 2018-11-14 8:35 UTC (permalink / raw)
To: Trahe, Fiona, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Zhang, Roy Fan, Tomasz Duszynski, Hemant Agrawal,
Natalie Samsonov, Dmitri Epshtein, Jay Zhou, Zhang, Roy Fan
> > > > @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > > >
> > > > /* Check that all device private data has been freed */
> > > > for (i = 0; i < nb_drivers; i++) {
> > > [Fiona] Use the sess.nb_drivers rather than the global.
> >
> > Ok, though doesn't really matter here.
> > get_sym_session_private_data() will return NULL for invalid index anyway.
> [Fiona] Except that you removed the NULL check below and are using that
> invalid index to deref the array.
Ah yes, you right - have to use sess.nb_drivers here.
>
> > > Actually maybe name slightly differently to reduce the chance of that mistake happening, e.g.
> > > rename sess.nb_drivers to sess.num_drivers?
> > >
> > > > - sess_priv = get_sym_session_private_data(sess, i);
> > > > - if (sess_priv != NULL)
> > > > + if (sess->sess_data[i].refcnt != 0)
> > > > return -EBUSY;
> > > > }
> > > >
> > > > @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > > > return 0;
> > > > }
> > > >
> > > > +unsigned int
> > > > +rte_cryptodev_sym_session_max_data_size(void)
> > > [Fiona] Suggest renaming this
> > > rte_cryptodev_sym_session_max_array_size()
> >
> > I usually don't care much about names, but that seems just confusing:
> > totally unclear which array we are talking about.
> > Any better naming ideas?
> [Fiona] Isn't it returning the size of the array of structs in the session hdr?
> Seems a bit more informative than "data".
Yes, but user doesn't have to know it is an array of pointers or
something different.
> Can't think of anything better, if you find array confusing then I
> don't feel that strongly about it, stick with data.
> Or just get rid of the function altogether and put inside
> rte_cryptodev_sym_session_max_size()
> Unless it's called elsewhere.
Sounds like a good option.
Konstantin
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session
2018-11-14 0:46 ` Trahe, Fiona
2018-11-14 8:35 ` Ananyev, Konstantin
@ 2018-11-14 10:14 ` Zhang, Roy Fan
1 sibling, 0 replies; 10+ messages in thread
From: Zhang, Roy Fan @ 2018-11-14 10:14 UTC (permalink / raw)
To: Trahe, Fiona, Ananyev, Konstantin, dev
Cc: De Lara Guarch, Pablo, Akhil Goyal, Doherty, Declan, Ravi Kumar,
Jerin Jacob, Tomasz Duszynski, Hemant Agrawal, Natalie Samsonov,
Dmitri Epshtein, Jay Zhou
Hi Fiona,
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Wednesday, November 14, 2018 12:46 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; Ravi
> Kumar <ravi1.kumar@amd.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Tomasz Duszynski <tdu@semihalf.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Natalie Samsonov
> <nsamsono@marvell.com>; Dmitri Epshtein <dima@marvell.com>; Jay Zhou
> <jianjay.zhou@huawei.com>; Trahe, Fiona <fiona.trahe@intel.com>; Zhang,
> Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [RFC] cryptodev: proposed changes in
> rte_cryptodev_sym_session
>
> Hi Konstantin,
>
>
> //snip///
> > Can you also have a look at related deprecation note:
> > http://patches.dpdk.org/patch/46633/
> > and provide the feedback?
> > Konstantin
> [Fiona] will do
>
>
> > > [Fiona] Ok, I agree with this issue and proposed fix.
> > > We need to also document that it's user's responsibility not to call
> > > either init() or clear() twice on same device, as that would break
> > > the ref count.
> >
> > I suppose it is obvious constrain, but sure, extra wording can be put
> > into the comments/docs, np with that.
> [Fiona] ok
>
> > > [Fiona] I agree, this is needed.
> > > But I propose to call it user_data_sz NOT priv_size.
> > > See https://patches.dpdk.org/patch/42515/ to understand why.
> >
> > Hmm that differs with mbuf naming scheme (which I tried to follow
> > here), but ok - I don't have strong opinion here.
> [Fiona] ok
>
> > > > 5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
> > > > I suppose that self-explanatory, and might be used in a lot of places
> > > > (would be quite useful for ipsec library we develop).
> > > [Fiona] Seems unnecessary - the set_user_data can be used. Why have 2
> > > separate user data spaces in the session? - it's confusing.
> >
> > It allows quickly set/get external metadata associated with that session.
> > As an example - we plan to use it for pointer to ipsec SA associated
> > with given session.
> > Storing it inside priv_data section (user_data in your naming convention)
> > has several limitations:
> > - extra function call and extra memory dereference.
> > - each app would have to take into account that field when calculates size
> > for session mempool element.
> > Also note that inside one app could exist several session pools,
> > possibly with different layout for user private data,
> > unknown for generic libs.
> >
> > Again, here I just used current mbuf approach:
> > userdata - (pointer to) some external metadata
> > (possibly temporally) associated with given mbuf.
> > priv_size (you suggest to call it user_data_sz) -
> > size of the application private data for given mbuf.
> >
> > > If these is some good reason, then a different name should be used for
> clarity.
> > > Not private. And not user. Possibly opaque data.
> >
> > Ok.
> [Fiona] ok, let's go with opaque data so.
> The naming problem arises only because the PMD data in the
> session is referred to as private data already in various APIs, so when user
> data got added it
> didn't make sense to also call it private data, so we named it user-data - so
> not consistent
> with mbuf.
>
>
> > > > @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct
> rte_cryptodev_sym_session *sess)
> > > >
> > > > /* Check that all device private data has been freed */
> > > > for (i = 0; i < nb_drivers; i++) {
> > > [Fiona] Use the sess.nb_drivers rather than the global.
> >
> > Ok, though doesn't really matter here.
> > get_sym_session_private_data() will return NULL for invalid index anyway.
> [Fiona] Except that you removed the NULL check below and are using that
> invalid index to deref the array.
>
> > > Actually maybe name slightly differently to reduce the chance of that
> mistake happening, e.g.
> > > rename sess.nb_drivers to sess.num_drivers?
> > >
> > > > - sess_priv = get_sym_session_private_data(sess, i);
> > > > - if (sess_priv != NULL)
> > > > + if (sess->sess_data[i].refcnt != 0)
> > > > return -EBUSY;
> > > > }
> > > >
> > > > @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct
> rte_cryptodev_sym_session *sess)
> > > > return 0;
> > > > }
> > > >
> > > > +unsigned int
> > > > +rte_cryptodev_sym_session_max_data_size(void)
> > > [Fiona] Suggest renaming this
> > > rte_cryptodev_sym_session_max_array_size()
> >
> > I usually don't care much about names, but that seems just confusing:
> > totally unclear which array we are talking about.
> > Any better naming ideas?
> [Fiona] Isn't it returning the size of the array of structs in the session hdr?
[Fan] I don't think this external API is needed as we already have
rte_cryptodev_sym_get_header_session_size(void). There is no point to
create an API to do exactly same thing.
However, to make the konstantin's idea works this API needs to
have an extra parameter "struct rte_cryptodev_sym_session *sess"
To use this parameter, the "sess" can either be NULL or a valid session pointer,
If it is NULL, the function call returns the current header size WITHOUT the private
data length. This is reasonable as it is user who knows the size of the private data
he wants to put in.
If it is not NULL, the function call returns the calculated header size based on
The sess's private data size and nb_drivers information. Here is the suggested
Code sample:
unsigned int
rte_cryptodev_sym_get_header_session_size(
struct rte_cryptodev_sym_session *sess)
{
if (!sess) {
struct rte_cryptodev_sym_session s = {0};
s.nb_drivers = nb_drivers;
s.priv_size = 0;
return (unsigned int)(sizeof(s) + SESSION_DATA_SIZE(&s);
} else
return (unsigned int)(sizeof(*sess) + SESSION_DATA_SIZE(sess);
}
> Seems a bit more informative than "data".
> Can't think of anything better, if you find array confusing then I
> don't feel that strongly about it, stick with data.
> Or just get rid of the function altogether and put inside
> rte_cryptodev_sym_session_max_size()
> Unless it's called elsewhere.
> Same applies to the other _data_size fn below.
>
> >
> > >
> > > > +{
> > > > + struct rte_cryptodev_sym_session *sess = NULL;
> > > > +
> > > > + return (sizeof(sess->sess_data[0]) * nb_drivers);
> > > > +}
> > > > +
> > > > +size_t
> > > > +rte_cryptodev_sym_session_max_size(uint16_t priv_size)
> > > > +{
> > > > + struct rte_cryptodev_sym_session *sess = NULL;
> > > > +
> > > > + return (sizeof(*sess) + priv_size +
> > > > + rte_cryptodev_sym_session_max_data_size());
> > > > +}
> > > > +
>
>
>
> > > >
> > > > +static inline size_t
> > > > +rte_cryptodev_sym_session_data_size(const struct
> rte_cryptodev_sym_session *s)
> > > > +{
> > > > + return (sizeof(s->sess_data[0]) * s->nb_drivers);
> > > > +}
> > > > +
> > > > +static inline size_t
> > > > +rte_cryptodev_sym_session_size(const struct
> rte_cryptodev_sym_session *s)
> > > > +{
> > > > + return (sizeof(*s) + (s)->priv_size +
> > > > + rte_cryptodev_sym_session_data_size(s));
> > > > +}
> > > > +
> > > [Fiona] Are above 2 fns used?
> > > Look like duplicates of the "max" fns?
> >
> > Not really: rte_cryptodev_sym_session_max_data_size() and
> > rte_cryptodev_sym_session_max_size() use global var nb_drivers.
> > Returns amount of space required to create a session that
> > can work with all attached at that moment drivers.
> > Planned to be used by in rte_cryptodev_sym_session_max_size().
> > While these 2 funcs return size of particular session object.
> [Fiona] ok
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-11-14 10:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-24 17:48 [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session Konstantin Ananyev
2018-10-05 11:05 ` Ananyev, Konstantin
2018-11-12 21:01 ` Trahe, Fiona
2018-11-12 23:16 ` Trahe, Fiona
2018-11-12 23:24 ` Trahe, Fiona
2018-11-13 18:56 ` Ananyev, Konstantin
2018-11-13 18:56 ` Ananyev, Konstantin
2018-11-14 0:46 ` Trahe, Fiona
2018-11-14 8:35 ` Ananyev, Konstantin
2018-11-14 10:14 ` Zhang, Roy Fan
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).