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

["Zhang, Fan" <royzhang1980@gmail.com>]

Hi Akhil,

On 06/10/2022 19:49, Akhil Goyal wrote:
>> 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.
>
> 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.
>
> 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.

This is a question we didn't think as comprehensive as you did. With the 
change Kai did at least all Intel PMDs will support that.

I assume we need some feature flag to state that?

>> 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.
>
Agreed. The PID storage is not mandatory for every PMD but only for some 
(ipsec-mb for example) so we should store the PID info inside the PMD 
queue pair data instead.


Regards,

Fan