diff mbox series

[v3] examples/ipsec-secgw: fix cryptodev to SA mapping

Message ID 20240226102545.18667-1-radu.nicolau@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v3] examples/ipsec-secgw: fix cryptodev to SA mapping |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance fail Performance Testing issues

Commit Message

Radu Nicolau Feb. 26, 2024, 10:25 a.m. UTC 
  There are use cases where a SA should be able to use different cryptodevs on
different lcores, for example there can be cryptodevs with just 1 qp per VF.
For this purpose this patch relaxes the check in create lookaside session function.
Also add a check to verify that a CQP is available for the current lcore.

Fixes: a8ade12123c3 ("examples/ipsec-secgw: create lookaside sessions at init")
Cc: stable@dpdk.org
Cc: vfialko@marvell.com

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
Tested-by: Ting-Kai Ku <ting-kai.ku@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Kai Ji <kai.ji@intel.com>
---
v3: check if the cryptodev are not of the same type

 examples/ipsec-secgw/ipsec.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

Patrick Robb Feb. 26, 2024, 11:02 p.m. UTC | #1 
Recheck-request: iol-broadcom-Performance

This patch should not fail a performance test in CI - checking with a rerun
now.

On Mon, Feb 26, 2024 at 5:25 AM Radu Nicolau <radu.nicolau@intel.com> wrote:

> There are use cases where a SA should be able to use different cryptodevs
> on
> different lcores, for example there can be cryptodevs with just 1 qp per
> VF.
> For this purpose this patch relaxes the check in create lookaside session
> function.
> Also add a check to verify that a CQP is available for the current lcore.
>
> Fixes: a8ade12123c3 ("examples/ipsec-secgw: create lookaside sessions at
> init")
> Cc: stable@dpdk.org
> Cc: vfialko@marvell.com
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Tested-by: Ting-Kai Ku <ting-kai.ku@intel.com>
> Acked-by: Ciara Power <ciara.power@intel.com>
> Acked-by: Kai Ji <kai.ji@intel.com>
> ---
> v3: check if the cryptodev are not of the same type
>
>  examples/ipsec-secgw/ipsec.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index f5cec4a928..b59576c049 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -288,10 +288,21 @@ create_lookaside_session(struct ipsec_ctx
> *ipsec_ctx_lcore[],
>                 if (cdev_id == RTE_CRYPTO_MAX_DEVS)
>                         cdev_id = ipsec_ctx->tbl[cdev_id_qp].id;
>                 else if (cdev_id != ipsec_ctx->tbl[cdev_id_qp].id) {
> -                       RTE_LOG(ERR, IPSEC,
> -                                       "SA mapping to multiple cryptodevs
> is "
> -                                       "not supported!");
> -                       return -EINVAL;
> +                       struct rte_cryptodev_info dev_info_1, dev_info_2;
> +                       rte_cryptodev_info_get(cdev_id, &dev_info_1);
> +
>  rte_cryptodev_info_get(ipsec_ctx->tbl[cdev_id_qp].id,
> +                                       &dev_info_2);
> +                       if (dev_info_1.driver_id == dev_info_2.driver_id) {
> +                               RTE_LOG(WARNING, IPSEC,
> +                                       "SA mapped to multiple cryptodevs
> for SPI %d\n",
> +                                       sa->spi);
> +
> +                       } else {
> +                               RTE_LOG(WARNING, IPSEC,
> +                                       "SA mapped to multiple cryptodevs
> of different types for SPI %d\n",
> +                                       sa->spi);
> +
> +                       }
>                 }
>
>                 /* Store per core queue pair information */
> @@ -908,7 +919,11 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct
> ipsec_ctx *ipsec_ctx,
>                         continue;
>                 }
>
> -               enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
> +               if (likely(sa->cqp[ipsec_ctx->lcore_id]))
> +                       enqueue_cop(sa->cqp[ipsec_ctx->lcore_id],
> &priv->cop);
> +               else
> +                       RTE_LOG(ERR, IPSEC, "No CQP available for lcore
> %d\n",
> +                                       ipsec_ctx->lcore_id);
>         }
>  }
>
> --
> 2.34.1
>
>
Anoob Joseph Feb. 27, 2024, 5:19 a.m. UTC | #2 
Hi Radu,

