DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
@ 2021-11-18 16:20 ` Stephen Hemminger
  2021-11-18 16:22   ` Elena Agostini
  2021-11-18 20:19 ` Tyler Retzlaff
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Stephen Hemminger @ 2021-11-18 16:20 UTC (permalink / raw)
  To: eagostini; +Cc: dev

On Thu, 18 Nov 2021 19:28:02 +0000
<eagostini@nvidia.com> wrote:

> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> index 2b174d8bd5..97575ed979 100644
> --- a/lib/gpudev/gpudev.c
> +++ b/lib/gpudev/gpudev.c
> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>  		return -rte_errno;
>  	}
>  
> +	if (ptr == NULL) {
> +		rte_errno = EINVAL;
> +		return -rte_errno;
> +	}
> +

The convention for free(), and rte_free() is that calling free
with a NULL pointer is a nop. Why not follow those?

This would keep programmers from having to view GPU as a
special case.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-18 16:20 ` Stephen Hemminger
@ 2021-11-18 16:22   ` Elena Agostini
  0 siblings, 0 replies; 25+ messages in thread
From: Elena Agostini @ 2021-11-18 16:22 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thursday, 18 November 2021 at 17:21
> To: Elena Agostini <eagostini@nvidia.com>
> Cc: dev@dpdk.org <dev@dpdk.org>
> Subject: Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
> External email: Use caution opening links or attachments>
>
> On Thu, 18 Nov 2021 19:28:02 +0000
> <eagostini@nvidia.com> wrote:>
> > diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> > index 2b174d8bd5..97575ed979 100644
> > --- a/lib/gpudev/gpudev.c
> > +++ b/lib/gpudev/gpudev.c
> > @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> >               return -rte_errno;
> >       }
> >
> > +     if (ptr == NULL) {
> > +             rte_errno = EINVAL;
> > +             return -rte_errno;
> > +     }
> > +>
> The convention for free(), and rte_free() is that calling free
> with a NULL pointer is a nop. Why not follow those?>
> This would keep programmers from having to view GPU as a
> special case.

Please look at v2 here https://patches.dpdk.org/project/dpdk/patch/20211118203354.25355-1-eagostini@nvidia.com/

[-- Attachment #2: Type: text/html, Size: 5019 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
@ 2021-11-18 19:28 eagostini
  2021-11-18 16:20 ` Stephen Hemminger
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: eagostini @ 2021-11-18 19:28 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 lib/gpudev/gpudev.c     | 10 ++++++++++
 lib/gpudev/rte_gpudev.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..97575ed979 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
+
 	if (dev->ops.mem_free == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
@@ -619,6 +624,11 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
+
 	if (dev->ops.mem_unregister == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
diff --git a/lib/gpudev/rte_gpudev.h b/lib/gpudev/rte_gpudev.h
index fa3f3aad4f..02014328f6 100644
--- a/lib/gpudev/rte_gpudev.h
+++ b/lib/gpudev/rte_gpudev.h
@@ -394,6 +394,7 @@ __rte_alloc_size(2);
  *   0 on success, -rte_errno otherwise:
  *   - ENODEV if invalid dev_id
  *   - ENOTSUP if operation not supported by the driver
+ *   - EINVAL if input ptr is invalid
  *   - EPERM if driver error
  */
 __rte_experimental
@@ -442,6 +443,7 @@ int rte_gpu_mem_register(int16_t dev_id, size_t size, void *ptr);
  *   0 on success, -rte_errno otherwise:
  *   - ENODEV if invalid dev_id
  *   - ENOTSUP if operation not supported by the driver
+ *   - EINVAL if input ptr is invalid
  *   - EPERM if driver error
  */
 __rte_experimental
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
  2021-11-18 16:20 ` Stephen Hemminger
@ 2021-11-18 20:19 ` Tyler Retzlaff
  2021-11-19  9:34   ` Ferruh Yigit
  2021-11-18 20:33 ` [PATCH v2] gpudev: free and unregister return gracefully if input pointer is NULL eagostini
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Tyler Retzlaff @ 2021-11-18 20:19 UTC (permalink / raw)
  To: eagostini; +Cc: dev

On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
> From: Elena Agostini <eagostini@nvidia.com>
> 
> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> ---
>  lib/gpudev/gpudev.c     | 10 ++++++++++
>  lib/gpudev/rte_gpudev.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> index 2b174d8bd5..97575ed979 100644
> --- a/lib/gpudev/gpudev.c
> +++ b/lib/gpudev/gpudev.c
> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>  		return -rte_errno;
>  	}
>  
> +	if (ptr == NULL) {
> +		rte_errno = EINVAL;
> +		return -rte_errno;
> +	}

in general dpdk has real problems with how it indicates that an error
occurred and what error occurred consistently.

some api's return 0 on success
  and maybe return -errno if ! 0
  and maybe return errno if ! 0
  and maybe set rte_errno if ! 0

some api's return -1 on failure
  and set rte_errno if -1

some api's return < 0 on failure
  and maybe set rte_errno
  and maybe return -errno
  and maybe set rte_errno and return -rte_errno

this isn't isiolated to only this change but since additions and context
in this patch highlight it maybe it's a good time to bring it up.

it's frustrating to have to carefully read the implementation every time
you want to make a function call to make sure you're handling the flavor
of error reporting for a particular function.

if this is new code could we please clearly identify the current best
practice and follow it as a standard going forward for all new public
apis.

thanks!

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2] gpudev: free and unregister return gracefully if input pointer is NULL
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
  2021-11-18 16:20 ` Stephen Hemminger
  2021-11-18 20:19 ` Tyler Retzlaff
