gpudev: return EINVAL if invalid input pointer for free and unregister

From: Elena Agostini <[email protected]>

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

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

Signed-off-by Elena Agostini <[email protected]>

---
 drivers/gpu/cuda/cuda.c | 6 ------
 lib/gpudev/gpudev.c     | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

From: Thomas Monjalon <[email protected]>
Date: Monday, 22 November 2021 at 12:23
To: Elena Agostini <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [PATCH v3] gpudev: manage NULL pointer External email: Use caution opening links or attachments>

22/11/2021 19:24, [email protected]:

Quoted 3 lines

From: Elena Agostini <[email protected]>

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

Quoted 1 line

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

It should be:

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

Quoted 6 lines

Signed-off-by Elena Agostini <[email protected]>

---
 drivers/gpu/cuda/cuda.c | 6 ------
 lib/gpudev/gpudev.c     | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

[...]

Quoted 6 lines

lib/gpudev/gpudev.c +0 -0

@@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 {
      struct rte_gpu *dev;
 

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr)

Quoted 6 lines

lib/gpudev/gpudev.c +3 -0

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

lib/gpudev/gpudev.c rte_gpu_mem_unregister(int16_t dev_id, void *ptr) · line 623

Quoted 10 lines

lib/gpudev/gpudev.c +3 -0

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

From: Thomas Monjalon <[email protected]>

Quoted 7 lines

22/11/2021 19:24, [email protected]:

Quoted 6 lines

lib/gpudev/gpudev.c +0 -0

@@ -569,6 +569,9 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
 {
      struct rte_gpu *dev;
 

Quoted 8 lines

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr)

Quoted 6 lines

lib/gpudev/gpudev.c +3 -0

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

From: Stephen Hemminger <[email protected]>
Date: Thursday, 18 November 2021 at 17:21
To: Elena Agostini <[email protected]>
Cc: [email protected] <[email protected]>
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
<[email protected]> wrote:>

Quoted 8 lines

lib/gpudev/gpudev.c +0 -0

index 2b174d8bd5..97575ed979 100644
@@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
              return -rte_errno;
      }
 

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr)

Quoted 5 lines

lib/gpudev/gpudev.c +5 -0

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

From: Elena Agostini <[email protected]>

Signed-off-by Elena Agostini <[email protected]>

---
 lib/gpudev/gpudev.c     | 10 ++++++++++
 lib/gpudev/rte_gpudev.h |  2 ++
 2 files changed, 12 insertions(+)

lib/gpudev/gpudev.c +0 -0

index 2b174d8bd5..97575ed979 100644

Quoted 13 lines · On Thu, Nov 18, 2021 at 07:28:02PM +0000, [email protected] wrote:

From: Elena Agostini <[email protected]>

Signed-off-by Elena Agostini <[email protected]>

---
  lib/gpudev/gpudev.c     | 10 ++++++++++
  lib/gpudev/rte_gpudev.h |  2 ++
  2 files changed, 12 insertions(+)

lib/gpudev/gpudev.c +0 -0

index 2b174d8bd5..97575ed979 100644

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr) · line 582

Quoted 7 lines

lib/gpudev/gpudev.c +4 -0

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

Quoted 25 lines · On Thu, Nov 18, 2021 at 07:28:02PM +0000, [email protected] wrote:

Quoted 13 lines

From: Elena Agostini <[email protected]>

Signed-off-by Elena Agostini <[email protected]>

---
  lib/gpudev/gpudev.c     | 10 ++++++++++
  lib/gpudev/rte_gpudev.h |  2 ++
  2 files changed, 12 insertions(+)

lib/gpudev/gpudev.c +0 -0

index 2b174d8bd5..97575ed979 100644

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr) · line 582

Quoted 7 lines

lib/gpudev/gpudev.c +4 -0

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

Quoted 3 lines

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

Quoted 5 lines

and maybe set rte_errno if ! 0

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

Quoted 4 lines

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.

Quoted 10 lines

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.

19/11/2021 10:34, Ferruh Yigit:

Quoted 8 lines

Quoted 8 lines

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr) · line 582

Quoted 4 lines

lib/gpudev/gpudev.c +4 -0

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

Quoted 3 lines

Quoted 3 lines

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?

Quoted 21 lines

Quoted 10 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

Quoted 3 lines

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

Quoted 9 lines · Thomas Monjalon

19/11/2021 10:34, Ferruh Yigit:

Quoted 8 lines

Quoted 8 lines

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr) · line 582

Quoted 4 lines

lib/gpudev/gpudev.c +4 -0

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

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 21 lines

Quoted 21 lines

Quoted 10 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 78 lines · Tyler Retzlaff

Quoted 9 lines · Thomas Monjalon

19/11/2021 10:34, Ferruh Yigit:

Quoted 8 lines

Quoted 8 lines

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr) · line 582

Quoted 4 lines

lib/gpudev/gpudev.c +4 -0

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

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 21 lines

Quoted 21 lines

Quoted 10 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 1 line

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

