Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda <a.hajda@samsung.com>]

On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
>>
>>> diff --git a/include/linux/err.h b/include/linux/err.h
>>> index b7d4a9ff6342..bd4936a2c352 100644
>>> --- a/include/linux/err.h
>>> +++ b/include/linux/err.h
>>> @@ -18,9 +18,7 @@
>>>  
>>>  #ifndef __ASSEMBLY__
>>>  
>>> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>>> -                             ? unlikely((x) <= -1) \
>>> -                             : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>> +#define IS_ERR_VALUE(x)      (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>>>  
>>>  static inline void * __must_check ERR_PTR(long error)
>>>  {
>>>
>>>
>>> I'm not sure if the cast to 'unsigned long long' might cause less
>>> efficient code to be generated by gcc. I would hope that it is smart
>>> enough to not actually extend shorter variables to 64 bit before
>>> doing the comparison but I have not checked yet.
>> I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
>> the text size from 9098 bytes to 9133.  That file has 11 instances of
>> IS_ERR_VALUE().  
> This seems to be because it brings back the logic to what it was before
> in case of 'int' arguments. I checked the assembly output and found mine
> to be identical to v4.4 in this case:
>
> 	 text    data     bss     dec     hex filename
> v4.4	 9942    1872    2856   14670    394e drivers/nvmem/core.o
> a.hajda  9922    1872    2856   14650    393a drivers/nvmem/core.o
> arnd	 9942    1872    2856   14670    394e drivers/nvmem/core.o

I have compared all proposed version with all compilers I have:

            text       data        bss        dec        hex    filename
gcc-4.4
old         8188       1016       2968      12172       2f8c   
.x86/drivers/nvmem/core.o
andrzej     8155       1016       2968      12139       2f6b   
.x86/drivers/nvmem/core.o
arnd        8188       1016       2968      12172       2f8c   
.x86/drivers/nvmem/core.o
rasmus      8266       1016       2968      12250       2fda   
.x86/drivers/nvmem/core.o
 ---
gcc-4.7
old         7642       3816       3248      14706       3972   
.x86/drivers/nvmem/core.o
andrzej     7606       3816       3248      14670       394e   
.x86/drivers/nvmem/core.o
arnd        7642       3816       3248      14706       3972   
.x86/drivers/nvmem/core.o
rasmus      7719       3816       3248      14783       39bf   
.x86/drivers/nvmem/core.o
 ---
gcc-4.8
old         7735       3888       3272      14895       3a2f   
.x86/drivers/nvmem/core.o
andrzej     7698       3888       3272      14858       3a0a   
.x86/drivers/nvmem/core.o
arnd        7735       3888       3272      14895       3a2f   
.x86/drivers/nvmem/core.o
rasmus      7812       3888       3272      14972       3a7c   
.x86/drivers/nvmem/core.o
 ---
arm-linux-gnueabi-gcc-4.7
old        12776       1680       3432      17888       45e0   
.arm/drivers/nvmem/core.o
andrzej    12772       1680       3432      17884       45dc   
.arm/drivers/nvmem/core.o
arnd       12776       1680       3432      17888       45e0   
.arm/drivers/nvmem/core.o
rasmus     12948       1680       3432      18060       468c   
.arm/drivers/nvmem/core.o
 ---
aarch64-linux-gnu-gcc-4.8
old         5967        440         48       6455       1937   
.arm64/drivers/nvmem/core.o
andrzej     5947        440         48       6435       1923   
.arm64/drivers/nvmem/core.o
arnd        5967        440         48       6455       1937   
.arm64/drivers/nvmem/core.o
rasmus      5991        440         48       6479       194f   
.arm64/drivers/nvmem/core.o
 ---

My version produces shortest code, Arnd's is the same as the old one.
On the other side Rasmus proposition seems to be the most straightforward
to me. Anyway I am not sure if the code length is the most important here.

By the way .data segment size grows almost 4 times between gcc 4.4 and
4.8 :)
Also numbers for arm64 looks interesting.

Just for the record below all proposed implementations:
#define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
#define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
                                ? unlikely((x) <= -1) \
                                : unlikely((x) >= (typeof(x))-MAX_ERRNO))
#define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
(unsigned long long)(typeof(x))-MAX_ERRNO))
#define IS_ERR_VALUE_rasmus(x) ({\
        typeof(x) _x = (x);\
        unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
})

>
> Andrzej's version is a little shorter on ARM because in case of signed numbers
> it only checks for negative values, rather than checking for values in the
> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
> in this case, and my version restores it.

As I looked at the usage of the macro in the kernel I have not found any
code
which could benefit from the original behavior, except some buggy code in
staging which have already pending fix[1].
But maybe it would be better to use IS_ERR_VALUE to always check if err
is in
range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
signed
types.

[1]: http://permalink.gmane.org/gmane.comp.file-systems.lustre.devel/4164

Regards
Andrzej