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

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

[Kai Ji]

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 <kai.ji@intel.com>
---
 lib/cryptodev/rte_cryptodev.c | 45 +++++++++++++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h | 14 +++++++++++
 2 files changed, 59 insertions(+)

diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 42f3221052..18fdbf6db6 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -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,
 		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 56f459c6a0..901465ca65 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -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

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

[Akhil Goyal]

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

> 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.
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
>  lib/cryptodev/rte_cryptodev.c | 45 +++++++++++++++++++++++++++++++++++
>  lib/cryptodev/rte_cryptodev.h | 14 +++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 42f3221052..18fdbf6db6 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -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,
>  		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
> diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
> index 56f459c6a0..901465ca65 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -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

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

[Zhang, Roy Fan]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, July 27, 2022 5:26 AM
> To: Ji, Kai <kai.ji@intel.com>; dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; hemant.agrawal@nxp.com;
> Zhang, Roy Fan <roy.fan.zhang@intel.com>; Chandubabu Namburu
> <chandu@amd.com>; Ruifeng Wang <ruifeng.wang@arm.com>;
> ajit.khaparde@broadcom.com; Michael Shamis <michaelsh@marvell.com>;
> Nagadheeraj Rottela <rnagadheeraj@marvell.com>; matan@nvidia.com; Jay
> Zhou <jianjay.zhou@huawei.com>
> 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.
Kai is in holiday at the moment and will be back in a week. I will sync with him then.
> 
> > 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 it
but use it instead.

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

> 
> Also, I see register API but not deregister.

There will be V2 for that.

Regards,
Fan

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

[Akhil Goyal]

Hi Fan,
> Hi Akhil,
> 
> > 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.
> >
> > > 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
> it
> but use it instead.

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

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

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

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

[Zhang, Roy Fan]

HI Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Monday, August 8, 2022 8:43 AM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Ji, Kai <kai.ji@intel.com>;
> dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; hemant.agrawal@nxp.com;
> Chandubabu Namburu <chandu@amd.com>; Ruifeng Wang
> <ruifeng.wang@arm.com>; ajit.khaparde@broadcom.com; Michael Shamis
> <michaelsh@marvell.com>; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; matan@nvidia.com; Jay Zhou
> <jianjay.zhou@huawei.com>
> Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request
> handler
> 
> Hi Fan,
> > Hi Akhil,
> >
> > > 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.
> > >
> > > > 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
> > it
> > but use it instead.
> 
> OK but the description says secondary would reconfigure the qp setup by
> primary.
> 
> >
> > > 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?
> 
> I believe we should not expose the PID/thread id via queue to the user
> application.
> This may be security issue.

You have a point.

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


> 
> Regards,
> Akhil

Regards,
Fan

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

[Akhil Goyal]

> >
> > 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.
This is specific to the PMD. Each PMD may have its own implementation.

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

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

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

[Akhil Goyal]

> Subject: RE: [EXT] [dpdk-dev v1] lib/cryptodev: multi-process IPC request handler
> 
> > >
> > > 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.
> This is specific to the PMD. Each PMD may have its own implementation.
> 
> > - 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?
> 
> > 		- or we may not allow secondary process to request freeing
> > 		  any queue pair, it is securer, but less flexible.
> >
Any update on this?

Marking this as Changes requested.
Please see that this change can only go in RC1 or else defer to next cycle.

[dpdk-dev v2] lib/cryptodev: multi-process IPC request handler

[Kai Ji]

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 <kai.ji@intel.com>
---
 lib/cryptodev/cryptodev_pmd.h |  3 +-
 lib/cryptodev/rte_cryptodev.c | 89 +++++++++++++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h | 37 +++++++++++++++
 lib/cryptodev/version.map     |  4 ++
 4 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
index 96d7e225b0..2433c4d109 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 5e25e607fa..2c6e9dd67b 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.
  *
@@ -1029,6 +1032,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;
 }
 
