DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [v4 0/3] support enqueue callbacks on cryptodev
@ 2020-10-25  9:44 Abhinandan Gujjar
  2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
                   ` (2 more replies)
  0 siblings, 3 replies; 28+ messages in thread
From: Abhinandan Gujjar @ 2020-10-25  9:44 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.

v3->v4:
    -Move callback init and cleanup under dev_configure
    -Update with memory ordering
    -Removed extra level of indirection
    -Add documentation

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 (3):
  cryptodev: support enqueue callback functions
  test: add testcase for crypto enqueue callback
  doc: add enqueue callback APIs

 app/test/test_cryptodev.c                      | 133 +++++++++++++-
 config/rte_config.h                            |   1 +
 doc/guides/prog_guide/cryptodev_lib.rst        |  22 +++
 doc/guides/rel_notes/release_20_11.rst         |   5 +
 lib/librte_cryptodev/meson.build               |   2 +-
 lib/librte_cryptodev/rte_cryptodev.c           | 230 +++++++++++++++++++++++++
 lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
 lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
 8 files changed, 549 insertions(+), 4 deletions(-)

-- 
1.9.1


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

* [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-25  9:44 [dpdk-dev] [v4 0/3] support enqueue callbacks on cryptodev Abhinandan Gujjar
@ 2020-10-25  9:44 ` Abhinandan Gujjar
  2020-10-27 12:47   ` Ananyev, Konstantin
                     ` (2 more replies)
  2020-10-25  9:44 ` [dpdk-dev] [v4 2/3] test: add testcase for crypto enqueue callback Abhinandan Gujjar
  2020-10-25  9:44 ` [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs Abhinandan Gujjar
  2 siblings, 3 replies; 28+ messages in thread
From: Abhinandan Gujjar @ 2020-10-25  9:44 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           | 230 +++++++++++++++++++++++++
 lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
 lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
 5 files changed, 391 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..0880d9b 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -448,6 +448,91 @@ 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_callback_lock = RTE_SPINLOCK_INITIALIZER;
+
+static void
+cryptodev_cb_cleanup(struct rte_cryptodev *dev)
+{
+	struct rte_cryptodev_cb **prev_cb, *curr_cb;
+	struct rte_cryptodev_enq_cb_rcu *list;
+	uint16_t qp_id;
+
+	if (dev->enq_cbs == NULL)
+		return;
+
+	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
+		list = &dev->enq_cbs[qp_id];
+		prev_cb = &list->next;
+
+		while (*prev_cb != NULL) {
+			curr_cb = *prev_cb;
+			/* Remove the user cb from the callback list. */
+			__atomic_store_n(prev_cb, curr_cb->next,
+				__ATOMIC_RELAXED);
+			rte_rcu_qsbr_synchronize(list->qsbr,
+				RTE_QSBR_THRID_INVALID);
+			rte_free(curr_cb);
+		}
+
+		rte_free(list->qsbr);
+	}
+
+	rte_free(dev->enq_cbs);
+	dev->enq_cbs = NULL;
+}
+
+static int
+cryptodev_cb_init(struct rte_cryptodev *dev)
+{
+	struct rte_cryptodev_enq_cb_rcu *list;
+	struct rte_rcu_qsbr *qsbr;
+	uint16_t qp_id;
+	size_t size;
+
+	/* Max thread set to 1, as one DP thread accessing a queue-pair */
+	const uint32_t max_threads = 1;
+
+	dev->enq_cbs = rte_zmalloc(NULL,
+				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
+				   dev->data->nb_queue_pairs, 0);
+	if (dev->enq_cbs == NULL) {
+		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
+		rte_errno = ENOMEM;
+		return -1;
+	}
+
+	/* Create RCU QSBR variable */
+	size = rte_rcu_qsbr_get_memsize(max_threads);
+
+	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
+		list = &dev->enq_cbs[qp_id];
+		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
+		if (qsbr == NULL) {
+			CDEV_LOG_ERR("Failed to allocate memory for RCU on "
+				"queue_pair_id=%d", qp_id);
+			goto cb_init_err;
+		}
+
+		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
+			CDEV_LOG_ERR("Failed to initialize for RCU on "
+				"queue_pair_id=%d", qp_id);
+			goto cb_init_err;
+		}
+
+		list->qsbr = qsbr;
+	}
+
+	return 0;
+
+cb_init_err:
+	rte_errno = ENOMEM;
+	cryptodev_cb_cleanup(dev);
+	return -1;
+
+}
+#endif
 
 const char *
 rte_cryptodev_get_feature_name(uint64_t flag)
@@ -927,6 +1012,11 @@ struct rte_cryptodev *
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
+#ifdef RTE_CRYPTO_CALLBACKS
+	rte_spinlock_lock(&rte_cryptodev_callback_lock);
+	cryptodev_cb_cleanup(dev);
+	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
+#endif
 	/* Setup new number of queue pairs and reconfigure device. */
 	diag = rte_cryptodev_queue_pairs_config(dev, config->nb_queue_pairs,
 			config->socket_id);
@@ -936,6 +1026,15 @@ struct rte_cryptodev *
 		return diag;
 	}
 
+#ifdef RTE_CRYPTO_CALLBACKS
+	rte_spinlock_lock(&rte_cryptodev_callback_lock);
+	diag = cryptodev_cb_init(dev);
+	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
+	if (diag) {
+		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
+		return -ENOMEM;
+	}
+#endif
 	rte_cryptodev_trace_configure(dev_id, config);
 	return (*dev->dev_ops->dev_configure)(dev, config);
 }
@@ -1136,6 +1235,137 @@ 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_enq_cb_rcu *list;
+	struct rte_cryptodev_cb *cb, *tail;
+
+	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;
+	}
+
+	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_errno = ENOMEM;
+		return NULL;
+	}
+
+	rte_spinlock_lock(&rte_cryptodev_callback_lock);
+
+	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;
+		/* Stores to cb->fn and cb->param should complete before
+		 * cb is visible to data plane.
+		 */
+		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
+	} else {
+		/* Stores to cb->fn and cb->param should complete before
+		 * cb is visible to data plane.
+		 */
+		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
+	}
+
+	rte_spinlock_unlock(&rte_cryptodev_callback_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;
+	int ret;
+
+	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;
+	}
+
+	rte_spinlock_lock(&rte_cryptodev_callback_lock);
+	if (dev->enq_cbs == NULL) {
+		CDEV_LOG_ERR("Callback not initialized");
+		goto cb_err;
+	}
+
+	list = &dev->enq_cbs[qp_id];
+	if (list == NULL) {
+		CDEV_LOG_ERR("Callback list is NULL");
+		goto cb_err;
+	}
+
+	if (list->qsbr == NULL) {
+		CDEV_LOG_ERR("Rcu qsbr is NULL");
+		goto cb_err;
+	}
+
+	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. */
+			__atomic_store_n(prev_cb, curr_cb->next,
+				__ATOMIC_RELAXED);
+			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);
+	}
+
+cb_err:
+	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct rte_cryptodev {
 	__extension__
 	uint8_t attached : 1;
 	/**< Flag indicating the device is attached */
+
+	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
+	/**< User application callback for pre enqueue processing */
+
 } __rte_cache_aligned;
 
 void *
@@ -989,6 +1048,31 @@ struct rte_cryptodev_data {
 {
 	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
 
+#ifdef RTE_CRYPTO_CALLBACKS
+	if (unlikely(dev->enq_cbs != NULL)) {
+		struct rte_cryptodev_enq_cb_rcu *list;
+		struct rte_cryptodev_cb *cb;
+
+		/* __ATOMIC_RELEASE memory order was used when the
+		* call back was inserted into the list.
+		* Since there is a clear dependency between loading
+		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
+		* not required.
+		*/
+		list = &dev->enq_cbs[qp_id];
+		rte_rcu_qsbr_thread_online(list->qsbr, 0);
+		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
+
+		while (cb != NULL) {
+			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
+					cb->arg);
+			cb = cb->next;
+		};
+
+		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 +1814,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] 28+ messages in thread

* [dpdk-dev] [v4 2/3] test: add testcase for crypto enqueue callback
  2020-10-25  9:44 [dpdk-dev] [v4 0/3] support enqueue callbacks on cryptodev Abhinandan Gujjar
  2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
@ 2020-10-25  9:44 ` Abhinandan Gujjar
  2020-10-25  9:44 ` [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs Abhinandan Gujjar
  2 siblings, 0 replies; 28+ messages in thread
From: Abhinandan Gujjar @ 2020-10-25  9:44 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] 28+ messages in thread

* [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs
  2020-10-25  9:44 [dpdk-dev] [v4 0/3] support enqueue callbacks on cryptodev Abhinandan Gujjar
  2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
  2020-10-25  9:44 ` [dpdk-dev] [v4 2/3] test: add testcase for crypto enqueue callback Abhinandan Gujjar
@ 2020-10-25  9:44 ` Abhinandan Gujjar
  2020-10-26 19:08   ` Akhil Goyal
  2020-10-27 12:51   ` Ananyev, Konstantin
  2 siblings, 2 replies; 28+ messages in thread
From: Abhinandan Gujjar @ 2020-10-25  9:44 UTC (permalink / raw)
  To: dev, declan.doherty, akhil.goyal, Honnappa.Nagarahalli,
	konstantin.ananyev
  Cc: narender.vangati, jerinj, abhinandan.gujjar

Add enqueue callback support for cryptodev library

Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
---
 doc/guides/prog_guide/cryptodev_lib.rst | 22 ++++++++++++++++++++++
 doc/guides/rel_notes/release_20_11.rst  |  5 +++++
 2 files changed, 27 insertions(+)

diff --git a/doc/guides/prog_guide/cryptodev_lib.rst b/doc/guides/prog_guide/cryptodev_lib.rst
index 72129e4..bb3de61 100644
--- a/doc/guides/prog_guide/cryptodev_lib.rst
+++ b/doc/guides/prog_guide/cryptodev_lib.rst
@@ -366,6 +366,28 @@ can never be larger than ``nb_ops``.
    uint16_t rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
                                         struct rte_crypto_op **ops, uint16_t nb_ops)
 
+User callback APIs
+~~~~~~~~~~~~~~~~~~
+The add API configures a callback 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 callback functions can be added for a given queue pair.
+
+.. code-block:: c
+
+   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);
+
+The remove API removes a callback function added by rte_cryptodev_add_enq_callback().
+
+.. code-block:: c
+
+	int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
+					      uint16_t qp_id,
+				              struct rte_cryptodev_cb *cb);
 
 Operation Representation
 ~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 48717ee..7e2fd30 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -285,6 +285,11 @@ New Features
   * Added scatter gather support.
   * Added NIST GCMVS complaint GMAC test method support.
 
+* **Added enqueue callback APIs for cryptodev library.**
+
+  Cryptodev is added with enqueue callback APIs to enable applications
+  to add/remove user callbacks which gets called for every enqueue
+  operations.
 
 Removed Items
 -------------
-- 
1.9.1


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

* Re: [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs
  2020-10-25  9:44 ` [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs Abhinandan Gujjar
@ 2020-10-26 19:08   ` Akhil Goyal
  2020-10-27  3:52     ` Gujjar, Abhinandan S
  2020-10-27 12:51   ` Ananyev, Konstantin
  1 sibling, 1 reply; 28+ messages in thread
From: Akhil Goyal @ 2020-10-26 19:08 UTC (permalink / raw)
  To: Abhinandan Gujjar, dev, declan.doherty, Honnappa.Nagarahalli,
	konstantin.ananyev
  Cc: narender.vangati, jerinj

> Subject: [v4 3/3] doc: add enqueue callback APIs
> 
> Add enqueue callback support for cryptodev library
> 
No need for separate documentation patch. It should be part of patch which is adding
The functionality.

> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  doc/guides/prog_guide/cryptodev_lib.rst | 22 ++++++++++++++++++++++
>  doc/guides/rel_notes/release_20_11.rst  |  5 +++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/cryptodev_lib.rst
> b/doc/guides/prog_guide/cryptodev_lib.rst
> index 72129e4..bb3de61 100644
> --- a/doc/guides/prog_guide/cryptodev_lib.rst
> +++ b/doc/guides/prog_guide/cryptodev_lib.rst
> @@ -366,6 +366,28 @@ can never be larger than ``nb_ops``.
>     uint16_t rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
>                                          struct rte_crypto_op **ops, uint16_t nb_ops)
> 
> +User callback APIs
> +~~~~~~~~~~~~~~~~~~
> +The add API configures a callback 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().

Does that mean callback cannot be called for each packet and it will be called for
Each burst only?

The usage of these APIs is not clear with this documentation. Probably you can add
More text/ pseudo code about the sequence of APIs to be used.

> +Multiple callback functions can be added for a given queue pair.

Can we add callbacks at runtime or is it before the first enqueue only?

> +
> +.. code-block:: c
> +
> +   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);
> +
> +The remove API removes a callback function added by
> rte_cryptodev_add_enq_callback().
> +
> +.. code-block:: c
> +
> +	int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +					      uint16_t qp_id,
> +				              struct rte_cryptodev_cb *cb);
> 
>  Operation Representation
>  ~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index 48717ee..7e2fd30 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -285,6 +285,11 @@ New Features
>    * Added scatter gather support.
>    * Added NIST GCMVS complaint GMAC test method support.
> 
> +* **Added enqueue callback APIs for cryptodev library.**
> +
> +  Cryptodev is added with enqueue callback APIs to enable applications
> +  to add/remove user callbacks which gets called for every enqueue
> +  operations.

Please follow the sequence mentioned. It should be after the other new features
Of cryptodev.
> 
>  Removed Items
>  -------------
> --
> 1.9.1


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

