Hi Radu,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Radu Nicolau <radu.nico...@intel.com>
> Sent: Tuesday, February 27, 2024 3:41 PM
> To: Anoob Joseph <ano...@marvell.com>
> Cc: sta...@dpdk.org; Volodymyr Fialko <vfia...@marvell.com>; Ting-Kai Ku
> <ting-kai...@intel.com>; Ciara Power <ciara.po...@intel.com>; Kai Ji
> <kai...@intel.com>; Akhil Goyal <gak...@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.nico...@intel.com>
> >> Sent: Monday, February 26, 2024 3:56 PM
> >> To: dev@dpdk.org
> >> Cc: Anoob Joseph <ano...@marvell.com>; Radu Nicolau
> >> <radu.nico...@intel.com>; sta...@dpdk.org; Volodymyr Fialko
> >> <vfia...@marvell.com>; Ting-Kai Ku <ting-kai...@intel.com>; Ciara
> >> Power <ciara.po...@intel.com>; Kai Ji <kai...@intel.com>; Akhil Goyal
> >> <gak...@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: sta...@dpdk.org
> >> Cc: vfia...@marvell.com
> >>
> >> Signed-off-by: Radu Nicolau <radu.nico...@intel.com>
> >> Tested-by: Ting-Kai Ku <ting-kai...@intel.com>
> >> Acked-by: Ciara Power <ciara.po...@intel.com>
> >> Acked-by: Kai Ji <kai...@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
