From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 18962A0350; Thu, 7 May 2020 16:53:23 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DB0051C1A7; Thu, 7 May 2020 16:53:21 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id ED6B91C127 for ; Thu, 7 May 2020 16:53:19 +0200 (CEST) IronPort-SDR: pTO6okKiDRAOXV+QSZmWi+xtPUbefDaNBXz3R2QJQe0msuwUwJAff3iiOLpO40JplQN6SWPnaF mxKA32CFgP7Q== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2020 07:53:18 -0700 IronPort-SDR: f8y+OUws1MbjMueYczHC1FdU/2RIhnvdj9Lw+xFq/mEnxPog9Hv1FVYcmCMvmZs5tOoZcIK0XM M4c0puTrV37w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,364,1583222400"; d="scan'208";a="296558136" Received: from kmsmsx156.gar.corp.intel.com ([172.21.138.133]) by orsmga008.jf.intel.com with ESMTP; 07 May 2020 07:53:17 -0700 Received: from pgsmsx102.gar.corp.intel.com ([169.254.6.12]) by KMSMSX156.gar.corp.intel.com ([169.254.1.224]) with mapi id 14.03.0439.000; Thu, 7 May 2020 22:46:39 +0800 From: "Gujjar, Abhinandan S" To: "Ananyev, Konstantin" , "Doherty, Declan" , "jerinj@marvell.com" , "akhil.goyal@nxp.com" , "dev@dpdk.org" CC: "Vangati, Narender" Thread-Topic: [dpdk-dev] [PATCH] cryptodev: add support for user callback functions Thread-Index: AQHWGkp99hXIT9VE1kucEyA9EHGsh6icxtlg Date: Thu, 7 May 2020 14:46:38 +0000 Message-ID: <5612CB344B05EE4F95FC5B729939F7807E27A446@PGSMSX102.gar.corp.intel.com> References: <1587371610-19878-1-git-send-email-abhinandan.gujjar@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [172.30.20.205] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] cryptodev: add support for user callback functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, Please find the comments inline. > -----Original Message----- > From: Ananyev, Konstantin > Sent: Friday, April 24, 2020 8:40 PM > To: Gujjar, Abhinandan S ; Doherty, Declan > ; jerinj@marvell.com; akhil.goyal@nxp.com; > dev@dpdk.org > Cc: Vangati, Narender ; Gujjar, Abhinandan S > > Subject: RE: [dpdk-dev] [PATCH] cryptodev: add support for user callback > functions >=20 >=20 > > 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 > > --- > > 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=3Dy > > # > > CONFIG_RTE_LIBRTE_CRYPTODEV=3Dy > > CONFIG_RTE_CRYPTO_MAX_DEVS=3D64 > > +CONFIG_RTE_CRYPTODEV_CALLBACKS=3Dy > > > > # > > # 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 =3D RTE_SPINLOCK_INITIALIZER; > > > > +/* spinlock for crypto device enq callbacks */ static rte_spinlock_t > > +rte_cryptodev_enq_cb_lock =3D 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=3D%" PRIu8, dev_id); return NULL; } > > + > > +dev =3D &rte_crypto_devices[dev_id]; > > +if (qp_id >=3D dev->data->nb_queue_pairs) { CDEV_LOG_ERR("Invalid > > +queue_pair_id=3D%d", qp_id); return NULL; } > > + > > +cb =3D rte_zmalloc(NULL, sizeof(*cb), 0); if (cb =3D=3D NULL) { > > +CDEV_LOG_ERR("Failed to allocate memory for callback on " > > + "dev=3D%d, queue_pair_id=3D%d", dev_id, qp_id); rte_errno =3D ENO= MEM; > > +return NULL; } > > + > > +cb->fn =3D cb_fn; > > +cb->arg =3D cb_arg; > > + > > +rte_spinlock_lock(&rte_cryptodev_enq_cb_lock); > > +if (dev->enq_cbs =3D=3D NULL) { > > +dev->enq_cbs =3D rte_zmalloc(NULL, sizeof(cb) * > > + dev->data->nb_queue_pairs, 0); > > +if (dev->enq_cbs =3D=3D NULL) { > > +CDEV_LOG_ERR("Failed to allocate memory for callbacks"); rte_errno =3D > > +ENOMEM; rte_free(cb); return NULL; } } > > + > > +/* Add the callbacks in fifo order. */ tail =3D dev->enq_cbs[qp_id]; i= f > > +(tail) { while (tail->next) tail =3D tail->next; > > +tail->next =3D cb; > > +} else > > +dev->enq_cbs[qp_id] =3D 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 =3D 1; int ret =3D -EINVAL; > > + > > +if (!cb) > > +return ret; > > + > > +if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) { CDEV_LOG_ERR("Invalid > > +dev_id=3D%" PRIu8, dev_id); return ret; } > > + > > +dev =3D &rte_crypto_devices[dev_id]; > > +if (qp_id >=3D dev->data->nb_queue_pairs) { CDEV_LOG_ERR("Invalid > > +queue_pair_id=3D%d", qp_id); return ret; } > > + > > +rte_spinlock_lock(&rte_cryptodev_enq_cb_lock); > > +if (dev->enq_cbs =3D=3D NULL) { > > +rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock); > > +return ret; > > +} > > + > > +prev_cb =3D &dev->enq_cbs[qp_id]; > > +for (; *prev_cb !=3D NULL; prev_cb =3D &curr_cb->next) { curr_cb =3D > > +*prev_cb; if (curr_cb =3D=3D cb) { > > +/* Remove the user cb from the callback list. */ *prev_cb =3D > > +curr_cb->next; ret =3D 0; break; } } > > + > > +for (qp =3D 0; qp < dev->data->nb_queue_pairs; qp++) if > > +(dev->enq_cbs[qp] !=3D NULL) { free_mem =3D 0; break; } > > + > > +if (free_mem) { > > +rte_free(dev->enq_cbs); > > +dev->enq_cbs =3D NULL; > > +} > > + > > +rte_spinlock_unlock(&rte_cryptodev_enq_cb_lock); > > + > > +return ret; > > +} >=20 > 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 t= o 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 a= fter > removal (memory leak), or invent some specific sync methods underneath th= at > API. > We probably need to introduce some sync mechanism here straightway (RCU o= r > so) to avoid same issue. Agree. I will try to explore more on sync mechanism using RCU/other options= . >=20 > > > > 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 processin= g. > > + * > > + * @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 appli= cation > > * 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 */ > > + >=20 > Why not to put it at the very end of the structure? > Would be safer in terms of ABI breakage problem, etc. Ok >=20 >=20 > > void *security_ctx; > > /**< Context for security ops */ > > > > @@ -966,6 +1005,18 @@ struct rte_cryptodev_data { { struct > > rte_cryptodev *dev =3D &rte_cryptodevs[dev_id]; > > > > +#ifdef RTE_CRYPTODEV_CALLBACKS > > +if (unlikely(dev->enq_cbs !=3D NULL && dev->enq_cbs[qp_id] !=3D NULL))= { > > +struct rte_cryptodev_enq_callback *cb =3D > > +dev->enq_cbs[qp_id]; > > + > > +do { > > +nb_ops =3D cb->fn(dev_id, qp_id, ops, nb_ops, > > +cb->arg); > > +cb =3D cb->next; > > +} while (cb !=3D 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 p= air. > > + * > > + * 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 e= nq/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 callb= ack > > + * 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 >=20