RE: [EXT] [dpdk-dev v5] lib/cryptodev: multi-process IPC request handler

["Ji, Kai" <kai.ji@intel.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