lib/cryptodev: multi-process IPC request handler

This patch add in multi-process IPC request handler function in rte cryptodev. This function intend to support a queue-pair configuration request to allow the secondary process to reconfigure the queue-pair setup'ed by the primary process.

Signed-off-by Kai Ji <[email protected]>

---
 lib/cryptodev/rte_cryptodev.c | 45 +++++++++++++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h | 14 +++++++++++
 2 files changed, 59 insertions(+)

lib/cryptodev/rte_cryptodev.c +45 -0

index 42f3221052..18fdbf6db6 100644
@@ -1202,6 +1202,51 @@ rte_cryptodev_get_qp_status(uint8_t dev_id,
 uint16_t queue_pair_id)
 	return 0;
 }
 
+/* crypto queue pair config */
+#define CRYPTODEV_MP_REQ "cryptodev_mp_request"
+
+static int
+rte_cryptodev_mp_request(const struct rte_mp_msg *mp_msg, const void
 *peer)
+{
+	struct rte_mp_msg mp_res;
+	struct rte_cryptodev_mp_param *res =
+		(struct rte_cryptodev_mp_param *)mp_res.param;
+	const struct rte_cryptodev_mp_param *param =
+		(const struct rte_cryptodev_mp_param *)mp_msg->param;
+
+	int ret;
+	int dev_id = 0;
+	int port_id = 0, socket_id = 1;
+	struct rte_cryptodev_qp_conf queue_conf;
+	queue_conf.nb_descriptors = 2;
+
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	switch (param->type) {
+	case RTE_CRYPTODEV_MP_REQ_QP:
+		ret = rte_cryptodev_queue_pair_setup(dev_id, port_id,
+				&queue_conf, socket_id);
+		res->result = ret;
+
+		ret = rte_mp_reply(&mp_res, peer);
+		break;
+	default:
+		CDEV_LOG_ERR("invalid mp request type\n");
+		return -EINVAL;
+	}
+	return ret;
+
+}
+
+int rte_cryptodev_mp_request_register(void)
+{
+	int ret;
+
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	ret = rte_mp_action_register(CRYPTODEV_MP_REQ,
+				rte_cryptodev_mp_request);
+	return ret;
+}
+
 int

rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,

lib/cryptodev/rte_cryptodev.c +0 -0

 		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)

lib/cryptodev/rte_cryptodev.h +14 -1

