DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [v3 0/2] support enqueue callbacks on cryptodev
@ 2020-10-19  2:57 Abhinandan Gujjar
  2020-10-19  2:57 ` [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions Abhinandan Gujjar
  2020-10-19  2:57 ` [dpdk-dev] [v3 2/2] test: add testcase for crypto enqueue callback Abhinandan Gujjar
  0 siblings, 2 replies; 7+ messages in thread
From: Abhinandan Gujjar @ 2020-10-19  2:57 UTC (permalink / raw)
  To: dev, declan.doherty, akhil.goyal, Honnappa.Nagarahalli,
	konstantin.ananyev
  Cc: narender.vangati, jerinj, abhinandan.gujjar

In an eventdev world, multiple workers (with ordered queue) will be
working on IPsec ESP processing. The ESP header's sequence number is
unique and has to be sequentially incremented in an orderly manner.
This rises a need for incrementing sequence number in crypto stage
especially in event crypto adapter. By adding a user callback to
cryptodev at enqueue burst, the user callback will get executed
in the context of event crypto adapter. This helps the application
to increment the ESP sequence number atomically and orderly manner.

v2->v3:
    -Moved RCU under the cryptodev APIs
    -RCU is maintained per queue-pair
    -Changed name of few variables
    -Updated callback test with negative cases
    -Updated with required changes for meson

v1->v2:
    -Moved callback related members to the end of cryptodev struct
    -Added support for RCU


Abhinandan Gujjar (2):
  cryptodev: support enqueue callback functions with RCU
  test: add testcase for crypto enqueue callback

 app/test/test_cryptodev.c                      | 133 +++++++++++++++-
 config/rte_config.h                            |   1 +
 lib/librte_cryptodev/meson.build               |   2 +-
 lib/librte_cryptodev/rte_cryptodev.c           | 201 +++++++++++++++++++++++++
 lib/librte_cryptodev/rte_cryptodev.h           | 153 ++++++++++++++++++-
 lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
 6 files changed, 488 insertions(+), 4 deletions(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions
  2020-10-19  2:57 [dpdk-dev] [v3 0/2] support enqueue callbacks on cryptodev Abhinandan Gujjar
@ 2020-10-19  2:57 ` Abhinandan Gujjar
  2020-10-21  7:28   ` Honnappa Nagarahalli
  2020-10-21 19:33   ` Ananyev, Konstantin
  2020-10-19  2:57 ` [dpdk-dev] [v3 2/2] test: add testcase for crypto enqueue callback Abhinandan Gujjar
  1 sibling, 2 replies; 7+ messages in thread
From: Abhinandan Gujjar @ 2020-10-19  2:57 UTC (permalink / raw)
  To: dev, declan.doherty, akhil.goyal, Honnappa.Nagarahalli,
	konstantin.ananyev
  Cc: narender.vangati, jerinj, abhinandan.gujjar

This patch adds APIs to add/remove callback functions. The callback
function will be called for each burst of crypto ops received on a
given crypto device queue pair.

Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
---
 config/rte_config.h                            |   1 +
 lib/librte_cryptodev/meson.build               |   2 +-
 lib/librte_cryptodev/rte_cryptodev.c           | 201 +++++++++++++++++++++++++
 lib/librte_cryptodev/rte_cryptodev.h           | 153 ++++++++++++++++++-
 lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
 5 files changed, 357 insertions(+), 2 deletions(-)

diff --git a/config/rte_config.h b/config/rte_config.h
index 03d90d7..e999d93 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -61,6 +61,7 @@
 /* cryptodev defines */
 #define RTE_CRYPTO_MAX_DEVS 64
 #define RTE_CRYPTODEV_NAME_LEN 64
+#define RTE_CRYPTO_CALLBACKS 1
 
 /* compressdev defines */
 #define RTE_COMPRESS_MAX_DEVS 64
diff --git a/lib/librte_cryptodev/meson.build b/lib/librte_cryptodev/meson.build
index c4c6b3b..8c5493f 100644
--- a/lib/librte_cryptodev/meson.build
+++ b/lib/librte_cryptodev/meson.build
@@ -9,4 +9,4 @@ headers = files('rte_cryptodev.h',
 	'rte_crypto.h',
 	'rte_crypto_sym.h',
 	'rte_crypto_asym.h')
-deps += ['kvargs', 'mbuf']
+deps += ['kvargs', 'mbuf', 'rcu']
diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 3d95ac6..5ba774a 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -448,6 +448,10 @@ struct rte_cryptodev_sym_session_pool_private_data {
 	return 0;
 }
 
+#ifdef RTE_CRYPTO_CALLBACKS
+/* spinlock for crypto device enq callbacks */
+static rte_spinlock_t rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER;
+#endif
 
 const char *
 rte_cryptodev_get_feature_name(uint64_t flag)
@@ -1136,6 +1140,203 @@ struct rte_cryptodev *
 			socket_id);
 }
 
+#ifdef RTE_CRYPTO_CALLBACKS
+
+struct rte_cryptodev_cb *
+rte_cryptodev_add_enq_callback(uint8_t dev_id,
+			       uint16_t qp_id,
+			       rte_cryptodev_callback_fn cb_fn,
+			       void *cb_arg)
+{
+	struct rte_cryptodev *dev;
+	struct rte_cryptodev_cb *cb, *tail;
+	struct rte_cryptodev_enq_cb_rcu *list;
+	struct rte_rcu_qsbr *qsbr;
+	size_t size;
+
+	/* Max thread set to 1, as one DP thread accessing a queue-pair */
+	const uint32_t max_threads = 1;
+
+	if (!cb_fn)
+		return NULL;
+
+	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
+		return NULL;
+	}
+
+	dev = &rte_crypto_devices[dev_id];
+	if (qp_id >= dev->data->nb_queue_pairs) {
+		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
+		return NULL;
+	}
+
+	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
+	if (dev->enq_cbs == NULL) {
+		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
+					   dev->data->nb_queue_pairs, 0);
+		if (dev->enq_cbs == NULL) {
+			CDEV_LOG_ERR("Failed to allocate memory for callbacks");
+			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+			rte_errno = ENOMEM;
+			return NULL;
+		}
+
+		list = rte_zmalloc(NULL, sizeof(*list), 0);
+		if (list == NULL) {
+			CDEV_LOG_ERR("Failed to allocate memory for list on "
+				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
+			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+			rte_errno = ENOMEM;
+			rte_free(dev->enq_cbs);
+			return NULL;
+		}
+
+		/* Create RCU QSBR variable */
+		size = rte_rcu_qsbr_get_memsize(max_threads);
+		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
+		if (qsbr == NULL) {
+			CDEV_LOG_ERR("Failed to allocate memory for RCU on "
+				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
+			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+			rte_errno = ENOMEM;
+			rte_free(list);
+			rte_free(dev->enq_cbs);
+			dev->enq_cbs[qp_id] = NULL;
+			return NULL;
+		}
+
+		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
+			CDEV_LOG_ERR("Failed to initialize for RCU on "
+				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
+			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+			rte_free(qsbr);
+			rte_free(list);
+			rte_free(dev->enq_cbs);
+			dev->enq_cbs[qp_id] = NULL;
+			return NULL;
+		}
+
+		dev->enq_cbs[qp_id] = list;
+		list->qsbr = qsbr;
+	}
+
+	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
+	if (cb == NULL) {
+		CDEV_LOG_ERR("Failed to allocate memory for callback on "
+			     "dev=%d, queue_pair_id=%d", dev_id, qp_id);
+		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+		rte_errno = ENOMEM;
+		return NULL;
+	}
+
+	cb->fn = cb_fn;
+	cb->arg = cb_arg;
+
+	/* Add the callbacks in fifo order. */
+	list = dev->enq_cbs[qp_id];
+	tail = list->next;
+	if (tail) {
+		while (tail->next)
+			tail = tail->next;
+		tail->next = cb;
+	} else
+		list->next = cb;
+
+	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+
+	return cb;
+}
+
+int
+rte_cryptodev_remove_enq_callback(uint8_t dev_id,
+				  uint16_t qp_id,
+				  struct rte_cryptodev_cb *cb)
+{
+	struct rte_cryptodev *dev;
+	struct rte_cryptodev_cb **prev_cb, *curr_cb;
+	struct rte_cryptodev_enq_cb_rcu *list;
+	uint16_t qp;
+	int free_mem;
+	int ret;
+
+	free_mem = 1;
+	ret = -EINVAL;
+
+	if (!cb) {
+		CDEV_LOG_ERR("cb is NULL");
+		return ret;
+	}
+
+	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
+		return ret;
+	}
+
+	dev = &rte_crypto_devices[dev_id];
+	if (qp_id >= dev->data->nb_queue_pairs) {
+		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
+		return ret;
+	}
+
+	list = dev->enq_cbs[qp_id];
+	if (list == NULL) {
+		CDEV_LOG_ERR("Callback list is NULL");
+		return ret;
+	}
+
+	if (list->qsbr == NULL) {
+		CDEV_LOG_ERR("Rcu qsbr is NULL");
+		return ret;
+	}
+
+	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
+	if (dev->enq_cbs == NULL) {
+		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+		return ret;
+	}
+
+	prev_cb = &list->next;
+	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
+		curr_cb = *prev_cb;
+		if (curr_cb == cb) {
+			/* Remove the user cb from the callback list. */
+			*prev_cb = curr_cb->next;
+			ret = 0;
+			break;
+		}
+	}
+
+	if (!ret) {
+		/* Call sync with invalid thread id as this is part of
+		 * control plane API
+		 */
+		rte_rcu_qsbr_synchronize(list->qsbr, RTE_QSBR_THRID_INVALID);
+		rte_free(cb);
+	}
+
+	if (list->next == NULL) {
+		rte_free(list->qsbr);
+		rte_free(list);
+		dev->enq_cbs[qp_id] = NULL;
+	}
+
+	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
+		if (dev->enq_cbs[qp] != NULL) {
+			free_mem = 0;
+			break;
+		}
+
+	if (free_mem) {
+		rte_free(dev->enq_cbs);
+		dev->enq_cbs = NULL;
+	}
+
+	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+
+	return ret;
+}
+#endif
 
 int
 rte_cryptodev_stats_get(uint8_t dev_id, struct rte_cryptodev_stats *stats)
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 0935fd5..669746d 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -23,6 +23,7 @@
 #include "rte_dev.h"
 #include <rte_common.h>
 #include <rte_config.h>
+#include <rte_rcu_qsbr.h>
 
 #include "rte_cryptodev_trace_fp.h"
 
@@ -522,6 +523,34 @@ struct rte_cryptodev_qp_conf {
 	/**< The mempool for creating sess private data in sessionless mode */
 };
 
+#ifdef RTE_CRYPTO_CALLBACKS
+/**
+ * Function type used for pre processing crypto ops when enqueue burst is
+ * called.
+ *
+ * The callback function is called on enqueue burst immediately
+ * before the crypto ops are put onto the hardware queue for processing.
+ *
+ * @param	dev_id		The identifier of the device.
+ * @param	qp_id		The index of the queue pair in which ops are
+ *				to be enqueued for processing. The value
+ *				must be in the range [0, nb_queue_pairs - 1]
+ *				previously supplied to
+ *				*rte_cryptodev_configure*.
+ * @param	ops		The address of an array of *nb_ops* pointers
+ *				to *rte_crypto_op* structures which contain
+ *				the crypto operations to be processed.
+ * @param	nb_ops		The number of operations to process.
+ * @param	user_param	The arbitrary user parameter passed in by the
+ *				application when the callback was originally
+ *				registered.
+ * @return			The number of ops to be enqueued to the
+ *				crypto device.
+ */
+typedef uint16_t (*rte_cryptodev_callback_fn)(uint16_t dev_id, uint16_t qp_id,
+		struct rte_crypto_op **ops, uint16_t nb_ops, void *user_param);
+#endif
+
 /**
  * Typedef for application callback function to be registered by application
  * software for notification of device events
@@ -822,7 +851,6 @@ struct rte_cryptodev_config {
 		enum rte_cryptodev_event_type event,
 		rte_cryptodev_cb_fn cb_fn, void *cb_arg);
 
-
 typedef uint16_t (*dequeue_pkt_burst_t)(void *qp,
 		struct rte_crypto_op **ops,	uint16_t nb_ops);
 /**< Dequeue processed packets from queue pair of a device. */
