[dpdk-dev] [PATCH] cryptodev: add support for user callback functions

[dpdk-dev] [PATCH] cryptodev: add support for user callback functions

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

Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
---
 config/common_base                             |   1 +
 lib/librte_cryptodev/rte_cryptodev.c           | 120 ++++++++++++++++++++++++
 lib/librte_cryptodev/rte_cryptodev.h           | 125 ++++++++++++++++++++++++-
 lib/librte_cryptodev/rte_cryptodev_version.map |   3 +
 4 files changed, 248 insertions(+), 1 deletion(-)

diff --git a/config/common_base b/config/common_base
index 9ec689d..6f93acb 100644
--- a/config/common_base
+++ b/config/common_base
@@ -586,6 +586,7 @@ CONFIG_RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC=y
 #
 CONFIG_RTE_LIBRTE_CRYPTODEV=y
 CONFIG_RTE_CRYPTO_MAX_DEVS=64
+CONFIG_RTE_CRYPTODEV_CALLBACKS=y
 
 #
 # Compile PMD for ARMv8 Crypto device
diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 2849b2e..5a4cba9 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -56,6 +56,9 @@
 /* spinlock for crypto device callbacks */
 static rte_spinlock_t rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER;
 
+/* spinlock for crypto device enq callbacks */
+static rte_spinlock_t rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER;
+
 
 /**
  * The user application callback description.
@@ -1256,6 +1259,123 @@ struct rte_cryptodev *
 	rte_spinlock_unlock(&rte_cryptodev_cb_lock);
 }
 
+const struct rte_cryptodev_enq_callback *__rte_experimental
+rte_cryptodev_add_enq_callback(uint8_t dev_id,
+			       uint16_t qp_id,
+			       rte_cryptodev_enq_cb_fn cb_fn,
+			       void *cb_arg)
+{
+	struct rte_cryptodev *dev;
+	struct rte_cryptodev_enq_callback *cb, *tail;
+
+	if (!cb_fn)
+		return NULL;
+
+	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, 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;
+	}
+
+	cb->fn = cb_fn;
+	cb->arg = cb_arg;
+
+	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
+	if (dev->enq_cbs == NULL) {
+		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
+					   dev->data->nb_queue_pairs, 0);
+		if (dev->enq_cbs == NULL) {
+			CDEV_LOG_ERR("Failed to allocate memory for callbacks");
+			rte_errno = ENOMEM;
+			rte_free(cb);
+			return NULL;
+		}
+	}
+
+	/* Add the callbacks in fifo order. */
+	tail = dev->enq_cbs[qp_id];
+	if (tail) {
+		while (tail->next)
+			tail = tail->next;
+		tail->next = cb;
+	} else
+		dev->enq_cbs[qp_id] = cb;
+
+	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+
+	return cb;
+}
+
+int __rte_experimental
+rte_cryptodev_remove_enq_callback(uint8_t dev_id,
+				  uint16_t qp_id,
+				  const struct rte_cryptodev_enq_callback *cb)
+{
+	struct rte_cryptodev *dev;
+	struct rte_cryptodev_enq_callback **prev_cb, *curr_cb;
+	uint16_t qp;
+	int free_mem = 1;
+	int ret = -EINVAL;
+
+	if (!cb)
+		return ret;
+
+	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, 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_enq_cb_lock);
+	if (dev->enq_cbs == NULL) {
+		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+		return ret;
+	}
+
+	prev_cb = &dev->enq_cbs[qp_id];
+	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
+		curr_cb = *prev_cb;
+		if (curr_cb == cb) {
+			/* Remove the user cb from the callback list. */
+			*prev_cb = curr_cb->next;
+			ret = 0;
+			break;
+		}
+	}
+
+	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
+		if (dev->enq_cbs[qp] != NULL) {
+			free_mem = 0;
+			break;
+		}
+
+	if (free_mem) {
+		rte_free(dev->enq_cbs);
+		dev->enq_cbs = NULL;
+	}
+
+	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
+
+	return ret;
+}
 
 int
 rte_cryptodev_sym_session_init(uint8_t dev_id,
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index f4846d2..2cf466b 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -518,6 +518,32 @@ struct rte_cryptodev_qp_conf {
 };
 
 /**
+ * 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_enq_cb_fn)(uint16_t dev_id, uint16_t qp_id,
+		struct rte_crypto_op **ops, uint16_t nb_ops, void *user_param);
+
+/**
  * Typedef for application callback function to be registered by application
  * software for notification of device events
  *
@@ -800,7 +826,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. */
@@ -817,6 +842,17 @@ 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);
 
