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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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
  2021-12-08 17:27                   ` Tyler Retzlaff
  0 siblings, 2 replies; 30+ 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] 30+ 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
  2021-12-03 10:37                     ` Morten Brørup
  2021-12-08 17:27                   ` Tyler Retzlaff
  1 sibling, 1 reply; 30+ 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] 30+ messages in thread

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

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday, 2 December 2021 14.56
> 
> > 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.
> 

I tested it on Godbolt, and (ret < 0) produces slightly smaller code than (ret == -1) on x86-64:

https://godbolt.org/z/3xME3jxq8

A binary test (Error or Data) uses 1 byte less, and a tristate test (Error, Zero or Data) uses 3 byte less.

Although there is no measurable performance difference for a single instance of this kind of test, we should consider that this kind of test appears many times in the code, so the saved bytes might add up to something slightly significant in the instruction cache.

My opinion is not so strong anymore... perhaps we should prefer performance over code readability, also in this case?



^ permalink raw reply	[flat|nested] 30+ 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
@ 2021-12-08 17:27                   ` Tyler Retzlaff
  1 sibling, 0 replies; 30+ messages in thread
From: Tyler Retzlaff @ 2021-12-08 17:27 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Morten Brørup, Thomas Monjalon, Richardson, Bruce,
	techboard, dev, Andrew Rybchenko, David Marchand, Yigit, Ferruh

On Thu, Dec 02, 2021 at 01:01:26PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > > 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.
> 

it's ambiguous and the contract allows it to be. being explicit prevents
it. don't mix your signaling with your info. you simply can't have the
following ever happen if you are explicit.

int somefunc(void)
{
    rte_errno = EPERM;
    return -EINVAL;
}

sure this example you can see is obviously wrong but when you start
dealing with callstacks that are propagating errors N levels down it
gets a lot harder to catch the fact that rte_errno wasn't set to -ret.

also there are many apis right now that do return -1 do you propose it
is suddenly valid to start return -rte_errno? when you do expect this
application code to break.

int somefunc3(void)
{
    rte_errno = EPERM;
    return -1;
}

int somefunc2(void *param)
{
    // some work
    return somefunc3();
}

int rv = somefunc2(param)
if (rv == -1)
    // handle rte_errno
else
    // no error

then we get the foolishness that was recently integrated en masse.

--- before.c    2021-12-08 09:22:10.491248400 -0800
+++ after.c     2021-12-08 09:22:45.859431300 -0800
@@ -1,5 +1,8 @@
 int somefunc2(void *param)
 {
+    if (param == NULL)
+        return -EINVAL;
+
     // some work
     return somefunc3();
 }

compatibility breaks happen when some application writes code in a way
you wouldn't expect. everytime this sort of stuff is done you create an
opportunity for compatibility break.

now you can spend your life writing unit tests that somehow exercise
every error path to make sure someone didn't introduce an inconsistent /
unmatching rte_errno to -ret or you can just stop inter-mixing
signalling with info and get rid of the ambiguity.

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

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

On Fri, Dec 03, 2021 at 11:37:10AM +0100, Morten Brørup wrote:
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Thursday, 2 December 2021 14.56
> > 
> > 
> > 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.
> > 
> 
> I tested it on Godbolt, and (ret < 0) produces slightly smaller code than (ret == -1) on x86-64:
> 
> https://godbolt.org/z/3xME3jxq8
> 
> A binary test (Error or Data) uses 1 byte less, and a tristate test (Error, Zero or Data) uses 3 byte less.
> 
> Although there is no measurable performance difference for a single instance of this kind of test, we should consider that this kind of test appears many times in the code, so the saved bytes might add up to something slightly significant in the instruction cache.
> 
> My opinion is not so strong anymore... perhaps we should prefer performance over code readability, also in this case?
> 

