Hi Radu, Please see inline.
Thanks, Anoob > -----Original Message----- > From: Radu Nicolau <[email protected]> > Sent: Tuesday, February 27, 2024 3:41 PM > To: Anoob Joseph <[email protected]> > Cc: [email protected]; Volodymyr Fialko <[email protected]>; Ting-Kai Ku > <[email protected]>; Ciara Power <[email protected]>; Kai Ji > <[email protected]>; Akhil Goyal <[email protected]>; [email protected] > 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 <[email protected]> > >> Sent: Monday, February 26, 2024 3:56 PM > >> To: [email protected] > >> Cc: Anoob Joseph <[email protected]>; Radu Nicolau > >> <[email protected]>; [email protected]; Volodymyr Fialko > >> <[email protected]>; Ting-Kai Ku <[email protected]>; Ciara > >> Power <[email protected]>; Kai Ji <[email protected]>; Akhil Goyal > >> <[email protected]> > >> 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: [email protected] > >> Cc: [email protected] > >> > >> Signed-off-by: Radu Nicolau <[email protected]> > >> Tested-by: Ting-Kai Ku <[email protected]> > >> Acked-by: Ciara Power <[email protected]> > >> Acked-by: Kai Ji <[email protected]> > >> --- > >> 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
