[5/6] crypto/dpaa_sec: fix session qp attach/detach

Message ID 20190327114407.13697-6-akhil.goyal@nxp.com
State Accepted, archived
Delegated to: akhil goyal
Headers show
Series
  • minor fixes and updates for NXP crypto PMDs
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Akhil Goyal March 27, 2019, 11:53 a.m.
session inq and qp are assigned for each core from which the
packets arrive. This was not correctly handled while supporting
multiple sessions per queue pair.
This patch fixes the attach and detach of queues for each core.

Fixes: e79416d10fa3 ("crypto/dpaa_sec: support multiple sessions per queue pair")
Cc: stable@dpdk.org
Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 drivers/crypto/dpaa_sec/dpaa_sec.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Kevin Traynor April 25, 2019, 4:24 p.m. | #1
On 27/03/2019 11:53, Akhil Goyal wrote:
> session inq and qp are assigned for each core from which the
> packets arrive. This was not correctly handled while supporting
> multiple sessions per queue pair.
> This patch fixes the attach and detach of queues for each core.
> 
> Fixes: e79416d10fa3 ("crypto/dpaa_sec: support multiple sessions per queue pair")
> Cc: stable@dpdk.org

Hi, this will not backport to 18.11 branch as:
4e694fe51171dcdbe94019189a0240833b45c943
Author: Akhil Goyal <akhil.goyal@nxp.com>
Date:   Wed Jan 9 15:14:17 2019 +0000

    crypto/dpaa_sec: support same session flows on multi-cores

was added after 18.11 and dpaa_sec_sym_session_clear() is quite
different. Please send a backport of the bugfix for 18.11 branch, or let
me know if it's not needed.

thanks,
Kevin.

> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>  drivers/crypto/dpaa_sec/dpaa_sec.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
> index cb99be4e1..8305f19a3 100644
> --- a/drivers/crypto/dpaa_sec/dpaa_sec.c
> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   *
>   *   Copyright (c) 2016 Freescale Semiconductor, Inc. All rights reserved.
> - *   Copyright 2017-2018 NXP
> + *   Copyright 2017-2019 NXP
>   *
>   */
>  
> @@ -1940,13 +1940,13 @@ dpaa_sec_attach_rxq(struct dpaa_sec_dev_private *qi)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < qi->max_nb_sessions; i++) {
> +	for (i = 0; i < qi->max_nb_sessions * MAX_DPAA_CORES; i++) {
>  		if (qi->inq_attach[i] == 0) {
>  			qi->inq_attach[i] = 1;
>  			return &qi->inq[i];
>  		}
>  	}
> -	DPAA_SEC_WARN("All ses session in use %x", qi->max_nb_sessions);
> +	DPAA_SEC_WARN("All session in use %u", qi->max_nb_sessions);
>  
>  	return NULL;
>  }
> @@ -2115,7 +2115,7 @@ dpaa_sec_sym_session_clear(struct rte_cryptodev *dev,
>  		struct rte_cryptodev_sym_session *sess)
>  {
>  	struct dpaa_sec_dev_private *qi = dev->data->dev_private;
> -	uint8_t index = dev->driver_id;
> +	uint8_t index = dev->driver_id, i;
>  	void *sess_priv = get_sym_session_private_data(sess, index);
>  
>  	PMD_INIT_FUNC_TRACE();
> @@ -2125,9 +2125,12 @@ dpaa_sec_sym_session_clear(struct rte_cryptodev *dev,
>  	if (sess_priv) {
>  		struct rte_mempool *sess_mp = rte_mempool_from_obj(sess_priv);
>  
> -		if (s->inq[rte_lcore_id() % MAX_DPAA_CORES])
> -			dpaa_sec_detach_rxq(qi,
> -				s->inq[rte_lcore_id() % MAX_DPAA_CORES]);
> +		for (i = 0; i < MAX_DPAA_CORES; i++) {
> +			if (s->inq[i])
> +				dpaa_sec_detach_rxq(qi, s->inq[i]);
> +			s->inq[i] = NULL;
> +			s->qp[i] = NULL;
> +		}
>  		rte_free(s->cipher_key.data);
>  		rte_free(s->auth_key.data);
>  		memset(s, 0, sizeof(dpaa_sec_session));
>
Kevin Traynor April 25, 2019, 5:42 p.m. | #2
On 25/04/2019 17:24, Kevin Traynor wrote:
> On 27/03/2019 11:53, Akhil Goyal wrote:
>> session inq and qp are assigned for each core from which the
>> packets arrive. This was not correctly handled while supporting
>> multiple sessions per queue pair.
>> This patch fixes the attach and detach of queues for each core.
>>
>> Fixes: e79416d10fa3 ("crypto/dpaa_sec: support multiple sessions per queue pair")
>> Cc: stable@dpdk.org
> 
> Hi, this will not backport to 18.11 branch as:
> 4e694fe51171dcdbe94019189a0240833b45c943
> Author: Akhil Goyal <akhil.goyal@nxp.com>
> Date:   Wed Jan 9 15:14:17 2019 +0000
> 
>     crypto/dpaa_sec: support same session flows on multi-cores
> 
> was added after 18.11 and dpaa_sec_sym_session_clear() is quite
> different. Please send a backport of the bugfix for 18.11 branch, or let
> me know if it's not needed.
> 
> thanks,
> Kevin.
> 
>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>> ---
>>  drivers/crypto/dpaa_sec/dpaa_sec.c | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
>> index cb99be4e1..8305f19a3 100644
>> --- a/drivers/crypto/dpaa_sec/dpaa_sec.c
>> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
>> @@ -1,7 +1,7 @@
>>  /* SPDX-License-Identifier: BSD-3-Clause
>>   *
>>   *   Copyright (c) 2016 Freescale Semiconductor, Inc. All rights reserved.
>> - *   Copyright 2017-2018 NXP
>> + *   Copyright 2017-2019 NXP
>>   *
>>   */
>>  
>> @@ -1940,13 +1940,13 @@ dpaa_sec_attach_rxq(struct dpaa_sec_dev_private *qi)
>>  {
>>  	unsigned int i;
>>  
>> -	for (i = 0; i < qi->max_nb_sessions; i++) {
>> +	for (i = 0; i < qi->max_nb_sessions * MAX_DPAA_CORES; i++) {
>>  		if (qi->inq_attach[i] == 0) {

Btw, without knowing the crypto code, the use of max_nb_sessions in for
loops with fixed size arrays being indexed looks suspect to me. In the
snippet above

unsigned char inq_attach[RTE_DPAA_MAX_RX_QUEUE];
which means inq_attach[4096]

At least in one place I can see
.max_nb_sessions = MRVL_PMD_DEFAULT_MAX_NB_SESSIONS
which makes max_nb_sessions=2048, while MAX_DPAA_CORES=4

>>  			qi->inq_attach[i] = 1;
>>  			return &qi->inq[i];
>>  		}
>>  	}
>> -	DPAA_SEC_WARN("All ses session in use %x", qi->max_nb_sessions);
>> +	DPAA_SEC_WARN("All session in use %u", qi->max_nb_sessions);
>>  
>>  	return NULL;
>>  }
>> @@ -2115,7 +2115,7 @@ dpaa_sec_sym_session_clear(struct rte_cryptodev *dev,
>>  		struct rte_cryptodev_sym_session *sess)
>>  {
>>  	struct dpaa_sec_dev_private *qi = dev->data->dev_private;
>> -	uint8_t index = dev->driver_id;
>> +	uint8_t index = dev->driver_id, i;
>>  	void *sess_priv = get_sym_session_private_data(sess, index);
>>  
>>  	PMD_INIT_FUNC_TRACE();
>> @@ -2125,9 +2125,12 @@ dpaa_sec_sym_session_clear(struct rte_cryptodev *dev,
>>  	if (sess_priv) {
>>  		struct rte_mempool *sess_mp = rte_mempool_from_obj(sess_priv);
>>  
>> -		if (s->inq[rte_lcore_id() % MAX_DPAA_CORES])
>> -			dpaa_sec_detach_rxq(qi,
>> -				s->inq[rte_lcore_id() % MAX_DPAA_CORES]);
>> +		for (i = 0; i < MAX_DPAA_CORES; i++) {
>> +			if (s->inq[i])
>> +				dpaa_sec_detach_rxq(qi, s->inq[i]);
>> +			s->inq[i] = NULL;
>> +			s->qp[i] = NULL;
>> +		}
>>  		rte_free(s->cipher_key.data);
>>  		rte_free(s->auth_key.data);
>>  		memset(s, 0, sizeof(dpaa_sec_session));
>>
>
Hemant Agrawal April 26, 2019, 7:14 a.m. | #3
Hi Kevin,
	Thanks we will look into it. 
Regards,
Hemant
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Thursday, April 25, 2019 11:13 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; dev@dpdk.org
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; stable@dpdk.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH 5/6] crypto/dpaa_sec: fix session qp
> attach/detach
> Importance: High
> 
> Caution: EXT Email
> 
> On 25/04/2019 17:24, Kevin Traynor wrote:
> > On 27/03/2019 11:53, Akhil Goyal wrote:
> >> session inq and qp are assigned for each core from which the packets
> >> arrive. This was not correctly handled while supporting multiple
> >> sessions per queue pair.
> >> This patch fixes the attach and detach of queues for each core.
> >>
> >> Fixes: e79416d10fa3 ("crypto/dpaa_sec: support multiple sessions per
> >> queue pair")
> >> Cc: stable@dpdk.org
> >
> > Hi, this will not backport to 18.11 branch as:
> > 4e694fe51171dcdbe94019189a0240833b45c943
> > Author: Akhil Goyal <akhil.goyal@nxp.com>
> > Date:   Wed Jan 9 15:14:17 2019 +0000
> >
> >     crypto/dpaa_sec: support same session flows on multi-cores
> >
> > was added after 18.11 and dpaa_sec_sym_session_clear() is quite
> > different. Please send a backport of the bugfix for 18.11 branch, or
> > let me know if it's not needed.
> >
> > thanks,
> > Kevin.
> >
> >> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> >> ---
> >>  drivers/crypto/dpaa_sec/dpaa_sec.c | 17 ++++++++++-------
> >>  1 file changed, 10 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c
> >> b/drivers/crypto/dpaa_sec/dpaa_sec.c
> >> index cb99be4e1..8305f19a3 100644
> >> --- a/drivers/crypto/dpaa_sec/dpaa_sec.c
> >> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
> >> @@ -1,7 +1,7 @@
> >>  /* SPDX-License-Identifier: BSD-3-Clause
> >>   *
> >>   *   Copyright (c) 2016 Freescale Semiconductor, Inc. All rights reserved.
> >> - *   Copyright 2017-2018 NXP
> >> + *   Copyright 2017-2019 NXP
> >>   *
> >>   */
> >>
> >> @@ -1940,13 +1940,13 @@ dpaa_sec_attach_rxq(struct
> >> dpaa_sec_dev_private *qi)  {
> >>      unsigned int i;
> >>
> >> -    for (i = 0; i < qi->max_nb_sessions; i++) {
> >> +    for (i = 0; i < qi->max_nb_sessions * MAX_DPAA_CORES; i++) {
> >>              if (qi->inq_attach[i] == 0) {
> 
> Btw, without knowing the crypto code, the use of max_nb_sessions in for
> loops with fixed size arrays being indexed looks suspect to me. In the snippet
> above
> 
> unsigned char inq_attach[RTE_DPAA_MAX_RX_QUEUE];
> which means inq_attach[4096]
> 
> At least in one place I can see
> .max_nb_sessions = MRVL_PMD_DEFAULT_MAX_NB_SESSIONS which
> makes max_nb_sessions=2048, while MAX_DPAA_CORES=4
> 
> >>                      qi->inq_attach[i] = 1;
> >>                      return &qi->inq[i];
> >>              }
> >>      }
> >> -    DPAA_SEC_WARN("All ses session in use %x", qi->max_nb_sessions);
> >> +    DPAA_SEC_WARN("All session in use %u", qi->max_nb_sessions);
> >>
> >>      return NULL;
> >>  }
> >> @@ -2115,7 +2115,7 @@ dpaa_sec_sym_session_clear(struct
> rte_cryptodev *dev,
> >>              struct rte_cryptodev_sym_session *sess)  {
> >>      struct dpaa_sec_dev_private *qi = dev->data->dev_private;
> >> -    uint8_t index = dev->driver_id;
> >> +    uint8_t index = dev->driver_id, i;
> >>      void *sess_priv = get_sym_session_private_data(sess, index);
> >>
> >>      PMD_INIT_FUNC_TRACE();
> >> @@ -2125,9 +2125,12 @@ dpaa_sec_sym_session_clear(struct
> rte_cryptodev *dev,
> >>      if (sess_priv) {
> >>              struct rte_mempool *sess_mp =
> >> rte_mempool_from_obj(sess_priv);
> >>
> >> -            if (s->inq[rte_lcore_id() % MAX_DPAA_CORES])
> >> -                    dpaa_sec_detach_rxq(qi,
> >> -                            s->inq[rte_lcore_id() % MAX_DPAA_CORES]);
> >> +            for (i = 0; i < MAX_DPAA_CORES; i++) {
> >> +                    if (s->inq[i])
> >> +                            dpaa_sec_detach_rxq(qi, s->inq[i]);
> >> +                    s->inq[i] = NULL;
> >> +                    s->qp[i] = NULL;
> >> +            }
> >>              rte_free(s->cipher_key.data);
> >>              rte_free(s->auth_key.data);
> >>              memset(s, 0, sizeof(dpaa_sec_session));
> >>
> >

Patch

diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index cb99be4e1..8305f19a3 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -1,7 +1,7 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  *
  *   Copyright (c) 2016 Freescale Semiconductor, Inc. All rights reserved.
- *   Copyright 2017-2018 NXP
+ *   Copyright 2017-2019 NXP
  *
  */
 
@@ -1940,13 +1940,13 @@  dpaa_sec_attach_rxq(struct dpaa_sec_dev_private *qi)
 {
 	unsigned int i;
 
-	for (i = 0; i < qi->max_nb_sessions; i++) {
+	for (i = 0; i < qi->max_nb_sessions * MAX_DPAA_CORES; i++) {
 		if (qi->inq_attach[i] == 0) {
 			qi->inq_attach[i] = 1;
 			return &qi->inq[i];
 		}
 	}
-	DPAA_SEC_WARN("All ses session in use %x", qi->max_nb_sessions);
+	DPAA_SEC_WARN("All session in use %u", qi->max_nb_sessions);
 
 	return NULL;
 }
@@ -2115,7 +2115,7 @@  dpaa_sec_sym_session_clear(struct rte_cryptodev *dev,
 		struct rte_cryptodev_sym_session *sess)
 {
 	struct dpaa_sec_dev_private *qi = dev->data->dev_private;
-	uint8_t index = dev->driver_id;
+	uint8_t index = dev->driver_id, i;
 	void *sess_priv = get_sym_session_private_data(sess, index);
 
 	PMD_INIT_FUNC_TRACE();
@@ -2125,9 +2125,12 @@  dpaa_sec_sym_session_clear(struct rte_cryptodev *dev,
 	if (sess_priv) {
 		struct rte_mempool *sess_mp = rte_mempool_from_obj(sess_priv);
 
-		if (s->inq[rte_lcore_id() % MAX_DPAA_CORES])
-			dpaa_sec_detach_rxq(qi,
-				s->inq[rte_lcore_id() % MAX_DPAA_CORES]);
+		for (i = 0; i < MAX_DPAA_CORES; i++) {
+			if (s->inq[i])
+				dpaa_sec_detach_rxq(qi, s->inq[i]);
+			s->inq[i] = NULL;
+			s->qp[i] = NULL;
+		}
 		rte_free(s->cipher_key.data);
 		rte_free(s->auth_key.data);
 		memset(s, 0, sizeof(dpaa_sec_session));