[dpdk-dev] [PATCH v2 0/2] rte_dev_probe() API change

[dpdk-dev] [PATCH v2 0/2] rte_dev_probe() API change

[Maxime Coquelin]

This series proposes a change in rte_dev_probe() API
for DPDK v20.11. First patch is the deprecation notice
to be applied in v20.11


Changes since v1:
-----------------
- Rebase on top of master
- Reword deprecation notice (Gaëtan)

Maxime Coquelin (2):
  doc: announce rte_dev_probe() API change
  eal: improve device probing API

 app/test-pmd/testpmd.c                 |  2 +-
 doc/guides/rel_notes/deprecation.rst   |  5 +++++
 drivers/net/failsafe/failsafe.c        |  5 +++--
 lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++--------
 lib/librte_eal/include/rte_dev.h       |  4 ++--
 5 files changed, 21 insertions(+), 13 deletions(-)

-- 
2.26.2

[dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[Maxime Coquelin]

In order to simplify the use of rte_dev_probe() and
rte_dev_remove() by applications, rte_dev_probe() will
return a reference on the rte_device stating DPDK v20.11.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index d1034f60fd..8661a673b5 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -130,3 +130,8 @@ Deprecation Notices
   Python 2 support will be completely removed in 20.11.
   In 20.08, explicit deprecation warnings will be displayed when running
   scripts with Python 2.
+
+* eal: Change ``rte_dev_probe`` API in DPDK v20.11 to return a pointer on the
+  probed rte_device or NULL instead of 0 or an error code. This change will
+  allow applications avoid iterating on devices after a probe to get access to
+  the new rte_device.
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[Maxime Coquelin]

Hi Thomas, Davis,

On 6/25/20 10:04 AM, Maxime Coquelin wrote:
> In order to simplify the use of rte_dev_probe() and
> rte_dev_remove() by applications, rte_dev_probe() will
> return a reference on the rte_device stating DPDK v20.11.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index d1034f60fd..8661a673b5 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -130,3 +130,8 @@ Deprecation Notices
>    Python 2 support will be completely removed in 20.11.
>    In 20.08, explicit deprecation warnings will be displayed when running
>    scripts with Python 2.
> +
> +* eal: Change ``rte_dev_probe`` API in DPDK v20.11 to return a pointer on the
> +  probed rte_device or NULL instead of 0 or an error code. This change will
> +  allow applications avoid iterating on devices after a probe to get access to
> +  the new rte_device.
> 

Any chance this patch goes into -rc4?
Patch 2 is for v20.11, but we would need the deprecation notice to be in
v20.08.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[Bruce Richardson]

On Thu, Jun 25, 2020 at 10:04:29AM +0200, Maxime Coquelin wrote:
> In order to simplify the use of rte_dev_probe() and
> rte_dev_remove() by applications, rte_dev_probe() will
> return a reference on the rte_device stating DPDK v20.11.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[Gaëtan Rivet]

On 31/07/20 10:26 +0100, Bruce Richardson wrote:
> On Thu, Jun 25, 2020 at 10:04:29AM +0200, Maxime Coquelin wrote:
> > In order to simplify the use of rte_dev_probe() and
> > rte_dev_remove() by applications, rte_dev_probe() will
> > return a reference on the rte_device stating DPDK v20.11.
> > 
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

I don't remember if I acked the reworded version, in anycase
Acked-by: Gaetan Rivet <grive@u256.net>

-- 
Gaëtan

Re: [dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[Maxime Coquelin]

On 7/31/20 11:44 AM, Gaëtan Rivet wrote:
> On 31/07/20 10:26 +0100, Bruce Richardson wrote:
>> On Thu, Jun 25, 2020 at 10:04:29AM +0200, Maxime Coquelin wrote:
>>> In order to simplify the use of rte_dev_probe() and
>>> rte_dev_remove() by applications, rte_dev_probe() will
>>> return a reference on the rte_device stating DPDK v20.11.

s/stating/starting/

>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> I don't remember if I acked the reworded version, in anycase
> Acked-by: Gaetan Rivet <grive@u256.net>
> 

No you didn't, but I didn't replied to your comment on patch 2,
so you are forgiven :)

Thanks!
Maxime

Re: [dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[David Marchand]

On Thu, Jun 25, 2020 at 10:04 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> In order to simplify the use of rte_dev_probe() and
> rte_dev_remove() by applications, rte_dev_probe() will
> return a reference on the rte_device stating DPDK v20.11.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Acked-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2 1/2] doc: announce rte_dev_probe() API change

[Jerin Jacob]

On Tue, Aug 4, 2020 at 1:42 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Thu, Jun 25, 2020 at 10:04 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
> >
> > In order to simplify the use of rte_dev_probe() and
> > rte_dev_remove() by applications, rte_dev_probe() will
> > return a reference on the rte_device stating DPDK v20.11.
> >
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Acked-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>


>
>
> --
> David Marchand
>

[dpdk-dev] [PATCH v2 (v20.11) 2/2] eal: improve device probing API

[Maxime Coquelin]

This patch makes rte_dev_probe() to return the
rte_device pointer on success and NULL on error
instead of returning 0 on success and negative
value on error.

The goal is to avoid that the calling application
iterates the devices list afterwards to retrieve
the pointer. Retrieving the pointer is required
for calling rte_dev_remove() later.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 app/test-pmd/testpmd.c                 |  2 +-
 drivers/net/failsafe/failsafe.c        |  5 +++--
 lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++--------
 lib/librte_eal/include/rte_dev.h       |  4 ++--
 4 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4989d22ca8..f777f449a8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2764,7 +2764,7 @@ attach_port(char *identifier)
 		return;
 	}
 
