[v2] security: update session create API

  The API ``rte_security_session_create`` takes only single
mempool for session and session private data. So the
application need to create mempool for twice the number of
sessions needed and will also lead to wastage of memory as
session private data need more memory compared to session.
Hence the API is modified to take two mempool pointers
- one for session and one for private data.
This is very similar to crypto based session create APIs.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---

Changes in V2:
incorporated comments from Lukasz and David.

 app/test-crypto-perf/cperf_ops.c       |  4 +-
 app/test-crypto-perf/main.c            | 12 +++--
 app/test/test_cryptodev.c              | 18 ++++++--
 app/test/test_ipsec.c                  |  3 +-
 app/test/test_security.c               | 61 ++++++++++++++++++++------
 doc/guides/prog_guide/rte_security.rst |  8 +++-
 doc/guides/rel_notes/deprecation.rst   |  7 ---
 doc/guides/rel_notes/release_20_11.rst |  6 +++
 examples/ipsec-secgw/ipsec-secgw.c     | 12 +----
 examples/ipsec-secgw/ipsec.c           |  9 ++--
 lib/librte_security/rte_security.c     |  7 ++-
 lib/librte_security/rte_security.h     |  4 +-
 12 files changed, 102 insertions(+), 49 deletions(-)
  

> 
> The API ``rte_security_session_create`` takes only single
> mempool for session and session private data. So the
> application need to create mempool for twice the number of
> sessions needed and will also lead to wastage of memory as
> session private data need more memory compared to session.
> Hence the API is modified to take two mempool pointers
> - one for session and one for private data.
> This is very similar to crypto based session create APIs.
> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
> 
> Changes in V2:
> incorporated comments from Lukasz and David.
> 
Hi Lukasz/David,

If no further comments, could you please ack the patch?

Thanks,
Akhil
  

Hi Akhil,

comments inline

