[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.
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.

Signed-off-by: Kai Ji <kai.ji@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
---
v5:
- fix of unittest error
- cryptodev prog_guide updates

v4:
- release note update
- Doxygen comments update

v3:
- addin missing free function for qp_in_use_by_pid

v2:
- code rework
---
 doc/guides/prog_guide/cryptodev_lib.rst | 21 ++++++
 doc/guides/rel_notes/release_22_11.rst  |  8 ++-
 lib/cryptodev/cryptodev_pmd.h           |  3 +-
 lib/cryptodev/rte_cryptodev.c           | 93 +++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h           | 46 ++++++++++++
 lib/cryptodev/version.map               |  2 +
 6 files changed, 171 insertions(+), 2 deletions(-)

--
2.17.1
  

> 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.

> 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.
  

06/10/2022 18:06, Kai Ji пишет:
> 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.
> 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.

A stupid question - how syncronisation is supposed to be guaranteed
with such approach?
Let say secondary process sends an IPC message to configure 
crypto-queue, and at the same moment primary process (for whatever reason)
desicdes to reconfigure same crypto-dev.
Are there any way to prevent such race-conditions to happen?


> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> Acked-by: Ciara Power <ciara.power@intel.com>
> ---
> v5:
> - fix of unittest error
> - cryptodev prog_guide updates
> 
> v4:
> - release note update
> - Doxygen comments update
> 
> v3:
> - addin missing free function for qp_in_use_by_pid
> 
> v2:
> - code rework
> ---
>   doc/guides/prog_guide/cryptodev_lib.rst | 21 ++++++
>   doc/guides/rel_notes/release_22_11.rst  |  8 ++-
>   lib/cryptodev/cryptodev_pmd.h           |  3 +-
>   lib/cryptodev/rte_cryptodev.c           | 93 +++++++++++++++++++++++++
>   lib/cryptodev/rte_cryptodev.h           | 46 ++++++++++++
>   lib/cryptodev/version.map               |  2 +
>   6 files changed, 171 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/cryptodev_lib.rst b/doc/guides/prog_guide/cryptodev_lib.rst
> index 01aad842a9..19bcda24bc 100644
> --- a/doc/guides/prog_guide/cryptodev_lib.rst
> +++ b/doc/guides/prog_guide/cryptodev_lib.rst
> @@ -1180,6 +1180,27 @@ Asymmetric Crypto Device API
>   The cryptodev Library API is described in the
>   `DPDK API Reference <https://doc.dpdk.org/api/>`_
> 
> +Multi-process IPC request register support
> +------------------------------------------
> +As some cryptode PMDs have multiprocess support, the queue-pairs used in the
> +secondary process are required to be configured by the primary process first.
> +The cryptodev IPC request handler provide an alternative way to setup
> +queue-pair from the secondary process via IPC messages in the runtime.
> +
> +The ``rte_cryptodev_mp_request_register`` function, called in primary process,
> +register the ``ret_cryptodev_ipc_request`` function for IPC message request
> +handling between primary and secondary process.
> +The supported IPC request types are:
> +
> +.. code-block:: c
> +    RTE_CRYPTODEV_MP_REQ_QP_SET
> +    RTE_CRYPTODEV_MP_REQ_QP_FREE
> +
> +It up to the secondary process to fill-in required params in order to allow
> +``rte_cryptodev_queue_pair_setup`` setup a queue-pair correctly.
> +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.
> 
>   Device Statistics
>   -----------------
> diff --git a/doc/guides/rel_notes/release_22_11.rst b/doc/guides/rel_notes/release_22_11.rst
> index 4e64710a69..d60756318b 100644
> --- a/doc/guides/rel_notes/release_22_11.rst
> +++ b/doc/guides/rel_notes/release_22_11.rst
> @@ -87,6 +87,13 @@ New Features
>     Added MACsec transform for rte_security session and added new API
>     to configure security associations (SA) and secure channels (SC).
> 
> +* **Added MP IPC request register function in cryptodev.**
> +
> +  Added new functions ``rte_cryptodev_mp_request_register()`` and
> +  ``rte_cryptodev_mp_request_unregister()``.
> +  The function helps the primary process to register IPC action that allow
> +  secondary process to request cryptodev queue pairs setups via IPC messages.
> +
>   * **Added new algorithms to cryptodev.**
> 
>     * Added symmetric hash algorithm ShangMi 3 (SM3).
> @@ -123,7 +130,6 @@ New Features
>     into single event containing ``rte_event_vector``
>     whose event type is ``RTE_EVENT_TYPE_CRYPTODEV_VECTOR``.
> 
> -
>   Removed Items
>   -------------
> 
> diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
> index f27b3249ea..574aebe279 100644
> --- a/lib/cryptodev/cryptodev_pmd.h
> +++ b/lib/cryptodev/cryptodev_pmd.h
> @@ -78,7 +78,8 @@ struct rte_cryptodev_data {
>   	void **queue_pairs;
>   	/** Number of device queue pairs. */
>   	uint16_t nb_queue_pairs;
> -
> +	/** Array of process id used for queue pairs **/
> +	uint16_t *qp_in_use_by_pid;
>   	/** PMD-specific private data */
>   	void *dev_private;
>   } __rte_cache_aligned;
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 2165a0688c..4c60aed481 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -49,6 +49,9 @@ struct rte_crypto_fp_ops rte_crypto_fp_ops[RTE_CRYPTO_MAX_DEVS];
>   /* spinlock for crypto device callbacks */
>   static rte_spinlock_t rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER;
> 
> +/* crypto queue pair config */
> +#define CRYPTODEV_MP_REQ "cryptodev_mp_request"
> +
>   /**
>    * The user application callback description.
>    *
> @@ -1047,6 +1050,9 @@ rte_cryptodev_pmd_release_device(struct rte_cryptodev *cryptodev)
>   			return ret;
>   	}
> 
> +	if (cryptodev->data->qp_in_use_by_pid)
> +		rte_free(cryptodev->data->qp_in_use_by_pid);
> +
>   	ret = rte_cryptodev_data_free(dev_id, &cryptodev_globals.data[dev_id]);
>   	if (ret < 0)
>   		return ret;
> @@ -1135,6 +1141,21 @@ rte_cryptodev_queue_pairs_config(struct rte_cryptodev *dev, uint16_t nb_qpairs,
> 
>   	}
>   	dev->data->nb_queue_pairs = nb_qpairs;
> +
> +	if (dev->data->qp_in_use_by_pid == NULL) {
> +		dev->data->qp_in_use_by_pid = rte_zmalloc_socket(
> +				"cryptodev->qp_in_use_by_pid",
> +				sizeof(dev->data->qp_in_use_by_pid[0]) *
> +				dev_info.max_nb_queue_pairs,
> +				RTE_CACHE_LINE_SIZE, socket_id);
> +		if (dev->data->qp_in_use_by_pid == NULL) {
> +			CDEV_LOG_ERR("failed to get memory for qp meta data, "
> +							"nb_queues %u",
> +							nb_qpairs);
> +			return -(ENOMEM);
> +		}
> +	}
> +
>   	return 0;
>   }
> 
> @@ -1401,6 +1422,78 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
>   			socket_id);
>   }
> 
> +static int
> +rte_cryptodev_ipc_request(const struct rte_mp_msg *mp_msg, const void *peer)
> +{
> +	struct rte_mp_msg mp_res;
> +	struct rte_cryptodev_mp_param *resp_param =
> +		(struct rte_cryptodev_mp_param *)mp_res.param;
> +	const struct rte_cryptodev_mp_param *req_param =
> +		(const struct rte_cryptodev_mp_param *)mp_msg->param;
> +
> +	int ret;
> +	struct rte_cryptodev *dev;
> +	uint16_t *qp_in_used_by_pid;
> +	int dev_id = req_param->dev_id;
> +	int qp_id = req_param->qp_id;
> +	struct rte_cryptodev_qp_conf *queue_conf = req_param->queue_conf;
> +
> +	resp_param->result = -EINVAL;
> +	if (!rte_cryptodev_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		goto out;
> +	}
> +
> +	if (!rte_cryptodev_get_qp_status(dev_id, qp_id))
> +		goto out;
> +
> +	dev = &rte_crypto_devices[dev_id];
> +	qp_in_used_by_pid = dev->data->qp_in_use_by_pid;
> +
> +	switch (req_param->type) {
> +	case RTE_CRYPTODEV_MP_REQ_QP_SET:
> +		ret = rte_cryptodev_queue_pair_setup(dev_id, qp_id,
> +				queue_conf, req_param->socket_id);
> +		if (!ret)
> +			qp_in_used_by_pid[qp_id] = req_param->process_id;
> +		resp_param->result = ret;
> +		break;
> +	case RTE_CRYPTODEV_MP_REQ_QP_FREE:
> +		if (qp_in_used_by_pid[qp_id] != req_param->process_id) {
> +			CDEV_LOG_ERR("Unable to release qp_id=%d", qp_id);
> +			goto out;
> +		}
> +
> +		ret = (*dev->dev_ops->queue_pair_release)(dev, qp_id);
> +		if (!ret)
> +			qp_in_used_by_pid[qp_id] = 0;
> +
> +		resp_param->result = ret;
> +		break;
> +	default:
> +		CDEV_LOG_ERR("invalid mp request type\n");
> +	}
> +
> +out:
> +	ret = rte_mp_reply(&mp_res, peer);
> +	return ret;
> +}
> +
> +int
> +rte_cryptodev_mp_request_register(void)
> +{
> +	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
> +	return rte_mp_action_register(CRYPTODEV_MP_REQ,
> +				rte_cryptodev_ipc_request);
> +}
> +
> +void
> +rte_cryptodev_mp_request_unregister(void)
> +{
> +	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
> +	rte_mp_action_unregister(CRYPTODEV_MP_REQ);
> +}
> +
>   struct rte_cryptodev_cb *
>   rte_cryptodev_add_enq_callback(uint8_t dev_id,
>   			       uint16_t qp_id,
> diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
> index ece7157970..80076f487c 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -539,6 +539,30 @@ enum rte_cryptodev_event_type {
>   	RTE_CRYPTODEV_EVENT_MAX		/**< max value of this enum */
>   };
> 
> +/** Request types for IPC. */
> +enum rte_cryptodev_mp_req_type {
> +	RTE_CRYPTODEV_MP_REQ_NONE, /**< unknown event type */
> +	RTE_CRYPTODEV_MP_REQ_QP_SET, /**< Queue pair setup request */
> +	RTE_CRYPTODEV_MP_REQ_QP_FREE /**< Queue pair free request */
> +};
> +
> +/** Parameters for IPC. */
> +struct rte_cryptodev_mp_param {
> +	enum rte_cryptodev_mp_req_type type; /**< IPC request type */
> +	int dev_id;
> +	/**< The identifier of the device */
> +	int qp_id;
> +	/**< The index of the queue pair to be configured */
> +	int socket_id;
> +	/**< Socket to allocate resources on */
> +	uint16_t process_id;
> +	/**< The pid who send out the requested */
> +	struct rte_cryptodev_qp_conf *queue_conf;
> +	/**< A pointer of Crypto device queue pair configuration structure */
> +	int result;
> +	/**< The request result for response message */
> +};
> +
>   /** Crypto device queue pair configuration structure. */
>   struct rte_cryptodev_qp_conf {
>   	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
> @@ -767,6 +791,28 @@ extern int
>   rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
>   		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id);
> 
> +/**
> + * Register multi process request IPC handler
> + *
> + * Allow secondary process to send IPC request to setup queue pairs
> + * once register function called in primary process.
> + *
> + * @return
> + *	 - 0: Success registered
> + *	 - 1: Failed registration failed
> + *	 - EINVAL: device was not configured
> + */
> +__rte_experimental
> +int
> +rte_cryptodev_mp_request_register(void);
> +
> +/**
> + * Unregister multi process unrequest IPC handler
> + */
> +__rte_experimental
> +void
> +rte_cryptodev_mp_request_unregister(void);
> +
>   /**
>    * Get the status of queue pairs setup on a specific crypto device
>    *
> diff --git a/lib/cryptodev/version.map b/lib/cryptodev/version.map
> index 00c99fb45c..e964a3d5ab 100644
> --- a/lib/cryptodev/version.map
> +++ b/lib/cryptodev/version.map
> @@ -150,6 +150,8 @@ EXPERIMENTAL {
>   	__rte_cryptodev_trace_sym_session_get_user_data;
>   	__rte_cryptodev_trace_sym_session_set_user_data;
>   	__rte_cryptodev_trace_count;
> +	rte_cryptodev_mp_request_register;
> +	rte_cryptodev_mp_request_unregister;
>   };
> 
>   INTERNAL {
> --
> 2.17.1
>
  

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
  

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