@ 2021-11-18 20:33 ` eagostini
  2021-11-22 18:24   ` [PATCH v3] gpudev: manage NULL pointer eagostini
  2021-11-22 23:52 ` [PATCH v4] " eagostini
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: eagostini @ 2021-11-18 20:33 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 drivers/gpu/cuda/cuda.c | 4 ++--
 lib/gpudev/gpudev.c     | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/cuda/cuda.c b/drivers/gpu/cuda/cuda.c
index 24ae630d04..f68e2b20b9 100644
--- a/drivers/gpu/cuda/cuda.c
+++ b/drivers/gpu/cuda/cuda.c
@@ -765,7 +765,7 @@ cuda_mem_free(struct rte_gpu *dev, void *ptr)
 		return -ENODEV;
 
 	if (ptr == NULL)
-		return -EINVAL;
+		return 0;
 
 	hk = get_hash_from_ptr((void *)ptr);
 
@@ -804,7 +804,7 @@ cuda_mem_unregister(struct rte_gpu *dev, void *ptr)
 		return -ENODEV;
 
 	if (ptr == NULL)
-		return -EINVAL;
+		return 0;
 
 	hk = get_hash_from_ptr((void *)ptr);
 
diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..b41c43016a 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 {
 	struct rte_gpu *dev;
 
+	if (ptr == NULL)
+		return 0;
+
 	dev = gpu_get_by_id(dev_id);
 	if (dev == NULL) {
 		GPU_LOG(ERR, "free mem for invalid device ID %d", dev_id);
@@ -612,6 +615,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 {
 	struct rte_gpu *dev;
 
+	if (ptr == NULL)
+		return 0;
+
 	dev = gpu_get_by_id(dev_id);
 	if (dev == NULL) {
 		GPU_LOG(ERR, "unregister mem for invalid device ID %d", dev_id);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-18 20:19 ` Tyler Retzlaff
@ 2021-11-19  9:34   ` Ferruh Yigit
  2021-11-19  9:56     ` Thomas Monjalon
  2021-11-19 10:15     ` Bruce Richardson
  0 siblings, 2 replies; 25+ messages in thread
From: Ferruh Yigit @ 2021-11-19  9:34 UTC (permalink / raw)
  To: Tyler Retzlaff, eagostini
  Cc: dev, techboard, Andrew Rybchenko, David Marchand

On 11/18/2021 8:19 PM, Tyler Retzlaff wrote:
> On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
>> From: Elena Agostini <eagostini@nvidia.com>
>>
>> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
>> ---
>>   lib/gpudev/gpudev.c     | 10 ++++++++++
>>   lib/gpudev/rte_gpudev.h |  2 ++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
>> index 2b174d8bd5..97575ed979 100644
>> --- a/lib/gpudev/gpudev.c
>> +++ b/lib/gpudev/gpudev.c
>> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>>   		return -rte_errno;
>>   	}
>>   
>> +	if (ptr == NULL) {
>> +		rte_errno = EINVAL;
>> +		return -rte_errno;
>> +	}
> 
> in general dpdk has real problems with how it indicates that an error
> occurred and what error occurred consistently.
> 
> some api's return 0 on success
>    and maybe return -errno if ! 0
>    and maybe return errno if ! 0
>    and maybe set rte_errno if ! 0
> 
> some api's return -1 on failure
>    and set rte_errno if -1
> 
> some api's return < 0 on failure
>    and maybe set rte_errno
>    and maybe return -errno
>    and maybe set rte_errno and return -rte_errno
> 

This is a generic comment, cc'ed a few more folks to make the comment more
visible.

> this isn't isiolated to only this change but since additions and context
> in this patch highlight it maybe it's a good time to bring it up.
> 
> it's frustrating to have to carefully read the implementation every time
> you want to make a function call to make sure you're handling the flavor
> of error reporting for a particular function.
> 
> if this is new code could we please clearly identify the current best
> practice and follow it as a standard going forward for all new public
> apis.
> 
> thanks!
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-19  9:34   ` Ferruh Yigit
@ 2021-11-19  9:56     ` Thomas Monjalon
  2021-11-24 17:24       ` Tyler Retzlaff
  2021-11-19 10:15     ` Bruce Richardson
  1 sibling, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2021-11-19  9:56 UTC (permalink / raw)
  To: Tyler Retzlaff
  Cc: eagostini, techboard, dev, Andrew Rybchenko, David Marchand,
	Ferruh Yigit

19/11/2021 10:34, Ferruh Yigit:
> On 11/18/2021 8:19 PM, Tyler Retzlaff wrote:
> > On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
> >> From: Elena Agostini <eagostini@nvidia.com>
> >>
> >> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> >> ---
> >>   lib/gpudev/gpudev.c     | 10 ++++++++++
> >>   lib/gpudev/rte_gpudev.h |  2 ++
> >>   2 files changed, 12 insertions(+)
> >>
> >> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> >> index 2b174d8bd5..97575ed979 100644
> >> --- a/lib/gpudev/gpudev.c
> >> +++ b/lib/gpudev/gpudev.c
> >> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> >>   		return -rte_errno;
> >>   	}
> >>   
> >> +	if (ptr == NULL) {
> >> +		rte_errno = EINVAL;
> >> +		return -rte_errno;
> >> +	}
> > 
> > in general dpdk has real problems with how it indicates that an error
> > occurred and what error occurred consistently.
> > 
> > some api's return 0 on success
> >    and maybe return -errno if ! 0
> >    and maybe return errno if ! 0

Which function returns a positive errno?

> >    and maybe set rte_errno if ! 0
> > 
> > some api's return -1 on failure
> >    and set rte_errno if -1
> > 
> > some api's return < 0 on failure
> >    and maybe set rte_errno
> >    and maybe return -errno
> >    and maybe set rte_errno and return -rte_errno
> 
> This is a generic comment, cc'ed a few more folks to make the comment more
> visible.
> 
> > this isn't isiolated to only this change but since additions and context
> > in this patch highlight it maybe it's a good time to bring it up.
> > 
> > it's frustrating to have to carefully read the implementation every time
> > you want to make a function call to make sure you're handling the flavor
> > of error reporting for a particular function.
> > 
> > if this is new code could we please clearly identify the current best
> > practice and follow it as a standard going forward for all new public
> > apis.

I think this patch is following the best practice.
1/ Return negative value in case of error
2/ Set rte_errno
3/ Set same absolute value in rte_errno and return code




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-19  9:34   ` Ferruh Yigit
  2021-11-19  9:56     ` Thomas Monjalon
@ 2021-11-19 10:15     ` Bruce Richardson
  1 sibling, 0 replies; 25+ messages in thread
From: Bruce Richardson @ 2021-11-19 10:15 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Tyler Retzlaff, eagostini, dev, techboard, Andrew Rybchenko,
	David Marchand