W dniu 11.10.2020 o 00:11, Akhil Goyal pisze:
> The API ``rte_security_session_create`` takes only single
> mempool for session and session private data. So the
> application need to create mempool for twice the number of
> sessions needed and will also lead to wastage of memory as
> session private data need more memory compared to session.
> Hence the API is modified to take two mempool pointers
> - one for session and one for private data.
> This is very similar to crypto based session create APIs.
>
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>
> Changes in V2:
> incorporated comments from Lukasz and David.
>
>   app/test-crypto-perf/cperf_ops.c       |  4 +-
>   app/test-crypto-perf/main.c            | 12 +++--
>   app/test/test_cryptodev.c              | 18 ++++++--
>   app/test/test_ipsec.c                  |  3 +-
>   app/test/test_security.c               | 61 ++++++++++++++++++++------
>   doc/guides/prog_guide/rte_security.rst |  8 +++-
>   doc/guides/rel_notes/deprecation.rst   |  7 ---
>   doc/guides/rel_notes/release_20_11.rst |  6 +++
>   examples/ipsec-secgw/ipsec-secgw.c     | 12 +----
>   examples/ipsec-secgw/ipsec.c           |  9 ++--
>   lib/librte_security/rte_security.c     |  7 ++-
>   lib/librte_security/rte_security.h     |  4 +-
>   12 files changed, 102 insertions(+), 49 deletions(-)
>
> diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
> index 3da835a9c..3a64a2c34 100644
> --- a/app/test-crypto-perf/cperf_ops.c
> +++ b/app/test-crypto-perf/cperf_ops.c
> @@ -621,7 +621,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
>   
>   		/* Create security session */
>   		return (void *)rte_security_session_create(ctx,
> -					&sess_conf, sess_mp);
> +					&sess_conf, sess_mp, priv_mp);
>   	}
>   	if (options->op_type == CPERF_DOCSIS) {
>   		enum rte_security_docsis_direction direction;
> @@ -664,7 +664,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
>   
>   		/* Create security session */
>   		return (void *)rte_security_session_create(ctx,
> -					&sess_conf, priv_mp);
> +					&sess_conf, sess_mp, priv_mp);
>   	}
>   #endif
>   	sess = rte_cryptodev_sym_session_create(sess_mp);
> diff --git a/app/test-crypto-perf/main.c b/app/test-crypto-perf/main.c
> index 62ae6048b..53864ffdd 100644
> --- a/app/test-crypto-perf/main.c
> +++ b/app/test-crypto-perf/main.c
> @@ -156,7 +156,14 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs)
>   		if (sess_size > max_sess_size)
>   			max_sess_size = sess_size;
>   	}
> -
> +#ifdef RTE_LIBRTE_SECURITY
> +	for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
> +		sess_size = rte_security_session_get_size(
> +				rte_cryptodev_get_sec_ctx(cdev_id));
> +		if (sess_size > max_sess_size)
> +			max_sess_size = sess_size;
> +	}
> +#endif
>   	/*
>   	 * Calculate number of needed queue pairs, based on the amount
>   	 * of available number of logical cores and crypto devices.
> @@ -247,8 +254,7 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs)
>   				opts->nb_qps * nb_slaves;
>   #endif
>   		} else
> -			sessions_needed = enabled_cdev_count *
> -						opts->nb_qps * 2;
> +			sessions_needed = enabled_cdev_count * opts->nb_qps;
>   
>   		/*
>   		 * A single session is required per queue pair
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> index ac2a36bc2..4bd9d8aff 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -553,9 +553,15 @@ testsuite_setup(void)
>   	unsigned int session_size =
>   		rte_cryptodev_sym_get_private_session_size(dev_id);
>   
> +#ifdef RTE_LIBRTE_SECURITY
> +	unsigned int security_session_size = rte_security_session_get_size(
> +			rte_cryptodev_get_sec_ctx(dev_id));
> +
> +	if (session_size < security_session_size)
> +			session_size = security_session_size;
> +#endif
>   	/*
> -	 * Create mempool with maximum number of sessions * 2,
> -	 * to include the session headers
> +	 * Create mempool with maximum number of sessions.
>   	 */
>   	if (info.sym.max_nb_sessions != 0 &&
>   			info.sym.max_nb_sessions < MAX_NB_SESSIONS) {
> @@ -7219,7 +7225,8 @@ test_pdcp_proto(int i, int oop,
>   
>   	/* Create security session */
>   	ut_params->sec_session = rte_security_session_create(ctx,
> -				&sess_conf, ts_params->session_priv_mpool);
> +				&sess_conf, ts_params->session_mpool,
> +				ts_params->session_priv_mpool);
>   
>   	if (!ut_params->sec_session) {
>   		printf("TestCase %s()-%d line %d failed %s: ",
> @@ -7479,7 +7486,8 @@ test_pdcp_proto_SGL(int i, int oop,
>   
>   	/* Create security session */
>   	ut_params->sec_session = rte_security_session_create(ctx,
> -				&sess_conf, ts_params->session_priv_mpool);
> +				&sess_conf, ts_params->session_mpool,
> +				ts_params->session_priv_mpool);
>   
>   	if (!ut_params->sec_session) {
>   		printf("TestCase %s()-%d line %d failed %s: ",
> @@ -7836,6 +7844,7 @@ test_docsis_proto_uplink(int i, struct docsis_test_data *d_td)
>   
>   	/* Create security session */
>   	ut_params->sec_session = rte_security_session_create(ctx, &sess_conf,
> +					ts_params->session_mpool,
>   					ts_params->session_priv_mpool);
>   
>   	if (!ut_params->sec_session) {
> @@ -8011,6 +8020,7 @@ test_docsis_proto_downlink(int i, struct docsis_test_data *d_td)
>   
>   	/* Create security session */
>   	ut_params->sec_session = rte_security_session_create(ctx, &sess_conf,
> +					ts_params->session_mpool,
>   					ts_params->session_priv_mpool);
>   
>   	if (!ut_params->sec_session) {
> diff --git a/app/test/test_ipsec.c b/app/test/test_ipsec.c
> index 79d00d7e0..9ad07a179 100644
> --- a/app/test/test_ipsec.c
> +++ b/app/test/test_ipsec.c
> @@ -632,7 +632,8 @@ create_dummy_sec_session(struct ipsec_unitest_params *ut,
>   	static struct rte_security_session_conf conf;
>   
>   	ut->ss[j].security.ses = rte_security_session_create(&dummy_sec_ctx,
> -					&conf, qp->mp_session_private);
> +					&conf, qp->mp_session,
> +					qp->mp_session_private);
>   
>   	if (ut->ss[j].security.ses == NULL)
>   		return -ENOMEM;
> diff --git a/app/test/test_security.c b/app/test/test_security.c
> index 77fd5adc6..bf6a3e9de 100644
> --- a/app/test/test_security.c
> +++ b/app/test/test_security.c
> @@ -237,24 +237,25 @@ static struct mock_session_create_data {
>   	struct rte_security_session_conf *conf;
>   	struct rte_security_session *sess;
>   	struct rte_mempool *mp;
> +	struct rte_mempool *priv_mp;
>   
>   	int ret;
>   
>   	int called;
>   	int failed;
> -} mock_session_create_exp = {NULL, NULL, NULL, NULL, 0, 0, 0};
> +} mock_session_create_exp = {NULL, NULL, NULL, NULL, NULL, 0, 0, 0};
>   
>   static int
>   mock_session_create(void *device,
>   		struct rte_security_session_conf *conf,
>   		struct rte_security_session *sess,
> -		struct rte_mempool *mp)
> +		struct rte_mempool *priv_mp)
>   {
>   	mock_session_create_exp.called++;
>   
>   	MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, device);
>   	MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, conf);
> -	MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, mp);
> +	MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, priv_mp);
>   
>   	mock_session_create_exp.sess = sess;
>   
> @@ -502,6 +503,7 @@ struct rte_security_ops mock_ops = {
>    */
>   static struct security_testsuite_params {
>   	struct rte_mempool *session_mpool;
> +	struct rte_mempool *session_priv_mpool;
>   } testsuite_params = { NULL };
>   
>   /**
> @@ -524,7 +526,8 @@ static struct security_unittest_params {
>   	.sess = NULL,
>   };
>   
> -#define SECURITY_TEST_MEMPOOL_NAME "SecurityTestsMempoolName"
> +#define SECURITY_TEST_MEMPOOL_NAME "SecurityTestMp"
> +#define SECURITY_TEST_PRIV_MEMPOOL_NAME "SecurityTestPrivMp"
>   #define SECURITY_TEST_MEMPOOL_SIZE 15
>   #define SECURITY_TEST_SESSION_OBJECT_SIZE sizeof(struct rte_security_session)
>   
> @@ -545,6 +548,22 @@ testsuite_setup(void)
>   			SOCKET_ID_ANY, 0);
>   	TEST_ASSERT_NOT_NULL(ts_params->session_mpool,
>   			"Cannot create mempool %s\n", rte_strerror(rte_errno));
> +
> +	ts_params->session_priv_mpool = rte_mempool_create(
> +			SECURITY_TEST_PRIV_MEMPOOL_NAME,
> +			SECURITY_TEST_MEMPOOL_SIZE,
> +			rte_security_session_get_size(&unittest_params.ctx),
Call to rte_security_session_get_size() will cause a mockup function 
mock_session_get_size() to be called, which will return 0.
Why do you call this function instead of defining some value for private 
mempool element size?
> +			0, 0, NULL, NULL, NULL, NULL,
> +			SOCKET_ID_ANY, 0);
> +	if (ts_params->session_priv_mpool == NULL) {
> +		printf("TestCase %s() line %d failed (null): "
> +				"Cannot create priv mempool %s\n",
> +				__func__, __LINE__, rte_strerror(rte_errno));
Instead of printf() use RTE_LOG(ERR, EAL,...). All other messages are 
printed this way. It allows control of error messages if required.
> +		rte_mempool_free(ts_params->session_mpool);
> +		ts_params->session_mpool = NULL;
> +		return TEST_FAILED;
> +	}
> +
>   	return TEST_SUCCESS;
>   }
>   
> @@ -559,6 +578,10 @@ testsuite_teardown(void)
>   		rte_mempool_free(ts_params->session_mpool);
>   		ts_params->session_mpool = NULL;
>   	}
> +	if (ts_params->session_priv_mpool) {
> +		rte_mempool_free(ts_params->session_priv_mpool);
> +		ts_params->session_priv_mpool = NULL;
> +	}
>   }
>   
>   /**
> @@ -659,7 +682,8 @@ ut_setup_with_session(void)
>   	mock_session_create_exp.ret = 0;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_NOT_NULL(rte_security_session_create,
>   			sess);
>   	TEST_ASSERT_EQUAL(sess, mock_session_create_exp.sess,
> @@ -701,7 +725,8 @@ test_session_create_inv_context(void)
>   	struct rte_security_session *sess;
>   
>   	sess = rte_security_session_create(NULL, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -725,7 +750,8 @@ test_session_create_inv_context_ops(void)
>   	ut_params->ctx.ops = NULL;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -749,7 +775,8 @@ test_session_create_inv_context_ops_fun(void)
>   	ut_params->ctx.ops = &empty_ops;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -770,7 +797,8 @@ test_session_create_inv_configuration(void)
>   	struct rte_security_session *sess;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, NULL,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -781,7 +809,7 @@ test_session_create_inv_configuration(void)
>   }
>   
>   /**
> - * Test execution of rte_security_session_create with NULL mp parameter
> + * Test execution of rte_security_session_create with NULL mempools
>    */
>   static int
>   test_session_create_inv_mempool(void)
> @@ -790,7 +818,7 @@ test_session_create_inv_mempool(void)
>   	struct rte_security_session *sess;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			NULL);
> +			NULL, NULL);
It would be best to add a new testcase for verification of passing NULL 
private mempool.
If you pass NULL as the primary mempool as in this testcase, the 
verification of priv mempool (rte_securitry.c:37) won't ever happen 
because rte_security_session_create() will return in line 36.
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -824,7 +852,8 @@ test_session_create_mempool_empty(void)
>   	TEST_ASSERT_MEMPOOL_USAGE(SECURITY_TEST_MEMPOOL_SIZE);
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -853,10 +882,12 @@ test_session_create_ops_failure(void)
>   	mock_session_create_exp.device = NULL;
>   	mock_session_create_exp.conf = &ut_params->conf;
>   	mock_session_create_exp.mp = ts_params->session_mpool;
> +	mock_session_create_exp.priv_mp = ts_params->session_priv_mpool;
>   	mock_session_create_exp.ret = -1;	/* Return failure status. */
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 1);
> @@ -879,10 +910,12 @@ test_session_create_success(void)
>   	mock_session_create_exp.device = NULL;
>   	mock_session_create_exp.conf = &ut_params->conf;
>   	mock_session_create_exp.mp = ts_params->session_mpool;
> +	mock_session_create_exp.priv_mp = ts_params->session_priv_mpool;
>   	mock_session_create_exp.ret = 0;	/* Return success status. */
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_NOT_NULL(rte_security_session_create,
>   			sess);
>   	TEST_ASSERT_EQUAL(sess, mock_session_create_exp.sess,
> diff --git a/doc/guides/prog_guide/rte_security.rst b/doc/guides/prog_guide/rte_security.rst
> index 127da2e4f..fdb469d5f 100644
> --- a/doc/guides/prog_guide/rte_security.rst
> +++ b/doc/guides/prog_guide/rte_security.rst
> @@ -533,8 +533,12 @@ and this allows further acceleration of the offload of Crypto workloads.
>   
>   The Security framework provides APIs to create and free sessions for crypto/ethernet
>   devices, where sessions are mempool objects. It is the application's responsibility
> -to create and manage the session mempools. The mempool object size should be able to
> -accommodate the driver's private data of security session.
> +to create and manage two session mempools - one for session and other for session
> +private data. The private session data mempool object size should be able to
> +accommodate the driver's private data of security session. The application can get
> +the size of session private data using API ``rte_security_session_get_size``.
> +And the session mempool object size should be enough to accomodate
> +``rte_security_session``.
>   
>   Once the session mempools have been created, ``rte_security_session_create()``
>   is used to allocate and initialize a session for the required crypto/ethernet device.
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 52f413e21..d956a76e7 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -164,13 +164,6 @@ Deprecation Notices
>     following the IPv6 header, as proposed in RFC
>     https://protect2.fireeye.com/v1/url?k=6c464261-31d8d98b-6c47c92e-0cc47a6cba04-7bff9381095a3baf&q=1&e=1d514a47-c7a4-4d29-b9af-8fe8863e27eb&u=https%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F2020-August%2F177257.html.
>   
> -* security: The API ``rte_security_session_create`` takes only single mempool
> -  for session and session private data. So the application need to create
> -  mempool for twice the number of sessions needed and will also lead to
> -  wastage of memory as session private data need more memory compared to session.
> -  Hence the API will be modified to take two mempool pointers - one for session
> -  and one for private data.
> -
>   * cryptodev: ``RTE_CRYPTO_AEAD_LIST_END`` from ``enum rte_crypto_aead_algorithm``,
>     ``RTE_CRYPTO_CIPHER_LIST_END`` from ``enum rte_crypto_cipher_algorithm`` and
>     ``RTE_CRYPTO_AUTH_LIST_END`` from ``enum rte_crypto_auth_algorithm``
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index c34ab5493..68b82ae4e 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -307,6 +307,12 @@ API Changes
>     ``rte_fpga_lte_fec_configure`` and structure ``fpga_lte_fec_conf`` to
>     ``rte_fpga_lte_fec_conf``.
>   
> +* security: The API ``rte_security_session_create`` is updated to take two
> +  mempool objects one for session and other for session private data.
> +  So the application need to create two mempools and get the size of session
> +  private data using API ``rte_security_session_get_size`` for private session
> +  mempool.
> +
>   
>   ABI Changes
>   -----------
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index 60132c4bd..2326089bb 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -2348,12 +2348,8 @@ session_pool_init(struct socket_ctx *ctx, int32_t socket_id, size_t sess_sz)
>   
>   	snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
>   			"sess_mp_%u", socket_id);
> -	/*
> -	 * Doubled due to rte_security_session_create() uses one mempool for
> -	 * session and for session private data.
> -	 */
>   	nb_sess = (get_nb_crypto_sessions() + CDEV_MP_CACHE_SZ *
> -		rte_lcore_count()) * 2;
> +		rte_lcore_count());
>   	sess_mp = rte_cryptodev_sym_session_pool_create(
>   			mp_name, nb_sess, sess_sz, CDEV_MP_CACHE_SZ, 0,
>   			socket_id);
> @@ -2376,12 +2372,8 @@ session_priv_pool_init(struct socket_ctx *ctx, int32_t socket_id,
>   
>   	snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
>   			"sess_mp_priv_%u", socket_id);
> -	/*
> -	 * Doubled due to rte_security_session_create() uses one mempool for
> -	 * session and for session private data.
> -	 */
>   	nb_sess = (get_nb_crypto_sessions() + CDEV_MP_CACHE_SZ *
> -		rte_lcore_count()) * 2;
> +		rte_lcore_count());
>   	sess_mp = rte_mempool_create(mp_name,
>   			nb_sess,
>   			sess_sz,
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index 01faa7ac7..6baeeb342 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -117,7 +117,8 @@ create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa,
>   			set_ipsec_conf(sa, &(sess_conf.ipsec));
>   
>   			ips->security.ses = rte_security_session_create(ctx,
> -					&sess_conf, ipsec_ctx->session_priv_pool);
> +					&sess_conf, ipsec_ctx->session_pool,
> +					ipsec_ctx->session_priv_pool);
>   			if (ips->security.ses == NULL) {
>   				RTE_LOG(ERR, IPSEC,
>   				"SEC Session init failed: err: %d\n", ret);
> @@ -198,7 +199,8 @@ create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
>   		}
>   
>   		ips->security.ses = rte_security_session_create(sec_ctx,
> -				&sess_conf, skt_ctx->session_pool);
> +				&sess_conf, skt_ctx->session_pool,
> +				skt_ctx->session_priv_pool);
>   		if (ips->security.ses == NULL) {
>   			RTE_LOG(ERR, IPSEC,
>   				"SEC Session init failed: err: %d\n", ret);
> @@ -378,7 +380,8 @@ create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
>   		sess_conf.userdata = (void *) sa;
>   
>   		ips->security.ses = rte_security_session_create(sec_ctx,
> -					&sess_conf, skt_ctx->session_pool);
> +					&sess_conf, skt_ctx->session_pool,
> +					skt_ctx->session_priv_pool);
>   		if (ips->security.ses == NULL) {
>   			RTE_LOG(ERR, IPSEC,
>   				"SEC Session init failed: err: %d\n", ret);
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index 515c29e04..ee4666026 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -26,18 +26,21 @@
>   struct rte_security_session *
>   rte_security_session_create(struct rte_security_ctx *instance,
>   			    struct rte_security_session_conf *conf,
> -			    struct rte_mempool *mp)
> +			    struct rte_mempool *mp,
> +			    struct rte_mempool *priv_mp)
>   {
>   	struct rte_security_session *sess = NULL;
>   
>   	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_create, NULL, NULL);
>   	RTE_PTR_OR_ERR_RET(conf, NULL);
>   	RTE_PTR_OR_ERR_RET(mp, NULL);
> +	RTE_PTR_OR_ERR_RET(priv_mp, NULL);
>   
>   	if (rte_mempool_get(mp, (void **)&sess))
>   		return NULL;
>   
> -	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
> +	if (instance->ops->session_create(instance->device, conf,
> +				sess, priv_mp)) {
>   		rte_mempool_put(mp, (void *)sess);
>   		return NULL;
>   	}
> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> index 16839e539..1710cdd6a 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -386,6 +386,7 @@ struct rte_security_session {
>    * @param   instance	security instance
>    * @param   conf	session configuration parameters
>    * @param   mp		mempool to allocate session objects from
> + * @param   priv_mp	mempool to allocate session private data objects from
>    * @return
>    *  - On success, pointer to session
>    *  - On failure, NULL
> @@ -393,7 +394,8 @@ struct rte_security_session {
>   struct rte_security_session *
>   rte_security_session_create(struct rte_security_ctx *instance,
>   			    struct rte_security_session_conf *conf,
> -			    struct rte_mempool *mp);
> +			    struct rte_mempool *mp,
> +			    struct rte_mempool *priv_mp);
>   
>   /**
>    * Update security session as specified by the session configuration

Best regards

Lukasz
  

Hi Lukasz,

> Hi Akhil,
> > -#define SECURITY_TEST_MEMPOOL_NAME "SecurityTestsMempoolName"
> > +#define SECURITY_TEST_MEMPOOL_NAME "SecurityTestMp"
> > +#define SECURITY_TEST_PRIV_MEMPOOL_NAME "SecurityTestPrivMp"
> >   #define SECURITY_TEST_MEMPOOL_SIZE 15
> >   #define SECURITY_TEST_SESSION_OBJECT_SIZE sizeof(struct
> rte_security_session)
> >
> > @@ -545,6 +548,22 @@ testsuite_setup(void)
> >   			SOCKET_ID_ANY, 0);
> >   	TEST_ASSERT_NOT_NULL(ts_params->session_mpool,
> >   			"Cannot create mempool %s\n",
> rte_strerror(rte_errno));
> > +
> > +	ts_params->session_priv_mpool = rte_mempool_create(
> > +			SECURITY_TEST_PRIV_MEMPOOL_NAME,
> > +			SECURITY_TEST_MEMPOOL_SIZE,
> > +			rte_security_session_get_size(&unittest_params.ctx),
> Call to rte_security_session_get_size() will cause a mockup function
> mock_session_get_size() to be called, which will return 0.
> Why do you call this function instead of defining some value for private
> mempool element size?

Fixed in v3

> > +			0, 0, NULL, NULL, NULL, NULL,
> > +			SOCKET_ID_ANY, 0);
> > +	if (ts_params->session_priv_mpool == NULL) {
> > +		printf("TestCase %s() line %d failed (null): "
> > +				"Cannot create priv mempool %s\n",
> > +				__func__, __LINE__, rte_strerror(rte_errno));
> Instead of printf() use RTE_LOG(ERR, EAL,...). All other messages are
> printed this way. It allows control of error messages if required.

Fixed in v3, should be USER1 instead of EAL though.

> > +		rte_mempool_free(ts_params->session_mpool);
> > +		ts_params->session_mpool = NULL;
> > +		return TEST_FAILED;
> > +	}
> > +
> >   	return TEST_SUCCESS;
> >   }
> >
> > @@ -559,6 +578,10 @@ testsuite_teardown(void)
> >   		rte_mempool_free(ts_params->session_mpool);
> >   		ts_params->session_mpool = NULL;
> >   	}
> > +	if (ts_params->session_priv_mpool) {
> > +		rte_mempool_free(ts_params->session_priv_mpool);
> > +		ts_params->session_priv_mpool = NULL;
> > +	}
> >   }
> >
> >   /**
> > @@ -659,7 +682,8 @@ ut_setup_with_session(void)
> >   	mock_session_create_exp.ret = 0;
> >
> >   	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
> >conf,
> > -			ts_params->session_mpool);
> > +			ts_params->session_mpool,
> > +			ts_params->session_priv_mpool);
> >
> 	TEST_ASSERT_MOCK_FUNCTION_CALL_NOT_NULL(rte_security_sessio
> n_create,
> >   			sess);
> >   	TEST_ASSERT_EQUAL(sess, mock_session_create_exp.sess,
> > @@ -701,7 +725,8 @@ test_session_create_inv_context(void)
> >   	struct rte_security_session *sess;
> >
> >   	sess = rte_security_session_create(NULL, &ut_params->conf,
> > -			ts_params->session_mpool);
> > +			ts_params->session_mpool,
> > +			ts_params->session_priv_mpool);
> >
> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
> e,
> >   			sess, NULL, "%p");
> >   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> > @@ -725,7 +750,8 @@ test_session_create_inv_context_ops(void)
> >   	ut_params->ctx.ops = NULL;
> >
> >   	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
> >conf,
> > -			ts_params->session_mpool);
> > +			ts_params->session_mpool,
> > +			ts_params->session_priv_mpool);
> >
> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
> e,
> >   			sess, NULL, "%p");
> >   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> > @@ -749,7 +775,8 @@ test_session_create_inv_context_ops_fun(void)
> >   	ut_params->ctx.ops = &empty_ops;
> >
> >   	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
> >conf,
> > -			ts_params->session_mpool);
> > +			ts_params->session_mpool,
> > +			ts_params->session_priv_mpool);
> >
> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
> e,
> >   			sess, NULL, "%p");
> >   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> > @@ -770,7 +797,8 @@ test_session_create_inv_configuration(void)
> >   	struct rte_security_session *sess;
> >
> >   	sess = rte_security_session_create(&ut_params->ctx, NULL,
> > -			ts_params->session_mpool);
> > +			ts_params->session_mpool,
> > +			ts_params->session_priv_mpool);
> >
> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
> e,
> >   			sess, NULL, "%p");
> >   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> > @@ -781,7 +809,7 @@ test_session_create_inv_configuration(void)
> >   }
> >
> >   /**
> > - * Test execution of rte_security_session_create with NULL mp parameter
> > + * Test execution of rte_security_session_create with NULL mempools
> >    */
> >   static int
> >   test_session_create_inv_mempool(void)
> > @@ -790,7 +818,7 @@ test_session_create_inv_mempool(void)
> >   	struct rte_security_session *sess;
> >
> >   	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
> >conf,
> > -			NULL);
> > +			NULL, NULL);
> It would be best to add a new testcase for verification of passing NULL
> private mempool.
> If you pass NULL as the primary mempool as in this testcase, the
> verification of priv mempool (rte_securitry.c:37) won't ever happen
> because rte_security_session_create() will return in line 36.