Thanks for making the changes. I've one more question. Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Radu Nicolau <radu.nicolau@intel.com>
> Sent: Monday, February 26, 2024 3:56 PM
> To: dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; Radu Nicolau
> <radu.nicolau@intel.com>; stable@dpdk.org; Volodymyr Fialko
> <vfialko@marvell.com>; Ting-Kai Ku <ting-kai.ku@intel.com>; Ciara Power
> <ciara.power@intel.com>; Kai Ji <kai.ji@intel.com>; Akhil Goyal
> <gakhil@marvell.com>
> Subject: [EXT] [PATCH v3] examples/ipsec-secgw: fix cryptodev to SA mapping
> 
> External Email
> 
> ----------------------------------------------------------------------
> There are use cases where a SA should be able to use different cryptodevs on
> different lcores, for example there can be cryptodevs with just 1 qp per VF.
> For this purpose this patch relaxes the check in create lookaside session function.
> Also add a check to verify that a CQP is available for the current lcore.
> 
> Fixes: a8ade12123c3 ("examples/ipsec-secgw: create lookaside sessions at init")
> Cc: stable@dpdk.org
> Cc: vfialko@marvell.com
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Tested-by: Ting-Kai Ku <ting-kai.ku@intel.com>
> Acked-by: Ciara Power <ciara.power@intel.com>
> Acked-by: Kai Ji <kai.ji@intel.com>
> ---
> v3: check if the cryptodev are not of the same type
> 
>  examples/ipsec-secgw/ipsec.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index
> f5cec4a928..b59576c049 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -288,10 +288,21 @@ create_lookaside_session(struct ipsec_ctx
> *ipsec_ctx_lcore[],
>  		if (cdev_id == RTE_CRYPTO_MAX_DEVS)
>  			cdev_id = ipsec_ctx->tbl[cdev_id_qp].id;
>  		else if (cdev_id != ipsec_ctx->tbl[cdev_id_qp].id) {
> -			RTE_LOG(ERR, IPSEC,
> -					"SA mapping to multiple cryptodevs is "
> -					"not supported!");
> -			return -EINVAL;
> +			struct rte_cryptodev_info dev_info_1, dev_info_2;
> +			rte_cryptodev_info_get(cdev_id, &dev_info_1);
> +			rte_cryptodev_info_get(ipsec_ctx->tbl[cdev_id_qp].id,
> +					&dev_info_2);
> +			if (dev_info_1.driver_id == dev_info_2.driver_id) {
> +				RTE_LOG(WARNING, IPSEC,
> +					"SA mapped to multiple cryptodevs for
> SPI %d\n",
> +					sa->spi);
> +
> +			} else {
> +				RTE_LOG(WARNING, IPSEC,
> +					"SA mapped to multiple cryptodevs of
> different types for SPI %d\n",
> +					sa->spi);
> +
> +			}
>  		}
> 
>  		/* Store per core queue pair information */ @@ -908,7 +919,11
> @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct ipsec_ctx *ipsec_ctx,
>  			continue;
>  		}
> 
> -		enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
> +		if (likely(sa->cqp[ipsec_ctx->lcore_id]))
> +			enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
> +		else
> +			RTE_LOG(ERR, IPSEC, "No CQP available for lcore %d\n",
> +					ipsec_ctx->lcore_id);

[Anoob] Throwing an error won't be good enough, right? Won't this lead to packet leaks? Since it is datapath, can't we assume that the configuration would be done correctly in control path?

I would suggest drop this specific change and we can enable multiple cryptodevs with lookaside SAs with the changes proposed.

>  	}
>  }
> 
> --
> 2.34.1
Radu Nicolau Feb. 27, 2024, 10:10 a.m. UTC | #3 
Hi Anoob, reply inline.

Regards,

Radu

