Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Stephen Hemminger]

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.

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Elena Agostini]

[-- 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 --]

[PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[eagostini]

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

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Tyler Retzlaff]

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!

[PATCH v2] gpudev: free and unregister return gracefully if input pointer is NULL

[eagostini]

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

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[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
>    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!
> 

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Thomas Monjalon]

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

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Bruce Richardson]

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

Re: [PATCH v3] gpudev: manage NULL pointer

[Thomas Monjalon]

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

Re: [PATCH v3] gpudev: manage NULL pointer

[Elena Agostini]

[-- 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 --]

Re: [PATCH v3] gpudev: manage NULL pointer

[Thomas Monjalon]

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.

Re: [PATCH v5] gpudev: manage NULL pointer

[Thomas Monjalon]

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.

[PATCH v3] gpudev: manage NULL pointer

[eagostini]

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

[PATCH v4] gpudev: manage NULL pointer

[eagostini]

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

[PATCH v5] gpudev: manage NULL pointer

[eagostini]

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

[PATCH v6] gpudev: manage NULL pointer

[eagostini]

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

[PATCH v7] gpudev: manage NULL pointer

[eagostini]

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

Re: [PATCH v7] gpudev: manage NULL pointer

[Thomas Monjalon]

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.

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Tyler Retzlaff]

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.

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Bruce Richardson]

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

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Tyler Retzlaff]

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.

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Thomas Monjalon]

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)" ?

RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Morten Brørup]

> 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

RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Ananyev, Konstantin]

> > 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.

RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Morten Brørup]

> 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.

RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Morten Brørup]

> 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?

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Tyler Retzlaff]

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.

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Tyler Retzlaff]

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.

RE: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Morten Brørup]

> 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).

Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

[Tyler Retzlaff]

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