Quoted 1 line

  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

From: Thomas Monjalon [mailto:[email protected]]
Sent: Thursday, 2 December 2021 08.19

01/12/2021 22:37, Tyler Retzlaff:

Quoted 3 lines · Bruce Richardson

Quoted 1 line

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

Quoted 1 line

i only urge that this be explicit as opposed to a range i.e. ret == -

1

Quoted 1 line

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

Quoted 13 lines

From: Thomas Monjalon [mailto:[email protected]]
Sent: Thursday, 2 December 2021 08.19

01/12/2021 22:37, Tyler Retzlaff:

Quoted 4 lines · Bruce Richardson

Quoted 1 line

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

i only urge that this be explicit as opposed to a range i.e. ret == -

1

Quoted 1 line

preferred over ret < 0

Quoted 3 lines

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.

From: Ananyev, Konstantin [mailto:[email protected]]
Sent: Thursday, 2 December 2021 14.01

Quoted 8 lines

Quoted 8 lines

From: Thomas Monjalon [mailto:[email protected]]
Sent: Thursday, 2 December 2021 08.19

01/12/2021 22:37, Tyler Retzlaff:

Quoted 4 lines · Bruce Richardson

Quoted 1 line

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

i only urge that this be explicit as opposed to a range i.e. ret

== -

Quoted 4 lines

Quoted 4 lines

1

Quoted 1 line

preferred over ret < 0

I don't understand why you think it is important to limit return

value

Quoted 5 lines

Quoted 2 lines

to -1.
Why "if (ret == -1)" is better than "if (ret < 0)" ?

Speaking for myself:

Quoted 1 line

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

Quoted 2 lines

-EINVAL.

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

From: Morten Brørup [mailto:[email protected]]
Sent: Thursday, 2 December 2021 14.56

Quoted 8 lines

From: Ananyev, Konstantin [mailto:[email protected]]
Sent: Thursday, 2 December 2021 14.01

Quoted 5 lines

Quoted 5 lines

From: Thomas Monjalon [mailto:[email protected]]
Sent: Thursday, 2 December 2021 08.19

01/12/2021 22:37, Tyler Retzlaff:

Quoted 1 line

On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson

Quoted 3 lines · wrote:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

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

i only urge that this be explicit as opposed to a range i.e.

ret

Quoted 5 lines

== -

Quoted 4 lines

Quoted 4 lines

1

Quoted 1 line

preferred over ret < 0

I don't understand why you think it is important to limit return

Quoted 8 lines

value

Quoted 6 lines

Quoted 2 lines

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

Quoted 6 lines

such as

Quoted 2 lines

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

Quoted 14 lines

From: Morten Brørup [mailto:[email protected]]
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.

Quoted 7 lines

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?

From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 8 December 2021 18.35

Quoted 10 lines · Morten Brørup

Quoted 10 lines

From: Morten Brørup [mailto:[email protected]]
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

Quoted 1 line

Quoted 1 line

than -1 could be returned as indication of failure, which is not

the

Quoted 1 line

Quoted 1 line

case when following the convention where the functions set errno

and

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

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:

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

My opinion is not so strong anymore... perhaps we should prefer

performance over code readability, also in this case?

Quoted 1 line

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.

Quoted 44 lines

From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 8 December 2021 18.35

Quoted 10 lines · Morten Brørup

Quoted 10 lines

From: Morten Brørup [mailto:[email protected]]
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

Quoted 1 line

Quoted 1 line

than -1 could be returned as indication of failure, which is not

the

Quoted 1 line

Quoted 1 line

case when following the convention where the functions set errno

and

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

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:

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

My opinion is not so strong anymore... perhaps we should prefer

Quoted 8 lines

performance over code readability, also in this case?

Quoted 1 line

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

Quoted 17 lines

Quoted 16 lines

From: Thomas Monjalon [mailto:[email protected]]
Sent: Thursday, 2 December 2021 08.19

01/12/2021 22:37, Tyler Retzlaff:

Quoted 4 lines · Bruce Richardson

Quoted 1 line

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

i only urge that this be explicit as opposed to a range i.e. ret == -

1

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 32 lines · Tyler Retzlaff

Quoted 13 lines · On Thu, Nov 18, 2021 at 07:28:02PM +0000, [email protected] wrote:

From: Elena Agostini <[email protected]>

Signed-off-by Elena Agostini <[email protected]>

---
  lib/gpudev/gpudev.c     | 10 ++++++++++
  lib/gpudev/rte_gpudev.h |  2 ++
  2 files changed, 12 insertions(+)

lib/gpudev/gpudev.c +0 -0

index 2b174d8bd5..97575ed979 100644

lib/gpudev/gpudev.c rte_gpu_mem_free(int16_t dev_id, void *ptr) · line 581

Quoted 6 lines

lib/gpudev/gpudev.c +4 -0

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

Quoted 5 lines

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.

From: Elena Agostini <[email protected]>

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.

From: Elena Agostini <[email protected]>

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