@@ -1284,6 +1302,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);
+	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 56f459c6a0..d8cadebd0c 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -539,6 +539,24 @@ 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_SET,
+	RTE_CRYPTODEV_MP_REQ_QP_FREE
+};
+
+/* Parameters for IPC. */
+struct rte_cryptodev_mp_param {
+	enum rte_cryptodev_mp_req_type type;
+	int dev_id;
+	int qp_id;
+	int socket_id;
+	uint16_t process_id;
+	struct rte_cryptodev_qp_conf *queue_conf;
+	int result;
+};
+
 /** Crypto device queue pair configuration structure. */
 struct rte_cryptodev_qp_conf {
 	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
@@ -769,6 +787,25 @@ 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
+ *
+ * @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 5aee87c6f7..ea62981f73 100644
--- a/lib/cryptodev/version.map
+++ b/lib/cryptodev/version.map
@@ -109,6 +109,10 @@ EXPERIMENTAL {
 	#added in 22.07
 	rte_cryptodev_session_event_mdata_set;
 	rte_crypto_asym_ke_strings;
+
+	#added in 22.11
+	rte_cryptodev_mp_request_register;
+	rte_cryptodev_mp_request_unregister;
 };
 
 INTERNAL {
-- 
2.17.1

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

[Akhil Goyal]

> 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 <kai.ji@intel.com>
I hope this patch is for next release now. It is too late for it to get merged in RC1

[dpdk-dev v3 1/1] lib/cryptodev: multi-process IPC request handler

[Kai Ji]

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 <kai.ji@intel.com>
---
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(-)

diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
index 09ba952455..f404604963 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 9e76a1c72d..dab6a37bff 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.
  *
@@ -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]);
 	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, "
+							"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);
+	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 56f459c6a0..d8cadebd0c 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -539,6 +539,24 @@ 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_SET,
+	RTE_CRYPTODEV_MP_REQ_QP_FREE
+};
+
+/* Parameters for IPC. */
+struct rte_cryptodev_mp_param {
+	enum rte_cryptodev_mp_req_type type;
+	int dev_id;
+	int qp_id;
+	int socket_id;
+	uint16_t process_id;
+	struct rte_cryptodev_qp_conf *queue_conf;
+	int result;
+};
+
 /** Crypto device queue pair configuration structure. */
 struct rte_cryptodev_qp_conf {
 	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
@@ -769,6 +787,25 @@ 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
+ *
+ * @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 6d9b3e01a6..4130c39e7c 100644
--- a/lib/cryptodev/version.map
+++ b/lib/cryptodev/version.map
@@ -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

RE: [dpdk-dev v3 1/1] lib/cryptodev: multi-process IPC request handler

[Power, Ciara]

Hi Kai,

Some small comments below.

> -----Original Message-----
> From: Kai Ji <kai.ji@intel.com>
> Sent: Sunday 2 October 2022 23:45
> To: dev@dpdk.org
> Cc: gakhil@marvell.com; Ji, Kai <kai.ji@intel.com>
> 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 <kai.ji@intel.com>
<snip>
> +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;
[CP] 
The "param" name could be improved I think - maybe follow the "res" naming and call this one "msg"
Both are "mp_***->param" so I think it is confusing to have one named "param"

> +
> +	int ret;
> +	struct rte_cryptodev *dev;
> +	uint16_t *qps_in_used_by_pid;
[CP] 
Possible typo -   change to "qp_in_use_by_pid" to match the naming of "dev->data->qp_in_use_by_pid"

<snip>
> +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);
> +}
> +
[CP] 
The above two functions should have the function return type on its own line.

<snip>
> +/**
> + * Register multi process request IPC handler
> + *
> + * @return
> + *   - 0: Success registered
> + *	 - 1: Failed registration failed
> + *	 - -EINVAL: device was not configured
[CP] 
Indent is off for success result

<snip>

Thanks,
Ciara

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

[Akhil Goyal]

> 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 <kai.ji@intel.com>

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.

> ---
> 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(-)
> 
> diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
> index 09ba952455..f404604963 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;
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.


>  	/** 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 9e76a1c72d..dab6a37bff 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.
>   *
> @@ -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]);
>  	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, "
> +							"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);
> +	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);
> +}
> +

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.

>  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 56f459c6a0..d8cadebd0c 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -539,6 +539,24 @@ 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_SET,
> +	RTE_CRYPTODEV_MP_REQ_QP_FREE
> +};

Doxygen comments missing.

> +
> +/* Parameters for IPC. */
> +struct rte_cryptodev_mp_param {
> +	enum rte_cryptodev_mp_req_type type;
> +	int dev_id;
> +	int qp_id;
> +	int socket_id;
> +	uint16_t process_id;
> +	struct rte_cryptodev_qp_conf *queue_conf;
> +	int result;
> +};
> +

Doxygen comments???
Writing just "Parameters for IPC" does not make any sense and it is not clear
How it will be used.


