Hi Kai,
A few comments below
Thanks,
Pablo
> -----Original Message-----
> From: Ji, Kai <kai.ji@intel.com>
> Sent: Tuesday, October 25, 2022 10:48 PM
> To: dev@dpdk.org
> Cc: gakhil@marvell.com; Ji, Kai <kai.ji@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [dpdk-dev v3] crypto/ipsec_mb: multi-process IPC request handler
>
> As the secondary process needs queue-pair to be configured by the primary
> process before use.
This sentence looks incomplete: as the secondary process needs...., then what?
> This patch adds an IPC register function to allow
> secondary process to setup cryptodev queue-pair via IPC messages during
> the runtime. 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 can be
> free'd in the request.
>
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
>
> v3:
> - remove shared memzone as qp_conf params can be passed directly from
> ipc message.
>
> v2:
> - add in shared memzone for data exchange between multi-process
> ---
> drivers/crypto/ipsec_mb/ipsec_mb_ops.c | 130
> ++++++++++++++++++++-
> drivers/crypto/ipsec_mb/ipsec_mb_private.c | 24 +++-
> drivers/crypto/ipsec_mb/ipsec_mb_private.h | 46 ++++++++
> 3 files changed, 196 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> index cedcaa2742..c29656b746 100644
> --- a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> +++ b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> @@ -3,6 +3,7 @@
> */
>
> #include <string.h>
> +#include <unistd.h>
>
> #include <rte_common.h>
> #include <rte_malloc.h>
> @@ -93,6 +94,46 @@ ipsec_mb_info_get(struct rte_cryptodev *dev,
> }
> }
>
> +static int
> +ipsec_mb_secondary_qp_op(int dev_id, int qp_id,
> + const struct rte_cryptodev_qp_conf *qp_conf,
> + int socket_id, uint16_t pid, enum ipsec_mb_mp_req_type
> op_type) {
> + int ret;
> + struct rte_mp_msg qp_req_msg;
> + struct rte_mp_msg *qp_resp_msg;
> + struct rte_mp_reply qp_resp;
> + struct ipsec_mb_mp_param *req_param;
> + struct ipsec_mb_mp_param *resp_param;
> + struct timespec ts = {.tv_sec = 1, .tv_nsec = 0};
> +
> + memset(&qp_req_msg, 0, sizeof(IPSEC_MB_MP_MSG));
> + memcpy(qp_req_msg.name, IPSEC_MB_MP_MSG,
> sizeof(IPSEC_MB_MP_MSG));
> + req_param = (struct ipsec_mb_mp_param *)&qp_req_msg.param;
> +
> + qp_req_msg.len_param = sizeof(struct ipsec_mb_mp_param);
> + req_param->type = op_type;
> + req_param->dev_id = dev_id;
> + req_param->qp_id = qp_id;
> + req_param->socket_id = socket_id;
> + req_param->process_id = pid;
I think you can use "get_pid()" here and not have "pid" argument in the function, as I believe it is always called within the same process.
> + if (qp_conf) {
> + req_param->nb_descriptors = qp_conf->nb_descriptors;
> + req_param->mp_session = (void *)qp_conf->mp_session;
> + }
> +
> + qp_req_msg.num_fds = 0;
> + ret = rte_mp_request_sync(&qp_req_msg, &qp_resp, &ts);
> + if (ret) {
> + RTE_LOG(ERR, USER1, "Create MR request to primary
> process failed.");
> + return -1;
> + }
> + qp_resp_msg = &qp_resp.msgs[0];
> + resp_param = (struct ipsec_mb_mp_param *)qp_resp_msg->param;
> +
> + return resp_param->result;
> +}
> +
..
> diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_private.h
> b/drivers/crypto/ipsec_mb/ipsec_mb_private.h
> index b56eaf061e..a2b52f808e 100644
> --- a/drivers/crypto/ipsec_mb/ipsec_mb_private.h
> +++ b/drivers/crypto/ipsec_mb/ipsec_mb_private.h
...
> +
> /** All supported device types */
> enum ipsec_mb_pmd_types {
> IPSEC_MB_PMD_TYPE_AESNI_MB = 0,
> @@ -149,11 +155,51 @@ struct ipsec_mb_qp {
> IMB_MGR *mb_mgr;
> /* Multi buffer manager */
> const struct rte_memzone *mb_mgr_mz;
> + /** The process id used for queue pairs **/
> + uint16_t qp_in_used_by_pid;
I would rename this to "qp_in_use_by_pid" or "qp_used_by_pid".
> /* Shared memzone for storing mb_mgr */
Check these comments. This last one "Shared memzone..." applies to the field "mb_mgr_mz".
Also, they should use "/**< ".
> __extension__ uint8_t additional_data[];
> /**< Storing PMD specific additional data */ };
>
@@ -3,6 +3,7 @@
*/
#include <string.h>
+#include <unistd.h>
#include <rte_common.h>
#include <rte_malloc.h>
@@ -93,6 +94,46 @@ ipsec_mb_info_get(struct rte_cryptodev *dev,
}
}
+static int
+ipsec_mb_secondary_qp_op(int dev_id, int qp_id,
+ const struct rte_cryptodev_qp_conf *qp_conf,
+ int socket_id, uint16_t pid, enum ipsec_mb_mp_req_type op_type)
+{
+ int ret;
+ struct rte_mp_msg qp_req_msg;
+ struct rte_mp_msg *qp_resp_msg;
+ struct rte_mp_reply qp_resp;
+ struct ipsec_mb_mp_param *req_param;
+ struct ipsec_mb_mp_param *resp_param;
+ struct timespec ts = {.tv_sec = 1, .tv_nsec = 0};
+
+ memset(&qp_req_msg, 0, sizeof(IPSEC_MB_MP_MSG));
+ memcpy(qp_req_msg.name, IPSEC_MB_MP_MSG, sizeof(IPSEC_MB_MP_MSG));
+ req_param = (struct ipsec_mb_mp_param *)&qp_req_msg.param;
+
+ qp_req_msg.len_param = sizeof(struct ipsec_mb_mp_param);
+ req_param->type = op_type;
+ req_param->dev_id = dev_id;
+ req_param->qp_id = qp_id;
+ req_param->socket_id = socket_id;
+ req_param->process_id = pid;
+ if (qp_conf) {
+ req_param->nb_descriptors = qp_conf->nb_descriptors;
+ req_param->mp_session = (void *)qp_conf->mp_session;
+ }
+
+ qp_req_msg.num_fds = 0;
+ ret = rte_mp_request_sync(&qp_req_msg, &qp_resp, &ts);
+ if (ret) {
+ RTE_LOG(ERR, USER1, "Create MR request to primary process failed.");
+ return -1;
+ }
+ qp_resp_msg = &qp_resp.msgs[0];
+ resp_param = (struct ipsec_mb_mp_param *)qp_resp_msg->param;
+
+ return resp_param->result;
+}
+
/** Release queue pair */
int
ipsec_mb_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
@@ -100,7 +141,10 @@ ipsec_mb_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
struct ipsec_mb_qp *qp = dev->data->queue_pairs[qp_id];
struct rte_ring *r = NULL;
- if (qp != NULL && rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ if (qp != NULL)
+ return 0;
+
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
r = rte_ring_lookup(qp->name);
rte_ring_free(r);
@@ -115,6 +159,9 @@ ipsec_mb_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
#endif
rte_free(qp);
dev->data->queue_pairs[qp_id] = NULL;
+ } else { /* secondary process */
+ return ipsec_mb_secondary_qp_op(dev->data->dev_id, qp_id,
+ NULL, 0, getpid(), RTE_IPSEC_MB_MP_REQ_QP_FREE);
}
return 0;
}
@@ -222,9 +269,14 @@ ipsec_mb_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
#endif
qp = dev->data->queue_pairs[qp_id];
if (qp == NULL) {
- IPSEC_MB_LOG(ERR, "Primary process hasn't configured device qp.");
- return -EINVAL;
+ IPSEC_MB_LOG(DEBUG, "Secondary process setting up device qp.");
+ return ipsec_mb_secondary_qp_op(dev->data->dev_id, qp_id,
+ qp_conf, socket_id, getpid(),
+ RTE_IPSEC_MB_MP_REQ_QP_SET);
}
+
+ IPSEC_MB_LOG(ERR, "Queue pair already setup'ed.");
+ return -EINVAL;
} else {
/* Free memory prior to re-allocation if needed. */
if (dev->data->queue_pairs[qp_id] != NULL)
@@ -296,6 +348,78 @@ ipsec_mb_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
return ret;
}
+int
+ipsec_mb_ipc_request(const struct rte_mp_msg *mp_msg, const void *peer)
+{
+ struct rte_mp_msg ipc_resp;
+ struct ipsec_mb_mp_param *resp_param =
+ (struct ipsec_mb_mp_param *)ipc_resp.param;
+ const struct ipsec_mb_mp_param *req_param =
+ (const struct ipsec_mb_mp_param *)mp_msg->param;
+
+ int ret;
+ struct rte_cryptodev *dev;
+ struct ipsec_mb_qp *qp;
+ struct rte_cryptodev_qp_conf queue_conf;
+ int dev_id = req_param->dev_id;
+ int qp_id = req_param->qp_id;
+
+ queue_conf.nb_descriptors = req_param->nb_descriptors;
+ queue_conf.mp_session = (struct rte_mempool *)req_param->mp_session;
+ memset(resp_param, 0, sizeof(struct ipsec_mb_mp_param));
+ memcpy(ipc_resp.name, IPSEC_MB_MP_MSG, sizeof(IPSEC_MB_MP_MSG));
+
+ if (!rte_cryptodev_is_valid_dev(dev_id)) {
+ CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
+ goto out;
+ }
+
+ dev = rte_cryptodev_pmd_get_dev(dev_id);
+ switch (req_param->type) {
+ case RTE_IPSEC_MB_MP_REQ_QP_SET:
+ qp = dev->data->queue_pairs[qp_id];
+ if (qp) {
+ CDEV_LOG_DEBUG("qp %d on dev %d is initialised", qp_id, dev_id);
+ goto out;
+ }
+
+ ret = ipsec_mb_qp_setup(dev, qp_id, &queue_conf, req_param->socket_id);
+ if (!ret) {
+ qp = dev->data->queue_pairs[qp_id];
+ if (!qp) {
+ CDEV_LOG_DEBUG("qp %d on dev %d is not initialised",
+ qp_id, dev_id);
+ goto out;
+ }
+ qp->qp_in_used_by_pid = req_param->process_id;
+ }
+ resp_param->result = ret;
+ break;
+ case RTE_IPSEC_MB_MP_REQ_QP_FREE:
+ qp = dev->data->queue_pairs[qp_id];
+ if (!qp) {
+ CDEV_LOG_DEBUG("qp %d on dev %d is not initialised",
+ qp_id, dev_id);
+ goto out;
+ }
+
+ if (qp->qp_in_used_by_pid != req_param->process_id) {
+ CDEV_LOG_ERR("Unable to release qp_id=%d", qp_id);
+ goto out;
+ }
+
+ qp->qp_in_used_by_pid = 0;
+ resp_param->result = ipsec_mb_qp_release(dev, qp_id);
+ break;
+ default:
+ CDEV_LOG_ERR("invalid mp request type\n");
+ }
+
+out:
+ ret = rte_mp_reply(&ipc_resp, peer);
+ return ret;
+}
+
/** Return the size of the specific pmd session structure */
unsigned
ipsec_mb_sym_session_get_size(struct rte_cryptodev *dev)
@@ -42,6 +42,22 @@ ipsec_mb_enqueue_burst(void *__qp, struct rte_crypto_op **ops,
return nb_enqueued;
}
+static int
+ipsec_mb_mp_request_register(void)
+{
+ RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+ IPSEC_MB_LOG(INFO, "Starting register MP IPC request\n");
+ return rte_mp_action_register(IPSEC_MB_MP_MSG,
+ ipsec_mb_ipc_request);
+}
+
+static void
+ipsec_mb_mp_request_unregister(void)
+{
+ RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+ rte_mp_action_unregister(IPSEC_MB_MP_MSG);
+}
+
int
ipsec_mb_create(struct rte_vdev_device *vdev,
enum ipsec_mb_pmd_types pmd_type)
@@ -152,7 +168,10 @@ ipsec_mb_create(struct rte_vdev_device *vdev,
IPSEC_MB_LOG(INFO, "IPSec Multi-buffer library version used: %s\n",
imb_get_version_str());
- return 0;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ retval = ipsec_mb_mp_request_register();
+
+ return retval;
}
int
@@ -187,5 +206,8 @@ ipsec_mb_remove(struct rte_vdev_device *vdev)
for (qp_id = 0; qp_id < cryptodev->data->nb_queue_pairs; qp_id++)
ipsec_mb_qp_release(cryptodev, qp_id);
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ ipsec_mb_mp_request_unregister();
+
return rte_cryptodev_pmd_destroy(cryptodev);
}
@@ -10,6 +10,7 @@
#else
#include <intel-ipsec-mb.h>
#endif
+#include <rte_cryptodev.h>
#include <cryptodev_pmd.h>
#include <bus_vdev_driver.h>
@@ -25,6 +26,9 @@
/* Maximum length for memzone name */
#define IPSEC_MB_MAX_MZ_NAME 32
+/* ipsec mb multi-process queue pair config */
+#define IPSEC_MB_MP_MSG "ipsec_mb_mp_msg"
+
enum ipsec_mb_vector_mode {
IPSEC_MB_NOT_SUPPORTED = 0,
IPSEC_MB_SSE,
@@ -63,6 +67,8 @@ extern int ipsec_mb_logtype_driver;
rte_log(RTE_LOG_##level, ipsec_mb_logtype_driver, \
"%s() line %u: " fmt "\n", __func__, __LINE__, ##__VA_ARGS__)
+#define IPSEC_MB_MP_SHARED_MZ_NAME "IPSEC_MB_MP_SHARED_MZ"
+
/** All supported device types */
enum ipsec_mb_pmd_types {
IPSEC_MB_PMD_TYPE_AESNI_MB = 0,
@@ -149,11 +155,51 @@ struct ipsec_mb_qp {
IMB_MGR *mb_mgr;
/* Multi buffer manager */
const struct rte_memzone *mb_mgr_mz;
+ /** The process id used for queue pairs **/
+ uint16_t qp_in_used_by_pid;
/* Shared memzone for storing mb_mgr */
__extension__ uint8_t additional_data[];
/**< Storing PMD specific additional data */
};
+/** Request types for IPC. */
+enum ipsec_mb_mp_req_type {
+ RTE_IPSEC_MB_MP_REQ_NONE, /**< unknown event type */
+ RTE_IPSEC_MB_MP_REQ_QP_SET, /**< Queue pair setup request */
+ RTE_IPSEC_MB_MP_REQ_QP_FREE /**< Queue pair free request */
+};
+
+/* multi-process shared data */
+struct ipsec_mb_mp_shared_data {
+ struct rte_cryptodev_qp_conf qp_conf;
+};
+
+/** Parameters for IPC. */
+struct ipsec_mb_mp_param {
+ enum ipsec_mb_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 */
+ uint32_t nb_descriptors;
+ /**< Number of descriptors per queue pair */
+ void *mp_session;
+ /**< The mempool for creating session in sessionless mode */
+ int result;
+ /**< The request result for response message */
+};
+
+/* memzone for multi-process shared data */
+const struct rte_memzone *ipsec_mb_mp_mz;
+struct ipsec_mb_mp_shared_data *mp_shared_data;
+
+int
+ipsec_mb_ipc_request(const struct rte_mp_msg *mp_msg, const void *peer);
+
static __rte_always_inline void *
ipsec_mb_get_qp_private_data(struct ipsec_mb_qp *qp)
{