-	if (rte_dev_probe(identifier) < 0) {
+	if (rte_dev_probe(identifier) == NULL) {
 		TESTPMD_LOG(ERR, "Failed to attach port %s\n", identifier);
 		return;
 	}
diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
index 72362f35de..e32effdef2 100644
--- a/drivers/net/failsafe/failsafe.c
+++ b/drivers/net/failsafe/failsafe.c
@@ -341,6 +341,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
 	struct rte_eth_dev *eth_dev;
 	struct sub_device  *sdev;
 	struct rte_devargs devargs;
+	struct rte_device *dev;
 	uint8_t i;
 	int ret;
 
@@ -378,8 +379,8 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
 				continue;
 			}
 			if (!devargs_already_listed(&devargs)) {
-				ret = rte_dev_probe(devargs.name);
-				if (ret < 0) {
+				dev = rte_dev_probe(devargs.name);
+				if (dev == NULL) {
 					ERROR("Failed to probe devargs %s",
 					      devargs.name);
 					continue;
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 9e4f09d83e..72baae2e48 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -120,7 +120,9 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
 	if (ret != 0)
 		return ret;
 
-	ret = rte_dev_probe(devargs);
+	if (rte_dev_probe(devargs) == NULL)
+		ret = -1;
+
 	free(devargs);
 
 	return ret;
@@ -192,7 +194,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
 	return ret;
 }
 
-int
+struct rte_device *
 rte_dev_probe(const char *devargs)
 {
 	struct eal_dev_mp_req req;
@@ -212,12 +214,12 @@ rte_dev_probe(const char *devargs)
 		if (ret != 0) {
 			RTE_LOG(ERR, EAL,
 				"Failed to send hotplug request to primary\n");
-			return -ENOMSG;
+			return NULL;
 		}
 		if (req.result != 0)
 			RTE_LOG(ERR, EAL,
 				"Failed to hotplug add device\n");
-		return req.result;
+		return NULL;
 	}
 
 	/* attach a shared device from primary start from here: */
@@ -236,7 +238,7 @@ rte_dev_probe(const char *devargs)
 		 * process.
 		 */
 		if (ret != -EEXIST)
-			return ret;
+			return dev;
 	}
 
 	/* primary send attach sync request to secondary. */
@@ -261,11 +263,11 @@ rte_dev_probe(const char *devargs)
 
 		/* for -EEXIST, we don't need to rollback. */
 		if (ret == -EEXIST)
-			return ret;
+			return dev;
 		goto rollback;
 	}
 
-	return 0;
+	return dev;
 
 rollback:
 	req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK;
@@ -282,7 +284,7 @@ rte_dev_probe(const char *devargs)
 			"Failed to rollback device attach on primary."
 			"Devices in secondary may not sync with primary\n");
 
-	return ret;
+	return NULL;
 }
 
 int
diff --git a/lib/librte_eal/include/rte_dev.h b/lib/librte_eal/include/rte_dev.h
index c8d985fb5c..9cf7c7fd71 100644
--- a/lib/librte_eal/include/rte_dev.h
+++ b/lib/librte_eal/include/rte_dev.h
@@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
  * @param devargs
  *   Device arguments including bus, class and driver properties.
  * @return
- *   0 on success, negative on error.
+ *   Generic device pointer on success, NULL on error.
  */