On 27-Feb-24 5:19 AM, Anoob Joseph wrote:
> Hi Radu,
>
> Thanks for making the changes. I've one more question. Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Radu Nicolau <radu.nicolau@intel.com>
>> Sent: Monday, February 26, 2024 3:56 PM
>> To: dev@dpdk.org
>> Cc: Anoob Joseph <anoobj@marvell.com>; Radu Nicolau
>> <radu.nicolau@intel.com>; stable@dpdk.org; Volodymyr Fialko
>> <vfialko@marvell.com>; Ting-Kai Ku <ting-kai.ku@intel.com>; Ciara Power
>> <ciara.power@intel.com>; Kai Ji <kai.ji@intel.com>; Akhil Goyal
>> <gakhil@marvell.com>
>> Subject: [EXT] [PATCH v3] examples/ipsec-secgw: fix cryptodev to SA mapping
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> There are use cases where a SA should be able to use different cryptodevs on
>> different lcores, for example there can be cryptodevs with just 1 qp per VF.
>> For this purpose this patch relaxes the check in create lookaside session function.
>> Also add a check to verify that a CQP is available for the current lcore.
>>
>> Fixes: a8ade12123c3 ("examples/ipsec-secgw: create lookaside sessions at init")
>> Cc: stable@dpdk.org
>> Cc: vfialko@marvell.com
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> Tested-by: Ting-Kai Ku <ting-kai.ku@intel.com>
>> Acked-by: Ciara Power <ciara.power@intel.com>
>> Acked-by: Kai Ji <kai.ji@intel.com>
>> ---
>> v3: check if the cryptodev are not of the same type
>>
>>   examples/ipsec-secgw/ipsec.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index
>> f5cec4a928..b59576c049 100644
>> --- a/examples/ipsec-secgw/ipsec.c
>> +++ b/examples/ipsec-secgw/ipsec.c
>> @@ -288,10 +288,21 @@ create_lookaside_session(struct ipsec_ctx
>> *ipsec_ctx_lcore[],
>>   		if (cdev_id == RTE_CRYPTO_MAX_DEVS)
>>   			cdev_id = ipsec_ctx->tbl[cdev_id_qp].id;
>>   		else if (cdev_id != ipsec_ctx->tbl[cdev_id_qp].id) {
>> -			RTE_LOG(ERR, IPSEC,
>> -					"SA mapping to multiple cryptodevs is "
>> -					"not supported!");
>> -			return -EINVAL;
>> +			struct rte_cryptodev_info dev_info_1, dev_info_2;
>> +			rte_cryptodev_info_get(cdev_id, &dev_info_1);
>> +			rte_cryptodev_info_get(ipsec_ctx->tbl[cdev_id_qp].id,
>> +					&dev_info_2);
>> +			if (dev_info_1.driver_id == dev_info_2.driver_id) {
>> +				RTE_LOG(WARNING, IPSEC,
>> +					"SA mapped to multiple cryptodevs for
>> SPI %d\n",
>> +					sa->spi);
>> +
>> +			} else {
>> +				RTE_LOG(WARNING, IPSEC,
>> +					"SA mapped to multiple cryptodevs of
>> different types for SPI %d\n",
>> +					sa->spi);
>> +
>> +			}
>>   		}
>>
>>   		/* Store per core queue pair information */ @@ -908,7 +919,11
>> @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct ipsec_ctx *ipsec_ctx,
>>   			continue;
>>   		}
>>
>> -		enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
>> +		if (likely(sa->cqp[ipsec_ctx->lcore_id]))
>> +			enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
>> +		else
>> +			RTE_LOG(ERR, IPSEC, "No CQP available for lcore %d\n",
>> +					ipsec_ctx->lcore_id);
> [Anoob] Throwing an error won't be good enough, right? Won't this lead to packet leaks? Since it is datapath, can't we assume that the configuration would be done correctly in control path?
>
> I would suggest drop this specific change and we can enable multiple cryptodevs with lookaside SAs with the changes proposed.
With the change that removed the lazy initialization of SAs 
("examples/ipsec-secgw: create lookaside sessions at init") we can't 
assume anymore that a worker core has the proper CQP assigned, that is 
the reason I added the check here, the control path had no errors but 
there was no CQP assigned to a lcore. Indeed there will be dropped 
packets but at least there will be no segfault and the user will have an 
indication on what needs to be done - assign more cryptodevs.
>>   	}
>>   }
>>
>> --
>> 2.34.1
Anoob Joseph Feb. 27, 2024, 11:22 a.m. UTC | #4 
Hi Radu,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Radu Nicolau <radu.nicolau@intel.com>
> Sent: Tuesday, February 27, 2024 3:41 PM
> To: Anoob Joseph <anoobj@marvell.com>
> Cc: stable@dpdk.org; Volodymyr Fialko <vfialko@marvell.com>; Ting-Kai Ku
> <ting-kai.ku@intel.com>; Ciara Power <ciara.power@intel.com>; Kai Ji
> <kai.ji@intel.com>; Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org
> Subject: Re: [EXT] [PATCH v3] examples/ipsec-secgw: fix cryptodev to SA
> mapping
> 
> Hi Anoob, reply inline.
> 
> Regards,
> 
> Radu
> 
> On 27-Feb-24 5:19 AM, Anoob Joseph wrote:
> > Hi Radu,
> >
> > Thanks for making the changes. I've one more question. Please see inline.
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: Radu Nicolau <radu.nicolau@intel.com>
> >> Sent: Monday, February 26, 2024 3:56 PM
> >> To: dev@dpdk.org
> >> Cc: Anoob Joseph <anoobj@marvell.com>; Radu Nicolau
> >> <radu.nicolau@intel.com>; stable@dpdk.org; Volodymyr Fialko
> >> <vfialko@marvell.com>; Ting-Kai Ku <ting-kai.ku@intel.com>; Ciara
> >> Power <ciara.power@intel.com>; Kai Ji <kai.ji@intel.com>; Akhil Goyal
> >> <gakhil@marvell.com>
> >> Subject: [EXT] [PATCH v3] examples/ipsec-secgw: fix cryptodev to SA
> >> mapping
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - There are use cases where a SA should be able to use different
> >> cryptodevs on different lcores, for example there can be cryptodevs
> >> with just 1 qp per VF.
> >> For this purpose this patch relaxes the check in create lookaside session
> function.
> >> Also add a check to verify that a CQP is available for the current lcore.
> >>
> >> Fixes: a8ade12123c3 ("examples/ipsec-secgw: create lookaside sessions
> >> at init")
> >> Cc: stable@dpdk.org
> >> Cc: vfialko@marvell.com
> >>
> >> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> >> Tested-by: Ting-Kai Ku <ting-kai.ku@intel.com>
> >> Acked-by: Ciara Power <ciara.power@intel.com>
> >> Acked-by: Kai Ji <kai.ji@intel.com>
> >> ---
> >> v3: check if the cryptodev are not of the same type
> >>
> >>   examples/ipsec-secgw/ipsec.c | 25 ++++++++++++++++++++-----
> >>   1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/examples/ipsec-secgw/ipsec.c
> >> b/examples/ipsec-secgw/ipsec.c index
> >> f5cec4a928..b59576c049 100644
> >> --- a/examples/ipsec-secgw/ipsec.c
> >> +++ b/examples/ipsec-secgw/ipsec.c
> >> @@ -288,10 +288,21 @@ create_lookaside_session(struct ipsec_ctx
> >> *ipsec_ctx_lcore[],
> >>   		if (cdev_id == RTE_CRYPTO_MAX_DEVS)
> >>   			cdev_id = ipsec_ctx->tbl[cdev_id_qp].id;
> >>   		else if (cdev_id != ipsec_ctx->tbl[cdev_id_qp].id) {
> >> -			RTE_LOG(ERR, IPSEC,
> >> -					"SA mapping to multiple cryptodevs is
> "
> >> -					"not supported!");
> >> -			return -EINVAL;
> >> +			struct rte_cryptodev_info dev_info_1, dev_info_2;
> >> +			rte_cryptodev_info_get(cdev_id, &dev_info_1);
> >> +			rte_cryptodev_info_get(ipsec_ctx->tbl[cdev_id_qp].id,
> >> +					&dev_info_2);
> >> +			if (dev_info_1.driver_id == dev_info_2.driver_id) {
> >> +				RTE_LOG(WARNING, IPSEC,
> >> +					"SA mapped to multiple cryptodevs
> for
> >> SPI %d\n",
> >> +					sa->spi);
> >> +
> >> +			} else {
> >> +				RTE_LOG(WARNING, IPSEC,
> >> +					"SA mapped to multiple cryptodevs of
> >> different types for SPI %d\n",
> >> +					sa->spi);
> >> +
> >> +			}
> >>   		}
> >>
> >>   		/* Store per core queue pair information */ @@ -908,7
> +919,11 @@
> >> ipsec_enqueue(ipsec_xform_fn xform_func, struct ipsec_ctx *ipsec_ctx,
> >>   			continue;
> >>   		}
> >>
> >> -		enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
> >> +		if (likely(sa->cqp[ipsec_ctx->lcore_id]))
> >> +			enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv-
> >cop);
> >> +		else
> >> +			RTE_LOG(ERR, IPSEC, "No CQP available for lcore
> %d\n",
> >> +					ipsec_ctx->lcore_id);
> > [Anoob] Throwing an error won't be good enough, right? Won't this lead to
> packet leaks? Since it is datapath, can't we assume that the configuration
> would be done correctly in control path?
> >
> > I would suggest drop this specific change and we can enable multiple
> cryptodevs with lookaside SAs with the changes proposed.
> With the change that removed the lazy initialization of SAs
> ("examples/ipsec-secgw: create lookaside sessions at init") we can't assume
> anymore that a worker core has the proper CQP assigned, that is the reason I
> added the check here, the control path had no errors but there was no CQP
> assigned to a lcore. Indeed there will be dropped packets but at least there will
> be no segfault and the user will have an indication on what needs to be done -
> assign more cryptodevs.

[Anoob] I understand your concern. I was just worried about an extra check coming in data path which can impact performance benchmarks with valid configuration. Can we keep the check under a debug flag? Is that okay? 

> >>   	}
> >>   }
> >>
> >> --
> >> 2.34.1
diff mbox series