* Re: [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs
  2020-10-26 19:08   ` Akhil Goyal
@ 2020-10-27  3:52     ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-27  3:52 UTC (permalink / raw)
  To: Akhil Goyal, dev, Doherty, Declan, Honnappa.Nagarahalli, Ananyev,
	Konstantin
  Cc: Vangati, Narender, jerinj

Hi Akhil,

Please find few comments inline.

Thanks
Abhinandan

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Tuesday, October 27, 2020 12:39 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>;
> Honnappa.Nagarahalli@arm.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com
> Subject: RE: [v4 3/3] doc: add enqueue callback APIs
> 
> > Subject: [v4 3/3] doc: add enqueue callback APIs
> >
> > Add enqueue callback support for cryptodev library
> >
> No need for separate documentation patch. It should be part of patch which is
> adding The functionality.
Ok
> 
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > ---
> >  doc/guides/prog_guide/cryptodev_lib.rst | 22 ++++++++++++++++++++++
> > doc/guides/rel_notes/release_20_11.rst  |  5 +++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/cryptodev_lib.rst
> > b/doc/guides/prog_guide/cryptodev_lib.rst
> > index 72129e4..bb3de61 100644
> > --- a/doc/guides/prog_guide/cryptodev_lib.rst
> > +++ b/doc/guides/prog_guide/cryptodev_lib.rst
> > @@ -366,6 +366,28 @@ can never be larger than ``nb_ops``.
> >     uint16_t rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
> >                                          struct rte_crypto_op **ops,
> > uint16_t nb_ops)
> >
> > +User callback APIs
> > +~~~~~~~~~~~~~~~~~~
> > +The add API configures a callback 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().
> 
> Does that mean callback cannot be called for each packet and it will be called
> for Each burst only?
Yes. This is similar to ethdev's rxtx callback which is called for a burst of packet,
where each packet can be processed one after the other.
> 
> The usage of these APIs is not clear with this documentation. Probably you can
> add More text/ pseudo code about the sequence of APIs to be used.
One of motivation for the callback is mentioned in the cover letter. Since the usage
from eventdev world & the callbacks are generic, I didn't not mention. Please let
me know, if you think of any text which adds more clarity to the usage.
> 
> > +Multiple callback functions can be added for a given queue pair.
> 
> Can we add callbacks at runtime or is it before the first enqueue only?
Yes. I will add some text for this. 
> 
> > +
> > +.. code-block:: c
> > +
> > +   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);
> > +
> > +The remove API removes a callback function added by
> > rte_cryptodev_add_enq_callback().
> > +
> > +.. code-block:: c
> > +
> > +	int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +					      uint16_t qp_id,
> > +				              struct rte_cryptodev_cb *cb);
> >
> >  Operation Representation
> >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > diff --git a/doc/guides/rel_notes/release_20_11.rst
> > b/doc/guides/rel_notes/release_20_11.rst
> > index 48717ee..7e2fd30 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -285,6 +285,11 @@ New Features
> >    * Added scatter gather support.
> >    * Added NIST GCMVS complaint GMAC test method support.
> >
> > +* **Added enqueue callback APIs for cryptodev library.**
> > +
> > +  Cryptodev is added with enqueue callback APIs to enable
> > + applications  to add/remove user callbacks which gets called for
> > + every enqueue  operations.
> 
> Please follow the sequence mentioned. It should be after the other new
> features Of cryptodev.
Ok
> >
> >  Removed Items
> >  -------------
> > --
> > 1.9.1


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
@ 2020-10-27 12:47   ` Ananyev, Konstantin
  2020-10-27 17:16     ` Gujjar, Abhinandan S
  2020-10-27 18:19   ` Akhil Goyal
  2020-10-27 18:28   ` Akhil Goyal
  2 siblings, 1 reply; 28+ messages in thread
From: Ananyev, Konstantin @ 2020-10-27 12:47 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj


> 
> 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           | 230 +++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
>  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
>  5 files changed, 391 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..0880d9b 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -448,6 +448,91 @@ 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_callback_lock = RTE_SPINLOCK_INITIALIZER;
> +
> +static void
> +cryptodev_cb_cleanup(struct rte_cryptodev *dev)
> +{
> +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	uint16_t qp_id;
> +
> +	if (dev->enq_cbs == NULL)
> +		return;
> +
> +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> +		list = &dev->enq_cbs[qp_id];
> +		prev_cb = &list->next;
> +
> +		while (*prev_cb != NULL) {
> +			curr_cb = *prev_cb;
> +			/* Remove the user cb from the callback list. */
> +			__atomic_store_n(prev_cb, curr_cb->next,
> +				__ATOMIC_RELAXED);
> +			rte_rcu_qsbr_synchronize(list->qsbr,
> +				RTE_QSBR_THRID_INVALID);

You call this function (cb_cleanup) only at dev_confiture().
At that moment DP threads can't do enqueue/dequeue anyway.
So you can safely skip all this synchronization code here and just do:

cb = list->next;
while (cb != NULL) {
	next = cb->next;
	rte_free(cb);
	cb = next;
}


> +			rte_free(curr_cb);

One thing that makes it sort of grey area:
we do free() for cb itself, but user provided data will be sort of 'lost'.
As it is not referenced from our cb struct anymore...
I see two options here - first just document explicitly that callbacks wouldn't
survive cryptodev_configure() and it is user responsibility to remove all
installed callbacks before doing dev_configure() to avoid possible memory leakage.
Another option - add user provided cleanup() function pointer into struct rte_cryptodev_cb
and call it here if provided:
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 */
	void (*cleanup)(void *);	
};

And here:
	If (curr_cb->cleanup != NULL)
		curr_cb->cleanup(curr_cb->arg);

 	rte_free(curr_cb);

Rest of the code - LGTM.
So with that addressed:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
 
> +		}
> +
> +		rte_free(list->qsbr);
> +	}
> +
> +	rte_free(dev->enq_cbs);
> +	dev->enq_cbs = NULL;
> +}
> +
> +static int
> +cryptodev_cb_init(struct rte_cryptodev *dev)
> +{
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	struct rte_rcu_qsbr *qsbr;
> +	uint16_t qp_id;
> +	size_t size;
> +
> +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> +	const uint32_t max_threads = 1;
> +
> +	dev->enq_cbs = rte_zmalloc(NULL,
> +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> +				   dev->data->nb_queue_pairs, 0);
> +	if (dev->enq_cbs == NULL) {
> +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> +		rte_errno = ENOMEM;
> +		return -1;
> +	}
> +
> +	/* Create RCU QSBR variable */
> +	size = rte_rcu_qsbr_get_memsize(max_threads);
> +
> +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> +		list = &dev->enq_cbs[qp_id];
> +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> +		if (qsbr == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for RCU on "
> +				"queue_pair_id=%d", qp_id);
> +			goto cb_init_err;
> +		}
> +
> +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> +				"queue_pair_id=%d", qp_id);
> +			goto cb_init_err;
> +		}
> +
> +		list->qsbr = qsbr;
> +	}
> +
> +	return 0;
> +
> +cb_init_err:
> +	rte_errno = ENOMEM;
> +	cryptodev_cb_cleanup(dev);
> +	return -1;
> +
> +}
> +#endif
> 
>  const char *
>  rte_cryptodev_get_feature_name(uint64_t flag)
> @@ -927,6 +1012,11 @@ struct rte_cryptodev *
> 
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +	cryptodev_cb_cleanup(dev);
> +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> +#endif
>  	/* Setup new number of queue pairs and reconfigure device. */
>  	diag = rte_cryptodev_queue_pairs_config(dev, config->nb_queue_pairs,
>  			config->socket_id);
> @@ -936,6 +1026,15 @@ struct rte_cryptodev *
>  		return diag;
>  	}
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +	diag = cryptodev_cb_init(dev);
> +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> +	if (diag) {
> +		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
> +		return -ENOMEM;
> +	}
> +#endif
>  	rte_cryptodev_trace_configure(dev_id, config);
>  	return (*dev->dev_ops->dev_configure)(dev, config);
>  }
> @@ -1136,6 +1235,137 @@ 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_enq_cb_rcu *list;
> +	struct rte_cryptodev_cb *cb, *tail;
> +
> +	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;
> +	}
> +
> +	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_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +
> +	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;
> +		/* Stores to cb->fn and cb->param should complete before
> +		 * cb is visible to data plane.
> +		 */
> +		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> +	} else {
> +		/* Stores to cb->fn and cb->param should complete before
> +		 * cb is visible to data plane.
> +		 */
> +		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
> +	}
> +
> +	rte_spinlock_unlock(&rte_cryptodev_callback_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;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +	if (dev->enq_cbs == NULL) {
> +		CDEV_LOG_ERR("Callback not initialized");
> +		goto cb_err;
> +	}
> +
> +	list = &dev->enq_cbs[qp_id];
> +	if (list == NULL) {
> +		CDEV_LOG_ERR("Callback list is NULL");
> +		goto cb_err;
> +	}
> +
> +	if (list->qsbr == NULL) {
> +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> +		goto cb_err;
> +	}
> +
> +	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. */
> +			__atomic_store_n(prev_cb, curr_cb->next,
> +				__ATOMIC_RELAXED);
> +			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);
> +	}
> +
> +cb_err:
> +	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct rte_cryptodev {
>  	__extension__
>  	uint8_t attached : 1;
>  	/**< Flag indicating the device is attached */
> +
> +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> +	/**< User application callback for pre enqueue processing */
> +
>  } __rte_cache_aligned;
> 
>  void *
> @@ -989,6 +1048,31 @@ struct rte_cryptodev_data {
>  {
>  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	if (unlikely(dev->enq_cbs != NULL)) {
> +		struct rte_cryptodev_enq_cb_rcu *list;
> +		struct rte_cryptodev_cb *cb;
> +
> +		/* __ATOMIC_RELEASE memory order was used when the
> +		* call back was inserted into the list.
> +		* Since there is a clear dependency between loading
> +		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
> +		* not required.
> +		*/
> +		list = &dev->enq_cbs[qp_id];
> +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> +
> +		while (cb != NULL) {
> +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> +					cb->arg);
> +			cb = cb->next;
> +		};
> +
> +		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 +1814,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] 28+ messages in thread

* Re: [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs
  2020-10-25  9:44 ` [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs Abhinandan Gujjar
  2020-10-26 19:08   ` Akhil Goyal
@ 2020-10-27 12:51   ` Ananyev, Konstantin
  2020-10-27 17:17     ` Gujjar, Abhinandan S
  1 sibling, 1 reply; 28+ messages in thread
From: Ananyev, Konstantin @ 2020-10-27 12:51 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj



> 
> Add enqueue callback support for cryptodev library
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  doc/guides/prog_guide/cryptodev_lib.rst | 22 ++++++++++++++++++++++
>  doc/guides/rel_notes/release_20_11.rst  |  5 +++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/cryptodev_lib.rst b/doc/guides/prog_guide/cryptodev_lib.rst
> index 72129e4..bb3de61 100644
> --- a/doc/guides/prog_guide/cryptodev_lib.rst
> +++ b/doc/guides/prog_guide/cryptodev_lib.rst
> @@ -366,6 +366,28 @@ can never be larger than ``nb_ops``.
>     uint16_t rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
>                                          struct rte_crypto_op **ops, uint16_t nb_ops)
> 
> +User callback APIs
> +~~~~~~~~~~~~~~~~~~
> +The add API configures a callback 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 callback functions can be added for a given queue pair.
> +
> +.. code-block:: c
> +
> +   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);
> +
> +The remove API removes a callback function added by rte_cryptodev_add_enq_callback().
> +
> +.. code-block:: c
> +
> +	int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +					      uint16_t qp_id,
> +				              struct rte_cryptodev_cb *cb);


Please add explicit statement that all installed callbacks wouldn't suruvive dev_configure().
So it is user responsibility to: remove them before dev_configure() and (re)install after.
With that in place:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 
>  Operation Representation
>  ~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index 48717ee..7e2fd30 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -285,6 +285,11 @@ New Features
>    * Added scatter gather support.
>    * Added NIST GCMVS complaint GMAC test method support.
> 
> +* **Added enqueue callback APIs for cryptodev library.**
> +
> +  Cryptodev is added with enqueue callback APIs to enable applications
> +  to add/remove user callbacks which gets called for every enqueue
> +  operations.
> 
>  Removed Items
>  -------------
> --
> 1.9.1


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 12:47   ` Ananyev, Konstantin
@ 2020-10-27 17:16     ` Gujjar, Abhinandan S
  2020-10-27 17:20       ` Ananyev, Konstantin
  0 siblings, 1 reply; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-27 17:16 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj



> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, October 27, 2020 6:18 PM
> 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: [v4 1/3] cryptodev: support enqueue callback functions
> 
> 
> >
> > 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           | 230
> +++++++++++++++++++++++++
> >  lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
> >  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
> >  5 files changed, 391 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..0880d9b 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -448,6 +448,91 @@ 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_callback_lock = RTE_SPINLOCK_INITIALIZER;
> > +
> > +static void
> > +cryptodev_cb_cleanup(struct rte_cryptodev *dev) {
> > +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	uint16_t qp_id;
> > +
> > +	if (dev->enq_cbs == NULL)
> > +		return;
> > +
> > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > +		list = &dev->enq_cbs[qp_id];
> > +		prev_cb = &list->next;
> > +
> > +		while (*prev_cb != NULL) {
> > +			curr_cb = *prev_cb;
> > +			/* Remove the user cb from the callback list. */
> > +			__atomic_store_n(prev_cb, curr_cb->next,
> > +				__ATOMIC_RELAXED);
> > +			rte_rcu_qsbr_synchronize(list->qsbr,
> > +				RTE_QSBR_THRID_INVALID);
> 
> You call this function (cb_cleanup) only at dev_confiture().
> At that moment DP threads can't do enqueue/dequeue anyway.
> So you can safely skip all this synchronization code here and just do:
> 
> cb = list->next;
> while (cb != NULL) {
> 	next = cb->next;
> 	rte_free(cb);
> 	cb = next;
> }
> 
Ok
> 
> > +			rte_free(curr_cb);
> 
> One thing that makes it sort of grey area:
> we do free() for cb itself, but user provided data will be sort of 'lost'.
> As it is not referenced from our cb struct anymore...
> I see two options here - first just document explicitly that callbacks wouldn't
> survive cryptodev_configure() and it is user responsibility to remove all
> installed callbacks before doing dev_configure() to avoid possible memory
> leakage.
Ok. I will update the documentation for this and send a new patch set.
> Another option - add user provided cleanup() function pointer into struct
> rte_cryptodev_cb and call it here if provided:
> 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 */
> 	void (*cleanup)(void *);
> };
> 
> And here:
> 	If (curr_cb->cleanup != NULL)
> 		curr_cb->cleanup(curr_cb->arg);
> 
>  	rte_free(curr_cb);
> 
> Rest of the code - LGTM.
> So with that addressed:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> > +		}
> > +
> > +		rte_free(list->qsbr);
> > +	}
> > +
> > +	rte_free(dev->enq_cbs);
> > +	dev->enq_cbs = NULL;
> > +}
> > +
> > +static int
> > +cryptodev_cb_init(struct rte_cryptodev *dev) {
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	struct rte_rcu_qsbr *qsbr;
> > +	uint16_t qp_id;
> > +	size_t size;
> > +
> > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > +	const uint32_t max_threads = 1;
> > +
> > +	dev->enq_cbs = rte_zmalloc(NULL,
> > +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> > +				   dev->data->nb_queue_pairs, 0);
> > +	if (dev->enq_cbs == NULL) {
> > +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> > +		rte_errno = ENOMEM;
> > +		return -1;
> > +	}
> > +
> > +	/* Create RCU QSBR variable */
> > +	size = rte_rcu_qsbr_get_memsize(max_threads);
> > +
> > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > +		list = &dev->enq_cbs[qp_id];
> > +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> > +		if (qsbr == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for RCU
> on "
> > +				"queue_pair_id=%d", qp_id);
> > +			goto cb_init_err;
> > +		}
> > +
> > +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> > +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> > +				"queue_pair_id=%d", qp_id);
> > +			goto cb_init_err;
> > +		}
> > +
> > +		list->qsbr = qsbr;
> > +	}
> > +
> > +	return 0;
> > +
> > +cb_init_err:
> > +	rte_errno = ENOMEM;
> > +	cryptodev_cb_cleanup(dev);
> > +	return -1;
> > +
> > +}
> > +#endif
> >
> >  const char *
> >  rte_cryptodev_get_feature_name(uint64_t flag) @@ -927,6 +1012,11 @@
> > struct rte_cryptodev *
> >
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> ENOTSUP);
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +	cryptodev_cb_cleanup(dev);
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > +#endif
> >  	/* Setup new number of queue pairs and reconfigure device. */
> >  	diag = rte_cryptodev_queue_pairs_config(dev, config-
> >nb_queue_pairs,
> >  			config->socket_id);
> > @@ -936,6 +1026,15 @@ struct rte_cryptodev *
> >  		return diag;
> >  	}
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +	diag = cryptodev_cb_init(dev);
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > +	if (diag) {
> > +		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
> > +		return -ENOMEM;
> > +	}
> > +#endif
> >  	rte_cryptodev_trace_configure(dev_id, config);
> >  	return (*dev->dev_ops->dev_configure)(dev, config);  } @@ -1136,6
> > +1235,137 @@ 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_enq_cb_rcu *list;
> > +	struct rte_cryptodev_cb *cb, *tail;
> > +
> > +	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;
> > +	}
> > +
> > +	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_errno = ENOMEM;
> > +		return NULL;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +
> > +	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;
> > +		/* Stores to cb->fn and cb->param should complete before
> > +		 * cb is visible to data plane.
> > +		 */
> > +		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> > +	} else {
> > +		/* Stores to cb->fn and cb->param should complete before
> > +		 * cb is visible to data plane.
> > +		 */
> > +		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
> > +	}
> > +
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_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;
> > +	int ret;
> > +
> > +	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;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +	if (dev->enq_cbs == NULL) {
> > +		CDEV_LOG_ERR("Callback not initialized");
> > +		goto cb_err;
> > +	}
> > +
> > +	list = &dev->enq_cbs[qp_id];
> > +	if (list == NULL) {
> > +		CDEV_LOG_ERR("Callback list is NULL");
> > +		goto cb_err;
> > +	}
> > +
> > +	if (list->qsbr == NULL) {
> > +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> > +		goto cb_err;
> > +	}
> > +
> > +	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. */
> > +			__atomic_store_n(prev_cb, curr_cb->next,
> > +				__ATOMIC_RELAXED);
> > +			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);
> > +	}
> > +
> > +cb_err:
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct rte_cryptodev {
> >  	__extension__
> >  	uint8_t attached : 1;
> >  	/**< Flag indicating the device is attached */
> > +
> > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > +	/**< User application callback for pre enqueue processing */
> > +
> >  } __rte_cache_aligned;
> >
> >  void *
> > @@ -989,6 +1048,31 @@ struct rte_cryptodev_data {  {
> >  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	if (unlikely(dev->enq_cbs != NULL)) {
> > +		struct rte_cryptodev_enq_cb_rcu *list;
> > +		struct rte_cryptodev_cb *cb;
> > +
> > +		/* __ATOMIC_RELEASE memory order was used when the
> > +		* call back was inserted into the list.
> > +		* Since there is a clear dependency between loading
> > +		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order
> is
> > +		* not required.
> > +		*/
> > +		list = &dev->enq_cbs[qp_id];
> > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> > +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> > +
> > +		while (cb != NULL) {
> > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > +					cb->arg);
> > +			cb = cb->next;
> > +		};
> > +
> > +		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 +1814,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] 28+ messages in thread

* Re: [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs
  2020-10-27 12:51   ` Ananyev, Konstantin
@ 2020-10-27 17:17     ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-27 17:17 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj



> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, October 27, 2020 6:22 PM
> 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: [v4 3/3] doc: add enqueue callback APIs
> 
> 
> 
> >
> > Add enqueue callback support for cryptodev library
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > ---
> >  doc/guides/prog_guide/cryptodev_lib.rst | 22 ++++++++++++++++++++++
> > doc/guides/rel_notes/release_20_11.rst  |  5 +++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/cryptodev_lib.rst
> > b/doc/guides/prog_guide/cryptodev_lib.rst
> > index 72129e4..bb3de61 100644
> > --- a/doc/guides/prog_guide/cryptodev_lib.rst
> > +++ b/doc/guides/prog_guide/cryptodev_lib.rst
> > @@ -366,6 +366,28 @@ can never be larger than ``nb_ops``.
> >     uint16_t rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
> >                                          struct rte_crypto_op **ops,
> > uint16_t nb_ops)
> >
> > +User callback APIs
> > +~~~~~~~~~~~~~~~~~~
> > +The add API configures a callback 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 callback functions can be added for a given queue pair.
> > +
> > +.. code-block:: c
> > +
> > +   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);
> > +
> > +The remove API removes a callback function added by
> rte_cryptodev_add_enq_callback().
> > +
> > +.. code-block:: c
> > +
> > +	int rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +					      uint16_t qp_id,
> > +				              struct rte_cryptodev_cb *cb);
> 
> 
> Please add explicit statement that all installed callbacks wouldn't suruvive
> dev_configure().
> So it is user responsibility to: remove them before dev_configure() and
> (re)install after.
> With that in place:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Ok. I will update the documentation and send a next version of patch.
> 
> >
> >  Operation Representation
> >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > diff --git a/doc/guides/rel_notes/release_20_11.rst
> > b/doc/guides/rel_notes/release_20_11.rst
> > index 48717ee..7e2fd30 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -285,6 +285,11 @@ New Features
> >    * Added scatter gather support.
> >    * Added NIST GCMVS complaint GMAC test method support.
> >
> > +* **Added enqueue callback APIs for cryptodev library.**
> > +
> > +  Cryptodev is added with enqueue callback APIs to enable
> > + applications  to add/remove user callbacks which gets called for
> > + every enqueue  operations.
> >
> >  Removed Items
> >  -------------
> > --
> > 1.9.1


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 17:16     ` Gujjar, Abhinandan S
@ 2020-10-27 17:20       ` Ananyev, Konstantin
  2020-10-27 17:22         ` Gujjar, Abhinandan S
  0 siblings, 1 reply; 28+ messages in thread
From: Ananyev, Konstantin @ 2020-10-27 17:20 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj

> 
> > -----Original Message-----
> > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Sent: Tuesday, October 27, 2020 6:18 PM
> > 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: [v4 1/3] cryptodev: support enqueue callback functions
> >
> >
> > >
> > > 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           | 230
> > +++++++++++++++++++++++++
> > >  lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
> > >  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
> > >  5 files changed, 391 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..0880d9b 100644
> > > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > > @@ -448,6 +448,91 @@ 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_callback_lock = RTE_SPINLOCK_INITIALIZER;
> > > +
> > > +static void
> > > +cryptodev_cb_cleanup(struct rte_cryptodev *dev) {
> > > +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> > > +	struct rte_cryptodev_enq_cb_rcu *list;
> > > +	uint16_t qp_id;
> > > +
> > > +	if (dev->enq_cbs == NULL)
> > > +		return;
> > > +
> > > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > > +		list = &dev->enq_cbs[qp_id];
> > > +		prev_cb = &list->next;
> > > +
> > > +		while (*prev_cb != NULL) {
> > > +			curr_cb = *prev_cb;
> > > +			/* Remove the user cb from the callback list. */
> > > +			__atomic_store_n(prev_cb, curr_cb->next,
> > > +				__ATOMIC_RELAXED);
> > > +			rte_rcu_qsbr_synchronize(list->qsbr,
> > > +				RTE_QSBR_THRID_INVALID);
> >
> > You call this function (cb_cleanup) only at dev_confiture().
> > At that moment DP threads can't do enqueue/dequeue anyway.
> > So you can safely skip all this synchronization code here and just do:
> >
> > cb = list->next;
> > while (cb != NULL) {
> > 	next = cb->next;
> > 	rte_free(cb);
> > 	cb = next;
> > }
> >
> Ok
> >
> > > +			rte_free(curr_cb);
> >
> > One thing that makes it sort of grey area:
> > we do free() for cb itself, but user provided data will be sort of 'lost'.
> > As it is not referenced from our cb struct anymore...
> > I see two options here - first just document explicitly that callbacks wouldn't
> > survive cryptodev_configure() and it is user responsibility to remove all
> > installed callbacks before doing dev_configure() to avoid possible memory
> > leakage.
> Ok. I will update the documentation for this and send a new patch set.

Ok, please keep my ack on your new version.

> > Another option - add user provided cleanup() function pointer into struct
> > rte_cryptodev_cb and call it here if provided:
> > 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 */
> > 	void (*cleanup)(void *);
> > };
> >
> > And here:
> > 	If (curr_cb->cleanup != NULL)
> > 		curr_cb->cleanup(curr_cb->arg);
> >
> >  	rte_free(curr_cb);
> >
> > Rest of the code - LGTM.
> > So with that addressed:
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >
> > > +		}
> > > +
> > > +		rte_free(list->qsbr);
> > > +	}
> > > +
> > > +	rte_free(dev->enq_cbs);
> > > +	dev->enq_cbs = NULL;
> > > +}
> > > +
> > > +static int
> > > +cryptodev_cb_init(struct rte_cryptodev *dev) {
> > > +	struct rte_cryptodev_enq_cb_rcu *list;
> > > +	struct rte_rcu_qsbr *qsbr;
> > > +	uint16_t qp_id;
> > > +	size_t size;
> > > +
> > > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > > +	const uint32_t max_threads = 1;
> > > +
> > > +	dev->enq_cbs = rte_zmalloc(NULL,
> > > +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> > > +				   dev->data->nb_queue_pairs, 0);
> > > +	if (dev->enq_cbs == NULL) {
> > > +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> > > +		rte_errno = ENOMEM;
> > > +		return -1;
> > > +	}
> > > +
> > > +	/* Create RCU QSBR variable */
> > > +	size = rte_rcu_qsbr_get_memsize(max_threads);
> > > +
> > > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > > +		list = &dev->enq_cbs[qp_id];
> > > +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> > > +		if (qsbr == NULL) {
> > > +			CDEV_LOG_ERR("Failed to allocate memory for RCU
> > on "
> > > +				"queue_pair_id=%d", qp_id);
> > > +			goto cb_init_err;
> > > +		}
> > > +
> > > +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> > > +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> > > +				"queue_pair_id=%d", qp_id);
> > > +			goto cb_init_err;
> > > +		}
> > > +
> > > +		list->qsbr = qsbr;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +cb_init_err:
> > > +	rte_errno = ENOMEM;
> > > +	cryptodev_cb_cleanup(dev);
> > > +	return -1;
> > > +
> > > +}
> > > +#endif
> > >
> > >  const char *
> > >  rte_cryptodev_get_feature_name(uint64_t flag) @@ -927,6 +1012,11 @@
> > > struct rte_cryptodev *
> > >
> > >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> > ENOTSUP);
> > >
> > > +#ifdef RTE_CRYPTO_CALLBACKS
> > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > +	cryptodev_cb_cleanup(dev);
> > > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > > +#endif
> > >  	/* Setup new number of queue pairs and reconfigure device. */
> > >  	diag = rte_cryptodev_queue_pairs_config(dev, config-
> > >nb_queue_pairs,
> > >  			config->socket_id);
> > > @@ -936,6 +1026,15 @@ struct rte_cryptodev *
> > >  		return diag;
> > >  	}
> > >
> > > +#ifdef RTE_CRYPTO_CALLBACKS
> > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > +	diag = cryptodev_cb_init(dev);
> > > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > > +	if (diag) {
> > > +		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
> > > +		return -ENOMEM;
> > > +	}
> > > +#endif
> > >  	rte_cryptodev_trace_configure(dev_id, config);
> > >  	return (*dev->dev_ops->dev_configure)(dev, config);  } @@ -1136,6
> > > +1235,137 @@ 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_enq_cb_rcu *list;
> > > +	struct rte_cryptodev_cb *cb, *tail;
> > > +
> > > +	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;
> > > +	}
> > > +
> > > +	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_errno = ENOMEM;
> > > +		return NULL;
> > > +	}
> > > +
> > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > +
> > > +	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;
> > > +		/* Stores to cb->fn and cb->param should complete before
> > > +		 * cb is visible to data plane.
> > > +		 */
> > > +		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> > > +	} else {
> > > +		/* Stores to cb->fn and cb->param should complete before
> > > +		 * cb is visible to data plane.
> > > +		 */
> > > +		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
> > > +	}
> > > +
> > > +	rte_spinlock_unlock(&rte_cryptodev_callback_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;
> > > +	int ret;
> > > +
> > > +	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;
> > > +	}
> > > +
> > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > +	if (dev->enq_cbs == NULL) {
> > > +		CDEV_LOG_ERR("Callback not initialized");
> > > +		goto cb_err;
> > > +	}
> > > +
> > > +	list = &dev->enq_cbs[qp_id];
> > > +	if (list == NULL) {
> > > +		CDEV_LOG_ERR("Callback list is NULL");
> > > +		goto cb_err;
> > > +	}
> > > +
> > > +	if (list->qsbr == NULL) {
> > > +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> > > +		goto cb_err;
> > > +	}
> > > +
> > > +	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. */
> > > +			__atomic_store_n(prev_cb, curr_cb->next,
> > > +				__ATOMIC_RELAXED);
> > > +			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);
> > > +	}
> > > +
> > > +cb_err:
> > > +	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct rte_cryptodev {
> > >  	__extension__
> > >  	uint8_t attached : 1;
> > >  	/**< Flag indicating the device is attached */
> > > +
> > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > +	/**< User application callback for pre enqueue processing */
> > > +
> > >  } __rte_cache_aligned;
> > >
> > >  void *
> > > @@ -989,6 +1048,31 @@ struct rte_cryptodev_data {  {
> > >  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> > >
> > > +#ifdef RTE_CRYPTO_CALLBACKS
> > > +	if (unlikely(dev->enq_cbs != NULL)) {
> > > +		struct rte_cryptodev_enq_cb_rcu *list;
> > > +		struct rte_cryptodev_cb *cb;
> > > +
> > > +		/* __ATOMIC_RELEASE memory order was used when the
> > > +		* call back was inserted into the list.
> > > +		* Since there is a clear dependency between loading
> > > +		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order
> > is
> > > +		* not required.
> > > +		*/
> > > +		list = &dev->enq_cbs[qp_id];
> > > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> > > +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> > > +
> > > +		while (cb != NULL) {
> > > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > > +					cb->arg);
> > > +			cb = cb->next;
> > > +		};
> > > +
> > > +		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 +1814,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] 28+ messages in thread

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 17:20       ` Ananyev, Konstantin
@ 2020-10-27 17:22         ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-27 17:22 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev, Doherty, Declan, akhil.goyal,
	Honnappa.Nagarahalli
  Cc: Vangati, Narender, jerinj

Sure Konstantin.

Thanks
Abhinandan

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, October 27, 2020 10:51 PM
> 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: [v4 1/3] cryptodev: support enqueue callback functions
> 
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Sent: Tuesday, October 27, 2020 6:18 PM
> > > 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: [v4 1/3] cryptodev: support enqueue callback functions
> > >
> > >
> > > >
> > > > 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           | 230
> > > +++++++++++++++++++++++++
> > > >  lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
> > > >  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
> > > >  5 files changed, 391 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..0880d9b 100644
> > > > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > > > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > > > @@ -448,6 +448,91 @@ 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_callback_lock =
> > > > +RTE_SPINLOCK_INITIALIZER;
> > > > +
> > > > +static void
> > > > +cryptodev_cb_cleanup(struct rte_cryptodev *dev) {
> > > > +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> > > > +	struct rte_cryptodev_enq_cb_rcu *list;
> > > > +	uint16_t qp_id;
> > > > +
> > > > +	if (dev->enq_cbs == NULL)
> > > > +		return;
> > > > +
> > > > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > > > +		list = &dev->enq_cbs[qp_id];
> > > > +		prev_cb = &list->next;
> > > > +
> > > > +		while (*prev_cb != NULL) {
> > > > +			curr_cb = *prev_cb;
> > > > +			/* Remove the user cb from the callback list. */
> > > > +			__atomic_store_n(prev_cb, curr_cb->next,
> > > > +				__ATOMIC_RELAXED);
> > > > +			rte_rcu_qsbr_synchronize(list->qsbr,
> > > > +				RTE_QSBR_THRID_INVALID);
> > >
> > > You call this function (cb_cleanup) only at dev_confiture().
> > > At that moment DP threads can't do enqueue/dequeue anyway.
> > > So you can safely skip all this synchronization code here and just do:
> > >
> > > cb = list->next;
> > > while (cb != NULL) {
> > > 	next = cb->next;
> > > 	rte_free(cb);
> > > 	cb = next;
> > > }
> > >
> > Ok
> > >
> > > > +			rte_free(curr_cb);
> > >
> > > One thing that makes it sort of grey area:
> > > we do free() for cb itself, but user provided data will be sort of 'lost'.
> > > As it is not referenced from our cb struct anymore...
> > > I see two options here - first just document explicitly that
> > > callbacks wouldn't survive cryptodev_configure() and it is user
> > > responsibility to remove all installed callbacks before doing
> > > dev_configure() to avoid possible memory leakage.
> > Ok. I will update the documentation for this and send a new patch set.
> 
> Ok, please keep my ack on your new version.
> 
> > > Another option - add user provided cleanup() function pointer into
> > > struct rte_cryptodev_cb and call it here if provided:
> > > 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 */
> > > 	void (*cleanup)(void *);
> > > };
> > >
> > > And here:
> > > 	If (curr_cb->cleanup != NULL)
> > > 		curr_cb->cleanup(curr_cb->arg);
> > >
> > >  	rte_free(curr_cb);
> > >
> > > Rest of the code - LGTM.
> > > So with that addressed:
> > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > >
> > > > +		}
> > > > +
> > > > +		rte_free(list->qsbr);
> > > > +	}
> > > > +
> > > > +	rte_free(dev->enq_cbs);
> > > > +	dev->enq_cbs = NULL;
> > > > +}
> > > > +
> > > > +static int
> > > > +cryptodev_cb_init(struct rte_cryptodev *dev) {
> > > > +	struct rte_cryptodev_enq_cb_rcu *list;
> > > > +	struct rte_rcu_qsbr *qsbr;
> > > > +	uint16_t qp_id;
> > > > +	size_t size;
> > > > +
> > > > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > > > +	const uint32_t max_threads = 1;
> > > > +
> > > > +	dev->enq_cbs = rte_zmalloc(NULL,
> > > > +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> > > > +				   dev->data->nb_queue_pairs, 0);
> > > > +	if (dev->enq_cbs == NULL) {
> > > > +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> > > > +		rte_errno = ENOMEM;
> > > > +		return -1;
> > > > +	}
> > > > +
> > > > +	/* Create RCU QSBR variable */
> > > > +	size = rte_rcu_qsbr_get_memsize(max_threads);
> > > > +
> > > > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > > > +		list = &dev->enq_cbs[qp_id];
> > > > +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> > > > +		if (qsbr == NULL) {
> > > > +			CDEV_LOG_ERR("Failed to allocate memory for RCU
> > > on "
> > > > +				"queue_pair_id=%d", qp_id);
> > > > +			goto cb_init_err;
> > > > +		}
> > > > +
> > > > +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> > > > +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> > > > +				"queue_pair_id=%d", qp_id);
> > > > +			goto cb_init_err;
> > > > +		}
> > > > +
> > > > +		list->qsbr = qsbr;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +
> > > > +cb_init_err:
> > > > +	rte_errno = ENOMEM;
> > > > +	cryptodev_cb_cleanup(dev);
> > > > +	return -1;
> > > > +
> > > > +}
> > > > +#endif
> > > >
> > > >  const char *
> > > >  rte_cryptodev_get_feature_name(uint64_t flag) @@ -927,6 +1012,11
> > > > @@ struct rte_cryptodev *
> > > >
> > > >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> > > ENOTSUP);
> > > >
> > > > +#ifdef RTE_CRYPTO_CALLBACKS
> > > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > > +	cryptodev_cb_cleanup(dev);
> > > > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > > > +#endif
> > > >  	/* Setup new number of queue pairs and reconfigure device. */
> > > >  	diag = rte_cryptodev_queue_pairs_config(dev, config-
> > > >nb_queue_pairs,
> > > >  			config->socket_id);
> > > > @@ -936,6 +1026,15 @@ struct rte_cryptodev *
> > > >  		return diag;
> > > >  	}
> > > >
> > > > +#ifdef RTE_CRYPTO_CALLBACKS
> > > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > > +	diag = cryptodev_cb_init(dev);
> > > > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > > > +	if (diag) {
> > > > +		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +#endif
> > > >  	rte_cryptodev_trace_configure(dev_id, config);
> > > >  	return (*dev->dev_ops->dev_configure)(dev, config);  } @@
> > > > -1136,6
> > > > +1235,137 @@ 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_enq_cb_rcu *list;
> > > > +	struct rte_cryptodev_cb *cb, *tail;
> > > > +
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	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_errno = ENOMEM;
> > > > +		return NULL;
> > > > +	}
> > > > +
> > > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > > +
> > > > +	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;
> > > > +		/* Stores to cb->fn and cb->param should complete before
> > > > +		 * cb is visible to data plane.
> > > > +		 */
> > > > +		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> > > > +	} else {
> > > > +		/* Stores to cb->fn and cb->param should complete before
> > > > +		 * cb is visible to data plane.
> > > > +		 */
> > > > +		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
> > > > +	}
> > > > +
> > > > +	rte_spinlock_unlock(&rte_cryptodev_callback_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;
> > > > +	int ret;
> > > > +
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > > > +	if (dev->enq_cbs == NULL) {
> > > > +		CDEV_LOG_ERR("Callback not initialized");
> > > > +		goto cb_err;
> > > > +	}
> > > > +
> > > > +	list = &dev->enq_cbs[qp_id];
> > > > +	if (list == NULL) {
> > > > +		CDEV_LOG_ERR("Callback list is NULL");
> > > > +		goto cb_err;
> > > > +	}
> > > > +
> > > > +	if (list->qsbr == NULL) {
> > > > +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> > > > +		goto cb_err;
> > > > +	}
> > > > +
> > > > +	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. */
> > > > +			__atomic_store_n(prev_cb, curr_cb->next,
> > > > +				__ATOMIC_RELAXED);
> > > > +			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);
> > > > +	}
> > > > +
> > > > +cb_err:
> > > > +	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct
> > > > rte_cryptodev {
> > > >  	__extension__
> > > >  	uint8_t attached : 1;
> > > >  	/**< Flag indicating the device is attached */
> > > > +
> > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > +	/**< User application callback for pre enqueue processing */
> > > > +
> > > >  } __rte_cache_aligned;
> > > >
> > > >  void *
> > > > @@ -989,6 +1048,31 @@ struct rte_cryptodev_data {  {
> > > >  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> > > >
> > > > +#ifdef RTE_CRYPTO_CALLBACKS
> > > > +	if (unlikely(dev->enq_cbs != NULL)) {
> > > > +		struct rte_cryptodev_enq_cb_rcu *list;
> > > > +		struct rte_cryptodev_cb *cb;
> > > > +
> > > > +		/* __ATOMIC_RELEASE memory order was used when the
> > > > +		* call back was inserted into the list.
> > > > +		* Since there is a clear dependency between loading
> > > > +		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order
> > > is
> > > > +		* not required.
> > > > +		*/
> > > > +		list = &dev->enq_cbs[qp_id];
> > > > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> > > > +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> > > > +
> > > > +		while (cb != NULL) {
> > > > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > > > +					cb->arg);
> > > > +			cb = cb->next;
> > > > +		};
> > > > +
> > > > +		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 +1814,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] 28+ messages in thread

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
  2020-10-27 12:47   ` Ananyev, Konstantin
@ 2020-10-27 18:19   ` Akhil Goyal
  2020-10-27 19:16     ` Gujjar, Abhinandan S
  2020-10-27 18:28   ` Akhil Goyal
  2 siblings, 1 reply; 28+ messages in thread
From: Akhil Goyal @ 2020-10-27 18:19 UTC (permalink / raw)
  To: Abhinandan Gujjar, dev, declan.doherty, Honnappa.Nagarahalli,
	konstantin.ananyev
  Cc: narender.vangati, jerinj

Hi Abhinandan,
> Subject: [v4 1/3] cryptodev: support enqueue callback functions
> 
> 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           | 230 +++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
>  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
>  5 files changed, 391 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..0880d9b 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -448,6 +448,91 @@ 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_callback_lock =
> RTE_SPINLOCK_INITIALIZER;
> +
> +static void
> +cryptodev_cb_cleanup(struct rte_cryptodev *dev)
> +{
> +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	uint16_t qp_id;
> +
> +	if (dev->enq_cbs == NULL)
> +		return;
> +
> +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> +		list = &dev->enq_cbs[qp_id];
> +		prev_cb = &list->next;
> +
> +		while (*prev_cb != NULL) {
> +			curr_cb = *prev_cb;
> +			/* Remove the user cb from the callback list. */
> +			__atomic_store_n(prev_cb, curr_cb->next,
> +				__ATOMIC_RELAXED);
> +			rte_rcu_qsbr_synchronize(list->qsbr,
> +				RTE_QSBR_THRID_INVALID);
> +			rte_free(curr_cb);
> +		}
> +
> +		rte_free(list->qsbr);
> +	}
> +
> +	rte_free(dev->enq_cbs);
> +	dev->enq_cbs = NULL;
> +}
> +
> +static int
> +cryptodev_cb_init(struct rte_cryptodev *dev)
> +{
> +	struct rte_cryptodev_enq_cb_rcu *list;
> +	struct rte_rcu_qsbr *qsbr;
> +	uint16_t qp_id;
> +	size_t size;
> +
> +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> +	const uint32_t max_threads = 1;
> +
> +	dev->enq_cbs = rte_zmalloc(NULL,
> +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> +				   dev->data->nb_queue_pairs, 0);
> +	if (dev->enq_cbs == NULL) {
> +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> +		rte_errno = ENOMEM;
> +		return -1;
> +	}

Why not return ENOMEM here? You are not using rte_errno while returning
from this function, so setting it does not have any meaning.

> +
> +	/* Create RCU QSBR variable */
> +	size = rte_rcu_qsbr_get_memsize(max_threads);
> +
> +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> +		list = &dev->enq_cbs[qp_id];
> +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> +		if (qsbr == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for RCU on
> "
> +				"queue_pair_id=%d", qp_id);
> +			goto cb_init_err;
> +		}
> +
> +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> +				"queue_pair_id=%d", qp_id);
> +			goto cb_init_err;
> +		}
> +
> +		list->qsbr = qsbr;
> +	}
> +
> +	return 0;
> +
> +cb_init_err:
> +	rte_errno = ENOMEM;
> +	cryptodev_cb_cleanup(dev);
> +	return -1;
Same here, return -ENOMEM

> +
Extra line

> +}
> +#endif
> 
>  const char *
>  rte_cryptodev_get_feature_name(uint64_t flag)
> @@ -927,6 +1012,11 @@ struct rte_cryptodev *
> 
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> ENOTSUP);
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +	cryptodev_cb_cleanup(dev);
> +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> +#endif
>  	/* Setup new number of queue pairs and reconfigure device. */
>  	diag = rte_cryptodev_queue_pairs_config(dev, config->nb_queue_pairs,
>  			config->socket_id);
> @@ -936,6 +1026,15 @@ struct rte_cryptodev *
>  		return diag;
>  	}
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +	diag = cryptodev_cb_init(dev);
> +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> +	if (diag) {
> +		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
> +		return -ENOMEM;
> +	}
> +#endif
>  	rte_cryptodev_trace_configure(dev_id, config);
>  	return (*dev->dev_ops->dev_configure)(dev, config);
>  }
> @@ -1136,6 +1235,137 @@ 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_enq_cb_rcu *list;
> +	struct rte_cryptodev_cb *cb, *tail;
> +
> +	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;
> +	}

Errno is not set before above three returns.

> +
> +	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_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +
> +	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;
> +		/* Stores to cb->fn and cb->param should complete before
> +		 * cb is visible to data plane.
> +		 */
> +		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> +	} else {
> +		/* Stores to cb->fn and cb->param should complete before
> +		 * cb is visible to data plane.
> +		 */
> +		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
> +	}
> +
> +	rte_spinlock_unlock(&rte_cryptodev_callback_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;
> +	int ret;
> +
> +	ret = -EINVAL;
No need to set EINVAL here. You are returning same value everywhere.
The error numbers can be different at each exit.

> +
> +	if (!cb) {
> +		CDEV_LOG_ERR("cb is NULL");
> +		return ret;
You should directly return -EINVAL here and in below cases as well.

> +	}
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> +		return ret;
Here return value should be -ENODEV


> +	}
> +
> +	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;
> +	}
> +
> +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> +	if (dev->enq_cbs == NULL) {
> +		CDEV_LOG_ERR("Callback not initialized");
> +		goto cb_err;
> +	}
> +
> +	list = &dev->enq_cbs[qp_id];
> +	if (list == NULL) {
> +		CDEV_LOG_ERR("Callback list is NULL");
> +		goto cb_err;
> +	}
> +
> +	if (list->qsbr == NULL) {
> +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> +		goto cb_err;
> +	}
> +
> +	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. */
> +			__atomic_store_n(prev_cb, curr_cb->next,
> +				__ATOMIC_RELAXED);
> +			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);
> +	}
> +
> +cb_err:
> +	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct rte_cryptodev {
>  	__extension__
>  	uint8_t attached : 1;
>  	/**< Flag indicating the device is attached */
> +
> +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> +	/**< User application callback for pre enqueue processing */
> +
Extra line

We should add support for post dequeue callbacks also. Since this is an LTS release
And we wont be very flexible in future quarterly release, we should do all the changes
In one go.
I believe we should also double check with techboard if this is an ABI breakage.
IMO, it is ABI breakage, rte_cryprodevs is part of stable APIs, but not sure.

>  } __rte_cache_aligned;
> 
>  void *
> @@ -989,6 +1048,31 @@ struct rte_cryptodev_data {
>  {
>  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> 
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	if (unlikely(dev->enq_cbs != NULL)) {
> +		struct rte_cryptodev_enq_cb_rcu *list;
> +		struct rte_cryptodev_cb *cb;
> +
> +		/* __ATOMIC_RELEASE memory order was used when the
> +		* call back was inserted into the list.
> +		* Since there is a clear dependency between loading
> +		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order
> is
> +		* not required.
> +		*/
> +		list = &dev->enq_cbs[qp_id];
> +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> +
> +		while (cb != NULL) {
> +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> +					cb->arg);
> +			cb = cb->next;
> +		};
> +
> +		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 +1814,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.

Is there a limit for the number of cbs that can be added? Better to add a
Comment here.

> + *
> + * @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] 28+ messages in thread

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
  2020-10-27 12:47   ` Ananyev, Konstantin
  2020-10-27 18:19   ` Akhil Goyal
@ 2020-10-27 18:28   ` Akhil Goyal
  2020-10-28  8:20     ` Gujjar, Abhinandan S
  2 siblings, 1 reply; 28+ messages in thread
From: Akhil Goyal @ 2020-10-27 18:28 UTC (permalink / raw)
  To: Abhinandan Gujjar, dev, declan.doherty, Honnappa.Nagarahalli,
	konstantin.ananyev, techboard
  Cc: narender.vangati, jerinj

Hi Tech board members,

I have a doubt about the ABI breakage in below addition of field.
Could you please comment.

>  /** The data structure associated with each crypto device. */
>  struct rte_cryptodev {
>  	dequeue_pkt_burst_t dequeue_burst;
> @@ -867,6 +922,10 @@ struct rte_cryptodev {
>  	__extension__
>  	uint8_t attached : 1;
>  	/**< Flag indicating the device is attached */
> +
> +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> +	/**< User application callback for pre enqueue processing */
> +
>  } __rte_cache_aligned;

Here rte_cryptodevs is defined in stable API list in map file which is a pointer
To all rte_cryptodev and the above change is changing the size of the structure.
IMO, it seems an ABI breakage, but not sure. So wanted to double check.
Now if it is an ABI breakage, then can we allow it? There was no deprecation notice
Prior to this release.

Also I think if we are allowing the above change, then we should also add another
Field for deq_cbs also for post crypto processing in this patch only.

Regards,
Akhil


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 18:19   ` Akhil Goyal
@ 2020-10-27 19:16     ` Gujjar, Abhinandan S
  2020-10-27 19:26       ` Akhil Goyal
  0 siblings, 1 reply; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-27 19:16 UTC (permalink / raw)
  To: Akhil Goyal, dev, Doherty, Declan, Honnappa.Nagarahalli, Ananyev,
	Konstantin
  Cc: Vangati, Narender, jerinj



> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Tuesday, October 27, 2020 11:49 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>;
> Honnappa.Nagarahalli@arm.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com
> Subject: RE: [v4 1/3] cryptodev: support enqueue callback functions
> 
> Hi Abhinandan,
> > Subject: [v4 1/3] cryptodev: support enqueue callback functions
> >
> > 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           | 230
> +++++++++++++++++++++++++
> >  lib/librte_cryptodev/rte_cryptodev.h           | 158 ++++++++++++++++-
> >  lib/librte_cryptodev/rte_cryptodev_version.map |   2 +
> >  5 files changed, 391 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..0880d9b 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -448,6 +448,91 @@ 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_callback_lock =
> > RTE_SPINLOCK_INITIALIZER;
> > +
> > +static void
> > +cryptodev_cb_cleanup(struct rte_cryptodev *dev) {
> > +	struct rte_cryptodev_cb **prev_cb, *curr_cb;
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	uint16_t qp_id;
> > +
> > +	if (dev->enq_cbs == NULL)
> > +		return;
> > +
> > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > +		list = &dev->enq_cbs[qp_id];
> > +		prev_cb = &list->next;
> > +
> > +		while (*prev_cb != NULL) {
> > +			curr_cb = *prev_cb;
> > +			/* Remove the user cb from the callback list. */
> > +			__atomic_store_n(prev_cb, curr_cb->next,
> > +				__ATOMIC_RELAXED);
> > +			rte_rcu_qsbr_synchronize(list->qsbr,
> > +				RTE_QSBR_THRID_INVALID);
> > +			rte_free(curr_cb);
> > +		}
> > +
> > +		rte_free(list->qsbr);
> > +	}
> > +
> > +	rte_free(dev->enq_cbs);
> > +	dev->enq_cbs = NULL;
> > +}
> > +
> > +static int
> > +cryptodev_cb_init(struct rte_cryptodev *dev) {
> > +	struct rte_cryptodev_enq_cb_rcu *list;
> > +	struct rte_rcu_qsbr *qsbr;
> > +	uint16_t qp_id;
> > +	size_t size;
> > +
> > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > +	const uint32_t max_threads = 1;
> > +
> > +	dev->enq_cbs = rte_zmalloc(NULL,
> > +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> > +				   dev->data->nb_queue_pairs, 0);
> > +	if (dev->enq_cbs == NULL) {
> > +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> > +		rte_errno = ENOMEM;
> > +		return -1;
> > +	}
> 
> Why not return ENOMEM here? You are not using rte_errno while returning
> from this function, so setting it does not have any meaning.
This is a internal function. The caller is returning ENOMEM.
> 
> > +
> > +	/* Create RCU QSBR variable */
> > +	size = rte_rcu_qsbr_get_memsize(max_threads);
> > +
> > +	for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) {
> > +		list = &dev->enq_cbs[qp_id];
> > +		qsbr = rte_zmalloc(NULL, size, RTE_CACHE_LINE_SIZE);
> > +		if (qsbr == NULL) {
> > +			CDEV_LOG_ERR("Failed to allocate memory for RCU
> on
> > "
> > +				"queue_pair_id=%d", qp_id);
> > +			goto cb_init_err;
> > +		}
> > +
> > +		if (rte_rcu_qsbr_init(qsbr, max_threads)) {
> > +			CDEV_LOG_ERR("Failed to initialize for RCU on "
> > +				"queue_pair_id=%d", qp_id);
> > +			goto cb_init_err;
> > +		}
> > +
> > +		list->qsbr = qsbr;
> > +	}
> > +
> > +	return 0;
> > +
> > +cb_init_err:
> > +	rte_errno = ENOMEM;
> > +	cryptodev_cb_cleanup(dev);
> > +	return -1;
> Same here, return -ENOMEM
Same as above
> 
> > +
> Extra line
ok
> 
> > +}
> > +#endif
> >
> >  const char *
> >  rte_cryptodev_get_feature_name(uint64_t flag) @@ -927,6 +1012,11 @@
> > struct rte_cryptodev *
> >
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> ENOTSUP);
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +	cryptodev_cb_cleanup(dev);
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > +#endif
> >  	/* Setup new number of queue pairs and reconfigure device. */
> >  	diag = rte_cryptodev_queue_pairs_config(dev, config-
> >nb_queue_pairs,
> >  			config->socket_id);
> > @@ -936,6 +1026,15 @@ struct rte_cryptodev *
> >  		return diag;
> >  	}
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +	diag = cryptodev_cb_init(dev);
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
> > +	if (diag) {
> > +		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
> > +		return -ENOMEM;
> > +	}
> > +#endif
> >  	rte_cryptodev_trace_configure(dev_id, config);
> >  	return (*dev->dev_ops->dev_configure)(dev, config);  } @@ -1136,6
> > +1235,137 @@ 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_enq_cb_rcu *list;
> > +	struct rte_cryptodev_cb *cb, *tail;
> > +
> > +	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;
> > +	}
> 
> Errno is not set before above three returns.
I will update it in the next version of the patch.
> 
> > +
> > +	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_errno = ENOMEM;
> > +		return NULL;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +
> > +	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;
> > +		/* Stores to cb->fn and cb->param should complete before
> > +		 * cb is visible to data plane.
> > +		 */
> > +		__atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> > +	} else {
> > +		/* Stores to cb->fn and cb->param should complete before
> > +		 * cb is visible to data plane.
> > +		 */
> > +		__atomic_store_n(&list->next, cb, __ATOMIC_RELEASE);
> > +	}
> > +
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_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;
> > +	int ret;
> > +
> > +	ret = -EINVAL;
> No need to set EINVAL here. You are returning same value everywhere.
> The error numbers can be different at each exit.
Sure. I will take care returning different error numbers.
The initialized is required because of below during just before calling
Rcu sync. 
> 
> > +
> > +	if (!cb) {
> > +		CDEV_LOG_ERR("cb is NULL");
> > +		return ret;
> You should directly return -EINVAL here and in below cases as well.
> 
> > +	}
> > +
> > +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> > +		CDEV_LOG_ERR("Invalid dev_id=%d", dev_id);
> > +		return ret;
> Here return value should be -ENODEV
> 
> 
> > +	}
> > +
> > +	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;
> > +	}
> > +
> > +	rte_spinlock_lock(&rte_cryptodev_callback_lock);
> > +	if (dev->enq_cbs == NULL) {
> > +		CDEV_LOG_ERR("Callback not initialized");
> > +		goto cb_err;
> > +	}
> > +
> > +	list = &dev->enq_cbs[qp_id];
> > +	if (list == NULL) {
> > +		CDEV_LOG_ERR("Callback list is NULL");
> > +		goto cb_err;
> > +	}
> > +
> > +	if (list->qsbr == NULL) {
> > +		CDEV_LOG_ERR("Rcu qsbr is NULL");
> > +		goto cb_err;
> > +	}
> > +
> > +	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. */
> > +			__atomic_store_n(prev_cb, curr_cb->next,
> > +				__ATOMIC_RELAXED);
> > +			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);
> > +	}
> > +
> > +cb_err:
> > +	rte_spinlock_unlock(&rte_cryptodev_callback_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..1b7d7ef 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,10 @@ struct rte_cryptodev {
> >  	__extension__
> >  	uint8_t attached : 1;
> >  	/**< Flag indicating the device is attached */
> > +
> > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > +	/**< User application callback for pre enqueue processing */
> > +
> Extra line
ok
> 
> We should add support for post dequeue callbacks also. Since this is an LTS
> release And we wont be very flexible in future quarterly release, we should do
> all the changes In one go.
This patch set is driven by requirement. Recently, we have a requirement to have
callback for dequeue as well. Looking at code freeze date, I am not sure we can
target that as well. Let this patch go in and I will send a separate patch for
dequeue callback.

> I believe we should also double check with techboard if this is an ABI breakage.
> IMO, it is ABI breakage, rte_cryprodevs is part of stable APIs, but not sure.
> 
> >  } __rte_cache_aligned;
> >
> >  void *
> > @@ -989,6 +1048,31 @@ struct rte_cryptodev_data {  {
> >  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> >
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	if (unlikely(dev->enq_cbs != NULL)) {
> > +		struct rte_cryptodev_enq_cb_rcu *list;
> > +		struct rte_cryptodev_cb *cb;
> > +
> > +		/* __ATOMIC_RELEASE memory order was used when the
> > +		* call back was inserted into the list.
> > +		* Since there is a clear dependency between loading
> > +		* cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order
> > is
> > +		* not required.
> > +		*/
> > +		list = &dev->enq_cbs[qp_id];
> > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> > +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> > +
> > +		while (cb != NULL) {
> > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > +					cb->arg);
> > +			cb = cb->next;
> > +		};
> > +
> > +		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 +1814,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.
> 
> Is there a limit for the number of cbs that can be added? Better to add a
> Comment here.
> 
> > + *
> > + * @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] 28+ messages in thread

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 19:16     ` Gujjar, Abhinandan S
@ 2020-10-27 19:26       ` Akhil Goyal
  2020-10-27 19:41         ` Gujjar, Abhinandan S
  0 siblings, 1 reply; 28+ messages in thread
From: Akhil Goyal @ 2020-10-27 19:26 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, dev, Doherty, Declan, Honnappa.Nagarahalli,
	Ananyev, Konstantin
  Cc: Vangati, Narender, jerinj

Hi Abhinandan,

> > > +static int
> > > +cryptodev_cb_init(struct rte_cryptodev *dev) {
> > > +	struct rte_cryptodev_enq_cb_rcu *list;
> > > +	struct rte_rcu_qsbr *qsbr;
> > > +	uint16_t qp_id;
> > > +	size_t size;
> > > +
> > > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > > +	const uint32_t max_threads = 1;
> > > +
> > > +	dev->enq_cbs = rte_zmalloc(NULL,
> > > +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> > > +				   dev->data->nb_queue_pairs, 0);
> > > +	if (dev->enq_cbs == NULL) {
> > > +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> > > +		rte_errno = ENOMEM;
> > > +		return -1;
> > > +	}
> >
> > Why not return ENOMEM here? You are not using rte_errno while returning
> > from this function, so setting it does not have any meaning.
> This is a internal function. The caller is returning ENOMEM.

The caller can return the returned value from cryptodev_cb_init, instead of explicitly
Returning ENOMEM.
There is no point of setting rte_errno here.


> > >  /** The data structure associated with each crypto device. */  struct
> > > rte_cryptodev {
> > >  	dequeue_pkt_burst_t dequeue_burst;
> > > @@ -867,6 +922,10 @@ struct rte_cryptodev {
> > >  	__extension__
> > >  	uint8_t attached : 1;
> > >  	/**< Flag indicating the device is attached */
> > > +
> > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > +	/**< User application callback for pre enqueue processing */
> > > +
> > Extra line
> ok
> >
> > We should add support for post dequeue callbacks also. Since this is an LTS
> > release And we wont be very flexible in future quarterly release, we should do
> > all the changes In one go.
> This patch set is driven by requirement. Recently, we have a requirement to have
> callback for dequeue as well. Looking at code freeze date, I am not sure we can
> target that as well. Let this patch go in and I will send a separate patch for
> dequeue callback.
> 

We may not be able to change the rte_cryptodev structure so frequently.
It may be allowed to change it 21.11 release. Which is too far.
I think atleast the cryptodev changes can go in RC2 and test app for deq cbs
Can go in RC3 if not RC2.

> > I believe we should also double check with techboard if this is an ABI breakage.
> > IMO, it is ABI breakage, rte_cryprodevs is part of stable APIs, but not sure.
> >
> > >  } __rte_cache_aligned;
> > >



> > >
> > > +#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.
> >
> > Is there a limit for the number of cbs that can be added? Better to add a
> > Comment here.

I think you missed this comment.



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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 19:26       ` Akhil Goyal
@ 2020-10-27 19:41         ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-27 19:41 UTC (permalink / raw)
  To: Akhil Goyal, dev, Doherty, Declan, Honnappa.Nagarahalli, Ananyev,
	Konstantin
  Cc: Vangati, Narender, jerinj

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Wednesday, October 28, 2020 12:56 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>;
> Honnappa.Nagarahalli@arm.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com
> Subject: RE: [v4 1/3] cryptodev: support enqueue callback functions
> 
> Hi Abhinandan,
> 
> > > > +static int
> > > > +cryptodev_cb_init(struct rte_cryptodev *dev) {
> > > > +	struct rte_cryptodev_enq_cb_rcu *list;
> > > > +	struct rte_rcu_qsbr *qsbr;
> > > > +	uint16_t qp_id;
> > > > +	size_t size;
> > > > +
> > > > +	/* Max thread set to 1, as one DP thread accessing a queue-pair */
> > > > +	const uint32_t max_threads = 1;
> > > > +
> > > > +	dev->enq_cbs = rte_zmalloc(NULL,
> > > > +				   sizeof(struct rte_cryptodev_enq_cb_rcu) *
> > > > +				   dev->data->nb_queue_pairs, 0);
> > > > +	if (dev->enq_cbs == NULL) {
> > > > +		CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> > > > +		rte_errno = ENOMEM;
> > > > +		return -1;
> > > > +	}
> > >
> > > Why not return ENOMEM here? You are not using rte_errno while
> > > returning from this function, so setting it does not have any meaning.
> > This is a internal function. The caller is returning ENOMEM.
> 
> The caller can return the returned value from cryptodev_cb_init, instead of
> explicitly Returning ENOMEM.
> There is no point of setting rte_errno here.
Ok. I will update the patch.
> 
> 
> > > >  /** The data structure associated with each crypto device. */
> > > > struct rte_cryptodev {
> > > >  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10 @@ struct
> > > > rte_cryptodev {
> > > >  	__extension__
> > > >  	uint8_t attached : 1;
> > > >  	/**< Flag indicating the device is attached */
> > > > +
> > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > +	/**< User application callback for pre enqueue processing */
> > > > +
> > > Extra line
> > ok
> > >
> > > We should add support for post dequeue callbacks also. Since this is
> > > an LTS release And we wont be very flexible in future quarterly
> > > release, we should do all the changes In one go.
> > This patch set is driven by requirement. Recently, we have a
> > requirement to have callback for dequeue as well. Looking at code
> > freeze date, I am not sure we can target that as well. Let this patch
> > go in and I will send a separate patch for dequeue callback.
> >
> 
> We may not be able to change the rte_cryptodev structure so frequently.
> It may be allowed to change it 21.11 release. Which is too far.
> I think atleast the cryptodev changes can go in RC2 and test app for deq cbs
> Can go in RC3 if not RC2.
" cryptodev changes " -> Is it rte_cryptodev structure changes alone or supporting
dequeue callback as well in RC2? And then have test app changes in RC3?
If it is about adding dequeue callback support in RC2, I will try.
If it does not work, hope we can still the get the enqueue callback + rte_cryptodev structure
changes to support dequeue callbacks in the next patch set.
> 
> > > I believe we should also double check with techboard if this is an ABI
> breakage.
> > > IMO, it is ABI breakage, rte_cryprodevs is part of stable APIs, but not sure.
> > >
> > > >  } __rte_cache_aligned;
> > > >
> 
> 
> 
> > > >
> > > > +#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.
> > >
> > > Is there a limit for the number of cbs that can be added? Better to
> > > add a Comment here.
> 
> I think you missed this comment.
There is not limitation as of now. I will add a comment on the same.
> 


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-27 18:28   ` Akhil Goyal
@ 2020-10-28  8:20     ` Gujjar, Abhinandan S
  2020-10-28 12:55       ` Ananyev, Konstantin
  0 siblings, 1 reply; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-28  8:20 UTC (permalink / raw)
  To: Akhil Goyal, dev, Doherty, Declan, Honnappa.Nagarahalli, Ananyev,
	Konstantin, techboard
  Cc: Vangati, Narender, jerinj

Hi Tech board members,

Could you please clarify the concern?
The latest patch (https://patches.dpdk.org/patch/82536/) supports both enqueue and dequeue callback functionality.

Thanks
Abhinandan

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Tuesday, October 27, 2020 11:59 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>;
> Honnappa.Nagarahalli@arm.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; techboard@dpdk.org
> Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com
> Subject: RE: [v4 1/3] cryptodev: support enqueue callback functions
> 
> Hi Tech board members,
> 
> I have a doubt about the ABI breakage in below addition of field.
> Could you please comment.
> 
> >  /** The data structure associated with each crypto device. */  struct
> > rte_cryptodev {
> >  	dequeue_pkt_burst_t dequeue_burst;
> > @@ -867,6 +922,10 @@ struct rte_cryptodev {
> >  	__extension__
> >  	uint8_t attached : 1;
> >  	/**< Flag indicating the device is attached */
> > +
> > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > +	/**< User application callback for pre enqueue processing */
> > +
> >  } __rte_cache_aligned;
> 
> Here rte_cryptodevs is defined in stable API list in map file which is a pointer
> To all rte_cryptodev and the above change is changing the size of the structure.
> IMO, it seems an ABI breakage, but not sure. So wanted to double check.
> Now if it is an ABI breakage, then can we allow it? There was no deprecation
> notice Prior to this release.
> 
> Also I think if we are allowing the above change, then we should also add
> another Field for deq_cbs also for post crypto processing in this patch only.
> 
> Regards,
> Akhil


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28  8:20     ` Gujjar, Abhinandan S
@ 2020-10-28 12:55       ` Ananyev, Konstantin
  2020-10-28 14:28         ` Akhil Goyal
  0 siblings, 1 reply; 28+ messages in thread
From: Ananyev, Konstantin @ 2020-10-28 12:55 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, Akhil Goyal, dev, Doherty, Declan,
	Honnappa.Nagarahalli, techboard
  Cc: Vangati, Narender, jerinj


> 
> Hi Tech board members,
> 
> Could you please clarify the concern?
> The latest patch (https://patches.dpdk.org/patch/82536/) supports both enqueue and dequeue callback functionality.
> 
> Thanks
> Abhinandan
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Tuesday, October 27, 2020 11:59 PM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> > Doherty, Declan <declan.doherty@intel.com>;
> > Honnappa.Nagarahalli@arm.com; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; techboard@dpdk.org
> > Cc: Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com
> > Subject: RE: [v4 1/3] cryptodev: support enqueue callback functions
> >
> > Hi Tech board members,
> >
> > I have a doubt about the ABI breakage in below addition of field.
> > Could you please comment.
> >
> > >  /** The data structure associated with each crypto device. */  struct
> > > rte_cryptodev {
> > >  	dequeue_pkt_burst_t dequeue_burst;
> > > @@ -867,6 +922,10 @@ struct rte_cryptodev {
> > >  	__extension__
> > >  	uint8_t attached : 1;
> > >  	/**< Flag indicating the device is attached */
> > > +
> > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > +	/**< User application callback for pre enqueue processing */
> > > +
> > >  } __rte_cache_aligned;
> >
> > Here rte_cryptodevs is defined in stable API list in map file which is a pointer
> > To all rte_cryptodev and the above change is changing the size of the structure.

While this patch adds new fields into rte_cryptodev structure,
it doesn't change the size of it.
struct rte_cryptodev is cache line aligned, so it's current size:
128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
So for 64-bit we have 47B implicitly reserved, and for 32-bit we have 19B reserved.
That's enough to add two pointers without changing size of this struct. 

> > IMO, it seems an ABI breakage, but not sure. So wanted to double check.
> > Now if it is an ABI breakage, then can we allow it? There was no deprecation
> > notice Prior to this release.

Yes, there was no deprecation note in advance.
Though I think the risk is minimal - size of the struct will remain unchanged (see above).
My vote to let it in for 20.11.

> > Also I think if we are allowing the above change, then we should also add
> > another Field for deq_cbs also for post crypto processing in this patch only.

+1 for this.
I think it was already addressed in v5.

Konstantin


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28 12:55       ` Ananyev, Konstantin
@ 2020-10-28 14:28         ` Akhil Goyal
  2020-10-28 14:52           ` Ananyev, Konstantin
  2020-10-28 15:11           ` [dpdk-dev] [dpdk-techboard] " Bruce Richardson
  0 siblings, 2 replies; 28+ messages in thread
From: Akhil Goyal @ 2020-10-28 14:28 UTC (permalink / raw)
  To: Ananyev, Konstantin, Gujjar, Abhinandan S, dev, Doherty,  Declan,
	Honnappa.Nagarahalli, techboard
  Cc: Vangati, Narender, jerinj


Hi Konstantin,

> > > Hi Tech board members,
> > >
> > > I have a doubt about the ABI breakage in below addition of field.
> > > Could you please comment.
> > >
> > > >  /** The data structure associated with each crypto device. */  struct
> > > > rte_cryptodev {
> > > >  	dequeue_pkt_burst_t dequeue_burst;
> > > > @@ -867,6 +922,10 @@ struct rte_cryptodev {
> > > >  	__extension__
> > > >  	uint8_t attached : 1;
> > > >  	/**< Flag indicating the device is attached */
> > > > +
> > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > +	/**< User application callback for pre enqueue processing */
> > > > +
> > > >  } __rte_cache_aligned;
> > >
> > > Here rte_cryptodevs is defined in stable API list in map file which is a pointer
> > > To all rte_cryptodev and the above change is changing the size of the
> structure.
> 
> While this patch adds new fields into rte_cryptodev structure,
> it doesn't change the size of it.
> struct rte_cryptodev is cache line aligned, so it's current size:
> 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> So for 64-bit we have 47B implicitly reserved, and for 32-bit we have 19B
> reserved.
> That's enough to add two pointers without changing size of this struct.
> 

The structure is cache aligned, and if the cache line size in 32Byte and the compilation
is done on 64bit machine, then we will be left with 15Bytes which is not sufficient for 2
pointers.
Do we have such systems? Am I missing something?

The reason I brought this into techboard is to have a consensus on such change
As rte_cryptodev is a very popular and stable structure. Any changes to it may
Have impacts which one person cannot judge all use cases.

> > > IMO, it seems an ABI breakage, but not sure. So wanted to double check.
> > > Now if it is an ABI breakage, then can we allow it? There was no deprecation
> > > notice Prior to this release.
> 
> Yes, there was no deprecation note in advance.
> Though I think the risk is minimal - size of the struct will remain unchanged (see
> above).
> My vote to let it in for 20.11.
> 
> > > Also I think if we are allowing the above change, then we should also add
> > > another Field for deq_cbs also for post crypto processing in this patch only.
> 
> +1 for this.
> I think it was already addressed in v5.
> 
> Konstantin


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

* Re: [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28 14:28         ` Akhil Goyal
@ 2020-10-28 14:52           ` Ananyev, Konstantin
  2020-10-28 15:11           ` [dpdk-dev] [dpdk-techboard] " Bruce Richardson
  1 sibling, 0 replies; 28+ messages in thread
From: Ananyev, Konstantin @ 2020-10-28 14:52 UTC (permalink / raw)
  To: Akhil Goyal, Gujjar, Abhinandan S, dev, Doherty, Declan,
	Honnappa.Nagarahalli, techboard
  Cc: Vangati, Narender, jerinj


Hi Akhil,
 
> Hi Konstantin,
> 
> > > > Hi Tech board members,
> > > >
> > > > I have a doubt about the ABI breakage in below addition of field.
> > > > Could you please comment.
> > > >
> > > > >  /** The data structure associated with each crypto device. */  struct
> > > > > rte_cryptodev {
> > > > >  	dequeue_pkt_burst_t dequeue_burst;
> > > > > @@ -867,6 +922,10 @@ struct rte_cryptodev {
> > > > >  	__extension__
> > > > >  	uint8_t attached : 1;
> > > > >  	/**< Flag indicating the device is attached */
> > > > > +
> > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > +	/**< User application callback for pre enqueue processing */
> > > > > +
> > > > >  } __rte_cache_aligned;
> > > >
> > > > Here rte_cryptodevs is defined in stable API list in map file which is a pointer
> > > > To all rte_cryptodev and the above change is changing the size of the
> > structure.
> >
> > While this patch adds new fields into rte_cryptodev structure,
> > it doesn't change the size of it.
> > struct rte_cryptodev is cache line aligned, so it's current size:
> > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > So for 64-bit we have 47B implicitly reserved, and for 32-bit we have 19B
> > reserved.
> > That's enough to add two pointers without changing size of this struct.
> >
> 
> The structure is cache aligned, and if the cache line size in 32Byte and the compilation
> is done on 64bit machine, then we will be left with 15Bytes which is not sufficient for 2
> pointers.
> Do we have such systems? 

AFAIK - no, minimal supported cache-line size: 64B:
lib/librte_eal/include/rte_common.h:#define RTE_CACHE_LINE_MIN_SIZE 64

> Am I missing something?

> The reason I brought this into techboard is to have a consensus on such change
> As rte_cryptodev is a very popular and stable structure. Any changes to it may
> Have impacts which one person cannot judge all use cases.

+1 here.
I also think it would be good to get other TB members opinion about proposed changes.
 
> > > > IMO, it seems an ABI breakage, but not sure. So wanted to double check.
> > > > Now if it is an ABI breakage, then can we allow it? There was no deprecation
> > > > notice Prior to this release.
> >
> > Yes, there was no deprecation note in advance.
> > Though I think the risk is minimal - size of the struct will remain unchanged (see
> > above).
> > My vote to let it in for 20.11.
> >
> > > > Also I think if we are allowing the above change, then we should also add
> > > > another Field for deq_cbs also for post crypto processing in this patch only.
> >
> > +1 for this.
> > I think it was already addressed in v5.
> >
> > Konstantin


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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28 14:28         ` Akhil Goyal
  2020-10-28 14:52           ` Ananyev, Konstantin
@ 2020-10-28 15:11           ` Bruce Richardson
  2020-10-28 15:22             ` Honnappa Nagarahalli
  1 sibling, 1 reply; 28+ messages in thread
From: Bruce Richardson @ 2020-10-28 15:11 UTC (permalink / raw)
  To: Akhil Goyal
  Cc: Ananyev, Konstantin, Gujjar, Abhinandan S, dev, Doherty, Declan,
	Honnappa.Nagarahalli, techboard, Vangati, Narender, jerinj

On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
> 
> Hi Konstantin,
> 
> > > > Hi Tech board members,
> > > >
> > > > I have a doubt about the ABI breakage in below addition of field.
> > > > Could you please comment.
> > > >
> > > > >  /** The data structure associated with each crypto device. */  struct
> > > > > rte_cryptodev {
> > > > >  	dequeue_pkt_burst_t dequeue_burst;
> > > > > @@ -867,6 +922,10 @@ struct rte_cryptodev {
> > > > >  	__extension__
> > > > >  	uint8_t attached : 1;
> > > > >  	/**< Flag indicating the device is attached */
> > > > > +
> > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > +	/**< User application callback for pre enqueue processing */
> > > > > +
> > > > >  } __rte_cache_aligned;
> > > >
> > > > Here rte_cryptodevs is defined in stable API list in map file which is a pointer
> > > > To all rte_cryptodev and the above change is changing the size of the
> > structure.
> > 
> > While this patch adds new fields into rte_cryptodev structure,
> > it doesn't change the size of it.
> > struct rte_cryptodev is cache line aligned, so it's current size:
> > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > So for 64-bit we have 47B implicitly reserved, and for 32-bit we have 19B
> > reserved.
> > That's enough to add two pointers without changing size of this struct.
> > 
> 
> The structure is cache aligned, and if the cache line size in 32Byte and the compilation
> is done on 64bit machine, then we will be left with 15Bytes which is not sufficient for 2
> pointers.
> Do we have such systems? Am I missing something?
> 

I don't think we support any such systems, so unless someone can point out
a specific case where we need to support 32-byte CLs, I'd tend towards
ignoring this as a non-issue.

> The reason I brought this into techboard is to have a consensus on such change
> As rte_cryptodev is a very popular and stable structure. Any changes to it may
> Have impacts which one person cannot judge all use cases.
>

Haven't been tracking this discussion much, but from what I read here, this
doesn't look like an ABI break and should be ok.

Regards,
/Bruce

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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28 15:11           ` [dpdk-dev] [dpdk-techboard] " Bruce Richardson
@ 2020-10-28 15:22             ` Honnappa Nagarahalli
  2020-10-29 13:52               ` Gujjar, Abhinandan S
  2020-10-29 14:26               ` Kinsella, Ray
  0 siblings, 2 replies; 28+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-28 15:22 UTC (permalink / raw)
  To: Bruce Richardson, Akhil.goyal@nxp.com, Ray Kinsella
  Cc: Ananyev, Konstantin, Gujjar, Abhinandan S, dev, Doherty,  Declan,
	techboard, Vangati, Narender, jerinj, nd

+ Ray for ABI

<snip>

> 
> On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
> >
> > Hi Konstantin,
> >
> > > > > Hi Tech board members,
> > > > >
> > > > > I have a doubt about the ABI breakage in below addition of field.
> > > > > Could you please comment.
> > > > >
> > > > > >  /** The data structure associated with each crypto device. */
> > > > > > struct rte_cryptodev {
> > > > > >  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10
> @@
> > > > > > struct rte_cryptodev {
> > > > > >  	__extension__
> > > > > >  	uint8_t attached : 1;
> > > > > >  	/**< Flag indicating the device is attached */
> > > > > > +
> > > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > > +	/**< User application callback for pre enqueue processing */
> > > > > > +
> > > > > >  } __rte_cache_aligned;
> > > > >
> > > > > Here rte_cryptodevs is defined in stable API list in map file
> > > > > which is a pointer To all rte_cryptodev and the above change is
> > > > > changing the size of the
> > > structure.
> > >
> > > While this patch adds new fields into rte_cryptodev structure, it
> > > doesn't change the size of it.
> > > struct rte_cryptodev is cache line aligned, so it's current size:
> > > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > > So for 64-bit we have 47B implicitly reserved, and for 32-bit we
> > > have 19B reserved.
> > > That's enough to add two pointers without changing size of this struct.
> > >
> >
> > The structure is cache aligned, and if the cache line size in 32Byte
> > and the compilation is done on 64bit machine, then we will be left
> > with 15Bytes which is not sufficient for 2 pointers.
> > Do we have such systems? Am I missing something?
> >
> 
> I don't think we support any such systems, so unless someone can point out
> a specific case where we need to support 32-byte CLs, I'd tend towards
> ignoring this as a non-issue.
Agree. I have not come across 32B cache line.

> 
> > The reason I brought this into techboard is to have a consensus on
> > such change As rte_cryptodev is a very popular and stable structure.
> > Any changes to it may Have impacts which one person cannot judge all use
> cases.
> >
> 
> Haven't been tracking this discussion much, but from what I read here, this
> doesn't look like an ABI break and should be ok.
If we are filling the holes in the cache line with new fields, it should not be an ABI break.

> 
> Regards,
> /Bruce

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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28 15:22             ` Honnappa Nagarahalli
@ 2020-10-29 13:52               ` Gujjar, Abhinandan S
  2020-10-29 14:00                 ` Akhil Goyal
  2020-10-29 14:26               ` Kinsella, Ray
  1 sibling, 1 reply; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-29 13:52 UTC (permalink / raw)
  To: Honnappa Nagarahalli, Richardson, Bruce, Akhil.goyal@nxp.com,
	Ray Kinsella
  Cc: Ananyev, Konstantin, dev, Doherty, Declan, techboard, Vangati,
	Narender, jerinj, nd

Hi Akhil,

Any updates on this?

Thanks
Abhinandan

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, October 28, 2020 8:52 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Akhil.goyal@nxp.com;
> Ray Kinsella <mdr@ashroe.eu>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>; dev@dpdk.org; Doherty, Declan
> <declan.doherty@intel.com>; techboard@dpdk.org; Vangati, Narender
> <narender.vangati@intel.com>; jerinj@marvell.com; nd <nd@arm.com>
> Subject: RE: [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback
> functions
> 
> + Ray for ABI
> 
> <snip>
> 
> >
> > On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
> > >
> > > Hi Konstantin,
> > >
> > > > > > Hi Tech board members,
> > > > > >
> > > > > > I have a doubt about the ABI breakage in below addition of field.
> > > > > > Could you please comment.
> > > > > >
> > > > > > >  /** The data structure associated with each crypto device.
> > > > > > > */ struct rte_cryptodev {
> > > > > > >  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10
> > @@
> > > > > > > struct rte_cryptodev {
> > > > > > >  	__extension__
> > > > > > >  	uint8_t attached : 1;
> > > > > > >  	/**< Flag indicating the device is attached */
> > > > > > > +
> > > > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > > > +	/**< User application callback for pre enqueue processing
> > > > > > > +*/
> > > > > > > +
> > > > > > >  } __rte_cache_aligned;
> > > > > >
> > > > > > Here rte_cryptodevs is defined in stable API list in map file
> > > > > > which is a pointer To all rte_cryptodev and the above change
> > > > > > is changing the size of the
> > > > structure.
> > > >
> > > > While this patch adds new fields into rte_cryptodev structure, it
> > > > doesn't change the size of it.
> > > > struct rte_cryptodev is cache line aligned, so it's current size:
> > > > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > > > So for 64-bit we have 47B implicitly reserved, and for 32-bit we
> > > > have 19B reserved.
> > > > That's enough to add two pointers without changing size of this struct.
> > > >
> > >
> > > The structure is cache aligned, and if the cache line size in 32Byte
> > > and the compilation is done on 64bit machine, then we will be left
> > > with 15Bytes which is not sufficient for 2 pointers.
> > > Do we have such systems? Am I missing something?
> > >
> >
> > I don't think we support any such systems, so unless someone can point
> > out a specific case where we need to support 32-byte CLs, I'd tend
> > towards ignoring this as a non-issue.
> Agree. I have not come across 32B cache line.
> 
> >
> > > The reason I brought this into techboard is to have a consensus on
> > > such change As rte_cryptodev is a very popular and stable structure.
> > > Any changes to it may Have impacts which one person cannot judge all
> > > use
> > cases.
> > >
> >
> > Haven't been tracking this discussion much, but from what I read here,
> > this doesn't look like an ABI break and should be ok.
> If we are filling the holes in the cache line with new fields, it should not be an
> ABI break.
> 
> >
> > Regards,
> > /Bruce

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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-29 13:52               ` Gujjar, Abhinandan S
@ 2020-10-29 14:00                 ` Akhil Goyal
  2020-10-30  4:24                   ` Gujjar, Abhinandan S
  0 siblings, 1 reply; 28+ messages in thread
From: Akhil Goyal @ 2020-10-29 14:00 UTC (permalink / raw)
  To: Gujjar, Abhinandan S, Honnappa Nagarahalli, Richardson, Bruce,
	Ray Kinsella, Thomas Monjalon
  Cc: Ananyev, Konstantin, dev, Doherty, Declan, techboard, Vangati,
	Narender, jerinj, nd

> 
> Hi Akhil,
> 
> Any updates on this?
> 
There has been no objections for this patch from techboard.

@Thomas Monjalon: could you please review the release notes.
I believe there should be a bullet for API changes to add 2 new fields in rte_cryptodev.
What do you suggest?

@Gujjar, Abhinandan S
Please send a new version for comments on errno.
If possible add cases for deq_cbs as well. If not, send it by next week.

Regards,
Akhil
> > + Ray for ABI
> >
> > <snip>
> >
> > >
> > > On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
> > > >
> > > > Hi Konstantin,
> > > >
> > > > > > > Hi Tech board members,
> > > > > > >
> > > > > > > I have a doubt about the ABI breakage in below addition of field.
> > > > > > > Could you please comment.
> > > > > > >
> > > > > > > >  /** The data structure associated with each crypto device.
> > > > > > > > */ struct rte_cryptodev {
> > > > > > > >  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10
> > > @@
> > > > > > > > struct rte_cryptodev {
> > > > > > > >  	__extension__
> > > > > > > >  	uint8_t attached : 1;
> > > > > > > >  	/**< Flag indicating the device is attached */
> > > > > > > > +
> > > > > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > > > > +	/**< User application callback for pre enqueue processing
> > > > > > > > +*/
> > > > > > > > +
> > > > > > > >  } __rte_cache_aligned;
> > > > > > >
> > > > > > > Here rte_cryptodevs is defined in stable API list in map file
> > > > > > > which is a pointer To all rte_cryptodev and the above change
> > > > > > > is changing the size of the
> > > > > structure.
> > > > >
> > > > > While this patch adds new fields into rte_cryptodev structure, it
> > > > > doesn't change the size of it.
> > > > > struct rte_cryptodev is cache line aligned, so it's current size:
> > > > > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > > > > So for 64-bit we have 47B implicitly reserved, and for 32-bit we
> > > > > have 19B reserved.
> > > > > That's enough to add two pointers without changing size of this struct.
> > > > >
> > > >
> > > > The structure is cache aligned, and if the cache line size in 32Byte
> > > > and the compilation is done on 64bit machine, then we will be left
> > > > with 15Bytes which is not sufficient for 2 pointers.
> > > > Do we have such systems? Am I missing something?
> > > >
> > >
> > > I don't think we support any such systems, so unless someone can point
> > > out a specific case where we need to support 32-byte CLs, I'd tend
> > > towards ignoring this as a non-issue.
> > Agree. I have not come across 32B cache line.
> >
> > >
> > > > The reason I brought this into techboard is to have a consensus on
> > > > such change As rte_cryptodev is a very popular and stable structure.
> > > > Any changes to it may Have impacts which one person cannot judge all
> > > > use
> > > cases.
> > > >
> > >
> > > Haven't been tracking this discussion much, but from what I read here,
> > > this doesn't look like an ABI break and should be ok.
> > If we are filling the holes in the cache line with new fields, it should not be an
> > ABI break.
> >
> > >
> > > Regards,
> > > /Bruce

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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-28 15:22             ` Honnappa Nagarahalli
  2020-10-29 13:52               ` Gujjar, Abhinandan S
@ 2020-10-29 14:26               ` Kinsella, Ray
  1 sibling, 0 replies; 28+ messages in thread
From: Kinsella, Ray @ 2020-10-29 14:26 UTC (permalink / raw)
  To: Honnappa Nagarahalli, Bruce Richardson, Akhil.goyal@nxp.com
  Cc: Ananyev, Konstantin, Gujjar, Abhinandan S, dev, Doherty, Declan,
	techboard, Vangati, Narender, jerinj, nd



On 28/10/2020 15:22, Honnappa Nagarahalli wrote:
> + Ray for ABI
> 
> <snip>
> 
>>
>> On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
>>>
>>> Hi Konstantin,
>>>
>>>>>> Hi Tech board members,
>>>>>>
>>>>>> I have a doubt about the ABI breakage in below addition of field.
>>>>>> Could you please comment.
>>>>>>
>>>>>>>  /** The data structure associated with each crypto device. */
>>>>>>> struct rte_cryptodev {
>>>>>>>  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10
>> @@
>>>>>>> struct rte_cryptodev {
>>>>>>>  	__extension__
>>>>>>>  	uint8_t attached : 1;
>>>>>>>  	/**< Flag indicating the device is attached */
>>>>>>> +
>>>>>>> +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
>>>>>>> +	/**< User application callback for pre enqueue processing */
>>>>>>> +
>>>>>>>  } __rte_cache_aligned;
>>>>>>
>>>>>> Here rte_cryptodevs is defined in stable API list in map file
>>>>>> which is a pointer To all rte_cryptodev and the above change is
>>>>>> changing the size of the
>>>> structure.
>>>>
>>>> While this patch adds new fields into rte_cryptodev structure, it
>>>> doesn't change the size of it.
>>>> struct rte_cryptodev is cache line aligned, so it's current size:
>>>> 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
>>>> So for 64-bit we have 47B implicitly reserved, and for 32-bit we
>>>> have 19B reserved.
>>>> That's enough to add two pointers without changing size of this struct.
>>>>
>>>
>>> The structure is cache aligned, and if the cache line size in 32Byte
>>> and the compilation is done on 64bit machine, then we will be left
>>> with 15Bytes which is not sufficient for 2 pointers.
>>> Do we have such systems? Am I missing something?
>>>
>>
>> I don't think we support any such systems, so unless someone can point out
>> a specific case where we need to support 32-byte CLs, I'd tend towards
>> ignoring this as a non-issue.
> Agree. I have not come across 32B cache line.
> 
>>
>>> The reason I brought this into techboard is to have a consensus on
>>> such change As rte_cryptodev is a very popular and stable structure.
>>> Any changes to it may Have impacts which one person cannot judge all use
>> cases.
>>>
>>
>> Haven't been tracking this discussion much, but from what I read here, this
>> doesn't look like an ABI break and should be ok.
> If we are filling the holes in the cache line with new fields, it should not be an ABI break.

Agreed, risk seems minimal  ... it is an ABI Breakage window in anycase.
 
>>
>> Regards,
>> /Bruce

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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-29 14:00                 ` Akhil Goyal
@ 2020-10-30  4:24                   ` Gujjar, Abhinandan S
  2020-10-30 17:18                     ` Gujjar, Abhinandan S
  0 siblings, 1 reply; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-30  4:24 UTC (permalink / raw)
  To: Akhil Goyal, Honnappa Nagarahalli, Richardson, Bruce,
	Ray Kinsella, Thomas Monjalon
  Cc: Ananyev, Konstantin, dev, Doherty, Declan, techboard, Vangati,
	Narender, jerinj, nd


Thanks Tech board & Akhil for clarifying the concern.
Sure. I will send the new version of the patch.

Regards
Abhinandan

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, October 29, 2020 7:31 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Thomas
> Monjalon <thomas@monjalon.net>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>; techboard@dpdk.org; Vangati,
> Narender <narender.vangati@intel.com>; jerinj@marvell.com; nd
> <nd@arm.com>
> Subject: RE: [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback
> functions
> 
> >
> > Hi Akhil,
> >
> > Any updates on this?
> >
> There has been no objections for this patch from techboard.
> 
> @Thomas Monjalon: could you please review the release notes.
> I believe there should be a bullet for API changes to add 2 new fields in
> rte_cryptodev.
> What do you suggest?
> 
> @Gujjar, Abhinandan S
> Please send a new version for comments on errno.
> If possible add cases for deq_cbs as well. If not, send it by next week.

> 
> Regards,
> Akhil
> > > + Ray for ABI
> > >
> > > <snip>
> > >
> > > >
> > > > On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
> > > > >
> > > > > Hi Konstantin,
> > > > >
> > > > > > > > Hi Tech board members,
> > > > > > > >
> > > > > > > > I have a doubt about the ABI breakage in below addition of field.
> > > > > > > > Could you please comment.
> > > > > > > >
> > > > > > > > >  /** The data structure associated with each crypto device.
> > > > > > > > > */ struct rte_cryptodev {
> > > > > > > > >  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10
> > > > @@
> > > > > > > > > struct rte_cryptodev {
> > > > > > > > >  	__extension__
> > > > > > > > >  	uint8_t attached : 1;
> > > > > > > > >  	/**< Flag indicating the device is attached */
> > > > > > > > > +
> > > > > > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > > > > > +	/**< User application callback for pre enqueue
> > > > > > > > > +processing */
> > > > > > > > > +
> > > > > > > > >  } __rte_cache_aligned;
> > > > > > > >
> > > > > > > > Here rte_cryptodevs is defined in stable API list in map
> > > > > > > > file which is a pointer To all rte_cryptodev and the above
> > > > > > > > change is changing the size of the
> > > > > > structure.
> > > > > >
> > > > > > While this patch adds new fields into rte_cryptodev structure,
> > > > > > it doesn't change the size of it.
> > > > > > struct rte_cryptodev is cache line aligned, so it's current size:
> > > > > > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > > > > > So for 64-bit we have 47B implicitly reserved, and for 32-bit
> > > > > > we have 19B reserved.
> > > > > > That's enough to add two pointers without changing size of this struct.
> > > > > >
> > > > >
> > > > > The structure is cache aligned, and if the cache line size in
> > > > > 32Byte and the compilation is done on 64bit machine, then we
> > > > > will be left with 15Bytes which is not sufficient for 2 pointers.
> > > > > Do we have such systems? Am I missing something?
> > > > >
> > > >
> > > > I don't think we support any such systems, so unless someone can
> > > > point out a specific case where we need to support 32-byte CLs,
> > > > I'd tend towards ignoring this as a non-issue.
> > > Agree. I have not come across 32B cache line.
> > >
> > > >
> > > > > The reason I brought this into techboard is to have a consensus
> > > > > on such change As rte_cryptodev is a very popular and stable structure.
> > > > > Any changes to it may Have impacts which one person cannot judge
> > > > > all use
> > > > cases.
> > > > >
> > > >
> > > > Haven't been tracking this discussion much, but from what I read
> > > > here, this doesn't look like an ABI break and should be ok.
> > > If we are filling the holes in the cache line with new fields, it
> > > should not be an ABI break.
> > >
> > > >
> > > > Regards,
> > > > /Bruce

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

* Re: [dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions
  2020-10-30  4:24                   ` Gujjar, Abhinandan S
@ 2020-10-30 17:18                     ` Gujjar, Abhinandan S
  0 siblings, 0 replies; 28+ messages in thread
From: Gujjar, Abhinandan S @ 2020-10-30 17:18 UTC (permalink / raw)
  To: Akhil Goyal, Honnappa Nagarahalli, Richardson, Bruce,
	Ray Kinsella, Thomas Monjalon
  Cc: Ananyev, Konstantin, dev, Doherty, Declan, techboard, Vangati,
	Narender, jerinj, nd

Hi Akhil,

I have sent the v6 patch for RC2.
As discussed, I will get the test app updated for dequeue callback for RC3.

Thanks
Abhinandan

> -----Original Message-----
> From: Gujjar, Abhinandan S
> Sent: Friday, October 30, 2020 9:54 AM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Thomas
> Monjalon <thomas@monjalon.net>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Doherty, Declan <declan.doherty@intel.com>; techboard@dpdk.org; Vangati,
> Narender <narender.vangati@intel.com>; jerinj@marvell.com; nd
> <nd@arm.com>
> Subject: RE: [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback
> functions
> 
> 
> Thanks Tech board & Akhil for clarifying the concern.
> Sure. I will send the new version of the patch.
> 
> Regards
> Abhinandan
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Thursday, October 29, 2020 7:31 PM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Thomas
> > Monjalon <thomas@monjalon.net>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > Doherty, Declan <declan.doherty@intel.com>; techboard@dpdk.org;
> > Vangati, Narender <narender.vangati@intel.com>; jerinj@marvell.com; nd
> > <nd@arm.com>
> > Subject: RE: [dpdk-techboard] [v4 1/3] cryptodev: support enqueue
> > callback functions
> >
> > >
> > > Hi Akhil,
> > >
> > > Any updates on this?
> > >
> > There has been no objections for this patch from techboard.
> >
> > @Thomas Monjalon: could you please review the release notes.
> > I believe there should be a bullet for API changes to add 2 new fields
> > in rte_cryptodev.
> > What do you suggest?
> >
> > @Gujjar, Abhinandan S
> > Please send a new version for comments on errno.
> > If possible add cases for deq_cbs as well. If not, send it by next week.
> 
> >
> > Regards,
> > Akhil
> > > > + Ray for ABI
> > > >
> > > > <snip>
> > > >
> > > > >
> > > > > On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
> > > > > >
> > > > > > Hi Konstantin,
> > > > > >
> > > > > > > > > Hi Tech board members,
> > > > > > > > >
> > > > > > > > > I have a doubt about the ABI breakage in below addition of field.
> > > > > > > > > Could you please comment.
> > > > > > > > >
> > > > > > > > > >  /** The data structure associated with each crypto device.
> > > > > > > > > > */ struct rte_cryptodev {
> > > > > > > > > >  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6
> +922,10
> > > > > @@
> > > > > > > > > > struct rte_cryptodev {
> > > > > > > > > >  	__extension__
> > > > > > > > > >  	uint8_t attached : 1;
> > > > > > > > > >  	/**< Flag indicating the device is attached */
> > > > > > > > > > +
> > > > > > > > > > +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
> > > > > > > > > > +	/**< User application callback for pre enqueue
> > > > > > > > > > +processing */
> > > > > > > > > > +
> > > > > > > > > >  } __rte_cache_aligned;
> > > > > > > > >
> > > > > > > > > Here rte_cryptodevs is defined in stable API list in map
> > > > > > > > > file which is a pointer To all rte_cryptodev and the
> > > > > > > > > above change is changing the size of the
> > > > > > > structure.
> > > > > > >
> > > > > > > While this patch adds new fields into rte_cryptodev
> > > > > > > structure, it doesn't change the size of it.
> > > > > > > struct rte_cryptodev is cache line aligned, so it's current size:
> > > > > > > 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
> > > > > > > So for 64-bit we have 47B implicitly reserved, and for
> > > > > > > 32-bit we have 19B reserved.
> > > > > > > That's enough to add two pointers without changing size of this
> struct.
> > > > > > >
> > > > > >
> > > > > > The structure is cache aligned, and if the cache line size in
> > > > > > 32Byte and the compilation is done on 64bit machine, then we
> > > > > > will be left with 15Bytes which is not sufficient for 2 pointers.
> > > > > > Do we have such systems? Am I missing something?
> > > > > >
> > > > >
> > > > > I don't think we support any such systems, so unless someone can
> > > > > point out a specific case where we need to support 32-byte CLs,
> > > > > I'd tend towards ignoring this as a non-issue.
> > > > Agree. I have not come across 32B cache line.
> > > >
> > > > >
> > > > > > The reason I brought this into techboard is to have a
> > > > > > consensus on such change As rte_cryptodev is a very popular and
> stable structure.
> > > > > > Any changes to it may Have impacts which one person cannot
> > > > > > judge all use
> > > > > cases.
> > > > > >
> > > > >
> > > > > Haven't been tracking this discussion much, but from what I read
> > > > > here, this doesn't look like an ABI break and should be ok.
> > > > If we are filling the holes in the cache line with new fields, it
> > > > should not be an ABI break.
> > > >
> > > > >
> > > > > Regards,
> > > > > /Bruce

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

end of thread, other threads:[~2020-10-30 17:19 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-25  9:44 [dpdk-dev] [v4 0/3] support enqueue callbacks on cryptodev Abhinandan Gujjar
2020-10-25  9:44 ` [dpdk-dev] [v4 1/3] cryptodev: support enqueue callback functions Abhinandan Gujjar
2020-10-27 12:47   ` Ananyev, Konstantin
2020-10-27 17:16     ` Gujjar, Abhinandan S
2020-10-27 17:20       ` Ananyev, Konstantin
2020-10-27 17:22         ` Gujjar, Abhinandan S
2020-10-27 18:19   ` Akhil Goyal
2020-10-27 19:16     ` Gujjar, Abhinandan S
2020-10-27 19:26       ` Akhil Goyal
2020-10-27 19:41         ` Gujjar, Abhinandan S
2020-10-27 18:28   ` Akhil Goyal
2020-10-28  8:20     ` Gujjar, Abhinandan S
2020-10-28 12:55       ` Ananyev, Konstantin
2020-10-28 14:28         ` Akhil Goyal
2020-10-28 14:52           ` Ananyev, Konstantin
2020-10-28 15:11           ` [dpdk-dev] [dpdk-techboard] " Bruce Richardson
2020-10-28 15:22             ` Honnappa Nagarahalli
2020-10-29 13:52               ` Gujjar, Abhinandan S
2020-10-29 14:00                 ` Akhil Goyal
2020-10-30  4:24                   ` Gujjar, Abhinandan S
2020-10-30 17:18                     ` Gujjar, Abhinandan S
2020-10-29 14:26               ` Kinsella, Ray
2020-10-25  9:44 ` [dpdk-dev] [v4 2/3] test: add testcase for crypto enqueue callback Abhinandan Gujjar
2020-10-25  9:44 ` [dpdk-dev] [v4 3/3] doc: add enqueue callback APIs Abhinandan Gujjar
2020-10-26 19:08   ` Akhil Goyal
2020-10-27  3:52     ` Gujjar, Abhinandan S
2020-10-27 12:51   ` Ananyev, Konstantin
2020-10-27 17:17     ` Gujjar, Abhinandan S

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