-int rte_dev_probe(const char *devargs);
+struct rte_device *rte_dev_probe(const char *devargs);
 
 /**
  * Hotplug remove a given device from a specific bus.
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 (v20.11) 2/2] eal: improve device probing API

[Gaëtan Rivet]

On 25/06/20 10:04 +0200, Maxime Coquelin wrote:
> This patch makes rte_dev_probe() to return the
> rte_device pointer on success and NULL on error
> instead of returning 0 on success and negative
> value on error.
> 
> The goal is to avoid that the calling application
> iterates the devices list afterwards to retrieve
> the pointer. Retrieving the pointer is required
> for calling rte_dev_remove() later.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  app/test-pmd/testpmd.c                 |  2 +-
>  drivers/net/failsafe/failsafe.c        |  5 +++--
>  lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++--------
>  lib/librte_eal/include/rte_dev.h       |  4 ++--
>  4 files changed, 16 insertions(+), 13 deletions(-)
> 

[...]

> diff --git a/lib/librte_eal/include/rte_dev.h b/lib/librte_eal/include/rte_dev.h
> index c8d985fb5c..9cf7c7fd71 100644
> --- a/lib/librte_eal/include/rte_dev.h
> +++ b/lib/librte_eal/include/rte_dev.h
> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
>   * @param devargs
>   *   Device arguments including bus, class and driver properties.
>   * @return
> - *   0 on success, negative on error.
> + *   Generic device pointer on success, NULL on error.

Hello Maxime,

Do you plan on a v3 with rte_errno support in the new API or do you
prefer to keep it this way after all?

-- 
Gaëtan

Re: [dpdk-dev] [PATCH v2 (v20.11) 2/2] eal: improve device probing API

[Thomas Monjalon]

25/06/2020 10:04, Maxime Coquelin:
> This patch makes rte_dev_probe() to return the
> rte_device pointer on success and NULL on error
> instead of returning 0 on success and negative
> value on error.
> 
> The goal is to avoid that the calling application
> iterates the devices list afterwards to retrieve
> the pointer. Retrieving the pointer is required
> for calling rte_dev_remove() later.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> --- a/lib/librte_eal/include/rte_dev.h
> +++ b/lib/librte_eal/include/rte_dev.h
> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
>   * @param devargs
>   *   Device arguments including bus, class and driver properties.
>   * @return
> - *   0 on success, negative on error.
> + *   Generic device pointer on success, NULL on error.
>   */
> -int rte_dev_probe(const char *devargs);
> +struct rte_device *rte_dev_probe(const char *devargs);

Sorry for not catching it earlier, I think this change is against
the idea of having a generic devargs syntax.
One string could identify multiple devices.
And a successful probe does not mean there is a new rte_device
(can be an update, allowing more ports on the same device).

Re: [dpdk-dev] [PATCH v2 (v20.11) 2/2] eal: improve device probing API

[Maxime Coquelin]

On 8/6/20 1:33 AM, Thomas Monjalon wrote:
> 25/06/2020 10:04, Maxime Coquelin:
>> This patch makes rte_dev_probe() to return the
>> rte_device pointer on success and NULL on error
>> instead of returning 0 on success and negative
>> value on error.
>>
>> The goal is to avoid that the calling application
>> iterates the devices list afterwards to retrieve
>> the pointer. Retrieving the pointer is required
>> for calling rte_dev_remove() later.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> --- a/lib/librte_eal/include/rte_dev.h
>> +++ b/lib/librte_eal/include/rte_dev.h
>> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
>>   * @param devargs
>>   *   Device arguments including bus, class and driver properties.
>>   * @return
>> - *   0 on success, negative on error.
>> + *   Generic device pointer on success, NULL on error.
>>   */
>> -int rte_dev_probe(const char *devargs);
>> +struct rte_device *rte_dev_probe(const char *devargs);
> 
> Sorry for not catching it earlier, I think this change is against
> the idea of having a generic devargs syntax.
> One string could identify multiple devices.

That sounds fragile to me. What if one of the multiple devices fails to
probe? Do the other ones are going to be removed?

> And a successful probe does not mean there is a new rte_device
> (can be an update, allowing more ports on the same device).

This should be done by a separate API in my opinion, ports may be seen
as a sub-function of the device.

But anyway, I am fine with dropping it. It is not blocking vDPA probing,
just making it more cumbersome.

Regards,
Maxime

Re: [dpdk-dev] [PATCH v2 (v20.11) 2/2] eal: improve device probing API

[Thomas Monjalon]

06/08/2020 12:29, Maxime Coquelin:
> On 8/6/20 1:33 AM, Thomas Monjalon wrote:
> > 25/06/2020 10:04, Maxime Coquelin:
> >> This patch makes rte_dev_probe() to return the
> >> rte_device pointer on success and NULL on error
> >> instead of returning 0 on success and negative
> >> value on error.
> >>
> >> The goal is to avoid that the calling application
> >> iterates the devices list afterwards to retrieve
> >> the pointer. Retrieving the pointer is required
> >> for calling rte_dev_remove() later.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >> --- a/lib/librte_eal/include/rte_dev.h
> >> +++ b/lib/librte_eal/include/rte_dev.h
> >> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
> >>   * @param devargs
> >>   *   Device arguments including bus, class and driver properties.
> >>   * @return
> >> - *   0 on success, negative on error.
> >> + *   Generic device pointer on success, NULL on error.
> >>   */
> >> -int rte_dev_probe(const char *devargs);
> >> +struct rte_device *rte_dev_probe(const char *devargs);
> > 
> > Sorry for not catching it earlier, I think this change is against
> > the idea of having a generic devargs syntax.
> > One string could identify multiple devices.
> 
> That sounds fragile to me. What if one of the multiple devices fails to
> probe? Do the other ones are going to be removed?

No, what is correctly probed should not be rolled back in my opinion.

> > And a successful probe does not mean there is a new rte_device
> > (can be an update, allowing more ports on the same device).
> 
> This should be done by a separate API in my opinion, ports may be seen
> as a sub-function of the device.
> 
> But anyway, I am fine with dropping it. It is not blocking vDPA probing,
> just making it more cumbersome.

Apps should not iterate on devices.
vDPA should have an event mechanism like in ethdev:
a callback with RTE_ETH_EVENT_NEW is called for each new port probed.