On Wed, Dec 08, 2021 at 07:40:10PM +0100, Morten Brørup wrote:

quoted

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:

quoted

quoted

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

quoted

quoted

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

the

quoted

quoted

case when following the convention where the functions set errno

and

quoted

quoted

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

quoted

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

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

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

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

performance over code readability, also in this case?

quoted

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