>  /** Crypto device queue pair configuration structure. */
>  struct rte_cryptodev_qp_conf {
>  	uint32_t nb_descriptors; /**< Number of descriptors per queue pair */
> @@ -769,6 +787,25 @@ 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
> + *

This comment is not sufficient to explain what this API will do exactly.

> + * @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 6d9b3e01a6..4130c39e7c 100644
> --- a/lib/cryptodev/version.map
> +++ b/lib/cryptodev/version.map
> @@ -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

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

[Ji, Kai]

Please see my comments inline

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, October 4, 2022 7:13 PM
> To: Ji, Kai <kai.ji@intel.com>; dev@dpdk.org
> Subject: RE: [EXT] [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 <kai.ji@intel.com>
> 
> 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.
> 
> > ---
> > 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(-)
> >
> > diff --git a/lib/cryptodev/cryptodev_pmd.h
> > b/lib/cryptodev/cryptodev_pmd.h index 09ba952455..f404604963 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;
> 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.
[KJ] the depth can be calculated using the max_nb_queue_pairs in dev_info. By stored this information in rte_cryptodev_data struct, we are no longer need to modify every queue private crypto PMDs,  and this pid info is not exposed to userspace neither.
> 
> >  	/** 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 9e76a1c72d..dab6a37bff 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.
> >   *
> > @@ -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);
[KJ] this is qp_in_use_by_pid get freed
> > +
> >  	ret = rte_cryptodev_data_free(dev_id,
> > &cryptodev_globals.data[dev_id]);
> >  	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);
[KJ] this is qp_in_use_by_pid get malloced
> > +		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;
> >  }
> >
> > @@ -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);
> > +	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);
> > +}
> > +
> 
> It looks an incomplete patch right now.
> - Documentation is missing on how this feature will be used.
[KJ] v4 updates release notes and commit message for usage of this feature.
> - Test cases?
[KJ]We have a standalone multi-process test case that I can submit but it needs some rework so it probably cant be included in the next revision. I can submit it anyway and we can do a revision of it later, after discussion. Ideally, the test app should be integrate into l2fwd test app. 
> - PMD implementation is key part for this feature to work.
[KJ] My understanding is that this design should not impact to any PMDs but offers an alternative way for secondary process to configure the queue-pair.  Its up to primary process to call the IPC request register function, otherwise no impact to any cryptodev multi-process functionalities. 
> 
> I believe it is better to defer this to next release as we don't have a clarity on
> how to use this.
[KJ] We would still like to get this feature into the release since we have a customer already using a version of this patchset. We will work to address your concerns/questions and resubmit.
> 

[dpdk-dev v4] lib/cryptodev: multi-process IPC request handler

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

Signed-off-by: Kai Ji <kai.ji@intel.com>
---
v4:
- release note update
- Doxygen comments update

v3:
- addin missing free function for qp_in_use_by_pid

v2:
- code rework
---
 doc/guides/rel_notes/release_22_11.rst |  8 ++-
 lib/cryptodev/cryptodev_pmd.h          |  3 +-
 lib/cryptodev/rte_cryptodev.c          | 91 ++++++++++++++++++++++++++
 lib/cryptodev/rte_cryptodev.h          | 46 +++++++++++++
 lib/cryptodev/version.map              |  2 +
 5 files changed, 148 insertions(+), 2 deletions(-)

diff --git a/doc/guides/rel_notes/release_22_11.rst b/doc/guides/rel_notes/release_22_11.rst
index 4e64710a69..fdcd479916 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..1313b9071b 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,76 @@ 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=%" PRIu8, 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=%" PRIu8, 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

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

[Power, Ciara]

Hi Kai,

> -----Original Message-----
> From: Kai Ji <kai.ji@intel.com>
> Sent: Thursday 6 October 2022 09:17
> To: dev@dpdk.org
> Cc: Ji, Kai <kai.ji@intel.com>; Akhil Goyal <gakhil@marvell.com>; Fan Zhang
> <royzhang1980@gmail.com>; Ray Kinsella <mdr@ashroe.eu>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> 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 <kai.ji@intel.com>
> ---
> v4:
> - release note update
> - Doxygen comments update
> 
> v3:
> - addin missing free function for qp_in_use_by_pid
> 
> v2:
> - code rework
[CP] 
Some small v3 comments left to address, otherwise:

Acked-by: Ciara Power <ciara.power@intel.com>

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

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

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

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

[Akhil Goyal]

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

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

[Konstantin Ananyev]

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
> 

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

[Ji, Kai]

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

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

[Zhang, Fan]

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