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

["Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>]

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
>