+/**
+ * @internal
+ * Structure used to hold information about the callbacks to be called for a
+ * queue pair on enqueue.
+ */
+struct rte_cryptodev_enq_callback {
+	struct rte_cryptodev_enq_callback *next;
+	rte_cryptodev_enq_cb_fn fn;
+	void *arg;
+};
+
 /** The data structure associated with each crypto device. */
 struct rte_cryptodev {
 	dequeue_pkt_burst_t dequeue_burst;
@@ -839,6 +875,9 @@ struct rte_cryptodev {
 	struct rte_cryptodev_cb_list link_intr_cbs;
 	/**< User application callback for interrupts if present */
 
+	struct rte_cryptodev_enq_callback **enq_cbs;
+	/**< User application callback for pre enqueue processing */
+
 	void *security_ctx;
 	/**< Context for security ops */
 
@@ -966,6 +1005,18 @@ struct rte_cryptodev_data {
 {
 	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
 
+#ifdef RTE_CRYPTODEV_CALLBACKS
+	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
+		struct rte_cryptodev_enq_callback *cb =
+			dev->enq_cbs[qp_id];
+
+		do {
+			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
+					cb->arg);
+			cb = cb->next;
+		} while (cb != NULL);
+	}
+#endif
 	return (*dev->enqueue_burst)(
 			dev->data->queue_pairs[qp_id], ops, nb_ops);
 }
@@ -1296,6 +1347,78 @@ struct rte_cryptodev_asym_session *
 	struct rte_cryptodev_sym_session *sess, union rte_crypto_sym_ofs ofs,
 	struct rte_crypto_sym_vec *vec);
 
+
+/**
+ * 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.
+ */
+
+const struct rte_cryptodev_enq_callback * __rte_experimental
+rte_cryptodev_add_enq_callback(uint8_t dev_id,
+			       uint16_t qp_id,
+			       rte_cryptodev_enq_cb_fn cb_fn,
+			       void *cb_arg);
+
+
+/**
+ * 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().
+ *
+ * Note: the callback is removed from the callback list but it isn't freed
+ * since the it may still be in use. The memory for the callback can be
+ * subsequently freed back by the application by calling rte_free().
+ *
+ * - Immediately - if the crypto device is stopped, or user knows that
+ *   no callbacks are in flight e.g. if called from the thread doing enq/deq
+ *   on that queue.
+ *
+ * - After a short delay - where the delay is sufficient to allow any
+ *   in-flight callbacks to complete.
+ *
+ * @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.
+ */
+
+int __rte_experimental
+rte_cryptodev_remove_enq_callback(uint8_t dev_id,
+				  uint16_t qp_id,
+				  const struct rte_cryptodev_enq_callback *cb);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map
index 6e41b4b..e8d3e77 100644
--- a/lib/librte_cryptodev/rte_cryptodev_version.map
+++ b/lib/librte_cryptodev/rte_cryptodev_version.map
@@ -58,9 +58,11 @@ DPDK_20.0 {
 	local: *;
 };
 