Patch

diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
index f5cec4a928..b59576c049 100644
--- a/examples/ipsec-secgw/ipsec.c
+++ b/examples/ipsec-secgw/ipsec.c
@@ -288,10 +288,21 @@  create_lookaside_session(struct ipsec_ctx *ipsec_ctx_lcore[],
 		if (cdev_id == RTE_CRYPTO_MAX_DEVS)
 			cdev_id = ipsec_ctx->tbl[cdev_id_qp].id;
 		else if (cdev_id != ipsec_ctx->tbl[cdev_id_qp].id) {
-			RTE_LOG(ERR, IPSEC,
-					"SA mapping to multiple cryptodevs is "
-					"not supported!");
-			return -EINVAL;
+			struct rte_cryptodev_info dev_info_1, dev_info_2;
+			rte_cryptodev_info_get(cdev_id, &dev_info_1);
+			rte_cryptodev_info_get(ipsec_ctx->tbl[cdev_id_qp].id,
+					&dev_info_2);
+			if (dev_info_1.driver_id == dev_info_2.driver_id) {
+				RTE_LOG(WARNING, IPSEC,
+					"SA mapped to multiple cryptodevs for SPI %d\n",
+					sa->spi);
+
+			} else {
+				RTE_LOG(WARNING, IPSEC,
+					"SA mapped to multiple cryptodevs of different types for SPI %d\n",
+					sa->spi);
+
+			}
 		}
 
 		/* Store per core queue pair information */
@@ -908,7 +919,11 @@  ipsec_enqueue(ipsec_xform_fn xform_func, struct ipsec_ctx *ipsec_ctx,
 			continue;
 		}
 
-		enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
+		if (likely(sa->cqp[ipsec_ctx->lcore_id]))
+			enqueue_cop(sa->cqp[ipsec_ctx->lcore_id], &priv->cop);
+		else
+			RTE_LOG(ERR, IPSEC, "No CQP available for lcore %d\n",
+					ipsec_ctx->lcore_id);
 	}
 }