@@ -839,6 +867,33 @@ typedef uint16_t (*enqueue_pkt_burst_t)(void *qp,
 /** Structure to keep track of registered callbacks */
 TAILQ_HEAD(rte_cryptodev_cb_list, rte_cryptodev_callback);
 
+#ifdef RTE_CRYPTO_CALLBACKS
+/**
+ * @internal
+ * Structure used to hold information about the callbacks to be called for a
+ * queue pair on enqueue.
+ */
+struct rte_cryptodev_cb {
+	struct rte_cryptodev_cb *next;
+	/** < Pointer to next callback */
+	rte_cryptodev_callback_fn fn;
+	/** < Pointer to callback function */
+	void *arg;
+	/** < Pointer to argument */
+};
+
+/**
+ * @internal
+ * Structure used to hold information about the RCU for a queue pair.
+ */
+struct rte_cryptodev_enq_cb_rcu {
+	struct rte_cryptodev_cb *next;
+	/** < Pointer to next callback */
+	struct rte_rcu_qsbr *qsbr;
+	/** < RCU QSBR variable per queue pair */
+};
+#endif
+
 /** The data structure associated with each crypto device. */
 struct rte_cryptodev {
 	dequeue_pkt_burst_t dequeue_burst;
@@ -867,6 +922,11 @@ struct rte_cryptodev {
 	__extension__
 	uint8_t attached : 1;
 	/**< Flag indicating the device is attached */
+
+#ifdef RTE_CRYPTO_CALLBACKS
+	struct rte_cryptodev_enq_cb_rcu **enq_cbs;
+	/**< User application callback for pre enqueue processing */
+#endif
 } __rte_cache_aligned;
 
 void *
@@ -989,6 +1049,25 @@ struct rte_cryptodev_data {
 {
 	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
 
+#ifdef RTE_CRYPTO_CALLBACKS
+	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
+		struct rte_cryptodev_enq_cb_rcu *list;
+		struct rte_cryptodev_cb *cb;
+
+		list = dev->enq_cbs[qp_id];
+		cb = list->next;
+		rte_rcu_qsbr_thread_online(list->qsbr, 0);
+
+		do {
+			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
+					cb->arg);
+			cb = cb->next;
+		} while (cb != NULL);
+
+		rte_rcu_qsbr_thread_offline(list->qsbr, 0);
+	}
+#endif
+
 	rte_cryptodev_trace_enqueue_burst(dev_id, qp_id, (void **)ops, nb_ops);
 	return (*dev->enqueue_burst)(
 			dev->data->queue_pairs[qp_id], ops, nb_ops);
@@ -1730,6 +1809,78 @@ struct rte_crypto_raw_dp_ctx {
 rte_cryptodev_raw_dequeue_done(struct rte_crypto_raw_dp_ctx *ctx,
 		uint32_t n);
 
+#ifdef RTE_CRYPTO_CALLBACKS
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Add a user callback for a given crypto device and queue pair which will be
+ * called on crypto ops enqueue.
+ *
+ * This API configures a function to be called for each burst of crypto ops
+ * received on a given crypto device queue pair. The return value is a pointer
+ * that can be used later to remove the callback using
+ * rte_cryptodev_remove_enq_callback().
+ *
+ * Multiple functions are called in the order that they are added.
+ *
+ * @param	dev_id		The identifier of the device.
+ * @param	qp_id		The index of the queue pair in which ops are
+ *				to be enqueued for processing. The value
+ *				must be in the range [0, nb_queue_pairs - 1]
+ *				previously supplied to
+ *				*rte_cryptodev_configure*.
+ * @param	cb_fn		The callback function
+ * @param	cb_arg		A generic pointer parameter which will be passed
+ *				to each invocation of the callback function on
+ *				this crypto device and queue pair.
+ *
+ * @return
+ *   NULL on error.
+ *   On success, a pointer value which can later be used to remove the callback.
+ */
+
+__rte_experimental
+struct rte_cryptodev_cb *
+rte_cryptodev_add_enq_callback(uint8_t dev_id,
+			       uint16_t qp_id,
+			       rte_cryptodev_callback_fn cb_fn,
+			       void *cb_arg);
+
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Remove a user callback function for given crypto device and queue pair.
+ *
+ * This function is used to removed callbacks that were added to a crypto
+ * device queue pair using rte_cryptodev_add_enq_callback().
+ *
+ *
+ *
+ * @param	dev_id		The identifier of the device.
+ * @param	qp_id		The index of the queue pair in which ops are
+ *				to be enqueued for processing. The value
+ *				must be in the range [0, nb_queue_pairs - 1]
+ *				previously supplied to
+ *				*rte_cryptodev_configure*.
+ * @param	cb		Pointer to user supplied callback created via
+ *				rte_cryptodev_add_enq_callback().
+ *
+ * @return
+ *   - 0: Success. Callback was removed.
+ *   - -EINVAL:  The dev_id or the qp_id is out of range, or the callback
+ *               is NULL or not found for the crypto device queue pair.
+ */
+
+__rte_experimental
+int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
+				      uint16_t qp_id,
+				      struct rte_cryptodev_cb *cb);
+
+#endif
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map
index 7e4360f..5d8d6b0 100644
--- a/lib/librte_cryptodev/rte_cryptodev_version.map
+++ b/lib/librte_cryptodev/rte_cryptodev_version.map
@@ -101,6 +101,7 @@ EXPERIMENTAL {
 	rte_cryptodev_get_qp_status;
 
 	# added in 20.11
+	rte_cryptodev_add_enq_callback;
 	rte_cryptodev_configure_raw_dp_ctx;
 	rte_cryptodev_get_raw_dp_ctx_size;
 	rte_cryptodev_raw_dequeue;
@@ -109,4 +110,5 @@ EXPERIMENTAL {
 	rte_cryptodev_raw_enqueue;
 	rte_cryptodev_raw_enqueue_burst;
 	rte_cryptodev_raw_enqueue_done;
+	rte_cryptodev_remove_enq_callback;
 };
-- 
1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [v3 2/2] test: add testcase for crypto enqueue callback
  2020-10-19  2:57 [dpdk-dev] [v3 0/2] support enqueue callbacks on cryptodev Abhinandan Gujjar
  2020-10-19  2:57 ` [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions Abhinandan Gujjar
@ 2020-10-19  2:57 ` Abhinandan Gujjar
  1 sibling, 0 replies; 7+ messages in thread
From: Abhinandan Gujjar @ 2020-10-19  2:57 UTC (permalink / raw)
  To: dev, declan.doherty, akhil.goyal, Honnappa.Nagarahalli,
	konstantin.ananyev
  Cc: narender.vangati, jerinj, abhinandan.gujjar

Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
---
 app/test/test_cryptodev.c | 133 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 131 insertions(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 1d4c46f..a083612 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -10640,6 +10640,133 @@ struct multi_session_params {
 	return TEST_SUCCESS;
 }
 
+#ifdef RTE_CRYPTO_CALLBACKS
+static uint16_t
+test_enq_callback(uint16_t dev_id, uint16_t qp_id, struct rte_crypto_op **ops,
+		  uint16_t nb_ops, void *user_param)
+{
+	RTE_SET_USED(dev_id);
+	RTE_SET_USED(qp_id);
+	RTE_SET_USED(ops);
+	RTE_SET_USED(user_param);
+
+	printf("crypto enqueue callback called\n");
+	return nb_ops;
+}
+
+/*
+ * Thread using enqueue callback with RCU.
+ */
+static int
+test_enq_callback_rcu_thread(void *arg)
+{
+	RTE_SET_USED(arg);
+	/* DP thread calls rte_cryptodev_enqueue_burst() and
+	 * invokes enqueue callback.
+	 */
+	test_null_burst_operation();
+	return 0;
+}
+
+static int
+test_enq_callback_setup(void)
+{
+	struct crypto_testsuite_params *ts_params = &testsuite_params;
+	struct rte_cryptodev_info dev_info;
+	struct rte_cryptodev_qp_conf qp_conf = {
+		.nb_descriptors = MAX_NUM_OPS_INFLIGHT
+	};
+
+	struct rte_cryptodev_cb *cb;
+	uint16_t qp_id = 0;
+
+	/* Stop the device in case it's started so it can be configured */
+	rte_cryptodev_stop(ts_params->valid_devs[0]);
+
+	rte_cryptodev_info_get(ts_params->valid_devs[0], &dev_info);
+
+	TEST_ASSERT_SUCCESS(rte_cryptodev_configure(ts_params->valid_devs[0],
+			&ts_params->conf),
+			"Failed to configure cryptodev %u",
+			ts_params->valid_devs[0]);
+
+	qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
+	qp_conf.mp_session = ts_params->session_mpool;
+	qp_conf.mp_session_private = ts_params->session_priv_mpool;
+
+	TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
+			ts_params->valid_devs[0], qp_id, &qp_conf,
+			rte_cryptodev_socket_id(ts_params->valid_devs[0])),
+			"Failed test for "
+			"rte_cryptodev_queue_pair_setup: num_inflights "
+			"%u on qp %u on cryptodev %u",
+			qp_conf.nb_descriptors, qp_id,
+			ts_params->valid_devs[0]);
+
+	/* Test with invalid crypto device */
+	cb = rte_cryptodev_add_enq_callback(RTE_CRYPTO_MAX_DEVS,
+			qp_id, test_enq_callback, NULL);
+	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
+			"cryptodev %u did not fail",
+			qp_id, RTE_CRYPTO_MAX_DEVS);
+
+	/* Test with invalid queue pair */
+	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
+			dev_info.max_nb_queue_pairs + 1,
+			test_enq_callback, NULL);
+	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
+			"cryptodev %u did not fail",
+			dev_info.max_nb_queue_pairs + 1,
+			ts_params->valid_devs[0]);
+
+	/* Test with NULL callback */
+	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
+			qp_id, NULL, NULL);
+	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
+			"cryptodev %u did not fail",
+			qp_id, ts_params->valid_devs[0]);
+
+	/* Test with valid configuration */
+	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
+			qp_id, test_enq_callback, NULL);
+	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
+			"qp %u on cryptodev %u",
+			qp_id, ts_params->valid_devs[0]);
+
+	/* Launch a thread */
+	rte_eal_remote_launch(test_enq_callback_rcu_thread, NULL,
+				rte_get_next_lcore(-1, 1, 0));
+
+	/* Wait until reader exited. */
+	rte_eal_mp_wait_lcore();
+
+	/* Test with invalid crypto device */
+	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
+			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
+			"Expected call to fail as crypto device is invalid");
+
+	/* Test with invalid queue pair */
+	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
+			ts_params->valid_devs[0],
+			dev_info.max_nb_queue_pairs + 1, cb),
+			"Expected call to fail as queue pair is invalid");
+
+	/* Test with NULL callback */
+	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
+			ts_params->valid_devs[0], qp_id, NULL),
+			"Expected call to fail as callback is NULL");
+
+	/* Test with valid configuration */
+	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_enq_callback(
+			ts_params->valid_devs[0], qp_id, cb),
+			"Failed test to remove callback on "
+			"qp %u on cryptodev %u",
+			qp_id, ts_params->valid_devs[0]);
+
+	return TEST_SUCCESS;
+}
+#endif
+
 static void
 generate_gmac_large_plaintext(uint8_t *data)
 {
@@ -12960,7 +13087,6 @@ struct test_crypto_vector {
 				test_queue_pair_descriptor_setup),
 		TEST_CASE_ST(ut_setup, ut_teardown,
 				test_device_configure_invalid_queue_pair_ids),
-
 		TEST_CASE_ST(ut_setup, ut_teardown,
 				test_multi_session),
 		TEST_CASE_ST(ut_setup, ut_teardown,
@@ -12969,7 +13095,6 @@ struct test_crypto_vector {
 		TEST_CASE_ST(ut_setup, ut_teardown,
 			test_null_invalid_operation),
 		TEST_CASE_ST(ut_setup, ut_teardown, test_null_burst_operation),
-
 		TEST_CASE_ST(ut_setup, ut_teardown, test_AES_chain_all),
 		TEST_CASE_ST(ut_setup, ut_teardown, test_AES_cipheronly_all),
 		TEST_CASE_ST(ut_setup, ut_teardown, test_3DES_chain_all),
@@ -13583,6 +13708,10 @@ struct test_crypto_vector {
 		TEST_CASE_ST(ut_setup_security, ut_teardown,
 			test_DOCSIS_PROTO_all),
 #endif
+
+#ifdef RTE_CRYPTO_CALLBACKS
+		TEST_CASE_ST(ut_setup, ut_teardown, test_enq_callback_setup),
+#endif
 		TEST_CASES_END() /**< NULL terminate unit test array */
 	}
 };
-- 
1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions
  2020-10-19  2:57 ` [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions Abhinandan Gujjar
@ 2020-10-21  7:28   ` Honnappa Nagarahalli
  2020-10-23 12:34     ` Gujjar, Abhinandan S
  2020-10-21 19:33   ` Ananyev, Konstantin
  1 sibling, 1 reply; 7+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-21  7:28 UTC (permalink / raw)
  To: Abhinandan Gujjar, dev, declan.doherty, Akhil.goyal@nxp.com,
	konstantin.ananyev
  Cc: narender.vangati, jerinj, nd, Honnappa Nagarahalli, nd

<snip>

> 
> This patch adds APIs to add/remove callback functions. The callback function
> will be called for each burst of crypto ops received on a given crypto device
> queue pair.
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  config/rte_config.h                            |   1 +
>  lib/librte_cryptodev/meson.build               |   2 +-
>  lib/librte_cryptodev/rte_cryptodev.c           | 201
> +++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev.h           | 153 ++++++++++++++++++-
>  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
>  5 files changed, 357 insertions(+), 2 deletions(-)
> 
> diff --git a/config/rte_config.h b/config/rte_config.h index 03d90d7..e999d93
> 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -61,6 +61,7 @@
>  /* cryptodev defines */
>  #define RTE_CRYPTO_MAX_DEVS 64
>  #define RTE_CRYPTODEV_NAME_LEN 64
> +#define RTE_CRYPTO_CALLBACKS 1
> 
>  /* compressdev defines */
>  #define RTE_COMPRESS_MAX_DEVS 64
> diff --git a/lib/librte_cryptodev/meson.build b/lib/librte_cryptodev/meson.build
> index c4c6b3b..8c5493f 100644
> --- a/lib/librte_cryptodev/meson.build
> +++ b/lib/librte_cryptodev/meson.build
> @@ -9,4 +9,4 @@ headers = files('rte_cryptodev.h',
>  	'rte_crypto.h',
>  	'rte_crypto_sym.h',
>  	'rte_crypto_asym.h')
> -deps += ['kvargs', 'mbuf']
> +deps += ['kvargs', 'mbuf', 'rcu']
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> b/lib/librte_cryptodev/rte_cryptodev.c
> index 3d95ac6..5ba774a 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -448,6 +448,10 @@ struct
> rte_cryptodev_sym_session_pool_private_data {
>  	return 0;
>  }
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/* spinlock for crypto device enq callbacks */ static rte_spinlock_t
> +rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER; #endif
> 
>  const char *
>  rte_cryptodev_get_feature_name(uint64_t flag) @@ -1136,6 +1140,203 @@
> struct rte_cryptodev *
>  			socket_id);
>  }
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +
> +struct rte_cryptodev_cb *
> +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> +			       uint16_t qp_id,
> +			       rte_cryptodev_callback_fn cb_fn,
> +			       void *cb_arg)
> +{
> +	struct rte_cryptodev *dev;
> +	struct rte_cryptodev_cb *cb, *tail;
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	struct rte_rcu_qsbr *qsbr;
> +	size_t size;
> +
> +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> +	const uint32_t max_threads = 1;
> +
> +	if (!cb_fn)
> +		return NULL;
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		return NULL;
> +	}
> +
> +	dev = &rte_crypto_devices[dev_id];
> +	if (qp_id >= dev->data->nb_queue_pairs) {
> +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> +		return NULL;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> +	if (dev->enq_cbs == NULL) {
> +		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
> +					   dev->data->nb_queue_pairs, 0);
> +		if (dev->enq_cbs == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for
> callbacks");
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_errno = ENOMEM;
> +			return NULL;
> +		}
> +
> +		list = rte_zmalloc(NULL, sizeof(*list), 0);
> +		if (list == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for list on "
> +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_errno = ENOMEM;
> +			rte_free(dev->enq_cbs);
> +			return NULL;
> +		}
> +
> +		/* Create RCU QSBR variable */
> +		size = rte_rcu_qsbr_get_memsize(max_threads);
> +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> +		if (qsbr == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for RCU on
> "
> +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_errno = ENOMEM;
> +			rte_free(list);
> +			rte_free(dev->enq_cbs);
> +			dev->enq_cbs[qp_id] = NULL;
> +			return NULL;
> +		}
> +
> +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_free(qsbr);
> +			rte_free(list);
> +			rte_free(dev->enq_cbs);
> +			dev->enq_cbs[qp_id] = NULL;
> +			return NULL;
> +		}
> +
> +		dev->enq_cbs[qp_id] = list;
> +		list->qsbr = qsbr;
> +	}
> +
> +	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> +	if (cb == NULL) {
> +		CDEV_LOG_ERR("Failed to allocate memory for callback on "
> +			     "dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	cb->fn = cb_fn;
> +	cb->arg = cb_arg;
> +
> +	/* Add the callbacks in fifo order. */
> +	list = dev->enq_cbs[qp_id];
> +	tail = list->next;
> +	if (tail) {
> +		while (tail->next)
> +			tail = tail->next;
> +		tail->next = cb;
> +	} else
> +		list->next = cb;
> +
Correct memory orderings need to be used to provide reader-writer concurrency. Please look at [1] for changes you need to make here.
[1] https://patches.dpdk.org/patch/80603/

> +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +
> +	return cb;
> +}
> +
> +int
> +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +				  uint16_t qp_id,
> +				  struct rte_cryptodev_cb *cb)
> +{
> +	struct rte_cryptodev *dev;
> +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	uint16_t qp;
> +	int free_mem;
> +	int ret;
> +
> +	free_mem = 1;
> +	ret = -EINVAL;
> +
> +	if (!cb) {
> +		CDEV_LOG_ERR("cb is NULL");
> +		return ret;
> +	}
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		return ret;
> +	}
> +
> +	dev = &rte_crypto_devices[dev_id];
> +	if (qp_id >= dev->data->nb_queue_pairs) {
> +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> +		return ret;
> +	}
> +
> +	list = dev->enq_cbs[qp_id];
> +	if (list == NULL) {
> +		CDEV_LOG_ERR("Callback list is NULL");
> +		return ret;
> +	}
> +
> +	if (list->qsbr == NULL) {
> +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> +		return ret;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> +	if (dev->enq_cbs == NULL) {
> +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +		return ret;
> +	}
> +
> +	prev_cb = &list->next;
> +	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
> +		curr_cb = *prev_cb;
> +		if (curr_cb == cb) {
> +			/* Remove the user cb from the callback list. */
> +			*prev_cb = curr_cb->next;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (!ret) {
> +		/* Call sync with invalid thread id as this is part of
> +		 * control plane API
> +		 */
> +		rte_rcu_qsbr_synchronize(list->qsbr,
> RTE_QSBR_THRID_INVALID);
> +		rte_free(cb);
> +	}
> +
> +	if (list->next == NULL) {
> +		rte_free(list->qsbr);
> +		rte_free(list);
> +		dev->enq_cbs[qp_id] = NULL;
> +	}
> +
> +	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
> +		if (dev->enq_cbs[qp] != NULL) {
> +			free_mem = 0;
> +			break;
> +		}
> +
> +	if (free_mem) {
> +		rte_free(dev->enq_cbs);
> +		dev->enq_cbs = NULL;
> +	}
> +
> +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +
> +	return ret;
> +}
> +#endif
> 
>  int
>  rte_cryptodev_stats_get(uint8_t dev_id, struct rte_cryptodev_stats *stats) diff
> --git a/lib/librte_cryptodev/rte_cryptodev.h
> b/lib/librte_cryptodev/rte_cryptodev.h
> index 0935fd5..669746d 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -23,6 +23,7 @@
>  #include "rte_dev.h"
>  #include <rte_common.h>
>  #include <rte_config.h>
> +#include <rte_rcu_qsbr.h>
> 
>  #include "rte_cryptodev_trace_fp.h"
> 
> @@ -522,6 +523,34 @@ struct rte_cryptodev_qp_conf {
>  	/**< The mempool for creating sess private data in sessionless mode */
> };
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/**
> + * Function type used for pre processing crypto ops when enqueue burst
> +is
> + * called.
> + *
> + * The callback function is called on enqueue burst immediately
> + * before the crypto ops are put onto the hardware queue for processing.
> + *
> + * @param	dev_id		The identifier of the device.
> + * @param	qp_id		The index of the queue pair in which ops are
> + *				to be enqueued for processing. The value
> + *				must be in the range [0, nb_queue_pairs - 1]
> + *				previously supplied to
> + *				*rte_cryptodev_configure*.
> + * @param	ops		The address of an array of *nb_ops* pointers
> + *				to *rte_crypto_op* structures which contain
> + *				the crypto operations to be processed.
> + * @param	nb_ops		The number of operations to process.
> + * @param	user_param	The arbitrary user parameter passed in by the
> + *				application when the callback was originally
> + *				registered.
> + * @return			The number of ops to be enqueued to the
> + *				crypto device.
> + */
> +typedef uint16_t (*rte_cryptodev_callback_fn)(uint16_t dev_id, uint16_t
> qp_id,
> +		struct rte_crypto_op **ops, uint16_t nb_ops, void
> *user_param);
> +#endif
> +
>  /**
>   * Typedef for application callback function to be registered by application
>   * software for notification of device events @@ -822,7 +851,6 @@ struct
> rte_cryptodev_config {
>  		enum rte_cryptodev_event_type event,
>  		rte_cryptodev_cb_fn cb_fn, void *cb_arg);
> 
> -
>  typedef uint16_t (*dequeue_pkt_burst_t)(void *qp,
>  		struct rte_crypto_op **ops,	uint16_t nb_ops);
>  /**< Dequeue processed packets from queue pair of a device. */ @@ -839,6
> +867,33 @@ typedef uint16_t (*enqueue_pkt_burst_t)(void *qp,
>  /** Structure to keep track of registered callbacks */
> TAILQ_HEAD(rte_cryptodev_cb_list, rte_cryptodev_callback);
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/**
> + * @internal
> + * Structure used to hold information about the callbacks to be called
> +for a
> + * queue pair on enqueue.
> + */
> +struct rte_cryptodev_cb {
> +	struct rte_cryptodev_cb *next;
> +	/** < Pointer to next callback */
> +	rte_cryptodev_callback_fn fn;
> +	/** < Pointer to callback function */
> +	void *arg;
> +	/** < Pointer to argument */
> +};
> +
> +/**
> + * @internal
> + * Structure used to hold information about the RCU for a queue pair.
> + */
> +struct rte_cryptodev_enq_cb_rcu {
> +	struct rte_cryptodev_cb *next;
> +	/** < Pointer to next callback */
> +	struct rte_rcu_qsbr *qsbr;
> +	/** < RCU QSBR variable per queue pair */ }; #endif
> +
>  /** The data structure associated with each crypto device. */  struct
> rte_cryptodev {
>  	dequeue_pkt_burst_t dequeue_burst;
> @@ -867,6 +922,11 @@ struct rte_cryptodev {
>  	__extension__
>  	uint8_t attached : 1;
>  	/**< Flag indicating the device is attached */
> +
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	struct rte_cryptodev_enq_cb_rcu **enq_cbs;
> +	/**< User application callback for pre enqueue processing */ #endif
>  } __rte_cache_aligned;
> 
>  void *
> @@ -989,6 +1049,25 @@ struct rte_cryptodev_data {  {
>  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
> +		struct rte_cryptodev_enq_cb_rcu *list;
> +		struct rte_cryptodev_cb *cb;
> +
> +		list = dev->enq_cbs[qp_id];
> +		cb = list->next;
> +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
Move this 2 lines up as you are reading the addresses.
Also, look at the patch mentioned earlier for memory orderings.

> +
> +		do {
> +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> +					cb->arg);
> +			cb = cb->next;
> +		} while (cb != NULL);
> +
> +		rte_rcu_qsbr_thread_offline(list->qsbr, 0);
> +	}
> +#endif
> +
>  	rte_cryptodev_trace_enqueue_burst(dev_id, qp_id, (void **)ops,
> nb_ops);
>  	return (*dev->enqueue_burst)(
>  			dev->data->queue_pairs[qp_id], ops, nb_ops); @@ -
> 1730,6 +1809,78 @@ struct rte_crypto_raw_dp_ctx {
> rte_cryptodev_raw_dequeue_done(struct rte_crypto_raw_dp_ctx *ctx,
>  		uint32_t n);
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Add a user callback for a given crypto device and queue pair which
> +will be
> + * called on crypto ops enqueue.
> + *
> + * This API configures a function to be called for each burst of crypto
> +ops
> + * received on a given crypto device queue pair. The return value is a
> +pointer
> + * that can be used later to remove the callback using
> + * rte_cryptodev_remove_enq_callback().
> + *
> + * Multiple functions are called in the order that they are added.
> + *
> + * @param	dev_id		The identifier of the device.
> + * @param	qp_id		The index of the queue pair in which ops are
> + *				to be enqueued for processing. The value
> + *				must be in the range [0, nb_queue_pairs - 1]
> + *				previously supplied to
> + *				*rte_cryptodev_configure*.
> + * @param	cb_fn		The callback function
> + * @param	cb_arg		A generic pointer parameter which will be
> passed
> + *				to each invocation of the callback function on
> + *				this crypto device and queue pair.
> + *
> + * @return
> + *   NULL on error.
> + *   On success, a pointer value which can later be used to remove the callback.
> + */
> +
> +__rte_experimental
> +struct rte_cryptodev_cb *
> +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> +			       uint16_t qp_id,
> +			       rte_cryptodev_callback_fn cb_fn,
> +			       void *cb_arg);
> +
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Remove a user callback function for given crypto device and queue pair.
> + *
> + * This function is used to removed callbacks that were added to a
> +crypto
> + * device queue pair using rte_cryptodev_add_enq_callback().
> + *
> + *
> + *
> + * @param	dev_id		The identifier of the device.
> + * @param	qp_id		The index of the queue pair in which ops are
> + *				to be enqueued for processing. The value
> + *				must be in the range [0, nb_queue_pairs - 1]
> + *				previously supplied to
> + *				*rte_cryptodev_configure*.
> + * @param	cb		Pointer to user supplied callback created via
> + *				rte_cryptodev_add_enq_callback().
> + *
> + * @return
> + *   - 0: Success. Callback was removed.
> + *   - -EINVAL:  The dev_id or the qp_id is out of range, or the callback
> + *               is NULL or not found for the crypto device queue pair.
> + */
> +
> +__rte_experimental
> +int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +				      uint16_t qp_id,
> +				      struct rte_cryptodev_cb *cb);
> +
> +#endif
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
> b/lib/librte_cryptodev/rte_cryptodev_version.map
> index 7e4360f..5d8d6b0 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> @@ -101,6 +101,7 @@ EXPERIMENTAL {
>  	rte_cryptodev_get_qp_status;
> 
>  	# added in 20.11
> +	rte_cryptodev_add_enq_callback;
>  	rte_cryptodev_configure_raw_dp_ctx;
>  	rte_cryptodev_get_raw_dp_ctx_size;
>  	rte_cryptodev_raw_dequeue;
> @@ -109,4 +110,5 @@ EXPERIMENTAL {
>  	rte_cryptodev_raw_enqueue;
>  	rte_cryptodev_raw_enqueue_burst;
>  	rte_cryptodev_raw_enqueue_done;
> +	rte_cryptodev_remove_enq_callback;
>  };
> --
> 1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions
  2020-10-19  2:57 ` [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions Abhinandan Gujjar
  2020-10-21  7:28   ` Honnappa Nagarahalli
@ 2020-10-21 19:33   ` Ananyev, Konstantin
  2020-10-23 12:36     ` Gujjar, Abhinandan S
  1 sibling, 1 reply; 7+ messages in thread
From: Ananyev, Konstantin @ 2020-10-21 19:33 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj


Hi Abhinandan,

Thanks for the effort, good progress.
Though few more comments, see below.

> This patch adds APIs to add/remove callback functions. The callback
> function will be called for each burst of crypto ops received on a
> given crypto device queue pair.
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  config/rte_config.h                            |   1 +
>  lib/librte_cryptodev/meson.build               |   2 +-
>  lib/librte_cryptodev/rte_cryptodev.c           | 201 +++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev.h           | 153 ++++++++++++++++++-
>  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
>  5 files changed, 357 insertions(+), 2 deletions(-)

Don't forget to update Release Notes and probably Prog Guide too.

> 
> diff --git a/config/rte_config.h b/config/rte_config.h
> index 03d90d7..e999d93 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -61,6 +61,7 @@
>  /* cryptodev defines */
>  #define RTE_CRYPTO_MAX_DEVS 64
>  #define RTE_CRYPTODEV_NAME_LEN 64
> +#define RTE_CRYPTO_CALLBACKS 1
> 
>  /* compressdev defines */
>  #define RTE_COMPRESS_MAX_DEVS 64
> diff --git a/lib/librte_cryptodev/meson.build b/lib/librte_cryptodev/meson.build
> index c4c6b3b..8c5493f 100644
> --- a/lib/librte_cryptodev/meson.build
> +++ b/lib/librte_cryptodev/meson.build
> @@ -9,4 +9,4 @@ headers = files('rte_cryptodev.h',
>  	'rte_crypto.h',
>  	'rte_crypto_sym.h',
>  	'rte_crypto_asym.h')
> -deps += ['kvargs', 'mbuf']
> +deps += ['kvargs', 'mbuf', 'rcu']
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 3d95ac6..5ba774a 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -448,6 +448,10 @@ struct rte_cryptodev_sym_session_pool_private_data {
>  	return 0;
>  }
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/* spinlock for crypto device enq callbacks */
> +static rte_spinlock_t rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER;
> +#endif
> 
>  const char *
>  rte_cryptodev_get_feature_name(uint64_t flag)
> @@ -1136,6 +1140,203 @@ struct rte_cryptodev *
>  			socket_id);
>  }
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +
> +struct rte_cryptodev_cb *
> +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> +			       uint16_t qp_id,
> +			       rte_cryptodev_callback_fn cb_fn,
> +			       void *cb_arg)
> +{
> +	struct rte_cryptodev *dev;
> +	struct rte_cryptodev_cb *cb, *tail;
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	struct rte_rcu_qsbr *qsbr;
> +	size_t size;
> +
> +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> +	const uint32_t max_threads = 1;
> +
> +	if (!cb_fn)
> +		return NULL;
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		return NULL;
> +	}
> +
> +	dev = &rte_crypto_devices[dev_id];
> +	if (qp_id >= dev->data->nb_queue_pairs) {
> +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> +		return NULL;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> +	if (dev->enq_cbs == NULL) {
> +		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
> +					   dev->data->nb_queue_pairs, 0);
> +		if (dev->enq_cbs == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);

It is a bit clumsy to do unlock() for every return with error.
Probably an  easier way - create an internal function that would do the actual job, and then
lock(); ret=actual_job_internal_functio(...); unlock();...

> +			rte_errno = ENOMEM;
> +			return NULL;
> +		}
> +
> +		list = rte_zmalloc(NULL, sizeof(*list), 0);