i would not expect many calls that return rte_errno to be made on the
hot path. most of the use of errno / rte_errno is control but it's good
to have considered it. if i start seeing a lot of error handling in hot
paths i ordinarily find a way to get rid of it through various
techniques.

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

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

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 8 December 2021 18.35
> 
> On Fri, Dec 03, 2021 at 11:37:10AM +0100, Morten Brørup wrote:
> > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > Sent: Thursday, 2 December 2021 14.56
> > >
> > >
> > > 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.
> > >
> >
> > I tested it on Godbolt, and (ret < 0) produces slightly smaller code
> than (ret == -1) on x86-64:
> >
> > https://godbolt.org/z/3xME3jxq8
> >
> > A binary test (Error or Data) uses 1 byte less, and a tristate test
> (Error, Zero or Data) uses 3 byte less.
> >
> > Although there is no measurable performance difference for a single
> instance of this kind of test, we should consider that this kind of
> test appears many times in the code, so the saved bytes might add up to
> something slightly significant in the instruction cache.
> >
> > My opinion is not so strong anymore... perhaps we should prefer
> performance over code readability, also in this case?
> >
> 
> i would not expect many calls that return rte_errno to be made on the
> hot path. most of the use of errno / rte_errno is control but it's good
> to have considered it. if i start seeing a lot of error handling in hot
> paths i ordinarily find a way to get rid of it through various
> techniques.

Tyler, I think you and I agree perfectly on this topic.

-1 should be returned as error, and rte_errno should provide details.

I'm only saying that comparing the return value with < 0 provides marginally less instruction bytes than comparing it with == -1, so even though -1 is the canonical indication of error, the comparison could be < 0 instead of == -1 (if weighing performance higher than code clarity).


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

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

On Wed, Dec 08, 2021 at 07:40:10PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Wednesday, 8 December 2021 18.35
> > 
> > On Fri, Dec 03, 2021 at 11:37:10AM +0100, Morten Brørup wrote:
> > > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > > Sent: Thursday, 2 December 2021 14.56
> > > >
> > > >
> > > > 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.
> > > >
> > >
> > > I tested it on Godbolt, and (ret < 0) produces slightly smaller code
> > than (ret == -1) on x86-64:
> > >
> > > https://godbolt.org/z/3xME3jxq8
> > >
> > > A binary test (Error or Data) uses 1 byte less, and a tristate test
> > (Error, Zero or Data) uses 3 byte less.
> > >
> > > Although there is no measurable performance difference for a single
> > instance of this kind of test, we should consider that this kind of
> > test appears many times in the code, so the saved bytes might add up to
> > something slightly significant in the instruction cache.
> > >
> > > My opinion is not so strong anymore... perhaps we should prefer
> > performance over code readability, also in this case?
> > >
> > 
> > i would not expect many calls that return rte_errno to be made on the
> > hot path. most of the use of errno / rte_errno is control but it's good
> > to have considered it. if i start seeing a lot of error handling in hot
> > paths i ordinarily find a way to get rid of it through various
> > techniques.
> 
> Tyler, I think you and I agree perfectly on this topic.
> 
> -1 should be returned as error, and rte_errno should provide details.
> 
> I'm only saying that comparing the return value with < 0 provides marginally less instruction bytes than comparing it with == -1, so even though -1 is the canonical indication of error, the comparison could be < 0 instead of == -1 (if weighing performance higher than code clarity).

sounds about right to me, i appreciate the extra analysis. certainly with
explicit -1 it doesn't stop an application gaining the slightly better
code generation with < 0.

thanks

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

end of thread, other threads:[~2021-12-09 19:43 UTC | newest]

Thread overview: 30+ 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-12-03 10:37                     ` Morten Brørup
2021-12-08 17:34                       ` Tyler Retzlaff
2021-12-08 18:40                         ` Morten Brørup
2021-12-09 19:43                           ` Tyler Retzlaff
2021-12-08 17:27                   ` Tyler Retzlaff
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