DPDK patches and discussions
 help / color / mirror / Atom feed
From: Anoob Joseph <anoobj@marvell.com>
To: Radu Nicolau <radu.nicolau@intel.com>
Cc: "stable@dpdk.org" <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" <dev@dpdk.org>
Subject: RE: [EXT] [PATCH v3] examples/ipsec-secgw: fix cryptodev to SA mapping
Date: Tue, 27 Feb 2024 11:22:15 +0000	[thread overview]
Message-ID: <PH0PR18MB4672FEF3385377F5FCABEE7BDF592@PH0PR18MB4672.namprd18.prod.outlook.com> (raw)
In-Reply-To: <d0b298d5-5af4-073f-71e4-c86a82839fcb@intel.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

  reply	other threads:[~2024-02-27 11:22 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-11  9:53 [PATCH] " Radu Nicolau
2023-12-12  1:36 ` Ku, Ting-Kai
2024-01-22 13:21 ` Power, Ciara
2024-01-22 13:52 ` Ji, Kai
2024-02-22 17:35 ` [EXT] " Akhil Goyal
2024-02-23 11:01 ` [PATCH v2] " Radu Nicolau
2024-02-26  4:48   ` [EXT] " Anoob Joseph
2024-02-26 10:25 ` [PATCH v3] " Radu Nicolau
2024-02-26 23:02   ` Patrick Robb
2024-02-27  5:19   ` [EXT] " Anoob Joseph
2024-02-27 10:10     ` Radu Nicolau
2024-02-27 11:22       ` Anoob Joseph [this message]
2024-02-27 13:28 ` [PATCH v4] " Radu Nicolau
2024-02-27 13:50   ` [EXT] " Anoob Joseph
2024-03-13 14:26     ` Akhil Goyal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR18MB4672FEF3385377F5FCABEE7BDF592@PH0PR18MB4672.namprd18.prod.outlook.com \
    --to=anoobj@marvell.com \
    --cc=ciara.power@intel.com \
    --cc=dev@dpdk.org \
    --cc=gakhil@marvell.com \
    --cc=kai.ji@intel.com \
    --cc=radu.nicolau@intel.com \
    --cc=stable@dpdk.org \
    --cc=ting-kai.ku@intel.com \
    --cc=vfialko@marvell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).