index 56f459c6a0..901465ca65 100644
@@ -539,6 +539,18 @@ 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,
+	RTE_CRYPTODEV_MP_REQ_QP
+};
+
+/* Parameters for IPC. */
+struct rte_cryptodev_mp_param {
+	enum rte_cryptodev_mp_req_type type;
+	int result;
+};
+
 /** Crypto device queue pair configuration structure. */
 struct rte_cryptodev_qp_conf {
 	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */

@@ -744,6 +756,8 @@ rte_cryptodev_stop(uint8_t dev_id);
 extern int
 rte_cryptodev_close(uint8_t dev_id);
 
+extern int rte_cryptodev_mp_request_register(void);
+
 /**
  * Allocate and set up a receive queue pair for a device.
  *
--

2.17.1

-----Original Message-----
From: Akhil Goyal <[email protected]>
Sent: Wednesday, July 27, 2022 5:26 AM
To: Ji, Kai <[email protected]>; [email protected]
Cc: Anoob Joseph <[email protected]>; [email protected]; Zhang, Roy Fan <[email protected]>; Chandubabu Namburu <[email protected]>; Ruifeng Wang <[email protected]>; [email protected]; Michael Shamis <[email protected]>; Nagadheeraj Rottela <[email protected]>; [email protected]; Jay Zhou <[email protected]>
Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request handler

This is a library change you should cc all PMD owners while sending patch.

Quoted 5 lines

This patch add in multi-process IPC request handler function in rte cryptodev. This function intend to support a queue-pair configuration request to allow the secondary process to reconfigure the queue-pair setup'ed by the primary process.

Who will release the queue pair already setup by primary in the first place?

Currently, all queues are setup by primary and secondary uses them. So if a queue is re-initialized by secondary, and if it is being used in primary process,
Wont that drop packets abruptly if the queue is re-initialized?

Also, I see register API but not deregister.

Hi Akhil,

Quoted 1 line

This is a library change you should cc all PMD owners while sending patch.

Kai is in holiday at the moment and will be back in a week. I will sync with him then.

Quoted 6 lines

Quoted 5 lines

This patch add in multi-process IPC request handler function in rte cryptodev. This function intend to support a queue-pair configuration request to allow the secondary process to reconfigure the queue-pair setup'ed by the primary process.

Quoted 1 line

Who will release the queue pair already setup by primary in the first place?

Fan: If the queue pair already setup by primary the secondary shall not recreate it
but use it instead.

Quoted 5 lines

Currently, all queues are setup by primary and secondary uses them. So if a queue is re-initialized by secondary, and if it is being used in primary process,
Wont that drop packets abruptly if the queue is re-initialized?

You are right. What about creating a variable in the queue pair with either PID or thread id who own the queue pair?

-----Original Message-----
From: Akhil Goyal <[email protected]>
Sent: Monday, August 8, 2022 8:43 AM
To: Zhang, Roy Fan <[email protected]>; Ji, Kai <[email protected]>; [email protected]
Cc: Anoob Joseph <[email protected]>; [email protected]; Chandubabu Namburu <[email protected]>; Ruifeng Wang <[email protected]>; [email protected]; Michael Shamis <[email protected]>; Nagadheeraj Rottela <[email protected]>; [email protected]; Jay Zhou <[email protected]>
Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request handler

Hi Fan,

Quoted 4 lines

Hi Akhil,

Quoted 1 line

This is a library change you should cc all PMD owners while sending patch.

Kai is in holiday at the moment and will be back in a week. I will sync with

him

Quoted 10 lines

then.

Quoted 7 lines

Quoted 5 lines

This patch add in multi-process IPC request handler function in rte cryptodev. This function intend to support a queue-pair configuration request to allow the secondary process to reconfigure the queue-pair setup'ed by the primary process.

Who will release the queue pair already setup by primary in the first place?

Fan: If the queue pair already setup by primary the secondary shall not

recreate

Quoted 2 lines

it
but use it instead.

OK but the description says secondary would reconfigure the qp setup by primary.

Quoted 3 lines

Quoted 3 lines

Currently, all queues are setup by primary and secondary uses them. So if a queue is re-initialized by secondary, and if it is being used in

primary

Quoted 3 lines

Quoted 2 lines

process,
Wont that drop packets abruptly if the queue is re-initialized?

Quoted 1 line

You are right. What about creating a variable in the queue pair with either

PID

Quoted 1 line

or thread id who own the queue pair?

I believe we should not expose the PID/thread id via queue to the user application.
This may be security issue.

Instead an "in_use" parameter can be added which can tell if sone other process is using it or not.
And this in_use param also need not be exposed to user. It can be completely hidden in the PMD.
User will get an error number(probably -EUSERS) indicating the queue pair is already in use.

Regards,
Akhil

Quoted 7 lines

Instead an "in_use" parameter can be added which can tell if sone other process is using it or not.
And this in_use param also need not be exposed to user. It can be completely hidden in the PMD.
User will get an error number(probably -EUSERS) indicating the queue pair is already in use.

Great idea. That's what I am after too. So can I sum up the following change?

- each queue pair has a "in_use" param. I believe we can refine this a bit by a "not_in_use", "in_use_by_primary" and "in_use_by_secondary" enum.

- the secondary process may request to configure a queue pair by sending message to primary
- as of requesting freeing a queue pair
- primary can free any queue pair.
- but for secondary to free a queue pair, we have a problem:
- we may allow secondary to request freeing the queue pair if it is "in_use_by_secondary". But then there may be a security issue as a secondary can free a queue pair used by different secondary process.

- or we may not allow secondary process to request freeing any queue pair, it is securer, but less flexible.

Quoted 3 lines

Regards,
Akhil

Regards,
Fan

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

Quoted 8 lines

Quoted 7 lines

Instead an "in_use" parameter can be added which can tell if sone other process is using it or not.
And this in_use param also need not be exposed to user. It can be completely hidden in the PMD.
User will get an error number(probably -EUSERS) indicating the queue pair is already in use.

Quoted 4 lines

Great idea. That's what I am after too. So can I sum up the following change?

- each queue pair has a "in_use" param. I believe we can refine this a bit by a "not_in_use", "in_use_by_primary" and "in_use_by_secondary" enum.

This is specific to the PMD. Each PMD may have its own implementation.

Quoted 9 lines

- the secondary process may request to configure a queue pair by sending message to primary
- as of requesting freeing a queue pair
- primary can free any queue pair.
- but for secondary to free a queue pair, we have a problem:
- we may allow secondary to request freeing the queue pair if it is "in_use_by_secondary". But then there may be a security issue as a secondary can free a queue pair used by different secondary process.

Is it not possible to save pid(inside PMD) of the requesting process in queue private data
which is being configured?

Quoted 3 lines

- or we may not allow secondary process to request freeing any queue pair, it is securer, but less flexible.

This patch add a function to support queue-pair configuration request to allow the primary or secondary process to setup/free the queue-pair via IPC handler.

Signed-off-by Kai Ji <[email protected]>

-----Original Message-----
From: Kai Ji <[email protected]>
Sent: Sunday 2 October 2022 23:45
To: [email protected]
Cc: [email protected]; Ji, Kai <[email protected]>
Subject: [dpdk-dev v3 1/1] lib/cryptodev: multi-process IPC request handler

This patch add a function to support queue-pair configuration request to allow the primary or secondary process to setup/free the queue-pair via IPC handler. Add in queue pair in-used by process id array in rte_cryptodev_data for pid tracking.

Signed-off-by Kai Ji <[email protected]>

+/**
+ * Register multi process request IPC handler
+ *
+ * @return
+ * - 0: Success registered
+ * - 1: Failed registration failed
+ * - -EINVAL: device was not configured

This patch add a function to support queue-pair configuration request to allow the primary or secondary process to setup/free the queue-pair via IPC handler. Add in queue pair in-used by process id array in rte_cryptodev_data for pid tracking.

Signed-off-by Kai Ji <[email protected]>

---
v3:
- addin missing free function for qp_in_use_by_pid

v2:
- code rework

---
 lib/cryptodev/cryptodev_pmd.h |  3 +-
 lib/cryptodev/rte_cryptodev.c | 92 +++++++++++++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h | 37 ++++++++++++++
 lib/cryptodev/version.map     |  2 +
 4 files changed, 133 insertions(+), 1 deletion(-)

lib/cryptodev/cryptodev_pmd.h +0 -0

index 09ba952455..f404604963 100644

lib/cryptodev/cryptodev_pmd.h +0 -0

 	/** PMD-specific private data */
 	void *dev_private;
 } __rte_cache_aligned;

