Hi Akhill, Thanks for your reply, please see my comments below.
> -----Original Message----- > From: Akhil Goyal <gak...@marvell.com> > Sent: Thursday, October 6, 2022 7:49 PM > To: Ji, Kai <kai...@intel.com>; dev@dpdk.org > Cc: Fan Zhang <royzhang1...@gmail.com>; Ray Kinsella <m...@ashroe.eu>; > Burakov, Anatoly <anatoly.bura...@intel.com>; Mcnamara, John > <john.mcnam...@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