On Fri, Nov 19, 2021 at 09:34:08AM +0000, Ferruh Yigit wrote:
> On 11/18/2021 8:19 PM, Tyler Retzlaff wrote:
> > On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
> > > From: Elena Agostini <eagostini@nvidia.com>
> > > 
> > > Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> > > ---
> > >   lib/gpudev/gpudev.c     | 10 ++++++++++
> > >   lib/gpudev/rte_gpudev.h |  2 ++
> > >   2 files changed, 12 insertions(+)
> > > 
> > > diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> > > index 2b174d8bd5..97575ed979 100644
> > > --- a/lib/gpudev/gpudev.c
> > > +++ b/lib/gpudev/gpudev.c
> > > @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> > >   		return -rte_errno;
> > >   	}
> > > +	if (ptr == NULL) {
> > > +		rte_errno = EINVAL;
> > > +		return -rte_errno;
> > > +	}
> > 
> > in general dpdk has real problems with how it indicates that an error
> > occurred and what error occurred consistently.
> > 
> > some api's return 0 on success
> >    and maybe return -errno if ! 0
> >    and maybe return errno if ! 0
> >    and maybe set rte_errno if ! 0
> > 
> > some api's return -1 on failure
> >    and set rte_errno if -1
> > 
> > some api's return < 0 on failure
> >    and maybe set rte_errno
> >    and maybe return -errno
> >    and maybe set rte_errno and return -rte_errno
> > 
> 
> This is a generic comment, cc'ed a few more folks to make the comment more
> visible.
>

Yes, it's inconsistent. However, this only applies to functions which
return integer values. Other functions which return pointer values are
hopefully standardized to return NULL on error. [Which means any error code
must be in rte_errno.]

/Bruce

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] gpudev: manage NULL pointer
  2021-11-22 18:24   ` [PATCH v3] gpudev: manage NULL pointer eagostini
@ 2021-11-22 11:23     ` Thomas Monjalon
  2021-11-22 11:34       ` Elena Agostini
  0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2021-11-22 11:23 UTC (permalink / raw)
  To: Elena Agostini; +Cc: dev

22/11/2021 19:24, eagostini@nvidia.com:
> From: Elena Agostini <eagostini@nvidia.com>
> 
> gpudev free and unregister functions return gracefully if input pointer is NULL

We could add that the API doc was indicating NULL as a no-op accepted value.

Another explanation to add: cuda driver checks are removed because redundant
with the checks added in gpudev library.

> Fixes: 818a067baf90 ("gpudev: manage NULL pointer")

It should be:
Fixes: e818c4e2bf50 ("gpudev: add memory API")

> 
> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> ---
>  drivers/gpu/cuda/cuda.c | 6 ------
>  lib/gpudev/gpudev.c     | 6 ++++++
>  2 files changed, 6 insertions(+), 6 deletions(-)
[...]
> --- a/lib/gpudev/gpudev.c
> +++ b/lib/gpudev/gpudev.c
> @@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>  {
>  	struct rte_gpu *dev;
>  
> +	if (ptr == NULL)
> +		return 0;
> +
>  	dev = gpu_get_by_id(dev_id);
>  	if (dev == NULL) {
>  		GPU_LOG(ERR, "free mem for invalid device ID %d", dev_id);

I think we should keep this check first.

> @@ -612,6 +615,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
>  {
>  	struct rte_gpu *dev;
>  
> +	if (ptr == NULL)
> +		return 0;
> +
>  	dev = gpu_get_by_id(dev_id);
>  	if (dev == NULL) {
>  		GPU_LOG(ERR, "unregister mem for invalid device ID %d", dev_id);

Same here.

There is third function where NULL should be accepted: rte_gpu_mem_register





^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] gpudev: manage NULL pointer
  2021-11-22 11:23     ` Thomas Monjalon
@ 2021-11-22 11:34       ` Elena Agostini
  2021-11-22 11:51         ` Thomas Monjalon
  0 siblings, 1 reply; 25+ messages in thread
From: Elena Agostini @ 2021-11-22 11:34 UTC (permalink / raw)
  To: NBU-Contact-Thomas Monjalon (EXTERNAL); +Cc: dev

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

> From: Thomas Monjalon <thomas@monjalon.net>
> Date: Monday, 22 November 2021 at 12:23
> To: Elena Agostini <eagostini@nvidia.com>
> Cc: dev@dpdk.org <dev@dpdk.org>
> Subject: Re: [PATCH v3] gpudev: manage NULL pointer
> External email: Use caution opening links or attachments>
>

> 22/11/2021 19:24, eagostini@nvidia.com:
> > From: Elena Agostini <eagostini@nvidia.com>
> >
> > gpudev free and unregister functions return gracefully if input pointer is NULL>

> We could add that the API doc was indicating NULL as a no-op accepted value.>

> Another explanation to add: cuda driver checks are removed because redundant
> with the checks added in gpudev library.>

> > Fixes: 818a067baf90 ("gpudev: manage NULL pointer")>

> It should be:
> Fixes: e818c4e2bf50 ("gpudev: add memory API")>