lib/cryptodev/rte_cryptodev.c +3 -0

index 9e76a1c72d..dab6a37bff 100644
@@ -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.

*

lib/cryptodev/rte_cryptodev.c +3 -0

@@ -1050,6 +1053,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]);

lib/cryptodev/rte_cryptodev.c +9 -0

 	if (ret < 0)
 		return ret;

@@ -1138,6 +1144,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, "

lib/cryptodev/rte_cryptodev.c +67 -0

+							"nb_queues %u",
+							nb_qpairs);
+			return -(ENOMEM);
+		}
+	}
+
 	return 0;
 }

@@ -1400,6 +1421,77 @@ 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 *res =
+		(struct rte_cryptodev_mp_param *)mp_res.param;
+	const struct rte_cryptodev_mp_param *param =
+		(const struct rte_cryptodev_mp_param *)mp_msg->param;
+
+	int ret;
+	struct rte_cryptodev *dev;
+	uint16_t *qps_in_used_by_pid;
+	int dev_id = param->dev_id;
+	int qp_id = param->qp_id;
+	struct rte_cryptodev_qp_conf *queue_conf = param->queue_conf;
+
+	res->result = -EINVAL;
+	if (!rte_cryptodev_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, dev_id);
+		goto out;
+	}
+
+	if (!rte_cryptodev_get_qp_status(dev_id, qp_id))
+		goto out;
+
+	dev = &rte_crypto_devices[dev_id];
+	qps_in_used_by_pid = dev->data->qp_in_use_by_pid;
+
+	switch (param->type) {
+	case RTE_CRYPTODEV_MP_REQ_QP_SET:
+		ret = rte_cryptodev_queue_pair_setup(dev_id, qp_id,
+				queue_conf, param->socket_id);
+		if (!ret)
+			qps_in_used_by_pid[qp_id] = param->process_id;
+		res->result = ret;
+		break;
+	case RTE_CRYPTODEV_MP_REQ_QP_FREE:
+		if ((rte_eal_process_type() == RTE_PROC_SECONDARY) &&
+			(qps_in_used_by_pid[qp_id] != param->process_id)) {
+			CDEV_LOG_ERR("Unable to release qp_id=%" PRIu8,
 qp_id);
+			goto out;
+		}
+
+		ret = (*dev->dev_ops->queue_pair_release)(dev, qp_id);
+		if (!ret)
+			qps_in_used_by_pid[qp_id] = 0;
+
+		res->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);