+
 EXPERIMENTAL {
 	global:
 
+	rte_cryptodev_add_enq_callback;
 	rte_cryptodev_asym_capability_get;
 	rte_cryptodev_asym_get_header_session_size;
 	rte_cryptodev_asym_get_private_session_size;
@@ -71,6 +73,7 @@ EXPERIMENTAL {
 	rte_cryptodev_asym_session_init;
 	rte_cryptodev_asym_xform_capability_check_modlen;
 	rte_cryptodev_asym_xform_capability_check_optype;
+	rte_cryptodev_remove_enq_callback;
 	rte_cryptodev_sym_cpu_crypto_process;
 	rte_cryptodev_sym_get_existing_header_session_size;
 	rte_cryptodev_sym_session_get_user_data;
-- 
1.9.1

Re: [dpdk-dev] [PATCH] cryptodev: add support for user callback functions

[Ananyev, Konstantin]

 
> 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.
> 
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  config/common_base                             |   1 +
>  lib/librte_cryptodev/rte_cryptodev.c           | 120 ++++++++++++++++++++++++
>  lib/librte_cryptodev/rte_cryptodev.h           | 125 ++++++++++++++++++++++++-
>  lib/librte_cryptodev/rte_cryptodev_version.map |   3 +
>  4 files changed, 248 insertions(+), 1 deletion(-)
> 
> diff --git a/config/common_base b/config/common_base
> index 9ec689d..6f93acb 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -586,6 +586,7 @@ CONFIG_RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC=y
>  #
>  CONFIG_RTE_LIBRTE_CRYPTODEV=y
>  CONFIG_RTE_CRYPTO_MAX_DEVS=64
> +CONFIG_RTE_CRYPTODEV_CALLBACKS=y
> 
>  #
>  # Compile PMD for ARMv8 Crypto device
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 2849b2e..5a4cba9 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -56,6 +56,9 @@
>  /* spinlock for crypto device callbacks */
>  static rte_spinlock_t rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER;
> 
> +/* spinlock for crypto device enq callbacks */
> +static rte_spinlock_t rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER;
> +
> 
>  /**
>   * The user application callback description.
> @@ -1256,6 +1259,123 @@ struct rte_cryptodev *
>  	rte_spinlock_unlock(&rte_cryptodev_cb_lock);
>  }
> 
> +const struct rte_cryptodev_enq_callback *__rte_experimental
> +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> +			       uint16_t qp_id,
> +			       rte_cryptodev_enq_cb_fn cb_fn,
> +			       void *cb_arg)
> +{
> +	struct rte_cryptodev *dev;
> +	struct rte_cryptodev_enq_callback *cb, *tail;
> +
> +	if (!cb_fn)
> +		return NULL;
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, 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;
> +	}
> +
> +	cb->fn = cb_fn;
> +	cb->arg = cb_arg;
> +
> +	rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> +	if (dev->enq_cbs == NULL) {
> +		dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
> +					   dev->data->nb_queue_pairs, 0);
> +		if (dev->enq_cbs == NULL) {
> +			CDEV_LOG_ERR("Failed to allocate memory for callbacks");
> +			rte_errno = ENOMEM;
> +			rte_free(cb);
> +			return NULL;
> +		}
> +	}
> +
> +	/* Add the callbacks in fifo order. */
> +	tail = dev->enq_cbs[qp_id];
> +	if (tail) {
> +		while (tail->next)
> +			tail = tail->next;
> +		tail->next = cb;
> +	} else
> +		dev->enq_cbs[qp_id] = cb;
> +
> +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +
> +	return cb;
> +}
> +
> +int __rte_experimental
> +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +				  uint16_t qp_id,
> +				  const struct rte_cryptodev_enq_callback *cb)
> +{
> +	struct rte_cryptodev *dev;
> +	struct rte_cryptodev_enq_callback **prev_cb, *curr_cb;
> +	uint16_t qp;
> +	int free_mem = 1;
> +	int ret = -EINVAL;
> +
> +	if (!cb)
> +		return ret;
> +
> +	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
> +		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, 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_enq_cb_lock);
> +	if (dev->enq_cbs == NULL) {
> +		rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +		return ret;
> +	}
> +
> +	prev_cb = &dev->enq_cbs[qp_id];
> +	for (; *prev_cb != NULL; prev_cb = &curr_cb->next) {
> +		curr_cb = *prev_cb;
> +		if (curr_cb == cb) {
> +			/* Remove the user cb from the callback list. */
> +			*prev_cb = curr_cb->next;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	for (qp = 0; qp < dev->data->nb_queue_pairs; qp++)
> +		if (dev->enq_cbs[qp] != NULL) {
> +			free_mem = 0;
> +			break;
> +		}
> +
> +	if (free_mem) {
> +		rte_free(dev->enq_cbs);
> +		dev->enq_cbs = NULL;
> +	}
> +
> +	rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> +
> +	return ret;
> +}

Pls don't re-implement pitfall we have with ethdev rx/tx callback:
right now with ethdev approach it is impossible to know when it is safe to free
removed CB and free used by CB resources if any.
Unless you do dev_stop() of course.  
So majority of ethdev CB uses have to either leave CB allocated forever
after removal (memory leak), or invent some specific sync methods underneath
that API.
We probably need to introduce some sync mechanism here straightway (RCU or so)
to avoid same issue. 

> 
>  int
>  rte_cryptodev_sym_session_init(uint8_t dev_id,
> diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
> index f4846d2..2cf466b 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> @@ -518,6 +518,32 @@ struct rte_cryptodev_qp_conf {
>  };
> 
>  /**
> + * 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_enq_cb_fn)(uint16_t dev_id, uint16_t qp_id,
> +		struct rte_crypto_op **ops, uint16_t nb_ops, void *user_param);
> +
> +/**
>   * Typedef for application callback function to be registered by application
>   * software for notification of device events
>   *
> @@ -800,7 +826,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. */
> @@ -817,6 +842,17 @@ 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);
> 
> +/**
> + * @internal
> + * Structure used to hold information about the callbacks to be called for a
> + * queue pair on enqueue.
> + */
> +struct rte_cryptodev_enq_callback {
> +	struct rte_cryptodev_enq_callback *next;
> +	rte_cryptodev_enq_cb_fn fn;
> +	void *arg;
> +};
> +
>  /** The data structure associated with each crypto device. */
>  struct rte_cryptodev {
>  	dequeue_pkt_burst_t dequeue_burst;
> @@ -839,6 +875,9 @@ struct rte_cryptodev {
>  	struct rte_cryptodev_cb_list link_intr_cbs;
>  	/**< User application callback for interrupts if present */
> 
> +	struct rte_cryptodev_enq_callback **enq_cbs;
> +	/**< User application callback for pre enqueue processing */
> +

Why not to put it at the very end of the structure?
Would be safer in terms of ABI breakage problem, etc.


>  	void *security_ctx;
>  	/**< Context for security ops */
> 
> @@ -966,6 +1005,18 @@ struct rte_cryptodev_data {
>  {
>  	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> 
> +#ifdef RTE_CRYPTODEV_CALLBACKS
> +	if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
> +		struct rte_cryptodev_enq_callback *cb =
> +			dev->enq_cbs[qp_id];
> +
> +		do {
> +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> +					cb->arg);
> +			cb = cb->next;
> +		} while (cb != NULL);
> +	}
> +#endif
>  	return (*dev->enqueue_burst)(
>  			dev->data->queue_pairs[qp_id], ops, nb_ops);
>  }
> @@ -1296,6 +1347,78 @@ struct rte_cryptodev_asym_session *
>  	struct rte_cryptodev_sym_session *sess, union rte_crypto_sym_ofs ofs,
>  	struct rte_crypto_sym_vec *vec);
> 
> +
> +/**
> + * 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.
> + */
> +
> +const struct rte_cryptodev_enq_callback * __rte_experimental
> +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> +			       uint16_t qp_id,
> +			       rte_cryptodev_enq_cb_fn cb_fn,
> +			       void *cb_arg);
> +
> +
> +/**
> + * 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().
> + *
> + * Note: the callback is removed from the callback list but it isn't freed
> + * since the it may still be in use. The memory for the callback can be
> + * subsequently freed back by the application by calling rte_free().
> + *
> + * - Immediately - if the crypto device is stopped, or user knows that
> + *   no callbacks are in flight e.g. if called from the thread doing enq/deq
> + *   on that queue.
> + *
> + * - After a short delay - where the delay is sufficient to allow any
> + *   in-flight callbacks to complete.
> + *
> + * @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.
> + */
> +
> +int __rte_experimental
> +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> +				  uint16_t qp_id,
> +				  const struct rte_cryptodev_enq_callback *cb);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map
> index 6e41b4b..e8d3e77 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> @@ -58,9 +58,11 @@ DPDK_20.0 {
>  	local: *;
>  };
> 
> +
>  EXPERIMENTAL {
>  	global:
> 
> +	rte_cryptodev_add_enq_callback;
>  	rte_cryptodev_asym_capability_get;
>  	rte_cryptodev_asym_get_header_session_size;
>  	rte_cryptodev_asym_get_private_session_size;
> @@ -71,6 +73,7 @@ EXPERIMENTAL {
>  	rte_cryptodev_asym_session_init;
>  	rte_cryptodev_asym_xform_capability_check_modlen;
>  	rte_cryptodev_asym_xform_capability_check_optype;
> +	rte_cryptodev_remove_enq_callback;
>  	rte_cryptodev_sym_cpu_crypto_process;
>  	rte_cryptodev_sym_get_existing_header_session_size;
>  	rte_cryptodev_sym_session_get_user_data;
> --
> 1.9.1

Re: [dpdk-dev] [PATCH] cryptodev: add support for user callback functions

[Gujjar, Abhinandan S]

Hi Konstantin,

Please find the comments inline.

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, April 24, 2020 8:40 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; jerinj@marvell.com; akhil.goyal@nxp.com;
> dev@dpdk.org
> Cc: Vangati, Narender <narender.vangati@intel.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] cryptodev: add support for user callback
> functions
> 
> 
> > 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.
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > ---
> >  config/common_base                             |   1 +
> >  lib/librte_cryptodev/rte_cryptodev.c           | 120 ++++++++++++++++++++++++
> >  lib/librte_cryptodev/rte_cryptodev.h           | 125
> ++++++++++++++++++++++++-
> >  lib/librte_cryptodev/rte_cryptodev_version.map |   3 +
> >  4 files changed, 248 insertions(+), 1 deletion(-)
> >
> > diff --git a/config/common_base b/config/common_base index
> > 9ec689d..6f93acb 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -586,6 +586,7 @@
> CONFIG_RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC=y
> >  #
> >  CONFIG_RTE_LIBRTE_CRYPTODEV=y
> >  CONFIG_RTE_CRYPTO_MAX_DEVS=64
> > +CONFIG_RTE_CRYPTODEV_CALLBACKS=y
> >
> >  #
> >  # Compile PMD for ARMv8 Crypto device diff --git
> > a/lib/librte_cryptodev/rte_cryptodev.c
> > b/lib/librte_cryptodev/rte_cryptodev.c
> > index 2849b2e..5a4cba9 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -56,6 +56,9 @@
> >  /* spinlock for crypto device callbacks */  static rte_spinlock_t
> > rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER;
> >
> > +/* spinlock for crypto device enq callbacks */ static rte_spinlock_t
> > +rte_cryptodev_enq_cb_lock = RTE_SPINLOCK_INITIALIZER;
> > +
> >
> >  /**
> >   * The user application callback description.
> > @@ -1256,6 +1259,123 @@ struct rte_cryptodev *
> > rte_spinlock_unlock(&rte_cryptodev_cb_lock);
> >  }
> >
> > +const struct rte_cryptodev_enq_callback *__rte_experimental
> > +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> > +       uint16_t qp_id,
> > +       rte_cryptodev_enq_cb_fn cb_fn,
> > +       void *cb_arg)
> > +{
> > +struct rte_cryptodev *dev;
> > +struct rte_cryptodev_enq_callback *cb, *tail;
> > +
> > +if (!cb_fn)
> > +return NULL;
> > +
> > +if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) { CDEV_LOG_ERR("Invalid
> > +dev_id=%" PRIu8, 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; }
> > +
> > +cb->fn = cb_fn;
> > +cb->arg = cb_arg;
> > +
> > +rte_spinlock_lock(&rte_cryptodev_enq_cb_lock);
> > +if (dev->enq_cbs == NULL) {
> > +dev->enq_cbs = rte_zmalloc(NULL, sizeof(cb) *
> > +   dev->data->nb_queue_pairs, 0);
> > +if (dev->enq_cbs == NULL) {
> > +CDEV_LOG_ERR("Failed to allocate memory for callbacks"); rte_errno =
> > +ENOMEM; rte_free(cb); return NULL; } }
> > +
> > +/* Add the callbacks in fifo order. */ tail = dev->enq_cbs[qp_id]; if
> > +(tail) { while (tail->next) tail = tail->next;
> > +tail->next = cb;
> > +} else
> > +dev->enq_cbs[qp_id] = cb;
> > +
> > +rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +
> > +return cb;
> > +}
> > +
> > +int __rte_experimental
> > +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +  uint16_t qp_id,
> > +  const struct rte_cryptodev_enq_callback *cb) { struct rte_cryptodev
> > +*dev; struct rte_cryptodev_enq_callback **prev_cb, *curr_cb; uint16_t
> > +qp; int free_mem = 1; int ret = -EINVAL;
> > +
> > +if (!cb)
> > +return ret;
> > +
> > +if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) { CDEV_LOG_ERR("Invalid
> > +dev_id=%" PRIu8, 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_enq_cb_lock);
> > +if (dev->enq_cbs == NULL) {
> > +rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +return ret;
> > +}
> > +
> > +prev_cb = &dev->enq_cbs[qp_id];
> > +for (; *prev_cb != NULL; prev_cb = &curr_cb->next) { curr_cb =
> > +*prev_cb; if (curr_cb == cb) {
> > +/* Remove the user cb from the callback list. */ *prev_cb =
> > +curr_cb->next; ret = 0; break; } }
> > +
> > +for (qp = 0; qp < dev->data->nb_queue_pairs; qp++) if
> > +(dev->enq_cbs[qp] != NULL) { free_mem = 0; break; }
> > +
> > +if (free_mem) {
> > +rte_free(dev->enq_cbs);
> > +dev->enq_cbs = NULL;
> > +}
> > +
> > +rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock);
> > +
> > +return ret;
> > +}
> 
> Pls don't re-implement pitfall we have with ethdev rx/tx callback:
> right now with ethdev approach it is impossible to know when it is safe to free
> removed CB and free used by CB resources if any.
> Unless you do dev_stop() of course.
> So majority of ethdev CB uses have to either leave CB allocated forever after
> removal (memory leak), or invent some specific sync methods underneath that
> API.
> We probably need to introduce some sync mechanism here straightway (RCU or
> so) to avoid same issue.
Agree. I will try to explore more on sync mechanism using RCU/other options.

