[dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Arek Kusztal]

From: Fiona Trahe <fiona.trahe@intel.com>

This patch adds function that can check if queue pair
was already setup. This may be useful when dealing with
multi process approach in cryptodev.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
v2:
- changed return values
- added function to map file

 lib/librte_cryptodev/rte_cryptodev.c           | 29 ++++++++++++++++++++++++++
 lib/librte_cryptodev/rte_cryptodev.h           | 17 +++++++++++++++
 lib/librte_cryptodev/rte_cryptodev_version.map |  3 +++
 3 files changed, 49 insertions(+)

diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index e37b83a..6f556c3 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -1080,6 +1080,35 @@ rte_cryptodev_close(uint8_t dev_id)
 }
 
 int
+rte_cryptodev_get_qp_status(uint8_t dev_id, uint16_t queue_pair_id)
+{
+	struct rte_cryptodev *dev;
+
+	if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) {
+		CDEV_LOG_ERR("Invalid dev_id=%" PRIu8, dev_id);
+		return -EINVAL;
+	}
+
+	dev = &rte_crypto_devices[dev_id];
+	if (queue_pair_id >= dev->data->nb_queue_pairs) {
+		CDEV_LOG_ERR("Invalid queue_pair_id=%d", queue_pair_id);
+		return -EINVAL;
+	}
+	void **qps = dev->data->queue_pairs;
+
+	if (qps[queue_pair_id])	{
+		CDEV_LOG_DEBUG("qp %d on dev %d is initialised",
+			queue_pair_id, dev_id);
+		return 1;
+	}
+
+	CDEV_LOG_DEBUG("qp %d on dev %d is not initialised",
+		queue_pair_id, dev_id);
+
+	return 0;
+}
+
+int
 rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
 		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id)
 
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 4aaee73..7b3ebc2 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -727,6 +727,23 @@ rte_cryptodev_queue_pair_setup(uint8_t dev_id, uint16_t queue_pair_id,
 		const struct rte_cryptodev_qp_conf *qp_conf, int socket_id);
 
 /**
+ * Get the status of queue pairs setup on a specific crypto device
+ *
+ * @param	dev_id		Crypto device identifier.
+ * @param	queue_pair_id	The index of the queue pairs to set up. The
+ *				value must be in the range [0, nb_queue_pair
+ *				- 1] previously supplied to
+ *				rte_cryptodev_configure().
+ * @return
+ *   - 0: qp was not configured
+ *	 - 1: qp was configured
+ *	 - -EINVAL: device was not configured
+ */
+__rte_experimental
+int
+rte_cryptodev_get_qp_status(uint8_t dev_id, uint16_t queue_pair_id);
+
+/**
  * Get the number of queue pairs on a specific crypto device
  *
  * @param	dev_id		Crypto device identifier.
diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map
index 07a2d2f..a7a78dc 100644
--- a/lib/librte_cryptodev/rte_cryptodev_version.map
+++ b/lib/librte_cryptodev/rte_cryptodev_version.map
@@ -103,4 +103,7 @@ EXPERIMENTAL {
 	__rte_cryptodev_trace_asym_session_clear;
 	__rte_cryptodev_trace_dequeue_burst;
 	__rte_cryptodev_trace_enqueue_burst;
+
+	# added in 20.08
+	rte_cryptodev_get_qp_status;
 };
-- 
2.1.0

Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Akhil Goyal]

Hi Arek/Fiona,
> 
> From: Fiona Trahe <fiona.trahe@intel.com>
> 
> This patch adds function that can check if queue pair
> was already setup. This may be useful when dealing with
> multi process approach in cryptodev.
> 
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
> v2:
> - changed return values
> - added function to map file
> 
Please mark the previous versions as superseded in the patchwork.
I see that previous version had 2 patches one was acked and so you skipped that in this version.
Or you do not want that patch?
http://patches.dpdk.org/patch/71212/

Those patches could have been sent separately but if you are sending the patches in a series,
Then next version should have all the patches unless you want to drop some of the patches.

Acked-by: Akhil Goyal <akhil.goyal@nxp.com>


Regards,
Akhil

Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Kusztal, ArkadiuszX]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, July 2, 2020 7:51 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v2] cryptodev: add function to check if qp was setup
> 
> Hi Arek/Fiona,
> >
> > From: Fiona Trahe <fiona.trahe@intel.com>
> >
> > This patch adds function that can check if queue pair was already
> > setup. This may be useful when dealing with multi process approach in
> > cryptodev.
> >
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> > v2:
> > - changed return values
> > - added function to map file
> >
> Please mark the previous versions as superseded in the patchwork.
> I see that previous version had 2 patches one was acked and so you skipped
> that in this version.
> Or you do not want that patch?
> http://patches.dpdk.org/patch/71212/
[Arek] - sorry for that, both patches should be included, should I send v2 with this one ? Or both with v3?
> 
> Those patches could have been sent separately but if you are sending the
> patches in a series, Then next version should have all the patches unless you
> want to drop some of the patches.
> 
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> 
> 
> Regards,
> Akhil

Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Akhil Goyal]

> > Hi Arek/Fiona,
> > >
> > > From: Fiona Trahe <fiona.trahe@intel.com>
> > >
> > > This patch adds function that can check if queue pair was already
> > > setup. This may be useful when dealing with multi process approach in
> > > cryptodev.
> > >
> > > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > > ---
> > > v2:
> > > - changed return values
> > > - added function to map file
> > >
> > Please mark the previous versions as superseded in the patchwork.
> > I see that previous version had 2 patches one was acked and so you skipped
> > that in this version.
> > Or you do not want that patch?
> >
> http://patches.dpdk.org/patch/71212/
> [Arek] - sorry for that, both patches should be included, should I send v2 with
> this one ? Or both with v3?

Please take care of this in future. I have applied both.
> >
> > Those patches could have been sent separately but if you are sending the
> > patches in a series, Then next version should have all the patches unless you
> > want to drop some of the patches.
> >
> > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> >
> >

Applied to dpdk-next-crypto

Thanks.

Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Thomas Monjalon]

24/06/2020 16:26, Arek Kusztal:
> From: Fiona Trahe <fiona.trahe@intel.com>
> 
> This patch adds function that can check if queue pair
> was already setup. This may be useful when dealing with
> multi process approach in cryptodev.

That's all? No more justification?
No usage in example apps?
No addition in test apps?
Is it needed for the application?

I don't know cryptodev enough, but I can tell with ethdev experience
that we are a lot more demanding when adding a new API in ethdev.
We are still fixing the API errors done years ago in ethdev,
and it is very difficult to deprecate what was used in the past.
I hope my fear is wrong and you are not doing the same errors
as we did in ethdev, it would be a pity.

Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Akhil Goyal]

Hi Thomas,
> 
> 24/06/2020 16:26, Arek Kusztal:
> > From: Fiona Trahe <fiona.trahe@intel.com>
> >
> > This patch adds function that can check if queue pair
> > was already setup. This may be useful when dealing with
> > multi process approach in cryptodev.
> 
> That's all? No more justification?
> No usage in example apps?
> No addition in test apps?
> Is it needed for the application?
The application is in review stage right now.
http://patches.dpdk.org/patch/72156/

The current patch is a cryptodev patch which should be part of RC1.
The app is targeted for RC2.
The API explanation in rte_cryptodev.h is I think sufficient and the usage will be
Demonstrated in the app.
/**
+ * Get the status of queue pairs setup on a specific crypto device
+ *
+ * @param	dev_id		Crypto device identifier.
+ * @param	queue_pair_id	The index of the queue pairs to set up. The
+ *				value must be in the range [0, nb_queue_pair
+ *				- 1] previously supplied to
+ *				rte_cryptodev_configure().
+ * @return
+ *   - 0: qp was not configured
+ *	 - 1: qp was configured
+ *	 - -EINVAL: device was not configured
+ */
> 
> I don't know cryptodev enough, but I can tell with ethdev experience
> that we are a lot more demanding when adding a new API in ethdev.
> We are still fixing the API errors done years ago in ethdev,
> and it is very difficult to deprecate what was used in the past.
> I hope my fear is wrong and you are not doing the same errors
> as we did in ethdev, it would be a pity.
> 

Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup

[Trahe, Fiona]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, July 8, 2020 2:38 PM
> To: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; orika@mellanox.com; jerinj@marvell.com;
> stephen@networkplumber.org; olivier.matz@6wind.com; hemant.agrawal@nxp.com; mdr@ashroe.eu
> Subject: Re: [dpdk-dev] [PATCH v2] cryptodev: add function to check if qp was setup
> 
> 24/06/2020 16:26, Arek Kusztal:
> > From: Fiona Trahe <fiona.trahe@intel.com>
> >
> > This patch adds function that can check if queue pair
> > was already setup. This may be useful when dealing with
> > multi process approach in cryptodev.
> 
> That's all? No more justification?
> No usage in example apps?
> No addition in test apps?
> Is it needed for the application?
> 
> I don't know cryptodev enough, but I can tell with ethdev experience
> that we are a lot more demanding when adding a new API in ethdev.
> We are still fixing the API errors done years ago in ethdev,
> and it is very difficult to deprecate what was used in the past.
> I hope my fear is wrong and you are not doing the same errors
> as we did in ethdev, it would be a pity.

This is used in a new example app which we expect will be applied in rc2.
There are use-cases where the primary process creates and configure the device and queue-pairs
and a secondary process uses the queue-pair on the data-path.
It seems safer to provide the secondary a way to ensure that the qp it's about to use is setup already
rather than it relying on assumptions based on timing or the primary process communicating this to the secondary.
It's quite a simple API and fulfils this purpose.
If anyone wants to propose any improvements to it feedback would be appreciated
https://patches.dpdk.org/patch/72157/