struct rte_cryptodev_cb *
rte_cryptodev_add_enq_callback(uint8_t dev_id,

lib/cryptodev/rte_cryptodev.c +0 -0

 			       uint16_t qp_id,

lib/cryptodev/rte_cryptodev.h +0 -0

index 56f459c6a0..d8cadebd0c 100644

lib/cryptodev/rte_cryptodev.h +16 -0

+ * @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
  *

lib/cryptodev/version.map +2 -1

index 6d9b3e01a6..4130c39e7c 100644
@@ -157,6 +157,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

-----Original Message-----
From: Akhil Goyal <[email protected]>
Sent: Tuesday, October 4, 2022 7:13 PM
To: Ji, Kai <[email protected]>; [email protected]
Subject: RE: [EXT] [dpdk-dev v3 1/1] lib/cryptodev: multi-process IPC request handler

Quoted 6 lines

This patch add a function to support queue-pair configuration request to allow the primary or secondary process to setup/free the queue-pair via IPC handler. Add in queue pair in-used by process id array in rte_cryptodev_data for pid tracking.

Signed-off-by Kai Ji <[email protected]>

I had a comment on v1, that you should include all PMD maintainers as well When sending any major change in library layer.
But I think you have not read the comments made on v1.

Quoted 18 lines

---
v3:
- addin missing free function for qp_in_use_by_pid

v2:
- code rework
---
lib/cryptodev/cryptodev_pmd.h | 3 +- lib/cryptodev/rte_cryptodev.c
| 92 +++++++++++++++++++++++++++++++++++

 lib/cryptodev/rte_cryptodev.h | 37 ++++++++++++++
 lib/cryptodev/version.map     |  2 +
 4 files changed, 133 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/lib/cryptodev/cryptodev_pmd.h index 09ba952455..f404604963 100644

lib/cryptodev/cryptodev_pmd.h struct rte_cryptodev_data { · line 82

Quoted 6 lines

lib/cryptodev/cryptodev_pmd.h +2 -1

@@ -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;

Why array? And if an array, how will its depth will be calculated.

I commented on v1, that the in_use pid can be stored inside queue private data.

Quoted 24 lines

lib/cryptodev/cryptodev_pmd.h +0 -0

 	/** PMD-specific private data */
 	void *dev_private;
 } __rte_cache_aligned;

(quoted diff) +0 -0

b/lib/cryptodev/rte_cryptodev.c index 9e76a1c72d..dab6a37bff 100644

lib/cryptodev/rte_cryptodev.c +3 -0

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

*

lib/cryptodev/rte_cryptodev.c +0 -0

@@ -1050,6 +1053,9 @@ rte_cryptodev_pmd_release_device(struct
 rte_cryptodev *cryptodev)
 			return ret;
 	}
 

lib/cryptodev/rte_cryptodev.c rte_cryptodev_pmd_release_device(struct rte_cryptodev *cryptodev) · line 1057

Quoted 2 lines

lib/cryptodev/rte_cryptodev.c +2 -0

+	if (cryptodev->data->qp_in_use_by_pid)
+		rte_free(cryptodev->data->qp_in_use_by_pid);

Quoted 8 lines

lib/cryptodev/rte_cryptodev.c +1 -0

+
 	ret = rte_cryptodev_data_free(dev_id,
 &cryptodev_globals.data[dev_id]);

lib/cryptodev/rte_cryptodev.c +0 -0

 	if (ret < 0)
 		return ret;

@@ -1138,6 +1144,21 @@ rte_cryptodev_queue_pairs_config(struct
 rte_cryptodev *dev, uint16_t nb_qpairs,
 

lib/cryptodev/rte_cryptodev.c rte_cryptodev_queue_pairs_config(struct rte_cryptodev *dev, uint16_t nb_qpairs, · line 1153

Quoted 9 lines

lib/cryptodev/rte_cryptodev.c +7 -0

 	}
 	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);

lib/cryptodev/rte_cryptodev.c rte_cryptodev_queue_pair_setup(uint8_t

Quoted 13 lines

lib/cryptodev/rte_cryptodev.c +2 -0

+		if (dev->data->qp_in_use_by_pid == NULL) {
+			CDEV_LOG_ERR("failed to get memory for qp meta

data, "

lib/cryptodev/rte_cryptodev.c +6 -0

+							"nb_queues %u",
+							nb_qpairs);
+			return -(ENOMEM);
+		}
+	}
+
 	return 0;
 }

@@ -1400,6 +1421,77 @@ rte_cryptodev_queue_pair_setup(uint8_t

dev_id,

lib/cryptodev/rte_cryptodev.c rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,

Quoted 6 lines

lib/cryptodev/rte_cryptodev.c +2 -0

 uint16_t queue_pair_id,
 			socket_id);
 }
 
