linux-kernel - Re: [RFC] Optimising IS_ERR_OR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <534025D9.7030204@ahsoftware.de>
Date:	Sat, 05 Apr 2014 17:48:41 +0200
From:	Alexander Holler <holler@...oftware.de>
To:	Matthew Wilcox <willy@...ux.intel.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC] Optimising IS_ERR_OR_NULL

Am 05.04.2014 16:43, schrieb Matthew Wilcox:
>
> (4 days too late for April Fools ... oh well :-)
>
> I don't like the look of IS_ERR_OR_NULL.  It does two tests when (due to
> the bit patterns used to represent errors and NULL pointers) it could
> use just one:
>
> #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>
> static inline long __must_check IS_ERR_OR_NULL(__force const void *ptr)
> {
> 	return !ptr || IS_ERR_VALUE((unsigned long)ptr);
> }
>
> It needs some performance testing to be sure that it's a win, but I'm
> essentially suggesting this:
>
> +++ b/include/linux/err.h
> @@ -34,10 +34,8 @@ static inline long __must_check IS_ERR(__force const void *pt
>          return IS_ERR_VALUE((unsigned long)ptr);
>   }
>
> -static inline long __must_check IS_ERR_OR_NULL(__force const void *ptr)
> -{
> -       return !ptr || IS_ERR_VALUE((unsigned long)ptr);
> -}
> +#define IS_ERR_OR_NULL(ptr) \
> +       unlikely(((unsigned long)PTR_ERR(ptr) + MAX_ERRNO) <= MAX_ERRNO)
>
>   /**
>    * ERR_CAST - Explicitly cast an error-valued pointer to another pointer type
>
> (deliberately whitespace-damaged to ensure it doesn't get applied
> without thought).
>
> Comments, before I go off and run some perf tests?
>

(... x86 asm)

> Now that's a genuine improvement; we saved one instruction (lea, cmp,
> jbe vs test, je, cmp, ja), and due to various alignment / padding / etc.
> we ended up saving 4 bytes on the length of the function which turns
> into 16 bytes due to function alignment.

A first comment, if that really will be changed, please leave to old 
function as comment for the lazy reader. The new one is pretty ugly and 
needs a turned on brain to understand (besides that the new one discards 
the __must_check, but I would assume that no one uses IS_ERR_OR_NULL() 
without checking the return value).

As I just was a bit bored, here's what happens on ARM so that others 
don't have to compile it on ARM:

armv5 old:

000011e0 <kern_unmount>:
     11e0:       e92d4010        push    {r4, lr}
     11e4:       e2504000        subs    r4, r0, #0
     11e8:       0a000007        beq     120c <kern_unmount+0x2c>
     11ec:       e3740a01        cmn     r4, #4096       ; 0x1000
     11f0:       8a000005        bhi     120c <kern_unmount+0x2c>
     11f4:       e3a03000        mov     r3, #0
     11f8:       e5843064        str     r3, [r4, #100]  ; 0x64
     11fc:       ebfffffe        bl      0 <synchronize_rcu>
     1200:       e1a00004        mov     r0, r4
     1204:       e8bd4010        pop     {r4, lr}
     1208:       eafffffe        b       c78 <mntput>
     120c:       e8bd8010        pop     {r4, pc}

armv5 new:

00000c98 <kern_unmount>:
      c98:       e2803eff        add     r3, r0, #4080   ; 0xff0
      c9c:       e283300f        add     r3, r3, #15
      ca0:       e3530a01        cmp     r3, #4096       ; 0x1000
      ca4:       e92d4010        push    {r4, lr}
      ca8:       e1a04000        mov     r4, r0
      cac:       3a000005        bcc     cc8 <kern_unmount+0x30>
      cb0:       e3a03000        mov     r3, #0
      cb4:       e5803064        str     r3, [r0, #100]  ; 0x64
      cb8:       ebfffffe        bl      0 <synchronize_rcu>
      cbc:       e1a00004        mov     r0, r4
      cc0:       e8bd4010        pop     {r4, lr}
      cc4:       eafffffe        b       c78 <mntput>
      cc8:       e8bd8010        pop     {r4, pc}

And armv7 old:

00000e60 <kern_unmount>:
      e60:       e92d4010        push    {r4, lr}
      e64:       e2504000        subs    r4, r0, #0
      e68:       08bd8010        popeq   {r4, pc}
      e6c:       e3740a01        cmn     r4, #4096       ; 0x1000
      e70:       88bd8010        pophi   {r4, pc}
      e74:       e3a03000        mov     r3, #0
      e78:       e5843064        str     r3, [r4, #100]  ; 0x64
      e7c:       ebfffffe        bl      0 <synchronize_sched>
      e80:       e1a00004        mov     r0, r4
      e84:       e8bd4010        pop     {r4, lr}
      e88:       eafffffe        b       a3c <mntput>

armv7 new:

00000a5c <kern_unmount>:
      a5c:       e2803eff        add     r3, r0, #4080   ; 0xff0
      a60:       e283300f        add     r3, r3, #15
      a64:       e3530a01        cmp     r3, #4096       ; 0x1000
      a68:       e92d4010        push    {r4, lr}
      a6c:       e1a04000        mov     r4, r0
      a70:       38bd8010        popcc   {r4, pc}
      a74:       e3a03000        mov     r3, #0
      a78:       e5803064        str     r3, [r0, #100]  ; 0x64
      a7c:       ebfffffe        bl      0 <synchronize_sched>
      a80:       e1a00004        mov     r0, r4
      a84:       e8bd4010        pop     {r4, lr}
      a88:       eafffffe        b       a3c <mntput>

Unfortunately I'm bad in interpreting assembler (I prefer higher 
languages like C++), therefor I don't even try it and leave further 
comments on the ARM assembler to other people. ;)

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/