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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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