> 
> >
> >  int
> >  rte_cryptodev_sym_session_init(uint8_t dev_id, diff --git
> > a/lib/librte_cryptodev/rte_cryptodev.h
> > b/lib/librte_cryptodev/rte_cryptodev.h
> > index f4846d2..2cf466b 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.h
> > +++ b/lib/librte_cryptodev/rte_cryptodev.h
> > @@ -518,6 +518,32 @@ struct rte_cryptodev_qp_conf {  };
> >
> >  /**
> > + * 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.
> > + *
> > + * @paramdev_idThe identifier of the device.
> > + * @paramqp_idThe 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*.
> > + * @paramopsThe address of an array of *nb_ops* pointers  *to
> > +*rte_crypto_op* structures which contain  *the crypto operations to
> > +be processed.
> > + * @paramnb_opsThe number of operations to process.
> > + * @paramuser_paramThe arbitrary user parameter passed in by the
> > +*application when the callback was originally  *registered.
> > + * @returnThe number of ops to be enqueued to the  *crypto device.
> > + */
> > +typedef uint16_t (*rte_cryptodev_enq_cb_fn)(uint16_t dev_id, uint16_t
> > +qp_id, struct rte_crypto_op **ops, uint16_t nb_ops, void
> > +*user_param);
> > +
> > +/**
> >   * Typedef for application callback function to be registered by application
> >   * software for notification of device events
> >   *
> > @@ -800,7 +826,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. */ @@ -817,6 +842,17 @@ 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);
> >
> > +/**
> > + * @internal
> > + * Structure used to hold information about the callbacks to be
> > +called for a
> > + * queue pair on enqueue.
> > + */
> > +struct rte_cryptodev_enq_callback {
> > +struct rte_cryptodev_enq_callback *next; rte_cryptodev_enq_cb_fn fn;
> > +void *arg; };
> > +
> >  /** The data structure associated with each crypto device. */  struct
> > rte_cryptodev {  dequeue_pkt_burst_t dequeue_burst; @@ -839,6 +875,9
> > @@ struct rte_cryptodev {  struct rte_cryptodev_cb_list link_intr_cbs;
> > /**< User application callback for interrupts if present */
> >
> > +struct rte_cryptodev_enq_callback **enq_cbs; /**< User application
> > +callback for pre enqueue processing */
> > +
> 
> Why not to put it at the very end of the structure?
> Would be safer in terms of ABI breakage problem, etc.
Ok
> 
> 
> >  void *security_ctx;
> >  /**< Context for security ops */
> >
> > @@ -966,6 +1005,18 @@ struct rte_cryptodev_data {  {  struct
> > rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> >
> > +#ifdef RTE_CRYPTODEV_CALLBACKS
> > +if (unlikely(dev->enq_cbs != NULL && dev->enq_cbs[qp_id] != NULL)) {
> > +struct rte_cryptodev_enq_callback *cb =
> > +dev->enq_cbs[qp_id];
> > +
> > +do {
> > +nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > +cb->arg);
> > +cb = cb->next;
> > +} while (cb != NULL);
> > +}
> > +#endif
> >  return (*dev->enqueue_burst)(
> >  dev->data->queue_pairs[qp_id], ops, nb_ops);  } @@ -1296,6 +1347,78
> > @@ struct rte_cryptodev_asym_session *  struct
> > rte_cryptodev_sym_session *sess, union rte_crypto_sym_ofs ofs,  struct
> > rte_crypto_sym_vec *vec);
> >
> > +
> > +/**
> > + * 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.
> > + *
> > + * @paramdev_idThe identifier of the device.
> > + * @paramqp_idThe 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*.
> > + * @paramcb_fnThe callback function
> > + * @paramcb_argA 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.
> > + */
> > +
> > +const struct rte_cryptodev_enq_callback * __rte_experimental
> > +rte_cryptodev_add_enq_callback(uint8_t dev_id,
> > +       uint16_t qp_id,
> > +       rte_cryptodev_enq_cb_fn cb_fn,
> > +       void *cb_arg);
> > +
> > +
> > +/**
> > + * 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().
> > + *
> > + * Note: the callback is removed from the callback list but it isn't
> > +freed
> > + * since the it may still be in use. The memory for the callback can
> > +be
> > + * subsequently freed back by the application by calling rte_free().
> > + *
> > + * - Immediately - if the crypto device is stopped, or user knows that
> > + *   no callbacks are in flight e.g. if called from the thread doing enq/deq
> > + *   on that queue.
> > + *
> > + * - After a short delay - where the delay is sufficient to allow any
> > + *   in-flight callbacks to complete.
> > + *
> > + * @paramdev_idThe identifier of the device.
> > + * @paramqp_idThe 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*.
> > + * @paramcbPointer 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.
> > + */
> > +
> > +int __rte_experimental
> > +rte_cryptodev_remove_enq_callback(uint8_t dev_id,
> > +  uint16_t qp_id,
> > +  const struct rte_cryptodev_enq_callback *cb);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
> > b/lib/librte_cryptodev/rte_cryptodev_version.map
> > index 6e41b4b..e8d3e77 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> > +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> > @@ -58,9 +58,11 @@ DPDK_20.0 {
> >  local: *;
> >  };
> >
> > +
> >  EXPERIMENTAL {
> >  global:
> >
> > +rte_cryptodev_add_enq_callback;
> >  rte_cryptodev_asym_capability_get;
> >  rte_cryptodev_asym_get_header_session_size;
> >  rte_cryptodev_asym_get_private_session_size;
> > @@ -71,6 +73,7 @@ EXPERIMENTAL {
> >  rte_cryptodev_asym_session_init;
> >  rte_cryptodev_asym_xform_capability_check_modlen;
> >  rte_cryptodev_asym_xform_capability_check_optype;
> > +rte_cryptodev_remove_enq_callback;
> >  rte_cryptodev_sym_cpu_crypto_process;
> >  rte_cryptodev_sym_get_existing_header_session_size;
> >  rte_cryptodev_sym_session_get_user_data;
> > --
> > 1.9.1
> 

Re: [dpdk-dev] [PATCH] cryptodev: add support for user callback functions

[Akhil Goyal]

Hi Abhinandan,

Are you planning to send update for this patch in 20.08 time line?
Or is it pushed for 20.11?

Regards,
Akhil

Re: [dpdk-dev] [PATCH] cryptodev: add support for user callback functions

[Gujjar, Abhinandan S]

Hi Akhil,

We can target this for 20.11.

Thanks
Abhinandan

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Wednesday, July 1, 2020 12:43 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; jerinj@marvell.com; dev@dpdk.org
> Cc: Vangati, Narender <narender.vangati@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] cryptodev: add support for user callback
> functions
> 
> Hi Abhinandan,
> 
> Are you planning to send update for this patch in 20.08 time line?
> Or is it pushed for 20.11?
> 
> Regards,
> Akhil