Added a new test. However that was really unnecessary and was an overkill
To add a new case for so many negative cases.

Please have a look at v3 and ack it if no further comments.

Regards,
Akhil
  

W dniu 14.10.2020 o 21:00, Akhil Goyal pisze:
> Hi Lukasz,
>
>> Hi Akhil,
>>> -#define SECURITY_TEST_MEMPOOL_NAME "SecurityTestsMempoolName"
>>> +#define SECURITY_TEST_MEMPOOL_NAME "SecurityTestMp"
>>> +#define SECURITY_TEST_PRIV_MEMPOOL_NAME "SecurityTestPrivMp"
>>>    #define SECURITY_TEST_MEMPOOL_SIZE 15
>>>    #define SECURITY_TEST_SESSION_OBJECT_SIZE sizeof(struct
>> rte_security_session)
>>> @@ -545,6 +548,22 @@ testsuite_setup(void)
>>>    			SOCKET_ID_ANY, 0);
>>>    	TEST_ASSERT_NOT_NULL(ts_params->session_mpool,
>>>    			"Cannot create mempool %s\n",
>> rte_strerror(rte_errno));
>>> +
>>> +	ts_params->session_priv_mpool = rte_mempool_create(
>>> +			SECURITY_TEST_PRIV_MEMPOOL_NAME,
>>> +			SECURITY_TEST_MEMPOOL_SIZE,
>>> +			rte_security_session_get_size(&unittest_params.ctx),
>> Call to rte_security_session_get_size() will cause a mockup function
>> mock_session_get_size() to be called, which will return 0.
>> Why do you call this function instead of defining some value for private
>> mempool element size?
> Fixed in v3
>
>>> +			0, 0, NULL, NULL, NULL, NULL,
>>> +			SOCKET_ID_ANY, 0);
>>> +	if (ts_params->session_priv_mpool == NULL) {
>>> +		printf("TestCase %s() line %d failed (null): "
>>> +				"Cannot create priv mempool %s\n",
>>> +				__func__, __LINE__, rte_strerror(rte_errno));
>> Instead of printf() use RTE_LOG(ERR, EAL,...). All other messages are
>> printed this way. It allows control of error messages if required.
> Fixed in v3, should be USER1 instead of EAL though.
Can you explain me why, there should be USER1?
All other errors are printed with EAL tag.
>
>>> +		rte_mempool_free(ts_params->session_mpool);
>>> +		ts_params->session_mpool = NULL;
>>> +		return TEST_FAILED;
>>> +	}
>>> +
>>>    	return TEST_SUCCESS;
>>>    }
>>>
>>> @@ -559,6 +578,10 @@ testsuite_teardown(void)
>>>    		rte_mempool_free(ts_params->session_mpool);
>>>    		ts_params->session_mpool = NULL;
>>>    	}
>>> +	if (ts_params->session_priv_mpool) {
>>> +		rte_mempool_free(ts_params->session_priv_mpool);
>>> +		ts_params->session_priv_mpool = NULL;
>>> +	}
>>>    }
>>>
>>>    /**
>>> @@ -659,7 +682,8 @@ ut_setup_with_session(void)
>>>    	mock_session_create_exp.ret = 0;
>>>
>>>    	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
>>> conf,
>>> -			ts_params->session_mpool);
>>> +			ts_params->session_mpool,
>>> +			ts_params->session_priv_mpool);
>>>
>> 	TEST_ASSERT_MOCK_FUNCTION_CALL_NOT_NULL(rte_security_sessio
>> n_create,
>>>    			sess);
>>>    	TEST_ASSERT_EQUAL(sess, mock_session_create_exp.sess,
>>> @@ -701,7 +725,8 @@ test_session_create_inv_context(void)
>>>    	struct rte_security_session *sess;
>>>
>>>    	sess = rte_security_session_create(NULL, &ut_params->conf,
>>> -			ts_params->session_mpool);
>>> +			ts_params->session_mpool,
>>> +			ts_params->session_priv_mpool);
>>>
>> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
>> e,
>>>    			sess, NULL, "%p");
>>>    	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
>>> @@ -725,7 +750,8 @@ test_session_create_inv_context_ops(void)
>>>    	ut_params->ctx.ops = NULL;
>>>
>>>    	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
>>> conf,
>>> -			ts_params->session_mpool);
>>> +			ts_params->session_mpool,
>>> +			ts_params->session_priv_mpool);
>>>
>> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
>> e,
>>>    			sess, NULL, "%p");
>>>    	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
>>> @@ -749,7 +775,8 @@ test_session_create_inv_context_ops_fun(void)
>>>    	ut_params->ctx.ops = &empty_ops;
>>>
>>>    	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
>>> conf,
>>> -			ts_params->session_mpool);
>>> +			ts_params->session_mpool,
>>> +			ts_params->session_priv_mpool);
>>>
>> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
>> e,
>>>    			sess, NULL, "%p");
>>>    	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
>>> @@ -770,7 +797,8 @@ test_session_create_inv_configuration(void)
>>>    	struct rte_security_session *sess;
>>>
>>>    	sess = rte_security_session_create(&ut_params->ctx, NULL,
>>> -			ts_params->session_mpool);
>>> +			ts_params->session_mpool,
>>> +			ts_params->session_priv_mpool);
>>>
>> 	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_creat
>> e,
>>>    			sess, NULL, "%p");
>>>    	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
>>> @@ -781,7 +809,7 @@ test_session_create_inv_configuration(void)
>>>    }
>>>
>>>    /**
>>> - * Test execution of rte_security_session_create with NULL mp parameter
>>> + * Test execution of rte_security_session_create with NULL mempools
>>>     */
>>>    static int
>>>    test_session_create_inv_mempool(void)
>>> @@ -790,7 +818,7 @@ test_session_create_inv_mempool(void)
>>>    	struct rte_security_session *sess;
>>>
>>>    	sess = rte_security_session_create(&ut_params->ctx, &ut_params-
>>> conf,
>>> -			NULL);
>>> +			NULL, NULL);
>> It would be best to add a new testcase for verification of passing NULL
>> private mempool.
>> If you pass NULL as the primary mempool as in this testcase, the
>> verification of priv mempool (rte_securitry.c:37) won't ever happen
>> because rte_security_session_create() will return in line 36.
> Added a new test. However that was really unnecessary and was an overkill
> To add a new case for so many negative cases.
>
> Please have a look at v3 and ack it if no further comments.
>
> Regards,
> Akhil
>