As I understand, list is per queue, while enq_cbs[] is per port.
So if enq_cbs is not null, it doesn't mean that list for that particular queue is
already properly initialized.

Another thing - is there any point for dev->enq_cbs[] to be a an array of pointers to
rte_cryptodev_enq_cb_rcu? Considering that rte_cryptodev_enq_cb_rcu itself contains
just two pointers inside, I think it enq_cbs can point just to an array of rte_cryptodev_enq_cb_rcu:

struct rte_cryptodev {
	...
	struct rte_cryptodev_enq_cb_rcu *enq_cbs;

And you can remove one level of indirection here and in other places.

> +		if (list == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for list on "
> +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_errno = ENOMEM;
> +			rte_free(dev->enq_cbs);

Here and in other places: you free dev->enq_cbs, but not set it to NULL.
In fact - probably a good idea to have one cleanup() function that would free
all necessary stuff and set it to null, and then use it in all such places. 

> +			return NULL;
> +		}
> +
> +		/* Create RCU QSBR variable */
> +		size = rte_rcu_qsbr_get_memsize(max_threads);
> +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> +		if (qsbr == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for RCU on "
> +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_errno = ENOMEM;
> +			rte_free(list);
> +			rte_free(dev->enq_cbs);
> +			dev->enq_cbs[qp_id] = NULL;
> +			return NULL;
> +		}
> +
> +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +			rte_free(qsbr);
> +			rte_free(list);
> +			rte_free(dev->enq_cbs);
> +			dev->enq_cbs[qp_id] = NULL;
> +			return NULL;
> +		}
> +
> +		dev->enq_cbs[qp_id] = list;
> +		list->qsbr = qsbr;
> +	}
> +
> +	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> +	if (cb == NULL) {
> +		CDEV_LOG_ERR("Failed to allocate memory for callback on "
> +			     "dev=%d, queue_pair_id=%d", dev_id, qp_id);
> +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	cb->fn = cb_fn;
> +	cb->arg = cb_arg;
> +
> +	/* Add the callbacks in fifo order. */
> +	list = dev->enq_cbs[qp_id];
> +	tail = list->next;
> +	if (tail) {
> +		while (tail->next)
> +			tail = tail->next;
> +		tail->next = cb;
> +	} else
> +		list->next = cb;
> +
> +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +
> +	return cb;
> +}
> +
> +int
> +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +				  uint16_t qp_id,
> +				  struct rte_cryptodev_cb *cb)
> +{
> +	struct rte_cryptodev *dev;
> +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	uint16_t qp;
> +	int free_mem;
> +	int ret;
> +
> +	free_mem = 1;
> +	ret = -EINVAL;
> +
> +	if (!cb) {
> +		CDEV_LOG_ERR("cb is NULL");
> +		return ret;
> +	}
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		return ret;
> +	}
> +
> +	dev = &rte_crypto_devices[dev_id];
> +	if (qp_id >= dev->data->nb_queue_pairs) {
> +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> +		return ret;
> +	}
> +
> +	list = dev->enq_cbs[qp_id];
> +	if (list == NULL) {
> +		CDEV_LOG_ERR("Callback list is NULL");
> +		return ret;
> +	}
> +
> +	if (list->qsbr == NULL) {
> +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> +		return ret;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> +	if (dev->enq_cbs == NULL) {
> +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +		return ret;
> +	}
> +
> +	prev_cb = &list->next;
> +	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
> +		curr_cb = *prev_cb;
> +		if (curr_cb == cb) {
> +			/* Remove the user cb from the callback list. */
> +			*prev_cb = curr_cb->next;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (!ret) {
> +		/* Call sync with invalid thread id as this is part of
> +		 * control plane API
> +		 */
> +		rte_rcu_qsbr_synchronize(list->qsbr, RTE_QSBR_THRID_INVALID);
> +		rte_free(cb);
> +	}
> +
> +	if (list->next == NULL) {
> +		rte_free(list->qsbr);

We can't destroy our sync variable while device is not stopped or destroyed.
It can be still used by DP.
Probably the easiest way to deal with it - allocate and initialize enq_cbs[] and all 
related qsbrs at first add_callback and free all that memory only on dev_destroy().

> +		rte_free(list);
> +		dev->enq_cbs[qp_id] = NULL;
> +	}
> +
> +	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
> +		if (dev->enq_cbs[qp] != NULL) {
> +			free_mem = 0;
> +			break;
> +		}
> +
> +	if (free_mem) {
> +		rte_free(dev->enq_cbs);

Again, not safe to do here, see above.

> +		dev->enq_cbs = NULL;
> +	}
> +
> +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +
> +	return ret;
> +}
> +#endif
> 
>  int
>  rte_cryptodev_stats_get(uint8_t dev_id, struct rte_cryptodev_stats *stats)
> diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
> index 0935fd5..669746d 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -23,6 +23,7 @@
>  #include "rte_dev.h"
>  #include <rte_common.h>
>  #include <rte_config.h>
> +#include <rte_rcu_qsbr.h>
> 
>  #include "rte_cryptodev_trace_fp.h"
> 
> @@ -522,6 +523,34 @@ struct rte_cryptodev_qp_conf {
>  	/**< The mempool for creating sess private data in sessionless mode */
>  };
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/**
> + * Function type used for pre processing crypto ops when enqueue burst is
> + * called.
> + *
> + * The callback function is called on enqueue burst immediately
> + * before the crypto ops are put onto the hardware queue for processing.
> + *
> + * @param	dev_id		The identifier of the device.
> + * @param	qp_id		The index of the queue pair in which ops are
> + *				to be enqueued for processing. The value
> + *				must be in the range [0, nb_queue_pairs - 1]
> + *				previously supplied to
> + *				*rte_cryptodev_configure*.
> + * @param	ops		The address of an array of *nb_ops* pointers
> + *				to *rte_crypto_op* structures which contain
> + *				the crypto operations to be processed.
> + * @param	nb_ops		The number of operations to process.
> + * @param	user_param	The arbitrary user parameter passed in by the
> + *				application when the callback was originally
> + *				registered.
> + * @return			The number of ops to be enqueued to the
> + *				crypto device.
> + */
> +typedef uint16_t (*rte_cryptodev_callback_fn)(uint16_t dev_id, uint16_t qp_id,
> +		struct rte_crypto_op **ops, uint16_t nb_ops, void *user_param);
> +#endif
> +
>  /**
>   * Typedef for application callback function to be registered by application
>   * software for notification of device events
> @@ -822,7 +851,6 @@ struct rte_cryptodev_config {
>  		enum rte_cryptodev_event_type event,
>  		rte_cryptodev_cb_fn cb_fn, void *cb_arg);
> 
> -
>  typedef uint16_t (*dequeue_pkt_burst_t)(void *qp,
>  		struct rte_crypto_op **ops,	uint16_t nb_ops);
>  /**< Dequeue processed packets from queue pair of a device. */
> @@ -839,6 +867,33 @@ typedef uint16_t (*enqueue_pkt_burst_t)(void *qp,
>  /** Structure to keep track of registered callbacks */
>  TAILQ_HEAD(rte_cryptodev_cb_list, rte_cryptodev_callback);
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/**
> + * @internal
> + * Structure used to hold information about the callbacks to be called for a
> + * queue pair on enqueue.
> + */
> +struct rte_cryptodev_cb {
> +	struct rte_cryptodev_cb *next;
> +	/** < Pointer to next callback */
> +	rte_cryptodev_callback_fn fn;
> +	/** < Pointer to callback function */
> +	void *arg;
> +	/** < Pointer to argument */
> +};
> +
> +/**
> + * @internal
> + * Structure used to hold information about the RCU for a queue pair.
> + */
> +struct rte_cryptodev_enq_cb_rcu {
> +	struct rte_cryptodev_cb *next;
> +	/** < Pointer to next callback */
> +	struct rte_rcu_qsbr *qsbr;
> +	/** < RCU QSBR variable per queue pair */
> +};
> +#endif
> +
>  /** The data structure associated with each crypto device. */
>  struct rte_cryptodev {
>  	dequeue_pkt_burst_t dequeue_burst;
> @@ -867,6 +922,11 @@ struct rte_cryptodev {
>  	__extension__
>  	uint8_t attached : 1;
>  	/**< Flag indicating the device is attached */
> +
> +#ifdef RTE_CRYPTO_CALLBACKS

I'd *always* reserve space for it.
No matter is RTE_CRYPTO_CALLBACKS defined or not.
To avoid difference in public structure layout.

> +	struct rte_cryptodev_enq_cb_rcu **enq_cbs;

As I said above, no need for extra level of indirection.

> +	/**< User application callback for pre enqueue processing */
> +#endif

As I understand, it is not an ABI breakage - as there are some free space right now
at the end of struct rte_cryptodev (due to it alignment), but definitely need to update RN.


>  } __rte_cache_aligned;
> 
>  void *
> @@ -989,6 +1049,25 @@ struct rte_cryptodev_data {
>  {
>  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {

Agree with Honnappa's comment for that piece of code.
Probably need to be something like:

if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id].next != NULL) {
	list = &dev->enq_cbs[qp_id];
	rte_rcu_qsbr_thread_online(list->qsbr, 0);
	for (cb = list->next; cb != NULL; cb = cb->next)
		....
	rte_rcu_qsbr_thread_offline(list->qsbr, 0);  
}


> +		struct rte_cryptodev_enq_cb_rcu *list;
> +		struct rte_cryptodev_cb *cb;
> +
> +		list = dev->enq_cbs[qp_id];
> +		cb = list->next;
> +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> +
> +		do {
> +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> +					cb->arg);
> +			cb = cb->next;
> +		} while (cb != NULL);
> +
> +		rte_rcu_qsbr_thread_offline(list->qsbr, 0);
> +	}
> +#endif
> +
>  	rte_cryptodev_trace_enqueue_burst(dev_id, qp_id, (void **)ops, nb_ops);
>  	return (*dev->enqueue_burst)(
>  			dev->data->queue_pairs[qp_id], ops, nb_ops);
> @@ -1730,6 +1809,78 @@ struct rte_crypto_raw_dp_ctx {
>  rte_cryptodev_raw_dequeue_done(struct rte_crypto_raw_dp_ctx *ctx,
>  		uint32_t n);
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Add a user callback for a given crypto device and queue pair which will be
> + * called on crypto ops enqueue.
> + *
> + * This API configures a function to be called for each burst of crypto ops
> + * received on a given crypto device queue pair. The return value is a pointer
> + * that can be used later to remove the callback using
> + * rte_cryptodev_remove_enq_callback().
> + *
> + * Multiple functions are called in the order that they are added.
> + *
> + * @param	dev_id		The identifier of the device.
> + * @param	qp_id		The index of the queue pair in which ops are
> + *				to be enqueued for processing. The value
> + *				must be in the range [0, nb_queue_pairs - 1]
> + *				previously supplied to
> + *				*rte_cryptodev_configure*.
> + * @param	cb_fn		The callback function
> + * @param	cb_arg		A generic pointer parameter which will be passed
> + *				to each invocation of the callback function on
> + *				this crypto device and queue pair.
> + *
> + * @return
> + *   NULL on error.
> + *   On success, a pointer value which can later be used to remove the callback.
> + */
> +
> +__rte_experimental
> +struct rte_cryptodev_cb *
> +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> +			       uint16_t qp_id,
> +			       rte_cryptodev_callback_fn cb_fn,
> +			       void *cb_arg);
> +
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Remove a user callback function for given crypto device and queue pair.
> + *
> + * This function is used to removed callbacks that were added to a crypto
> + * device queue pair using rte_cryptodev_add_enq_callback().
> + *
> + *
> + *
> + * @param	dev_id		The identifier of the device.
> + * @param	qp_id		The index of the queue pair in which ops are
> + *				to be enqueued for processing. The value
> + *				must be in the range [0, nb_queue_pairs - 1]
> + *				previously supplied to
> + *				*rte_cryptodev_configure*.
> + * @param	cb		Pointer to user supplied callback created via
> + *				rte_cryptodev_add_enq_callback().
> + *
> + * @return
> + *   - 0: Success. Callback was removed.
> + *   - -EINVAL:  The dev_id or the qp_id is out of range, or the callback
> + *               is NULL or not found for the crypto device queue pair.
> + */
> +
> +__rte_experimental
> +int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +				      uint16_t qp_id,
> +				      struct rte_cryptodev_cb *cb);
> +
> +#endif
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map
> index 7e4360f..5d8d6b0 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> @@ -101,6 +101,7 @@ EXPERIMENTAL {
>  	rte_cryptodev_get_qp_status;
> 
>  	# added in 20.11
> +	rte_cryptodev_add_enq_callback;
>  	rte_cryptodev_configure_raw_dp_ctx;
>  	rte_cryptodev_get_raw_dp_ctx_size;
>  	rte_cryptodev_raw_dequeue;
> @@ -109,4 +110,5 @@ EXPERIMENTAL {
>  	rte_cryptodev_raw_enqueue;
>  	rte_cryptodev_raw_enqueue_burst;
>  	rte_cryptodev_raw_enqueue_done;
> +	rte_cryptodev_remove_enq_callback;
>  };
> --
> 1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions
  2020-10-21  7:28   ` Honnappa Nagarahalli
@ 2020-10-23 12:34     ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 7+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-23 12:34 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, Doherty, Declan, Akhil.goyal@nxp.com,
	Ananyev, Konstantin
  Cc: Vangati, Narender, jerinj, nd, nd

Hi Honnappa,

I will take care of this in next patch set.

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, October 21, 2020 12:58 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>; Akhil.goyal@nxp.com; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>
> Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com; nd
> <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> nd <nd@arm.com>
> Subject: RE: [v3 1/2] cryptodev: support enqueue callback functions
> 
> <snip>
> 
> >
> > This patch adds APIs to add/remove callback functions. The callback
> > function will be called for each burst of crypto ops received on a
> > given crypto device queue pair.
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > ---
> >  config/rte_config.h                            |   1 +
> >  lib/librte_cryptodev/meson.build               |   2 +-
> >  lib/librte_cryptodev/rte_cryptodev.c           | 201
> > +++++++++++++++++++++++++
> >  lib/librte_cryptodev/rte_cryptodev.h           | 153 ++++++++++++++++++-
> >  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
> >  5 files changed, 357 insertions(+), 2 deletions(-)
> >
> > diff --git a/config/rte_config.h b/config/rte_config.h index
> > 03d90d7..e999d93
> > 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -61,6 +61,7 @@
> >  /* cryptodev defines */
> >  #define RTE_CRYPTO_MAX_DEVS 64
> >  #define RTE_CRYPTODEV_NAME_LEN 64
> > +#define RTE_CRYPTO_CALLBACKS 1
> >
> >  /* compressdev defines */
> >  #define RTE_COMPRESS_MAX_DEVS 64
> > diff --git a/lib/librte_cryptodev/meson.build
> > b/lib/librte_cryptodev/meson.build
> > index c4c6b3b..8c5493f 100644
> > --- a/lib/librte_cryptodev/meson.build
> > +++ b/lib/librte_cryptodev/meson.build
> > @@ -9,4 +9,4 @@ headers = files('rte_cryptodev.h',
> >  	'rte_crypto.h',
> >  	'rte_crypto_sym.h',
> >  	'rte_crypto_asym.h')
> > -deps += ['kvargs', 'mbuf']
> > +deps += ['kvargs', 'mbuf', 'rcu']
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> > b/lib/librte_cryptodev/rte_cryptodev.c
> > index 3d95ac6..5ba774a 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -448,6 +448,10 @@ struct
> > rte_cryptodev_sym_session_pool_private_data {
> >  	return 0;
> >  }
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/* spinlock for crypto device enq callbacks */ static rte_spinlock_t
> > +rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER; #endif
> >
> >  const char *
> >  rte_cryptodev_get_feature_name(uint64_t flag) @@ -1136,6 +1140,203 @@
> > struct rte_cryptodev *
> >  			socket_id);
> >  }
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +
> > +struct rte_cryptodev_cb *
> > +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> > +			       uint16_t qp_id,
> > +			       rte_cryptodev_callback_fn cb_fn,
> > +			       void *cb_arg)
> > +{
> > +	struct rte_cryptodev *dev;
> > +	struct rte_cryptodev_cb *cb, *tail;
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	struct rte_rcu_qsbr *qsbr;
> > +	size_t size;
> > +
> > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > +	const uint32_t max_threads = 1;
> > +
> > +	if (!cb_fn)
> > +		return NULL;
> > +
> > +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> > +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> > +		return NULL;
> > +	}
> > +
> > +	dev = &rte_crypto_devices[dev_id];
> > +	if (qp_id >= dev->data->nb_queue_pairs) {
> > +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> > +		return NULL;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> > +	if (dev->enq_cbs == NULL) {
> > +		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
> > +					   dev->data->nb_queue_pairs, 0);
> > +		if (dev->enq_cbs == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for
> > callbacks");
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_errno = ENOMEM;
> > +			return NULL;
> > +		}
> > +
> > +		list = rte_zmalloc(NULL, sizeof(*list), 0);
> > +		if (list == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for list on
> "
> > +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_errno = ENOMEM;
> > +			rte_free(dev->enq_cbs);
> > +			return NULL;
> > +		}
> > +
> > +		/* Create RCU QSBR variable */
> > +		size = rte_rcu_qsbr_get_memsize(max_threads);
> > +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> > +		if (qsbr == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for RCU
> on
> > "
> > +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_errno = ENOMEM;
> > +			rte_free(list);
> > +			rte_free(dev->enq_cbs);
> > +			dev->enq_cbs[qp_id] = NULL;
> > +			return NULL;
> > +		}
> > +
> > +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> > +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> > +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_free(qsbr);
> > +			rte_free(list);
> > +			rte_free(dev->enq_cbs);
> > +			dev->enq_cbs[qp_id] = NULL;
> > +			return NULL;
> > +		}
> > +
> > +		dev->enq_cbs[qp_id] = list;
> > +		list->qsbr = qsbr;
> > +	}
> > +
> > +	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> > +	if (cb == NULL) {
> > +		CDEV_LOG_ERR("Failed to allocate memory for callback on "
> > +			     "dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +		rte_errno = ENOMEM;
> > +		return NULL;
> > +	}
> > +
> > +	cb->fn = cb_fn;
> > +	cb->arg = cb_arg;
> > +
> > +	/* Add the callbacks in fifo order. */
> > +	list = dev->enq_cbs[qp_id];
> > +	tail = list->next;
> > +	if (tail) {
> > +		while (tail->next)
> > +			tail = tail->next;
> > +		tail->next = cb;
> > +	} else
> > +		list->next = cb;
> > +
> Correct memory orderings need to be used to provide reader-writer
> concurrency. Please look at [1] for changes you need to make here.
> [1] https://patches.dpdk.org/patch/80603/
> 
> > +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +
> > +	return cb;
> > +}
> > +
> > +int
> > +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +				  uint16_t qp_id,
> > +				  struct rte_cryptodev_cb *cb)
> > +{
> > +	struct rte_cryptodev *dev;
> > +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	uint16_t qp;
> > +	int free_mem;
> > +	int ret;
> > +
> > +	free_mem = 1;
> > +	ret = -EINVAL;
> > +
> > +	if (!cb) {
> > +		CDEV_LOG_ERR("cb is NULL");
> > +		return ret;
> > +	}
> > +
> > +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> > +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> > +		return ret;
> > +	}
> > +
> > +	dev = &rte_crypto_devices[dev_id];
> > +	if (qp_id >= dev->data->nb_queue_pairs) {
> > +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> > +		return ret;
> > +	}
> > +
> > +	list = dev->enq_cbs[qp_id];
> > +	if (list == NULL) {
> > +		CDEV_LOG_ERR("Callback list is NULL");
> > +		return ret;
> > +	}
> > +
> > +	if (list->qsbr == NULL) {
> > +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> > +		return ret;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> > +	if (dev->enq_cbs == NULL) {
> > +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +		return ret;
> > +	}
> > +
> > +	prev_cb = &list->next;
> > +	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
> > +		curr_cb = *prev_cb;
> > +		if (curr_cb == cb) {
> > +			/* Remove the user cb from the callback list. */
> > +			*prev_cb = curr_cb->next;
> > +			ret = 0;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!ret) {
> > +		/* Call sync with invalid thread id as this is part of
> > +		 * control plane API
> > +		 */
> > +		rte_rcu_qsbr_synchronize(list->qsbr,
> > RTE_QSBR_THRID_INVALID);
> > +		rte_free(cb);
> > +	}
> > +
> > +	if (list->next == NULL) {
> > +		rte_free(list->qsbr);
> > +		rte_free(list);
> > +		dev->enq_cbs[qp_id] = NULL;
> > +	}
> > +
> > +	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
> > +		if (dev->enq_cbs[qp] != NULL) {
> > +			free_mem = 0;
> > +			break;
> > +		}
> > +
> > +	if (free_mem) {
> > +		rte_free(dev->enq_cbs);
> > +		dev->enq_cbs = NULL;
> > +	}
> > +
> > +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +
> > +	return ret;
> > +}
> > +#endif
> >
> >  int
> >  rte_cryptodev_stats_get(uint8_t dev_id, struct rte_cryptodev_stats
> > *stats) diff --git a/lib/librte_cryptodev/rte_cryptodev.h
> > b/lib/librte_cryptodev/rte_cryptodev.h
> > index 0935fd5..669746d 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.h
> > +++ b/lib/librte_cryptodev/rte_cryptodev.h
> > @@ -23,6 +23,7 @@
> >  #include "rte_dev.h"
> >  #include <rte_common.h>
> >  #include <rte_config.h>
> > +#include <rte_rcu_qsbr.h>
> >
> >  #include "rte_cryptodev_trace_fp.h"
> >
> > @@ -522,6 +523,34 @@ struct rte_cryptodev_qp_conf {
> >  	/**< The mempool for creating sess private data in sessionless mode
> > */ };
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/**
> > + * Function type used for pre processing crypto ops when enqueue
> > +burst is
> > + * called.
> > + *
> > + * The callback function is called on enqueue burst immediately
> > + * before the crypto ops are put onto the hardware queue for processing.
> > + *
> > + * @param	dev_id		The identifier of the device.
> > + * @param	qp_id		The index of the queue pair in which ops are
> > + *				to be enqueued for processing. The value
> > + *				must be in the range [0, nb_queue_pairs - 1]
> > + *				previously supplied to
> > + *				*rte_cryptodev_configure*.
> > + * @param	ops		The address of an array of *nb_ops* pointers
> > + *				to *rte_crypto_op* structures which contain
> > + *				the crypto operations to be processed.
> > + * @param	nb_ops		The number of operations to process.
> > + * @param	user_param	The arbitrary user parameter passed in by the
> > + *				application when the callback was originally
> > + *				registered.
> > + * @return			The number of ops to be enqueued to the
> > + *				crypto device.
> > + */
> > +typedef uint16_t (*rte_cryptodev_callback_fn)(uint16_t dev_id,
> > +uint16_t
> > qp_id,
> > +		struct rte_crypto_op **ops, uint16_t nb_ops, void
> > *user_param);
> > +#endif
> > +
> >  /**
> >   * Typedef for application callback function to be registered by application
> >   * software for notification of device events @@ -822,7 +851,6 @@
> > struct rte_cryptodev_config {
> >  		enum rte_cryptodev_event_type event,
> >  		rte_cryptodev_cb_fn cb_fn, void *cb_arg);
> >
> > -
> >  typedef uint16_t (*dequeue_pkt_burst_t)(void *qp,
> >  		struct rte_crypto_op **ops,	uint16_t nb_ops);
> >  /**< Dequeue processed packets from queue pair of a device. */ @@
> > -839,6
> > +867,33 @@ typedef uint16_t (*enqueue_pkt_burst_t)(void *qp,
> >  /** Structure to keep track of registered callbacks */
> > TAILQ_HEAD(rte_cryptodev_cb_list, rte_cryptodev_callback);
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/**
> > + * @internal
> > + * Structure used to hold information about the callbacks to be
> > +called for a
> > + * queue pair on enqueue.
> > + */
> > +struct rte_cryptodev_cb {
> > +	struct rte_cryptodev_cb *next;
> > +	/** < Pointer to next callback */
> > +	rte_cryptodev_callback_fn fn;
> > +	/** < Pointer to callback function */
> > +	void *arg;
> > +	/** < Pointer to argument */
> > +};
> > +
> > +/**
> > + * @internal
> > + * Structure used to hold information about the RCU for a queue pair.
> > + */
> > +struct rte_cryptodev_enq_cb_rcu {
> > +	struct rte_cryptodev_cb *next;
> > +	/** < Pointer to next callback */
> > +	struct rte_rcu_qsbr *qsbr;
> > +	/** < RCU QSBR variable per queue pair */ }; #endif
> > +
> >  /** The data structure associated with each crypto device. */  struct
> > rte_cryptodev {
> >  	dequeue_pkt_burst_t dequeue_burst;
> > @@ -867,6 +922,11 @@ struct rte_cryptodev {
> >  	__extension__
> >  	uint8_t attached : 1;
> >  	/**< Flag indicating the device is attached */
> > +
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	struct rte_cryptodev_enq_cb_rcu **enq_cbs;
> > +	/**< User application callback for pre enqueue processing */ #endif
> >  } __rte_cache_aligned;
> >
> >  void *
> > @@ -989,6 +1049,25 @@ struct rte_cryptodev_data {  {
> >  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
> > +		struct rte_cryptodev_enq_cb_rcu *list;
> > +		struct rte_cryptodev_cb *cb;
> > +
> > +		list = dev->enq_cbs[qp_id];
> > +		cb = list->next;
> > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> Move this 2 lines up as you are reading the addresses.
> Also, look at the patch mentioned earlier for memory orderings.
> 
> > +
> > +		do {
> > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > +					cb->arg);
> > +			cb = cb->next;
> > +		} while (cb != NULL);
> > +
> > +		rte_rcu_qsbr_thread_offline(list->qsbr, 0);
> > +	}
> > +#endif
> > +
> >  	rte_cryptodev_trace_enqueue_burst(dev_id, qp_id, (void **)ops,
> > nb_ops);
> >  	return (*dev->enqueue_burst)(
> >  			dev->data->queue_pairs[qp_id], ops, nb_ops); @@ -
> > 1730,6 +1809,78 @@ struct rte_crypto_raw_dp_ctx {
> > rte_cryptodev_raw_dequeue_done(struct rte_crypto_raw_dp_ctx *ctx,
> >  		uint32_t n);
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Add a user callback for a given crypto device and queue pair which
> > +will be
> > + * called on crypto ops enqueue.
> > + *
> > + * This API configures a function to be called for each burst of
> > +crypto ops
> > + * received on a given crypto device queue pair. The return value is
> > +a pointer
> > + * that can be used later to remove the callback using
> > + * rte_cryptodev_remove_enq_callback().
> > + *
> > + * Multiple functions are called in the order that they are added.
> > + *
> > + * @param	dev_id		The identifier of the device.
> > + * @param	qp_id		The index of the queue pair in which ops are
> > + *				to be enqueued for processing. The value
> > + *				must be in the range [0, nb_queue_pairs - 1]
> > + *				previously supplied to
> > + *				*rte_cryptodev_configure*.
> > + * @param	cb_fn		The callback function
> > + * @param	cb_arg		A generic pointer parameter which will be
> > passed
> > + *				to each invocation of the callback function on
> > + *				this crypto device and queue pair.
> > + *
> > + * @return
> > + *   NULL on error.
> > + *   On success, a pointer value which can later be used to remove the
> callback.
> > + */
> > +
> > +__rte_experimental
> > +struct rte_cryptodev_cb *
> > +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> > +			       uint16_t qp_id,
> > +			       rte_cryptodev_callback_fn cb_fn,
> > +			       void *cb_arg);
> > +
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Remove a user callback function for given crypto device and queue pair.
> > + *
> > + * This function is used to removed callbacks that were added to a
> > +crypto
> > + * device queue pair using rte_cryptodev_add_enq_callback().
> > + *
> > + *
> > + *
> > + * @param	dev_id		The identifier of the device.
> > + * @param	qp_id		The index of the queue pair in which ops are
> > + *				to be enqueued for processing. The value
> > + *				must be in the range [0, nb_queue_pairs - 1]
> > + *				previously supplied to
> > + *				*rte_cryptodev_configure*.
> > + * @param	cb		Pointer to user supplied callback created via
> > + *				rte_cryptodev_add_enq_callback().
> > + *
> > + * @return
> > + *   - 0: Success. Callback was removed.
> > + *   - -EINVAL:  The dev_id or the qp_id is out of range, or the callback
> > + *               is NULL or not found for the crypto device queue pair.
> > + */
> > +
> > +__rte_experimental
> > +int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +				      uint16_t qp_id,
> > +				      struct rte_cryptodev_cb *cb);
> > +
> > +#endif
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
> > b/lib/librte_cryptodev/rte_cryptodev_version.map
> > index 7e4360f..5d8d6b0 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> > +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> > @@ -101,6 +101,7 @@ EXPERIMENTAL {
> >  	rte_cryptodev_get_qp_status;
> >
> >  	# added in 20.11
> > +	rte_cryptodev_add_enq_callback;
> >  	rte_cryptodev_configure_raw_dp_ctx;
> >  	rte_cryptodev_get_raw_dp_ctx_size;
> >  	rte_cryptodev_raw_dequeue;
> > @@ -109,4 +110,5 @@ EXPERIMENTAL {
> >  	rte_cryptodev_raw_enqueue;
> >  	rte_cryptodev_raw_enqueue_burst;
> >  	rte_cryptodev_raw_enqueue_done;
> > +	rte_cryptodev_remove_enq_callback;
> >  };
> > --
> > 1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions
  2020-10-21 19:33   ` Ananyev, Konstantin
@ 2020-10-23 12:36     ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 7+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-23 12:36 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj

Hi Konstantin,

Thanks. I will generate a new patch with suggested changes.

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, October 22, 2020 1:04 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>; akhil.goyal@nxp.com;
> Honnappa.Nagarahalli@arm.com
> Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com
> Subject: RE: [v3 1/2] cryptodev: support enqueue callback functions
> 
> 
> Hi Abhinandan,
> 
> Thanks for the effort, good progress.
> Though few more comments, see below.
> 
> > This patch adds APIs to add/remove callback functions. The callback
> > function will be called for each burst of crypto ops received on a
> > given crypto device queue pair.
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > ---
> >  config/rte_config.h                            |   1 +
> >  lib/librte_cryptodev/meson.build               |   2 +-
> >  lib/librte_cryptodev/rte_cryptodev.c           | 201
> +++++++++++++++++++++++++
> >  lib/librte_cryptodev/rte_cryptodev.h           | 153 ++++++++++++++++++-
> >  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
> >  5 files changed, 357 insertions(+), 2 deletions(-)
> 
> Don't forget to update Release Notes and probably Prog Guide too.
> 
> >
> > diff --git a/config/rte_config.h b/config/rte_config.h index
> > 03d90d7..e999d93 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -61,6 +61,7 @@
> >  /* cryptodev defines */
> >  #define RTE_CRYPTO_MAX_DEVS 64
> >  #define RTE_CRYPTODEV_NAME_LEN 64
> > +#define RTE_CRYPTO_CALLBACKS 1
> >
> >  /* compressdev defines */
> >  #define RTE_COMPRESS_MAX_DEVS 64
> > diff --git a/lib/librte_cryptodev/meson.build
> > b/lib/librte_cryptodev/meson.build
> > index c4c6b3b..8c5493f 100644
> > --- a/lib/librte_cryptodev/meson.build
> > +++ b/lib/librte_cryptodev/meson.build
> > @@ -9,4 +9,4 @@ headers = files('rte_cryptodev.h',
> >  	'rte_crypto.h',
> >  	'rte_crypto_sym.h',
> >  	'rte_crypto_asym.h')
> > -deps += ['kvargs', 'mbuf']
> > +deps += ['kvargs', 'mbuf', 'rcu']
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> > b/lib/librte_cryptodev/rte_cryptodev.c
> > index 3d95ac6..5ba774a 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -448,6 +448,10 @@ struct
> rte_cryptodev_sym_session_pool_private_data {
> >  	return 0;
> >  }
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/* spinlock for crypto device enq callbacks */ static rte_spinlock_t
> > +rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER; #endif
> >
> >  const char *
> >  rte_cryptodev_get_feature_name(uint64_t flag) @@ -1136,6 +1140,203 @@
> > struct rte_cryptodev *
> >  			socket_id);
> >  }
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +
> > +struct rte_cryptodev_cb *
> > +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> > +			       uint16_t qp_id,
> > +			       rte_cryptodev_callback_fn cb_fn,
> > +			       void *cb_arg)
> > +{
> > +	struct rte_cryptodev *dev;
> > +	struct rte_cryptodev_cb *cb, *tail;
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	struct rte_rcu_qsbr *qsbr;
> > +	size_t size;
> > +
> > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > +	const uint32_t max_threads = 1;
> > +
> > +	if (!cb_fn)
> > +		return NULL;
> > +
> > +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> > +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> > +		return NULL;
> > +	}
> > +
> > +	dev = &rte_crypto_devices[dev_id];
> > +	if (qp_id >= dev->data->nb_queue_pairs) {
> > +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> > +		return NULL;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> > +	if (dev->enq_cbs == NULL) {
> > +		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
> > +					   dev->data->nb_queue_pairs, 0);
> > +		if (dev->enq_cbs == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for
> callbacks");
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> 
> It is a bit clumsy to do unlock() for every return with error.
> Probably an  easier way - create an internal function that would do the actual
> job, and then lock(); ret=actual_job_internal_functio(...); unlock();...
> 
> > +			rte_errno = ENOMEM;
> > +			return NULL;
> > +		}
> > +
> > +		list = rte_zmalloc(NULL, sizeof(*list), 0);
> 
> As I understand, list is per queue, while enq_cbs[] is per port.
> So if enq_cbs is not null, it doesn't mean that list for that particular queue is
> already properly initialized.
> 
> Another thing - is there any point for dev->enq_cbs[] to be a an array of
> pointers to rte_cryptodev_enq_cb_rcu? Considering that
> rte_cryptodev_enq_cb_rcu itself contains just two pointers inside, I think it
> enq_cbs can point just to an array of rte_cryptodev_enq_cb_rcu:
> 
> struct rte_cryptodev {
> 	...
> 	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> 
> And you can remove one level of indirection here and in other places.
> 
> > +		if (list == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for list on
> "
> > +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_errno = ENOMEM;
> > +			rte_free(dev->enq_cbs);
> 
> Here and in other places: you free dev->enq_cbs, but not set it to NULL.
> In fact - probably a good idea to have one cleanup() function that would free all
> necessary stuff and set it to null, and then use it in all such places.
> 
> > +			return NULL;
> > +		}
> > +
> > +		/* Create RCU QSBR variable */
> > +		size = rte_rcu_qsbr_get_memsize(max_threads);
> > +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> > +		if (qsbr == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for RCU
> on "
> > +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_errno = ENOMEM;
> > +			rte_free(list);
> > +			rte_free(dev->enq_cbs);
> > +			dev->enq_cbs[qp_id] = NULL;
> > +			return NULL;
> > +		}
> > +
> > +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> > +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> > +				"dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +			rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +			rte_free(qsbr);
> > +			rte_free(list);
> > +			rte_free(dev->enq_cbs);
> > +			dev->enq_cbs[qp_id] = NULL;
> > +			return NULL;
> > +		}
> > +
> > +		dev->enq_cbs[qp_id] = list;
> > +		list->qsbr = qsbr;
> > +	}
> > +
> > +	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> > +	if (cb == NULL) {
> > +		CDEV_LOG_ERR("Failed to allocate memory for callback on "
> > +			     "dev=%d, queue_pair_id=%d", dev_id, qp_id);
> > +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +		rte_errno = ENOMEM;
> > +		return NULL;
> > +	}
> > +
> > +	cb->fn = cb_fn;
> > +	cb->arg = cb_arg;
> > +
> > +	/* Add the callbacks in fifo order. */
> > +	list = dev->enq_cbs[qp_id];
> > +	tail = list->next;
> > +	if (tail) {
> > +		while (tail->next)
> > +			tail = tail->next;
> > +		tail->next = cb;
> > +	} else
> > +		list->next = cb;
> > +
> > +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +
> > +	return cb;
> > +}
> > +
> > +int
> > +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +				  uint16_t qp_id,
> > +				  struct rte_cryptodev_cb *cb)
> > +{
> > +	struct rte_cryptodev *dev;
> > +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	uint16_t qp;
> > +	int free_mem;
> > +	int ret;
> > +
> > +	free_mem = 1;
> > +	ret = -EINVAL;
> > +
> > +	if (!cb) {
> > +		CDEV_LOG_ERR("cb is NULL");
> > +		return ret;
> > +	}
> > +
> > +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> > +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> > +		return ret;
> > +	}
> > +
> > +	dev = &rte_crypto_devices[dev_id];
> > +	if (qp_id >= dev->data->nb_queue_pairs) {
> > +		CDEV_LOG_ERR("Invalid queue_pair_id=%d", qp_id);
> > +		return ret;
> > +	}
> > +
> > +	list = dev->enq_cbs[qp_id];
> > +	if (list == NULL) {
> > +		CDEV_LOG_ERR("Callback list is NULL");
> > +		return ret;
> > +	}
> > +
> > +	if (list->qsbr == NULL) {
> > +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> > +		return ret;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> > +	if (dev->enq_cbs == NULL) {
> > +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +		return ret;
> > +	}
> > +
> > +	prev_cb = &list->next;
> > +	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
> > +		curr_cb = *prev_cb;
> > +		if (curr_cb == cb) {
> > +			/* Remove the user cb from the callback list. */
> > +			*prev_cb = curr_cb->next;
> > +			ret = 0;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!ret) {
> > +		/* Call sync with invalid thread id as this is part of
> > +		 * control plane API
> > +		 */
> > +		rte_rcu_qsbr_synchronize(list->qsbr,
> RTE_QSBR_THRID_INVALID);
> > +		rte_free(cb);
> > +	}
> > +
> > +	if (list->next == NULL) {
> > +		rte_free(list->qsbr);
> 
> We can't destroy our sync variable while device is not stopped or destroyed.
> It can be still used by DP.
> Probably the easiest way to deal with it - allocate and initialize enq_cbs[] and
> all related qsbrs at first add_callback and free all that memory only on
> dev_destroy().
> 
> > +		rte_free(list);
> > +		dev->enq_cbs[qp_id] = NULL;
> > +	}
> > +
> > +	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
> > +		if (dev->enq_cbs[qp] != NULL) {
> > +			free_mem = 0;
> > +			break;
> > +		}
> > +
> > +	if (free_mem) {
> > +		rte_free(dev->enq_cbs);
> 
> Again, not safe to do here, see above.
> 
> > +		dev->enq_cbs = NULL;
> > +	}
> > +
> > +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +
> > +	return ret;
> > +}
> > +#endif
> >
> >  int
> >  rte_cryptodev_stats_get(uint8_t dev_id, struct rte_cryptodev_stats
> > *stats) diff --git a/lib/librte_cryptodev/rte_cryptodev.h
> > b/lib/librte_cryptodev/rte_cryptodev.h
> > index 0935fd5..669746d 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.h
> > +++ b/lib/librte_cryptodev/rte_cryptodev.h
> > @@ -23,6 +23,7 @@
> >  #include "rte_dev.h"
> >  #include <rte_common.h>
> >  #include <rte_config.h>
> > +#include <rte_rcu_qsbr.h>
> >
> >  #include "rte_cryptodev_trace_fp.h"
> >
> > @@ -522,6 +523,34 @@ struct rte_cryptodev_qp_conf {
> >  	/**< The mempool for creating sess private data in sessionless mode
> > */  };
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/**
> > + * Function type used for pre processing crypto ops when enqueue
> > +burst is
> > + * called.
> > + *
> > + * The callback function is called on enqueue burst immediately
> > + * before the crypto ops are put onto the hardware queue for processing.
> > + *
> > + * @param	dev_id		The identifier of the device.
> > + * @param	qp_id		The index of the queue pair in which ops are
> > + *				to be enqueued for processing. The value
> > + *				must be in the range [0, nb_queue_pairs - 1]
> > + *				previously supplied to
> > + *				*rte_cryptodev_configure*.
> > + * @param	ops		The address of an array of *nb_ops* pointers
> > + *				to *rte_crypto_op* structures which contain
> > + *				the crypto operations to be processed.
> > + * @param	nb_ops		The number of operations to process.
> > + * @param	user_param	The arbitrary user parameter passed in by the
> > + *				application when the callback was originally
> > + *				registered.
> > + * @return			The number of ops to be enqueued to the
> > + *				crypto device.
> > + */
> > +typedef uint16_t (*rte_cryptodev_callback_fn)(uint16_t dev_id, uint16_t
> qp_id,
> > +		struct rte_crypto_op **ops, uint16_t nb_ops, void
> *user_param);
> > +#endif
> > +
> >  /**
> >   * Typedef for application callback function to be registered by application
> >   * software for notification of device events @@ -822,7 +851,6 @@
> > struct rte_cryptodev_config {
> >  		enum rte_cryptodev_event_type event,
> >  		rte_cryptodev_cb_fn cb_fn, void *cb_arg);
> >
> > -
> >  typedef uint16_t (*dequeue_pkt_burst_t)(void *qp,
> >  		struct rte_crypto_op **ops,	uint16_t nb_ops);
> >  /**< Dequeue processed packets from queue pair of a device. */ @@
> > -839,6 +867,33 @@ typedef uint16_t (*enqueue_pkt_burst_t)(void *qp,
> >  /** Structure to keep track of registered callbacks */
> > TAILQ_HEAD(rte_cryptodev_cb_list, rte_cryptodev_callback);
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/**
> > + * @internal
> > + * Structure used to hold information about the callbacks to be
> > +called for a
> > + * queue pair on enqueue.
> > + */
> > +struct rte_cryptodev_cb {
> > +	struct rte_cryptodev_cb *next;
> > +	/** < Pointer to next callback */
> > +	rte_cryptodev_callback_fn fn;
> > +	/** < Pointer to callback function */
> > +	void *arg;
> > +	/** < Pointer to argument */
> > +};
> > +
> > +/**
> > + * @internal
> > + * Structure used to hold information about the RCU for a queue pair.
> > + */
> > +struct rte_cryptodev_enq_cb_rcu {
> > +	struct rte_cryptodev_cb *next;
> > +	/** < Pointer to next callback */
> > +	struct rte_rcu_qsbr *qsbr;
> > +	/** < RCU QSBR variable per queue pair */ }; #endif
> > +
> >  /** The data structure associated with each crypto device. */  struct
> > rte_cryptodev {
> >  	dequeue_pkt_burst_t dequeue_burst;
> > @@ -867,6 +922,11 @@ struct rte_cryptodev {
> >  	__extension__
> >  	uint8_t attached : 1;
> >  	/**< Flag indicating the device is attached */
> > +
> > +#ifdef RTE_CRYPTO_CALLBACKS
> 
> I'd *always* reserve space for it.
> No matter is RTE_CRYPTO_CALLBACKS defined or not.
> To avoid difference in public structure layout.
> 
> > +	struct rte_cryptodev_enq_cb_rcu **enq_cbs;
> 
> As I said above, no need for extra level of indirection.
> 
> > +	/**< User application callback for pre enqueue processing */ #endif
> 
> As I understand, it is not an ABI breakage - as there are some free space right
> now at the end of struct rte_cryptodev (due to it alignment), but definitely need
> to update RN.
> 
> 
> >  } __rte_cache_aligned;
> >
> >  void *
> > @@ -989,6 +1049,25 @@ struct rte_cryptodev_data {  {
> >  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
> 
> Agree with Honnappa's comment for that piece of code.
> Probably need to be something like:
> 
> if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id].next != NULL) {
> 	list = &dev->enq_cbs[qp_id];
> 	rte_rcu_qsbr_thread_online(list->qsbr, 0);
> 	for (cb = list->next; cb != NULL; cb = cb->next)
> 		....
> 	rte_rcu_qsbr_thread_offline(list->qsbr, 0); }
> 
> 
> > +		struct rte_cryptodev_enq_cb_rcu *list;
> > +		struct rte_cryptodev_cb *cb;
> > +
> > +		list = dev->enq_cbs[qp_id];
> > +		cb = list->next;
> > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> > +
> > +		do {
> > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > +					cb->arg);
> > +			cb = cb->next;
> > +		} while (cb != NULL);
> > +
> > +		rte_rcu_qsbr_thread_offline(list->qsbr, 0);
> > +	}
> > +#endif
> > +
> >  	rte_cryptodev_trace_enqueue_burst(dev_id, qp_id, (void **)ops,
> nb_ops);
> >  	return (*dev->enqueue_burst)(
> >  			dev->data->queue_pairs[qp_id], ops, nb_ops); @@ -
> 1730,6 +1809,78
> > @@ struct rte_crypto_raw_dp_ctx {
> > rte_cryptodev_raw_dequeue_done(struct rte_crypto_raw_dp_ctx *ctx,
> >  		uint32_t n);
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Add a user callback for a given crypto device and queue pair which
> > +will be
> > + * called on crypto ops enqueue.
> > + *
> > + * This API configures a function to be called for each burst of
> > +crypto ops
> > + * received on a given crypto device queue pair. The return value is
> > +a pointer
> > + * that can be used later to remove the callback using
> > + * rte_cryptodev_remove_enq_callback().
> > + *
> > + * Multiple functions are called in the order that they are added.
> > + *
> > + * @param	dev_id		The identifier of the device.
> > + * @param	qp_id		The index of the queue pair in which ops are
> > + *				to be enqueued for processing. The value
> > + *				must be in the range [0, nb_queue_pairs - 1]
> > + *				previously supplied to
> > + *				*rte_cryptodev_configure*.
> > + * @param	cb_fn		The callback function
> > + * @param	cb_arg		A generic pointer parameter which will be
> passed
> > + *				to each invocation of the callback function on
> > + *				this crypto device and queue pair.
> > + *
> > + * @return
> > + *   NULL on error.
> > + *   On success, a pointer value which can later be used to remove the
> callback.
> > + */
> > +
> > +__rte_experimental
> > +struct rte_cryptodev_cb *
> > +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> > +			       uint16_t qp_id,
> > +			       rte_cryptodev_callback_fn cb_fn,
> > +			       void *cb_arg);
> > +
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Remove a user callback function for given crypto device and queue pair.
> > + *
> > + * This function is used to removed callbacks that were added to a
> > +crypto
> > + * device queue pair using rte_cryptodev_add_enq_callback().
> > + *
> > + *
> > + *
> > + * @param	dev_id		The identifier of the device.
> > + * @param	qp_id		The index of the queue pair in which ops are
> > + *				to be enqueued for processing. The value
> > + *				must be in the range [0, nb_queue_pairs - 1]
> > + *				previously supplied to
> > + *				*rte_cryptodev_configure*.
> > + * @param	cb		Pointer to user supplied callback created via
> > + *				rte_cryptodev_add_enq_callback().
> > + *
> > + * @return
> > + *   - 0: Success. Callback was removed.
> > + *   - -EINVAL:  The dev_id or the qp_id is out of range, or the callback
> > + *               is NULL or not found for the crypto device queue pair.
> > + */
> > +
> > +__rte_experimental
> > +int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +				      uint16_t qp_id,
> > +				      struct rte_cryptodev_cb *cb);
> > +
> > +#endif
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
> > b/lib/librte_cryptodev/rte_cryptodev_version.map
> > index 7e4360f..5d8d6b0 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> > +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> > @@ -101,6 +101,7 @@ EXPERIMENTAL {
> >  	rte_cryptodev_get_qp_status;
> >
> >  	# added in 20.11
> > +	rte_cryptodev_add_enq_callback;
> >  	rte_cryptodev_configure_raw_dp_ctx;
> >  	rte_cryptodev_get_raw_dp_ctx_size;
> >  	rte_cryptodev_raw_dequeue;
> > @@ -109,4 +110,5 @@ EXPERIMENTAL {
> >  	rte_cryptodev_raw_enqueue;
> >  	rte_cryptodev_raw_enqueue_burst;
> >  	rte_cryptodev_raw_enqueue_done;
> > +	rte_cryptodev_remove_enq_callback;
> >  };
> > --
> > 1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-23 12:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-19  2:57 [dpdk-dev] [v3 0/2] support enqueue callbacks on cryptodev Abhinandan Gujjar
2020-10-19  2:57 ` [dpdk-dev] [v3 1/2] cryptodev: support enqueue callback functions Abhinandan Gujjar
2020-10-21  7:28   ` Honnappa Nagarahalli
2020-10-23 12:34     ` Gujjar, Abhinandan S
2020-10-21 19:33   ` Ananyev, Konstantin
2020-10-23 12:36     ` Gujjar, Abhinandan S
2020-10-19  2:57 ` [dpdk-dev] [v3 2/2] test: add testcase for crypto enqueue callback Abhinandan Gujjar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).