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

["Morten Brørup" <mb@smartsharesystems.com>]

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