From: Radu Nicolau <radu.nicolau@intel.com>
To: Anoob Joseph <anoobj@marvell.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 10:10:34 +0000 [thread overview]
Message-ID: <d0b298d5-5af4-073f-71e4-c86a82839fcb@intel.com> (raw)
In-Reply-To: <PH0PR18MB467278E713754C9A6DB505EADF592@PH0PR18MB4672.namprd18.prod.outlook.com>
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
next prev parent reply other threads:[~2024-02-27 10:10 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 [this message]
2024-02-27 11:22 ` Anoob Joseph
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=d0b298d5-5af4-073f-71e4-c86a82839fcb@intel.com \
--to=radu.nicolau@intel.com \
--cc=anoobj@marvell.com \
--cc=ciara.power@intel.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=kai.ji@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).