linux-kernel - Re: [PATCH] enforce function inlining for hot functions

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150425103141.GG5561@linux.vnet.ibm.com>
Date:	Sat, 25 Apr 2015 03:31:41 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Hagen Paul Pfeifer <hagen@...u.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"David S. Miller" <davem@...emloft.net>, x86@...nel.org
Subject: Re: [PATCH] enforce function inlining for hot functions

On Sat, Apr 25, 2015 at 01:10:56AM +0200, Hagen Paul Pfeifer wrote:
> * Paul E. McKenney | 2015-04-24 13:13:40 [-0700]:
> 
> >Hmmm...  allyesconfig would have PROVE_RCU=y, which would mean that the
> >above two would contain lockdep calls that might in some cases defeat
> >inlining.  With the more typical production choice of PROVE_RCU=n, I would
> >expect these to just be a call instruction, which should get inlined.
> 
> 
> Ok, here are the results:
> 
> with PROVE_RCU=y:
>     rcu_read_lock: 383 duplicates
> with PROVE_RCU=n:
>     rcu_read_lock: 114 duplicates
> 
> 
> If you look at the function anatomy of rcu_read_lock you often see the
> following definitions:
> 
> <rcu_read_lock>:
>  55                        push   %rbp
>  48 89 e5                  mov    %rsp,%rbp
>  48 c7 c7 50 64 e7 85      mov    $0xffffffff85e76450,%rdi
>  e8 ce ff ff ff            callq  ffffffff816af206 <rcu_lock_acquire>
>  5d                        pop    %rbp
>  c3                        retq

OK, so you have PROVE_RCU=n and CONFIG_DEBUG_LOCK_ALLOC=y in this
case?  That would get rid of the rcu_lockdep_assert(), but keep the
rcu_lock_acquire().

> but sometimes rcu_read_lock looks:
> 
> <rcu_read_lock>:
>  55                        push   %rbp
>  48 89 e5                  mov    %rsp,%rbp
>  50                        push   %rax
>  68 83 1e 1c 81            pushq  $0xffffffff811c1e83
>  b9 02 00 00 00            mov    $0x2,%ecx
>  31 d2                     xor    %edx,%edx
>  45 31 c9                  xor    %r9d,%r9d
>  45 31 c0                  xor    %r8d,%r8d
>  31 f6                     xor    %esi,%esi
>  48 c7 c7 50 64 e7 85      mov    $0xffffffff85e76450,%rdi
>  e8 86 4c f9 ff            callq  ffffffff81156b2e <lock_acquire>
>  5a                        pop    %rdx
>  59                        pop    %rcx
>  c9                        leaveq   
>  c3                        retq
> 
> 
> Means rcu_lock_acquire() is inlined here - but not in every compilation unit.
> Don't know exactly what forces gcc to inline not everywhere. Maybe register
> pressure in the function unit, or at least gcc is think that. I don't know.
> 
> At the end you may notice that gcc inlining decisions are not always perfect
> and a little bit fuzzy (sure, they have their metric/scoring system). And
> sometimes the inlining should be enforced - as this patch do for some important
> functions. But as I said we should not enforce it everywhere, rather we should
> pray for better heuristics and let the compiler choose the best strategy (and
> incorporate -Os/-O2 decisions too). I think this is the best compromise here.

I am not arguing either way on the wisdom or lack thereof of gcc's
inlining decisions.  But PROVE_RCU=n and CONFIG_DEBUG_LOCK_ALLOC=n should
make rcu_read_lock() and rcu_read_unlock() both be empty functions in
a CONFIG_PREEMPT=n, which should hopefully trivialize gcc's inlining
decisions in that particular case.

Apologies for not identifying CONFIG_DEBUG_LOCK_ALLOC=n to begin with.

							Thanx, Paul

--
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/