> >
> > Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> > ---
> >  drivers/gpu/cuda/cuda.c | 6 ------
> >  lib/gpudev/gpudev.c     | 6 ++++++
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> [...]
> > --- a/lib/gpudev/gpudev.c
> > +++ b/lib/gpudev/gpudev.c
> > @@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> >  {
> >       struct rte_gpu *dev;
> >
> > +     if (ptr == NULL)
> > +             return 0;
> > +
> >       dev = gpu_get_by_id(dev_id);
> >       if (dev == NULL) {
> >               GPU_LOG(ERR, "free mem for invalid device ID %d", dev_id);>

> I think we should keep this check first.

Why should gpudev waste more latency in looking for the device if the ptr is NULL?

> > @@ -612,6 +615,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
> >  {
> >       struct rte_gpu *dev;
> >
> > +     if (ptr == NULL)
> > +             return 0;
> > +
> >       dev = gpu_get_by_id(dev_id);
> >       if (dev == NULL) {
> >               GPU_LOG(ERR, "unregister mem for invalid device ID %d", dev_id);>

> Same here.

> There is third function where NULL should be accepted: rte_gpu_mem_register>

Thanks, let me add it

[-- Attachment #2: Type: text/html, Size: 8790 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] gpudev: manage NULL pointer
  2021-11-22 11:34       ` Elena Agostini
@ 2021-11-22 11:51         ` Thomas Monjalon
  0 siblings, 0 replies; 25+ messages in thread
From: Thomas Monjalon @ 2021-11-22 11:51 UTC (permalink / raw)
  To: Elena Agostini; +Cc: dev

22/11/2021 12:34, Elena Agostini:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 22/11/2021 19:24, eagostini@nvidia.com:
> > > --- a/lib/gpudev/gpudev.c
> > > +++ b/lib/gpudev/gpudev.c
> > > @@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> > >  {
> > >       struct rte_gpu *dev;
> > >
> > > +     if (ptr == NULL)
> > > +             return 0;
> > > +
> > >       dev = gpu_get_by_id(dev_id);
> > >       if (dev == NULL) {
> > >               GPU_LOG(ERR, "free mem for invalid device ID %d", dev_id);>
> 
> > I think we should keep this check first.
> 
> Why should gpudev waste more latency in looking for the device if the ptr is NULL?

Freeing with NULL pointer is not in the datapath I think,
probably just a failure cleanup case.
Having the dev_id check first allows to catch more bugs.
Returning 0 without checking the id looks weird to me.




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5] gpudev: manage NULL pointer
  2021-11-22 23:55 ` [PATCH v5] " eagostini
@ 2021-11-22 16:01   ` Thomas Monjalon
  0 siblings, 0 replies; 25+ messages in thread
From: Thomas Monjalon @ 2021-11-22 16:01 UTC (permalink / raw)
  To: Elena Agostini; +Cc: dev

23/11/2021 00:55, eagostini@nvidia.com:
> From: Elena Agostini <eagostini@nvidia.com>
> 
> gpudev free and unregister functions return
> gracefully if input pointer is NULL because
> API doc was indicating NULL as a no-op
> accepted value.
> 
> cuda driver checks are removed because
> redundant with the checks added
> in gpudev library.

You didn't remove the check in cuda_mem_register.




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3] gpudev: manage NULL pointer
  2021-11-18 20:33 ` [PATCH v2] gpudev: free and unregister return gracefully if input pointer is NULL eagostini
@ 2021-11-22 18:24   ` eagostini
  2021-11-22 11:23     ` Thomas Monjalon
  0 siblings, 1 reply; 25+ messages in thread
From: eagostini @ 2021-11-22 18:24 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

gpudev free and unregister functions return gracefully if input pointer is NULL

Fixes: 818a067baf90 ("gpudev: manage NULL pointer")

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 drivers/gpu/cuda/cuda.c | 6 ------
 lib/gpudev/gpudev.c     | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/cuda/cuda.c b/drivers/gpu/cuda/cuda.c
index 24ae630d04..47a3bbf256 100644
--- a/drivers/gpu/cuda/cuda.c
+++ b/drivers/gpu/cuda/cuda.c
@@ -764,9 +764,6 @@ cuda_mem_free(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
@@ -803,9 +800,6 @@ cuda_mem_unregister(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..b41c43016a 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 {
 	struct rte_gpu *dev;
 
+	if (ptr == NULL)
+		return 0;
+
 	dev = gpu_get_by_id(dev_id);
 	if (dev == NULL) {
 		GPU_LOG(ERR, "free mem for invalid device ID %d", dev_id);
@@ -612,6 +615,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 {
 	struct rte_gpu *dev;
 
+	if (ptr == NULL)
+		return 0;
+
 	dev = gpu_get_by_id(dev_id);
 	if (dev == NULL) {
 		GPU_LOG(ERR, "unregister mem for invalid device ID %d", dev_id);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v4] gpudev: manage NULL pointer
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
                   ` (2 preceding siblings ...)
  2021-11-18 20:33 ` [PATCH v2] gpudev: free and unregister return gracefully if input pointer is NULL eagostini
@ 2021-11-22 23:52 ` eagostini
  2021-11-22 23:55 ` [PATCH v5] " eagostini
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: eagostini @ 2021-11-22 23:52 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

gpudev free and unregister functions return
gracefully if input pointer is NULL because
API doc was indicating NULL as a no-op
accepted value.

cuda driver checks are removed because
redundant with the checks added
in gpudev library.

Fixes: e818c4e2bf50 ("gpudev: add memory API")

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 drivers/gpu/cuda/cuda.c | 6 ------
 lib/gpudev/gpudev.c     | 9 +++++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/cuda/cuda.c b/drivers/gpu/cuda/cuda.c
index 24ae630d04..47a3bbf256 100644
--- a/drivers/gpu/cuda/cuda.c
+++ b/drivers/gpu/cuda/cuda.c
@@ -764,9 +764,6 @@ cuda_mem_free(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
@@ -803,9 +800,6 @@ cuda_mem_unregister(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..d6224a3828 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -576,6 +576,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_free == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
@@ -595,6 +598,9 @@ rte_gpu_mem_register(int16_t dev_id, size_t size, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_register == NULL) {
 		GPU_LOG(ERR, "mem registration not supported");
 		rte_errno = ENOTSUP;
@@ -619,6 +625,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if( ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_unregister == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v5] gpudev: manage NULL pointer
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
                   ` (3 preceding siblings ...)
  2021-11-22 23:52 ` [PATCH v4] " eagostini
@ 2021-11-22 23:55 ` eagostini
  2021-11-22 16:01   ` Thomas Monjalon
  2021-11-23  0:15 ` [PATCH v6] " eagostini
  2021-11-23  0:42 ` [PATCH v7] " eagostini
  6 siblings, 1 reply; 25+ messages in thread
From: eagostini @ 2021-11-22 23:55 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

gpudev free and unregister functions return
gracefully if input pointer is NULL because
API doc was indicating NULL as a no-op
accepted value.

cuda driver checks are removed because
redundant with the checks added
in gpudev library.

Fixes: e818c4e2bf50 ("gpudev: add memory API")

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 drivers/gpu/cuda/cuda.c | 6 ------
 lib/gpudev/gpudev.c     | 9 +++++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/cuda/cuda.c b/drivers/gpu/cuda/cuda.c
index 24ae630d04..47a3bbf256 100644
--- a/drivers/gpu/cuda/cuda.c
+++ b/drivers/gpu/cuda/cuda.c
@@ -764,9 +764,6 @@ cuda_mem_free(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
@@ -803,9 +800,6 @@ cuda_mem_unregister(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..7aeaf931c3 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -576,6 +576,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_free == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
@@ -595,6 +598,9 @@ rte_gpu_mem_register(int16_t dev_id, size_t size, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_register == NULL) {
 		GPU_LOG(ERR, "mem registration not supported");
 		rte_errno = ENOTSUP;
@@ -619,6 +625,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_unregister == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v6] gpudev: manage NULL pointer
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
                   ` (4 preceding siblings ...)
  2021-11-22 23:55 ` [PATCH v5] " eagostini
@ 2021-11-23  0:15 ` eagostini
  2021-11-23  0:42 ` [PATCH v7] " eagostini
  6 siblings, 0 replies; 25+ messages in thread
From: eagostini @ 2021-11-23  0:15 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

gpudev free and unregister functions return
gracefully if input pointer is NULL because
API doc was indicating NULL as a no-op
accepted value.

cuda driver checks are removed because
redundant with the checks added
in gpudev library.

Fixes: e818c4e2bf50 ("gpudev: add memory API")

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 drivers/gpu/cuda/cuda.c | 8 +-------
 lib/gpudev/gpudev.c     | 9 +++++++++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/cuda/cuda.c b/drivers/gpu/cuda/cuda.c
index 24ae630d04..f59951f11c 100644
--- a/drivers/gpu/cuda/cuda.c
+++ b/drivers/gpu/cuda/cuda.c
@@ -652,7 +652,7 @@ cuda_mem_register(struct rte_gpu *dev, size_t size, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (size == 0 || ptr == NULL)
+	if (size == 0)
 		return -EINVAL;
 
 	/* Store current ctx */
@@ -764,9 +764,6 @@ cuda_mem_free(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
@@ -803,9 +800,6 @@ cuda_mem_unregister(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..7aeaf931c3 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -576,6 +576,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_free == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
@@ -595,6 +598,9 @@ rte_gpu_mem_register(int16_t dev_id, size_t size, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_register == NULL) {
 		GPU_LOG(ERR, "mem registration not supported");
 		rte_errno = ENOTSUP;
@@ -619,6 +625,9 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 		return -rte_errno;
 	}
 
+	if (ptr == NULL)
+		return 0;
+
 	if (dev->ops.mem_unregister == NULL) {
 		rte_errno = ENOTSUP;
 		return -rte_errno;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v7] gpudev: manage NULL pointer
  2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
                   ` (5 preceding siblings ...)
  2021-11-23  0:15 ` [PATCH v6] " eagostini
@ 2021-11-23  0:42 ` eagostini
  2021-11-24  8:40   ` Thomas Monjalon
  6 siblings, 1 reply; 25+ messages in thread
From: eagostini @ 2021-11-23  0:42 UTC (permalink / raw)
  To: dev; +Cc: Elena Agostini

From: Elena Agostini <eagostini@nvidia.com>

gpudev free and unregister functions return
gracefully if input pointer is NULL because
API doc was indicating NULL as a no-op
accepted value.

cuda driver checks are removed because
redundant with the checks added
in gpudev library.

Fixes: e818c4e2bf50 ("gpudev: add memory API")

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 drivers/gpu/cuda/cuda.c | 11 -----------
 lib/gpudev/gpudev.c     | 12 ++++++++++--
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/cuda/cuda.c b/drivers/gpu/cuda/cuda.c
index 24ae630d04..a4869da186 100644
--- a/drivers/gpu/cuda/cuda.c
+++ b/drivers/gpu/cuda/cuda.c
@@ -569,8 +569,6 @@ cuda_mem_alloc(struct rte_gpu *dev, size_t size, void **ptr)
 
 	if (dev == NULL)
 		return -ENODEV;
-	if (size == 0)
-		return -EINVAL;
 
 	/* Store current ctx */
 	res = pfn_cuCtxGetCurrent(&current_ctx);
@@ -652,9 +650,6 @@ cuda_mem_register(struct rte_gpu *dev, size_t size, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (size == 0 || ptr == NULL)
-		return -EINVAL;
-
 	/* Store current ctx */
 	res = pfn_cuCtxGetCurrent(&current_ctx);
 	if (res != 0) {
@@ -764,9 +759,6 @@ cuda_mem_free(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
@@ -803,9 +795,6 @@ cuda_mem_unregister(struct rte_gpu *dev, void *ptr)
 	if (dev == NULL)
 		return -ENODEV;
 
-	if (ptr == NULL)
-		return -EINVAL;
-
 	hk = get_hash_from_ptr((void *)ptr);
 
 	mem_item = mem_list_find_item(hk);
diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
index 2b174d8bd5..1d8200e71b 100644
--- a/lib/gpudev/gpudev.c
+++ b/lib/gpudev/gpudev.c
@@ -580,6 +580,10 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 		rte_errno = ENOTSUP;
 		return -rte_errno;
 	}
+
+	if (ptr == NULL) /* dry-run */
+		return 0;
+
 	return GPU_DRV_RET(dev->ops.mem_free(dev, ptr));
 }
 
@@ -601,8 +605,8 @@ rte_gpu_mem_register(int16_t dev_id, size_t size, void *ptr)
 		return -rte_errno;
 	}
 
-	if (size == 0 || ptr == NULL) /* dry-run */
-		return -EINVAL;
+	if (ptr == NULL || size == 0) /* dry-run  */
+		return 0;
 
 	return GPU_DRV_RET(dev->ops.mem_register(dev, size, ptr));
 }
@@ -623,6 +627,10 @@ rte_gpu_mem_unregister(int16_t dev_id, void *ptr)
 		rte_errno = ENOTSUP;
 		return -rte_errno;
 	}
+
+	if (ptr == NULL) /* dry-run */
+		return 0;
+
 	return GPU_DRV_RET(dev->ops.mem_unregister(dev, ptr));
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v7] gpudev: manage NULL pointer
  2021-11-23  0:42 ` [PATCH v7] " eagostini
@ 2021-11-24  8:40   ` Thomas Monjalon
  0 siblings, 0 replies; 25+ messages in thread
From: Thomas Monjalon @ 2021-11-24  8:40 UTC (permalink / raw)
  To: Elena Agostini; +Cc: dev

23/11/2021 01:42, eagostini@nvidia.com:
> From: Elena Agostini <eagostini@nvidia.com>
> 
> gpudev free and unregister functions return
> gracefully if input pointer is NULL because
> API doc was indicating NULL as a no-op
> accepted value.
> 
> cuda driver checks are removed because
> redundant with the checks added
> in gpudev library.
> 
> Fixes: e818c4e2bf50 ("gpudev: add memory API")
> 
> Signed-off-by: Elena Agostini <eagostini@nvidia.com>

Applied with updated commit message:

    gpudev: manage null parameters in memory functions
    
    The gpudev functions free, register and unregister
    return gracefully if input pointer is NULL or size 0,
    as API doc was indicating no-op accepted values.
    
    CUDA driver checks are removed because redundant
    with the checks added in gpudev library.




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-19  9:56     ` Thomas Monjalon
@ 2021-11-24 17:24       ` Tyler Retzlaff
  2021-11-24 18:04         ` Bruce Richardson
  0 siblings, 1 reply; 25+ messages in thread
From: Tyler Retzlaff @ 2021-11-24 17:24 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: eagostini, techboard, dev, Andrew Rybchenko, David Marchand,
	Ferruh Yigit

On Fri, Nov 19, 2021 at 10:56:36AM +0100, Thomas Monjalon wrote:
> 19/11/2021 10:34, Ferruh Yigit:
> > >> +	if (ptr == NULL) {
> > >> +		rte_errno = EINVAL;
> > >> +		return -rte_errno;
> > >> +	}
> > > 
> > > in general dpdk has real problems with how it indicates that an error
> > > occurred and what error occurred consistently.
> > > 
> > > some api's return 0 on success
> > >    and maybe return -errno if ! 0
> > >    and maybe return errno if ! 0
> 
> Which function returns a positive errno?

i may have mispoke about this variant, it may be something i recall
seeing in a posted patch that was resolved before integration.

> 
> > >    and maybe set rte_errno if ! 0
> > > 
> > > some api's return -1 on failure
> > >    and set rte_errno if -1
> > > 
> > > some api's return < 0 on failure
> > >    and maybe set rte_errno
> > >    and maybe return -errno
> > >    and maybe set rte_errno and return -rte_errno
> > 
> > This is a generic comment, cc'ed a few more folks to make the comment more
> > visible.
> > 
> > > this isn't isiolated to only this change but since additions and context
> > > in this patch highlight it maybe it's a good time to bring it up.
> > > 
> > > it's frustrating to have to carefully read the implementation every time
> > > you want to make a function call to make sure you're handling the flavor
> > > of error reporting for a particular function.
> > > 
> > > if this is new code could we please clearly identify the current best
> > > practice and follow it as a standard going forward for all new public
> > > apis.
> 
> I think this patch is following the best practice.
> 1/ Return negative value in case of error
> 2/ Set rte_errno
> 3/ Set same absolute value in rte_errno and return code

with the approach proposed as best practice above it results in at least the 
applicaiton code variations as follows.

int rv = rte_func_call();

1. if (rv < 0 && rte_errno == EAGAIN)

2. if (rv == -1 && rte_errno == EAGAIN)

3. if (rv < 0 && -rv == EAGAIN)

4. if (rv < 0 && rv == -EAGAIN)

(and incorrectly)

5. // ignore rv
  if (rte_errno == EAGAIN)

it might be better practice if indication that an error occurs is
signaled distinctly from the error that occurred. otherwise why use
rte_errno at all instead returning -rte_errno always?

this philosophy would align better with modern posix / unix platform
apis. often documented in the RETURN VALUE section of the manpage as:

    ``Upon successful completion, somefunction() shall return 0;
      otherwise, -1 shall be returned and errno set to indicate the
      error.''

therefore returning a value outside of the set {0, -1} is an abi break.

separately i have misgivings about how many patches have been integrated
and in some instances backported to dpdk stable that have resulted in
new return values and / or set new values to rte_errno outside of the
set of values initially possible when the dpdk release was made.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-24 17:24       ` Tyler Retzlaff
@ 2021-11-24 18:04         ` Bruce Richardson
  2021-12-01 21:37           ` Tyler Retzlaff
  0 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2021-11-24 18:04 UTC (permalink / raw)
  To: Tyler Retzlaff
  Cc: Thomas Monjalon, eagostini, techboard, dev, Andrew Rybchenko,
	David Marchand, Ferruh Yigit

On Wed, Nov 24, 2021 at 09:24:42AM -0800, Tyler Retzlaff wrote:
> On Fri, Nov 19, 2021 at 10:56:36AM +0100, Thomas Monjalon wrote:
> > 19/11/2021 10:34, Ferruh Yigit:
> > > >> +	if (ptr == NULL) {
> > > >> +		rte_errno = EINVAL;
> > > >> +		return -rte_errno;
> > > >> +	}
> > > > 
> > > > in general dpdk has real problems with how it indicates that an error
> > > > occurred and what error occurred consistently.
> > > > 
> > > > some api's return 0 on success
> > > >    and maybe return -errno if ! 0
> > > >    and maybe return errno if ! 0
> > 
> > Which function returns a positive errno?
> 
> i may have mispoke about this variant, it may be something i recall
> seeing in a posted patch that was resolved before integration.
> 
> > 
> > > >    and maybe set rte_errno if ! 0
> > > > 
> > > > some api's return -1 on failure
> > > >    and set rte_errno if -1
> > > > 
> > > > some api's return < 0 on failure
> > > >    and maybe set rte_errno
> > > >    and maybe return -errno
> > > >    and maybe set rte_errno and return -rte_errno
> > > 
> > > This is a generic comment, cc'ed a few more folks to make the comment more
> > > visible.
> > > 
> > > > this isn't isiolated to only this change but since additions and context
> > > > in this patch highlight it maybe it's a good time to bring it up.
> > > > 
> > > > it's frustrating to have to carefully read the implementation every time
> > > > you want to make a function call to make sure you're handling the flavor
> > > > of error reporting for a particular function.
> > > > 
> > > > if this is new code could we please clearly identify the current best
> > > > practice and follow it as a standard going forward for all new public
> > > > apis.
> > 
> > I think this patch is following the best practice.
> > 1/ Return negative value in case of error
> > 2/ Set rte_errno
> > 3/ Set same absolute value in rte_errno and return code
> 
> with the approach proposed as best practice above it results in at least the 
> applicaiton code variations as follows.
> 
> int rv = rte_func_call();
> 
> 1. if (rv < 0 && rte_errno == EAGAIN)
> 
> 2. if (rv == -1 && rte_errno == EAGAIN)
> 
> 3. if (rv < 0 && -rv == EAGAIN)
> 
> 4. if (rv < 0 && rv == -EAGAIN)
> 
> (and incorrectly)
> 
> 5. // ignore rv
>   if (rte_errno == EAGAIN)
> 
> it might be better practice if indication that an error occurs is
> signaled distinctly from the error that occurred. otherwise why use
> rte_errno at all instead returning -rte_errno always?
> 
> this philosophy would align better with modern posix / unix platform
> apis. often documented in the RETURN VALUE section of the manpage as:
> 
>     ``Upon successful completion, somefunction() shall return 0;
>       otherwise, -1 shall be returned and errno set to indicate the
>       error.''
> 
> therefore returning a value outside of the set {0, -1} is an abi break.
 
I like using this standard, because it also allows consistent behaviour for
non-integer returning functions, e.g. object creation functions returning
pointers.

  if (ret < 0 && rte_errno == EAGAIN)

becomes for a pointer:

  if (ret == NULL && rte_errno == EAGAIN)

Regards,
/Bruce

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-11-24 18:04         ` Bruce Richardson
@ 2021-12-01 21:37           ` Tyler Retzlaff
  2021-12-02  7:18             ` Thomas Monjalon
  0 siblings, 1 reply; 25+ messages in thread
From: Tyler Retzlaff @ 2021-12-01 21:37 UTC (permalink / raw)
  To: Bruce Richardson
  Cc: Thomas Monjalon, eagostini, techboard, dev, Andrew Rybchenko,
	David Marchand, Ferruh Yigit

On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> On Wed, Nov 24, 2021 at 09:24:42AM -0800, Tyler Retzlaff wrote:
> > On Fri, Nov 19, 2021 at 10:56:36AM +0100, Thomas Monjalon wrote:
> > > 19/11/2021 10:34, Ferruh Yigit:
> > > > >> +	if (ptr == NULL) {
> > > > >> +		rte_errno = EINVAL;
> > > > >> +		return -rte_errno;
> > > > >> +	}
> > > > > 
> > > > > in general dpdk has real problems with how it indicates that an error
> > > > > occurred and what error occurred consistently.
> > > > > 
> > > > > some api's return 0 on success
> > > > >    and maybe return -errno if ! 0
> > > > >    and maybe return errno if ! 0
> > > 
> > > Which function returns a positive errno?
> > 
> > i may have mispoke about this variant, it may be something i recall
> > seeing in a posted patch that was resolved before integration.
> > 
> > > 
> > > > >    and maybe set rte_errno if ! 0
> > > > > 
> > > > > some api's return -1 on failure
> > > > >    and set rte_errno if -1
> > > > > 
> > > > > some api's return < 0 on failure
> > > > >    and maybe set rte_errno
> > > > >    and maybe return -errno
> > > > >    and maybe set rte_errno and return -rte_errno
> > > > 
> > > > This is a generic comment, cc'ed a few more folks to make the comment more
> > > > visible.
> > > > 
> > > > > this isn't isiolated to only this change but since additions and context
> > > > > in this patch highlight it maybe it's a good time to bring it up.
> > > > > 
> > > > > it's frustrating to have to carefully read the implementation every time
> > > > > you want to make a function call to make sure you're handling the flavor
> > > > > of error reporting for a particular function.
> > > > > 
> > > > > if this is new code could we please clearly identify the current best
> > > > > practice and follow it as a standard going forward for all new public
> > > > > apis.
> > > 
> > > I think this patch is following the best practice.
> > > 1/ Return negative value in case of error
> > > 2/ Set rte_errno
> > > 3/ Set same absolute value in rte_errno and return code
> > 
> > with the approach proposed as best practice above it results in at least the 
> > applicaiton code variations as follows.
> > 
> > int rv = rte_func_call();
> > 
> > 1. if (rv < 0 && rte_errno == EAGAIN)
> > 
> > 2. if (rv == -1 && rte_errno == EAGAIN)
> > 
> > 3. if (rv < 0 && -rv == EAGAIN)
> > 
> > 4. if (rv < 0 && rv == -EAGAIN)
> > 
> > (and incorrectly)
> > 
> > 5. // ignore rv
> >   if (rte_errno == EAGAIN)
> > 
> > it might be better practice if indication that an error occurs is
> > signaled distinctly from the error that occurred. otherwise why use
> > rte_errno at all instead returning -rte_errno always?
> > 
> > this philosophy would align better with modern posix / unix platform
> > apis. often documented in the RETURN VALUE section of the manpage as:
> > 
> >     ``Upon successful completion, somefunction() shall return 0;
> >       otherwise, -1 shall be returned and errno set to indicate the
> >       error.''
> > 
> > therefore returning a value outside of the set {0, -1} is an abi break.
>  
> I like using this standard, because it also allows consistent behaviour for
> non-integer returning functions, e.g. object creation functions returning
> pointers.
> 
>   if (ret < 0 && rte_errno == EAGAIN)

i only urge that this be explicit as opposed to a range i.e. ret == -1
preferred over ret < 0

> 
> becomes for a pointer:
> 
>   if (ret == NULL && rte_errno == EAGAIN)
> 
> Regards,
> /Bruce

but otherwise i agree, ret indicates an error happened and rte_errno
provides the detail.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-12-01 21:37           ` Tyler Retzlaff
@ 2021-12-02  7:18             ` Thomas Monjalon
  2021-12-02 12:33               ` Morten Brørup
  0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2021-12-02  7:18 UTC (permalink / raw)
  To: Tyler Retzlaff
  Cc: Bruce Richardson, techboard, dev, Andrew Rybchenko,
	David Marchand, Ferruh Yigit

01/12/2021 22:37, Tyler Retzlaff:
> On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> >   if (ret < 0 && rte_errno == EAGAIN)
> 
> i only urge that this be explicit as opposed to a range i.e. ret == -1
> preferred over ret < 0

I don't understand why you think it is important to limit return value to -1.
Why "if (ret == -1)" is better than "if (ret < 0)" ?



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-12-02  7:18             ` Thomas Monjalon
@ 2021-12-02 12:33               ` Morten Brørup
  2021-12-02 13:01                 ` Ananyev, Konstantin
  0 siblings, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2021-12-02 12:33 UTC (permalink / raw)
  To: Thomas Monjalon, Tyler Retzlaff, Bruce Richardson
  Cc: techboard, dev, Andrew Rybchenko, David Marchand, Ferruh Yigit

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 2 December 2021 08.19
> 
> 01/12/2021 22:37, Tyler Retzlaff:
> > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > >   if (ret < 0 && rte_errno == EAGAIN)
> >
> > i only urge that this be explicit as opposed to a range i.e. ret == -
> 1
> > preferred over ret < 0
> 
> I don't understand why you think it is important to limit return value
> to -1.
> Why "if (ret == -1)" is better than "if (ret < 0)" ?

Speaking for myself:

For clarity. It leaves no doubt that "it failed" is represented by the return value -1, and that the function does not return errno values such as -EINVAL.

-Morten


^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-12-02 12:33               ` Morten Brørup
@ 2021-12-02 13:01                 ` Ananyev, Konstantin
  2021-12-02 13:56                   ` Morten Brørup
  0 siblings, 1 reply; 25+ messages in thread
From: Ananyev, Konstantin @ 2021-12-02 13:01 UTC (permalink / raw)
  To: Morten Brørup, Thomas Monjalon, Tyler Retzlaff, Richardson, Bruce
  Cc: techboard, dev, Andrew Rybchenko, David Marchand, Yigit, Ferruh



> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, 2 December 2021 08.19
> >
> > 01/12/2021 22:37, Tyler Retzlaff:
> > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > > >   if (ret < 0 && rte_errno == EAGAIN)
> > >
> > > i only urge that this be explicit as opposed to a range i.e. ret == -
> > 1
> > > preferred over ret < 0
> >
> > I don't understand why you think it is important to limit return value
> > to -1.
> > Why "if (ret == -1)" is better than "if (ret < 0)" ?
> 
> Speaking for myself:
> 
> For clarity. It leaves no doubt that "it failed" is represented by the return value -1, and that the function does not return errno values such as
> -EINVAL.
> 

But why '< 0' gives you less clarity?
Negative value means failure - seems perfectly clear to me.



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
  2021-12-02 13:01                 ` Ananyev, Konstantin
@ 2021-12-02 13:56                   ` Morten Brørup
  0 siblings, 0 replies; 25+ messages in thread
From: Morten Brørup @ 2021-12-02 13:56 UTC (permalink / raw)
  To: Ananyev, Konstantin, Thomas Monjalon, Tyler Retzlaff, Richardson, Bruce
  Cc: techboard, dev, Andrew Rybchenko, David Marchand, Yigit, Ferruh

> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Thursday, 2 December 2021 14.01
> 
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Thursday, 2 December 2021 08.19
> > >
> > > 01/12/2021 22:37, Tyler Retzlaff:
> > > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > > > >   if (ret < 0 && rte_errno == EAGAIN)
> > > >
> > > > i only urge that this be explicit as opposed to a range i.e. ret
> == -
> > > 1
> > > > preferred over ret < 0
> > >
> > > I don't understand why you think it is important to limit return
> value
> > > to -1.
> > > Why "if (ret == -1)" is better than "if (ret < 0)" ?
> >
> > Speaking for myself:
> >
> > For clarity. It leaves no doubt that "it failed" is represented by
> the return value -1, and that the function does not return errno values
> such as
> > -EINVAL.
> >
> 
> But why '< 0' gives you less clarity?
> Negative value means failure - seems perfectly clear to me.

I disagree: Negative value does not mean failure. Only -1 means failure.

There is no -2 return value. There is no -EINVAL return value.

Testing for (ret < 0) might confuse someone to think that other values than -1 could be returned as indication of failure, which is not the case when following the convention where the functions set errno and return -1 in case of failure.

It would be different if following a convention where the functions return -errno in case of failure. In this case, testing (ret < 0) would be appropriate.

So explicitly testing (ret == -1) clarifies which of the two conventions are relevant.



^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-12-02 13:56 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 19:28 [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister eagostini
2021-11-18 16:20 ` Stephen Hemminger
2021-11-18 16:22   ` Elena Agostini
2021-11-18 20:19 ` Tyler Retzlaff
2021-11-19  9:34   ` Ferruh Yigit
2021-11-19  9:56     ` Thomas Monjalon
2021-11-24 17:24       ` Tyler Retzlaff
2021-11-24 18:04         ` Bruce Richardson
2021-12-01 21:37           ` Tyler Retzlaff
2021-12-02  7:18             ` Thomas Monjalon
2021-12-02 12:33               ` Morten Brørup
2021-12-02 13:01                 ` Ananyev, Konstantin
2021-12-02 13:56                   ` Morten Brørup
2021-11-19 10:15     ` Bruce Richardson
2021-11-18 20:33 ` [PATCH v2] gpudev: free and unregister return gracefully if input pointer is NULL eagostini
2021-11-22 18:24   ` [PATCH v3] gpudev: manage NULL pointer eagostini
2021-11-22 11:23     ` Thomas Monjalon
2021-11-22 11:34       ` Elena Agostini
2021-11-22 11:51         ` Thomas Monjalon
2021-11-22 23:52 ` [PATCH v4] " eagostini
2021-11-22 23:55 ` [PATCH v5] " eagostini
2021-11-22 16:01   ` Thomas Monjalon
2021-11-23  0:15 ` [PATCH v6] " eagostini
2021-11-23  0:42 ` [PATCH v7] " eagostini
2021-11-24  8:40   ` Thomas Monjalon

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git