+static int
+rte_cryptodev_ipc_request(const struct rte_mp_msg *mp_msg, const

void

Quoted 66 lines

lib/cryptodev/rte_cryptodev.c +64 -0

 *peer)
+{
+	struct rte_mp_msg mp_res;
+	struct rte_cryptodev_mp_param *res =
+		(struct rte_cryptodev_mp_param *)mp_res.param;
+	const struct rte_cryptodev_mp_param *param =
+		(const struct rte_cryptodev_mp_param *)mp_msg->param;
+
+	int ret;
+	struct rte_cryptodev *dev;
+	uint16_t *qps_in_used_by_pid;
+	int dev_id = param->dev_id;
+	int qp_id = param->qp_id;
+	struct rte_cryptodev_qp_conf *queue_conf = param->queue_conf;
+
+	res->result = -EINVAL;
+	if (!rte_cryptodev_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, dev_id);
+		goto out;
+	}
+
+	if (!rte_cryptodev_get_qp_status(dev_id, qp_id))
+		goto out;
+
+	dev = &rte_crypto_devices[dev_id];
+	qps_in_used_by_pid = dev->data->qp_in_use_by_pid;
+
+	switch (param->type) {
+	case RTE_CRYPTODEV_MP_REQ_QP_SET:
+		ret = rte_cryptodev_queue_pair_setup(dev_id, qp_id,
+				queue_conf, param->socket_id);
+		if (!ret)
+			qps_in_used_by_pid[qp_id] = param->process_id;
+		res->result = ret;
+		break;
+	case RTE_CRYPTODEV_MP_REQ_QP_FREE:
+		if ((rte_eal_process_type() == RTE_PROC_SECONDARY) &&
+			(qps_in_used_by_pid[qp_id] != param->process_id)) {
+			CDEV_LOG_ERR("Unable to release qp_id=%" PRIu8,
 qp_id);
+			goto out;
+		}
+
+		ret = (*dev->dev_ops->queue_pair_release)(dev, qp_id);
+		if (!ret)
+			qps_in_used_by_pid[qp_id] = 0;
+
+		res->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)

lib/cryptodev/rte_cryptodev.c rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id, · line 1494

Quoted 5 lines

lib/cryptodev/rte_cryptodev.c +5 -0

+{
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	rte_mp_action_unregister(CRYPTODEV_MP_REQ);
+}
+

It looks an incomplete patch right now.
- Documentation is missing on how this feature will be used.

- Test cases?

- PMD implementation is key part for this feature to work.

I believe it is better to defer this to next release as we don't have a clarity on how to use this.

-----Original Message-----
From: Kai Ji <[email protected]>
Sent: Thursday 6 October 2022 09:17
To: [email protected]
Cc: Ji, Kai <[email protected]>; Akhil Goyal <[email protected]>; Fan Zhang <[email protected]>; Ray Kinsella <[email protected]>; Burakov, Anatoly <[email protected]>
Subject: [dpdk-dev v4] 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 <[email protected]>

---
v4:
- release note update
- Doxygen comments update

v3:
- addin missing free function for qp_in_use_by_pid

v2:
- code rework

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.

-----Original Message-----
From: Akhil Goyal <[email protected]>
Sent: Thursday, October 6, 2022 7:49 PM
To: Ji, Kai <[email protected]>; [email protected]
Cc: Fan Zhang <[email protected]>; Ray Kinsella <[email protected]>; Burakov, Anatoly <[email protected]>; Mcnamara, John <[email protected]>
Subject: RE: [EXT] [dpdk-dev v5] lib/cryptodev: multi-process IPC request handler

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

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 <[email protected]>

Acked-by Ciara Power <[email protected]>

---
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(-)

doc/guides/prog_guide/cryptodev_lib.rst +21 -0

index 01aad842a9..19bcda24bc 100644
@@ -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
  -----------------

doc/guides/rel_notes/release_22_11.rst +7 -1

index 4e64710a69..d60756318b 100644
@@ -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
  -------------
 

lib/cryptodev/cryptodev_pmd.h +2 -1

index f27b3249ea..574aebe279 100644
@@ -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;

lib/cryptodev/rte_cryptodev.c +93 -0

index 2165a0688c..4c60aed481 100644
@@ -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,

lib/cryptodev/rte_cryptodev.h +46 -0

index ece7157970..80076f487c 100644
@@ -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
   *

lib/cryptodev/version.map +2 -1

index 00c99fb45c..e964a3d5ab 100644
@@ -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