From: "Ji, Kai" <kai.ji@intel.com>
To: Akhil Goyal <gakhil@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Fan Zhang <royzhang1980@gmail.com>, Ray Kinsella <mdr@ashroe.eu>,
"Burakov, Anatoly" <anatoly.burakov@intel.com>,
"Mcnamara, John" <john.mcnamara@intel.com>
Subject: RE: [EXT] [dpdk-dev v5] lib/cryptodev: multi-process IPC request handler
Date: Thu, 6 Oct 2022 23:11:59 +0000 [thread overview]
Message-ID: <SN6PR11MB3408135A4C4E72F8840DA7F8815C9@SN6PR11MB3408.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB448435DBC1328BE045926218D85C9@CO6PR18MB4484.namprd18.prod.outlook.com>
Hi Akhill,
Thanks for your reply, please see my comments below.
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, October 6, 2022 7:49 PM
> To: Ji, Kai <kai.ji@intel.com>; dev@dpdk.org
> Cc: Fan Zhang <royzhang1980@gmail.com>; Ray Kinsella <mdr@ashroe.eu>;
> Burakov, Anatoly <anatoly.burakov@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>
> Subject: RE: [EXT] [dpdk-dev v5] lib/cryptodev: multi-process IPC request
> handler
>
> > As some cryptode PMDs have multiprocess support, the secondary process
> > needs queue-pair to be configured by the primary process before to
> > use. This patch adds an IPC register function to help the primary
> > process to register IPC action that allow secondary process to
> > configure cryptodev queue-pair via IPC messages during the runtime.
>
> Why are we forcing user another alternate API for secondary process to
> work?
> Can we not register the IPC inside rte_cryptodev_queue_pair_setup() ?
>
> As I understand till now,
> You have introduced another API rte_cryptodev_mp_request_register(),
> Which will be called by application if primary-secondary communication is
> required.
> And if it is registered, rte_cryptodev_ipc_request() will be called from
> somewhere(not sure when this will be called).
> And the call to rte_cryptodev_queue_pair_setup() from the secondary will do
> nothing.
[KJ] I'm try to solve the following setups:
The primary process initialized crypto device, but the secondary process is setting up the queue pairs by calling rte_cryptodev_queue_pair_setup(). Although DPDK memzone is visible between processes, the ipsec-mb external library will allocate a buffer using regular malloc and write function pointers to this buffer – the ipsec mb PMD had to call them later in dequeue function to process crypto. Since the function pointer addresses are not shared between processes , so letting secondary process to dequeue a crypto op will case segfault. With above issue in mind, the ipsec_mb PMD add process check to prevent secondary process to setup queue pairs.
In this design, before secondary process calling rte_cryptodev_queue_pair_setup, I would expect it send out IPC message to primary first. then the rte_cryptodev_ipc_request () will be executed in primary context where rte_cryptodev_queue_pair_setup() is allowed to configure queue pair based on IPC request. Once the new queue pair setup'ed and related memory been populated by primary, the secondary can call rte_cryptodev_queue_pair_setup() to use it.
>
> Is this a correct understanding? If it is correct, then it is an unnecessary
> overhead for the application.
> We should update the rte_cryptodev_queue_pair_setup instead to handle
> primary and secondary configuration.
> IMO, you do not need to change anything in the library.
> Everything can be handled in the PMD. When the queue_pair_setup is called
> for particular qp_id, Store the getpid() of the calling process into the priv
> data of queue pair if it is not already configured And if configured return
> failure.
> And in case of release you can also check the same.
[KJ] I think you are right, all the problems I'm try to resolve can be done in ipsec-mb PMD level, it is unnecessary to add new API to the cryptodev library. I will change the design to pmd level and rework the patch.
>
> The configuration of queues for multi process is specific to PMDs.
> There may be PMDs which may support same queue pair to be used by
> different processes.
> Rx queue from the qp by one process and Tx queue from the qp by another
> process.
> This will be needed if one process is doing only enqueue and the other only
> dequeue on the same qp.
> So in that case, your implementation will not work.
>
> > After setup, a new "qp_in_used_pid" param stores the PID to provide
> > the ownership of the queue-pair so that only the PID matched
> > queue-pair free request is allowed in the future.
> >
> qp_in_used_pid looks very cryptic, I believe this should be part of queue pair
> private data of PMD.
> Adding this in cryptodev data is not justified. This property is per queue and
> not per crypto device.
> Hence adding in device data does not make sense to me.
>
[KJ] point made
next prev parent reply other threads:[~2022-10-06 23:12 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-26 23:08 [dpdk-dev v1] " Kai Ji
2022-07-27 4:25 ` [EXT] " Akhil Goyal
2022-08-05 8:51 ` Zhang, Roy Fan
2022-08-08 7:43 ` Akhil Goyal
2022-08-12 8:06 ` Zhang, Roy Fan
2022-08-12 8:25 ` Akhil Goyal
2022-09-21 18:37 ` Akhil Goyal
2022-10-02 1:43 ` [dpdk-dev v2] " Kai Ji
2022-10-02 18:57 ` [EXT] " Akhil Goyal
2022-10-02 22:44 ` [dpdk-dev v3 1/1] " Kai Ji
2022-10-03 16:39 ` Power, Ciara
2022-10-04 18:12 ` [EXT] " Akhil Goyal
2022-10-06 0:57 ` Ji, Kai
2022-10-06 8:16 ` [dpdk-dev v4] " Kai Ji
2022-10-06 16:19 ` Power, Ciara
2022-10-06 17:06 ` [dpdk-dev v5] " Kai Ji
2022-10-06 18:49 ` [EXT] " Akhil Goyal
2022-10-06 23:11 ` Ji, Kai [this message]
2022-10-07 9:37 ` Zhang, Fan
2022-10-06 22:41 ` Konstantin Ananyev
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=SN6PR11MB3408135A4C4E72F8840DA7F8815C9@SN6PR11MB3408.namprd11.prod.outlook.com \
--to=kai.ji@intel.com \
--cc=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=john.mcnamara@intel.com \
--cc=mdr@ashroe.eu \
--cc=royzhang1980@gmail.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).