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

[Anoob Joseph <anoobj@marvell.com>]

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