* [PATCH] asm-generic: force inlining of some atomic_long operations
@ 2016-02-04 19:45 Denys Vlasenko
2016-02-04 19:45 ` [PATCH] force inlining of some byteswap operations Denys Vlasenko
2016-02-04 19:45 ` [PATCH] force inlining of unaligned byteswap operations Denys Vlasenko
0 siblings, 2 replies; 45+ messages in thread
From: Denys Vlasenko @ 2016-02-04 19:45 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel
Sometimes gcc mysteriously doesn't inline
very small functions we expect to be inlined. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
With this .config:
http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
atomic_long_inc(), atomic_long_dec() and atomic_long_add()
functions get deinlined about 40 times. Examples of disassembly:
<atomic_long_inc> (21 copies, 147 calls):
55 push %rbp
48 89 e5 mov %rsp,%rbp
f0 48 ff 07 lock incq (%rdi)
5d pop %rbp
c3 retq
<atomic_long_dec> (4 copies, 14 calls) is similar to inc.
<atomic_long_add> (11 copies, 41 calls):
55 push %rbp
48 89 e5 mov %rsp,%rbp
f0 48 01 3e lock add %rdi,(%rsi)
5d pop %rbp
c3 retq
This patch fixes this via s/inline/__always_inline/.
Code size decrease after the patch is ~1.3k:
text data bss dec hex filename
92203657 20826112 36417536 149447305 8e86289 vmlinux
92202377 20826112 36417536 149446025 8e85d89 vmlinux4_atomiclong_after
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
---
include/asm-generic/atomic-long.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/asm-generic/atomic-long.h b/include/asm-generic/atomic-long.h
index eb1973b..5e1f345 100644
--- a/include/asm-generic/atomic-long.h
+++ b/include/asm-generic/atomic-long.h
@@ -98,14 +98,14 @@ ATOMIC_LONG_ADD_SUB_OP(sub, _release)
#define atomic_long_xchg(v, new) \
(ATOMIC_LONG_PFX(_xchg)((ATOMIC_LONG_PFX(_t) *)(v), (new)))
-static inline void atomic_long_inc(atomic_long_t *l)
+static __always_inline void atomic_long_inc(atomic_long_t *l)
{
ATOMIC_LONG_PFX(_t) *v = (ATOMIC_LONG_PFX(_t) *)l;
ATOMIC_LONG_PFX(_inc)(v);
}
-static inline void atomic_long_dec(atomic_long_t *l)
+static __always_inline void atomic_long_dec(atomic_long_t *l)
{
ATOMIC_LONG_PFX(_t) *v = (ATOMIC_LONG_PFX(_t) *)l;
@@ -113,7 +113,7 @@ static inline void atomic_long_dec(atomic_long_t *l)
}
#define ATOMIC_LONG_OP(op) \
-static inline void \
+static __always_inline void \
atomic_long_##op(long i, atomic_long_t *l) \
{ \
ATOMIC_LONG_PFX(_t) *v = (ATOMIC_LONG_PFX(_t) *)l; \
--
1.8.1.4
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH] force inlining of some byteswap operations
2016-02-04 19:45 [PATCH] asm-generic: force inlining of some atomic_long operations Denys Vlasenko
@ 2016-02-04 19:45 ` Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
2016-04-13 3:36 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf
2016-02-04 19:45 ` [PATCH] force inlining of unaligned byteswap operations Denys Vlasenko
1 sibling, 2 replies; 45+ messages in thread
From: Denys Vlasenko @ 2016-02-04 19:45 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel
Sometimes gcc mysteriously doesn't inline
very small functions we expect to be inlined. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
With this .config:
http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
the following functions get deinlined many times.
Examples of disassembly:
<get_unaligned_be16> (12 copies, 51 calls):
66 8b 07 mov (%rdi),%ax
55 push %rbp
48 89 e5 mov %rsp,%rbp
86 e0 xchg %ah,%al
5d pop %rbp
c3 retq
<get_unaligned_be32> (12 copies, 135 calls):
8b 07 mov (%rdi),%eax
55 push %rbp
48 89 e5 mov %rsp,%rbp
0f c8 bswap %eax
5d pop %rbp
c3 retq
<get_unaligned_be64> (2 copies, 20 calls):
48 8b 07 mov (%rdi),%rax
55 push %rbp
48 89 e5 mov %rsp,%rbp
48 0f c8 bswap %rax
5d pop %rbp
c3 retq
<__swab16p> (16 copies, 146 calls):
55 push %rbp
89 f8 mov %edi,%eax
86 e0 xchg %ah,%al
48 89 e5 mov %rsp,%rbp
5d pop %rbp
c3 retq
<__swab32p> (43 copies, ~560 calls):
55 push %rbp
89 f8 mov %edi,%eax
0f c8 bswap %eax
48 89 e5 mov %rsp,%rbp
5d pop %rbp
c3 retq
<__swab64p> (21 copies, 119 calls):
55 push %rbp
48 89 f8 mov %rdi,%rax
48 0f c8 bswap %rax
48 89 e5 mov %rsp,%rbp
5d pop %rbp
c3 retq
<__swab32s> (6 copies, 47 calls):
8b 07 mov (%rdi),%eax
55 push %rbp
48 89 e5 mov %rsp,%rbp
0f c8 bswap %eax
89 07 mov %eax,(%rdi)
5d pop %rbp
c3 retq
This patch fixes this via s/inline/__always_inline/.
Code size decrease after the patch is ~4.5k:
text data bss dec hex filename
92202377 20826112 36417536 149446025 8e85d89 vmlinux
92197848 20826112 36417536 149441496 8e84bd8 vmlinux5_swap_after
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
---
include/uapi/linux/byteorder/big_endian.h | 24 ++++++++++++------------
include/uapi/linux/byteorder/little_endian.h | 24 ++++++++++++------------
include/uapi/linux/swab.h | 10 +++++-----
3 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/include/uapi/linux/byteorder/big_endian.h b/include/uapi/linux/byteorder/big_endian.h
index 6723744..cdab17a 100644
--- a/include/uapi/linux/byteorder/big_endian.h
+++ b/include/uapi/linux/byteorder/big_endian.h
@@ -40,51 +40,51 @@
#define __cpu_to_be16(x) ((__force __be16)(__u16)(x))
#define __be16_to_cpu(x) ((__force __u16)(__be16)(x))
-static inline __le64 __cpu_to_le64p(const __u64 *p)
+static __always_inline __le64 __cpu_to_le64p(const __u64 *p)
{
return (__force __le64)__swab64p(p);
}
-static inline __u64 __le64_to_cpup(const __le64 *p)
+static __always_inline __u64 __le64_to_cpup(const __le64 *p)
{
return __swab64p((__u64 *)p);
}
-static inline __le32 __cpu_to_le32p(const __u32 *p)
+static __always_inline __le32 __cpu_to_le32p(const __u32 *p)
{
return (__force __le32)__swab32p(p);
}
-static inline __u32 __le32_to_cpup(const __le32 *p)
+static __always_inline __u32 __le32_to_cpup(const __le32 *p)
{
return __swab32p((__u32 *)p);
}
-static inline __le16 __cpu_to_le16p(const __u16 *p)
+static __always_inline __le16 __cpu_to_le16p(const __u16 *p)
{
return (__force __le16)__swab16p(p);
}
-static inline __u16 __le16_to_cpup(const __le16 *p)
+static __always_inline __u16 __le16_to_cpup(const __le16 *p)
{
return __swab16p((__u16 *)p);
}
-static inline __be64 __cpu_to_be64p(const __u64 *p)
+static __always_inline __be64 __cpu_to_be64p(const __u64 *p)
{
return (__force __be64)*p;
}
-static inline __u64 __be64_to_cpup(const __be64 *p)
+static __always_inline __u64 __be64_to_cpup(const __be64 *p)
{
return (__force __u64)*p;
}
-static inline __be32 __cpu_to_be32p(const __u32 *p)
+static __always_inline __be32 __cpu_to_be32p(const __u32 *p)
{
return (__force __be32)*p;
}
-static inline __u32 __be32_to_cpup(const __be32 *p)
+static __always_inline __u32 __be32_to_cpup(const __be32 *p)
{
return (__force __u32)*p;
}
-static inline __be16 __cpu_to_be16p(const __u16 *p)
+static __always_inline __be16 __cpu_to_be16p(const __u16 *p)
{
return (__force __be16)*p;
}
-static inline __u16 __be16_to_cpup(const __be16 *p)
+static __always_inline __u16 __be16_to_cpup(const __be16 *p)
{
return (__force __u16)*p;
}
diff --git a/include/uapi/linux/byteorder/little_endian.h b/include/uapi/linux/byteorder/little_endian.h
index d876736..4b93f2b 100644
--- a/include/uapi/linux/byteorder/little_endian.h
+++ b/include/uapi/linux/byteorder/little_endian.h
@@ -40,51 +40,51 @@
#define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
#define __be16_to_cpu(x) __swab16((__force __u16)(__be16)(x))
-static inline __le64 __cpu_to_le64p(const __u64 *p)
+static __always_inline __le64 __cpu_to_le64p(const __u64 *p)
{
return (__force __le64)*p;
}
-static inline __u64 __le64_to_cpup(const __le64 *p)
+static __always_inline __u64 __le64_to_cpup(const __le64 *p)
{
return (__force __u64)*p;
}
-static inline __le32 __cpu_to_le32p(const __u32 *p)
+static __always_inline __le32 __cpu_to_le32p(const __u32 *p)
{
return (__force __le32)*p;
}
-static inline __u32 __le32_to_cpup(const __le32 *p)
+static __always_inline __u32 __le32_to_cpup(const __le32 *p)
{
return (__force __u32)*p;
}
-static inline __le16 __cpu_to_le16p(const __u16 *p)
+static __always_inline __le16 __cpu_to_le16p(const __u16 *p)
{
return (__force __le16)*p;
}
-static inline __u16 __le16_to_cpup(const __le16 *p)
+static __always_inline __u16 __le16_to_cpup(const __le16 *p)
{
return (__force __u16)*p;
}
-static inline __be64 __cpu_to_be64p(const __u64 *p)
+static __always_inline __be64 __cpu_to_be64p(const __u64 *p)
{
return (__force __be64)__swab64p(p);
}
-static inline __u64 __be64_to_cpup(const __be64 *p)
+static __always_inline __u64 __be64_to_cpup(const __be64 *p)
{
return __swab64p((__u64 *)p);
}
-static inline __be32 __cpu_to_be32p(const __u32 *p)
+static __always_inline __be32 __cpu_to_be32p(const __u32 *p)
{
return (__force __be32)__swab32p(p);
}
-static inline __u32 __be32_to_cpup(const __be32 *p)
+static __always_inline __u32 __be32_to_cpup(const __be32 *p)
{
return __swab32p((__u32 *)p);
}
-static inline __be16 __cpu_to_be16p(const __u16 *p)
+static __always_inline __be16 __cpu_to_be16p(const __u16 *p)
{
return (__force __be16)__swab16p(p);
}
-static inline __u16 __be16_to_cpup(const __be16 *p)
+static __always_inline __u16 __be16_to_cpup(const __be16 *p)
{
return __swab16p((__u16 *)p);
}
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index 0e011eb..3f10e53 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -151,7 +151,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
* __swab16p - return a byteswapped 16-bit value from a pointer
* @p: pointer to a naturally-aligned 16-bit value
*/
-static inline __u16 __swab16p(const __u16 *p)
+static __always_inline __u16 __swab16p(const __u16 *p)
{
#ifdef __arch_swab16p
return __arch_swab16p(p);
@@ -164,7 +164,7 @@ static inline __u16 __swab16p(const __u16 *p)
* __swab32p - return a byteswapped 32-bit value from a pointer
* @p: pointer to a naturally-aligned 32-bit value
*/
-static inline __u32 __swab32p(const __u32 *p)
+static __always_inline __u32 __swab32p(const __u32 *p)
{
#ifdef __arch_swab32p
return __arch_swab32p(p);
@@ -177,7 +177,7 @@ static inline __u32 __swab32p(const __u32 *p)
* __swab64p - return a byteswapped 64-bit value from a pointer
* @p: pointer to a naturally-aligned 64-bit value
*/
-static inline __u64 __swab64p(const __u64 *p)
+static __always_inline __u64 __swab64p(const __u64 *p)
{
#ifdef __arch_swab64p
return __arch_swab64p(p);
@@ -232,7 +232,7 @@ static inline void __swab16s(__u16 *p)
* __swab32s - byteswap a 32-bit value in-place
* @p: pointer to a naturally-aligned 32-bit value
*/
-static inline void __swab32s(__u32 *p)
+static __always_inline void __swab32s(__u32 *p)
{
#ifdef __arch_swab32s
__arch_swab32s(p);
@@ -245,7 +245,7 @@ static inline void __swab32s(__u32 *p)
* __swab64s - byteswap a 64-bit value in-place
* @p: pointer to a naturally-aligned 64-bit value
*/
-static inline void __swab64s(__u64 *p)
+static __always_inline void __swab64s(__u64 *p)
{
#ifdef __arch_swab64s
__arch_swab64s(p);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH] force inlining of unaligned byteswap operations
2016-02-04 19:45 [PATCH] asm-generic: force inlining of some atomic_long operations Denys Vlasenko
2016-02-04 19:45 ` [PATCH] force inlining of some byteswap operations Denys Vlasenko
@ 2016-02-04 19:45 ` Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
1 sibling, 1 reply; 45+ messages in thread
From: Denys Vlasenko @ 2016-02-04 19:45 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel
Sometimes gcc mysteriously doesn't inline
very small functions we expect to be inlined. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
With this .config:
http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
the following functions get deinlined many times.
Examples of disassembly:
<get_unaligned_be16> (24 copies, 108 calls):
66 8b 07 mov (%rdi),%ax
55 push %rbp
48 89 e5 mov %rsp,%rbp
86 e0 xchg %ah,%al
5d pop %rbp
c3 retq
<get_unaligned_be32> (25 copies, 181 calls):
8b 07 mov (%rdi),%eax
55 push %rbp
48 89 e5 mov %rsp,%rbp
0f c8 bswap %eax
5d pop %rbp
c3 retq
<get_unaligned_be64> (23 copies, 94 calls):
48 8b 07 mov (%rdi),%rax
55 push %rbp
48 89 e5 mov %rsp,%rbp
48 0f c8 bswap %rax
5d pop %rbp
c3 retq
<put_unaligned_be16> (2 copies, 11 calls):
89 f8 mov %edi,%eax
55 push %rbp
c1 ef 08 shr $0x8,%edi
c1 e0 08 shl $0x8,%eax
09 c7 or %eax,%edi
48 89 e5 mov %rsp,%rbp
66 89 3e mov %di,(%rsi)
<put_unaligned_be32> (8 copies, 43 calls):
55 push %rbp
0f cf bswap %edi
89 3e mov %edi,(%rsi)
48 89 e5 mov %rsp,%rbp
5d pop %rbp
c3 retq
<put_unaligned_be64> (26 copies, 157 calls):
55 push %rbp
48 0f cf bswap %rdi
48 89 3e mov %rdi,(%rsi)
48 89 e5 mov %rsp,%rbp
5d pop %rbp
c3 retq
This patch fixes this via s/inline/__always_inline/.
It only affects arches with efficient unaligned access insns, such as x86.
(arched which lack such ops do not include linux/unaligned/access_ok.h)
Code size decrease after the patch is ~8.5k:
text data bss dec hex filename
92197848 20826112 36417536 149441496 8e84bd8 vmlinux
92189231 20826144 36417536 149432911 8e82a4f vmlinux6_unaligned_be_after
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
---
include/linux/unaligned/access_ok.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/include/linux/unaligned/access_ok.h b/include/linux/unaligned/access_ok.h
index 99c1b4d..33383ca 100644
--- a/include/linux/unaligned/access_ok.h
+++ b/include/linux/unaligned/access_ok.h
@@ -4,62 +4,62 @@
#include <linux/kernel.h>
#include <asm/byteorder.h>
-static inline u16 get_unaligned_le16(const void *p)
+static __always_inline u16 get_unaligned_le16(const void *p)
{
return le16_to_cpup((__le16 *)p);
}
-static inline u32 get_unaligned_le32(const void *p)
+static __always_inline u32 get_unaligned_le32(const void *p)
{
return le32_to_cpup((__le32 *)p);
}
-static inline u64 get_unaligned_le64(const void *p)
+static __always_inline u64 get_unaligned_le64(const void *p)
{
return le64_to_cpup((__le64 *)p);
}
-static inline u16 get_unaligned_be16(const void *p)
+static __always_inline u16 get_unaligned_be16(const void *p)
{
return be16_to_cpup((__be16 *)p);
}
-static inline u32 get_unaligned_be32(const void *p)
+static __always_inline u32 get_unaligned_be32(const void *p)
{
return be32_to_cpup((__be32 *)p);
}
-static inline u64 get_unaligned_be64(const void *p)
+static __always_inline u64 get_unaligned_be64(const void *p)
{
return be64_to_cpup((__be64 *)p);
}
-static inline void put_unaligned_le16(u16 val, void *p)
+static __always_inline void put_unaligned_le16(u16 val, void *p)
{
*((__le16 *)p) = cpu_to_le16(val);
}
-static inline void put_unaligned_le32(u32 val, void *p)
+static __always_inline void put_unaligned_le32(u32 val, void *p)
{
*((__le32 *)p) = cpu_to_le32(val);
}
-static inline void put_unaligned_le64(u64 val, void *p)
+static __always_inline void put_unaligned_le64(u64 val, void *p)
{
*((__le64 *)p) = cpu_to_le64(val);
}
-static inline void put_unaligned_be16(u16 val, void *p)
+static __always_inline void put_unaligned_be16(u16 val, void *p)
{
*((__be16 *)p) = cpu_to_be16(val);
}
-static inline void put_unaligned_be32(u32 val, void *p)
+static __always_inline void put_unaligned_be32(u32 val, void *p)
{
*((__be32 *)p) = cpu_to_be32(val);
}
-static inline void put_unaligned_be64(u64 val, void *p)
+static __always_inline void put_unaligned_be64(u64 val, void *p)
{
*((__be64 *)p) = cpu_to_be64(val);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH] force inlining of some byteswap operations
2016-02-04 19:45 ` [PATCH] force inlining of some byteswap operations Denys Vlasenko
@ 2016-02-05 7:28 ` Ingo Molnar
2016-04-13 3:36 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf
1 sibling, 0 replies; 45+ messages in thread
From: Ingo Molnar @ 2016-02-05 7:28 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Thomas Graf, Peter Zijlstra, David Rientjes, Andrew Morton,
linux-kernel
* Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Sometimes gcc mysteriously doesn't inline
> very small functions we expect to be inlined. See
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
>
> With this .config:
> http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
> the following functions get deinlined many times.
> Examples of disassembly:
>
> <get_unaligned_be16> (12 copies, 51 calls):
> 66 8b 07 mov (%rdi),%ax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 86 e0 xchg %ah,%al
> 5d pop %rbp
> c3 retq
>
> <get_unaligned_be32> (12 copies, 135 calls):
> 8b 07 mov (%rdi),%eax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 0f c8 bswap %eax
> 5d pop %rbp
> c3 retq
>
> <get_unaligned_be64> (2 copies, 20 calls):
> 48 8b 07 mov (%rdi),%rax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 48 0f c8 bswap %rax
> 5d pop %rbp
> c3 retq
>
> <__swab16p> (16 copies, 146 calls):
> 55 push %rbp
> 89 f8 mov %edi,%eax
> 86 e0 xchg %ah,%al
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab32p> (43 copies, ~560 calls):
> 55 push %rbp
> 89 f8 mov %edi,%eax
> 0f c8 bswap %eax
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab64p> (21 copies, 119 calls):
> 55 push %rbp
> 48 89 f8 mov %rdi,%rax
> 48 0f c8 bswap %rax
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab32s> (6 copies, 47 calls):
> 8b 07 mov (%rdi),%eax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 0f c8 bswap %eax
> 89 07 mov %eax,(%rdi)
> 5d pop %rbp
> c3 retq
>
> This patch fixes this via s/inline/__always_inline/.
> Code size decrease after the patch is ~4.5k:
>
> text data bss dec hex filename
> 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> 92197848 20826112 36417536 149441496 8e84bd8 vmlinux5_swap_after
Acked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] force inlining of unaligned byteswap operations
2016-02-04 19:45 ` [PATCH] force inlining of unaligned byteswap operations Denys Vlasenko
@ 2016-02-05 7:28 ` Ingo Molnar
0 siblings, 0 replies; 45+ messages in thread
From: Ingo Molnar @ 2016-02-05 7:28 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Thomas Graf, Peter Zijlstra, David Rientjes, Andrew Morton,
linux-kernel
* Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Code size decrease after the patch is ~8.5k:
>
> text data bss dec hex filename
> 92197848 20826112 36417536 149441496 8e84bd8 vmlinux
> 92189231 20826144 36417536 149432911 8e82a4f vmlinux6_unaligned_be_after
Acked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
^ permalink raw reply [flat|nested] 45+ messages in thread
* This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-02-04 19:45 ` [PATCH] force inlining of some byteswap operations Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
@ 2016-04-13 3:36 ` Josh Poimboeuf
2016-04-13 12:12 ` Denys Vlasenko
1 sibling, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-13 3:36 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel, Arnd Bergmann
On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> Sometimes gcc mysteriously doesn't inline
> very small functions we expect to be inlined. See
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
>
> With this .config:
> http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
> the following functions get deinlined many times.
> Examples of disassembly:
>
> <get_unaligned_be16> (12 copies, 51 calls):
> 66 8b 07 mov (%rdi),%ax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 86 e0 xchg %ah,%al
> 5d pop %rbp
> c3 retq
>
> <get_unaligned_be32> (12 copies, 135 calls):
> 8b 07 mov (%rdi),%eax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 0f c8 bswap %eax
> 5d pop %rbp
> c3 retq
>
> <get_unaligned_be64> (2 copies, 20 calls):
> 48 8b 07 mov (%rdi),%rax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 48 0f c8 bswap %rax
> 5d pop %rbp
> c3 retq
>
> <__swab16p> (16 copies, 146 calls):
> 55 push %rbp
> 89 f8 mov %edi,%eax
> 86 e0 xchg %ah,%al
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab32p> (43 copies, ~560 calls):
> 55 push %rbp
> 89 f8 mov %edi,%eax
> 0f c8 bswap %eax
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab64p> (21 copies, 119 calls):
> 55 push %rbp
> 48 89 f8 mov %rdi,%rax
> 48 0f c8 bswap %rax
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab32s> (6 copies, 47 calls):
> 8b 07 mov (%rdi),%eax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 0f c8 bswap %eax
> 89 07 mov %eax,(%rdi)
> 5d pop %rbp
> c3 retq
>
> This patch fixes this via s/inline/__always_inline/.
> Code size decrease after the patch is ~4.5k:
>
> text data bss dec hex filename
> 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> 92197848 20826112 36417536 149441496 8e84bd8 vmlinux5_swap_after
>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org
> ---
> include/uapi/linux/byteorder/big_endian.h | 24 ++++++++++++------------
> include/uapi/linux/byteorder/little_endian.h | 24 ++++++++++++------------
> include/uapi/linux/swab.h | 10 +++++-----
> 3 files changed, 29 insertions(+), 29 deletions(-)
Hi,
This patch seems to trigger a gcc bug which can produce corrupt code. I
discovered it when investigating an objtool warning reported by kbuild
bot:
http://www.spinics.net/lists/linux-scsi/msg95481.html
>From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
0000000000002f53 <qla2x00_get_host_fabric_name>:
2f53: 55 push %rbp
2f54: 48 89 e5 mov %rsp,%rbp
0000000000002f57 <qla2x00_get_fc_host_stats>:
2f57: 55 push %rbp
2f58: b9 e8 00 00 00 mov $0xe8,%ecx
2f5d: 48 89 e5 mov %rsp,%rbp
...
Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
setting up the frame pointer. It falls through to the next function, which is
very wrong.
I can recreate it with either gcc 5.3.1 or gcc 6.0 on linus/master with
the .config from the above link.
The call chain which appears to trigger the problem is:
qla2x00_get_host_fabric_name()
wwn_to_u64()
get_unaligned_be64()
be64_to_cpup()
__be64_to_cpup() <- changed to __always_inline by this patch
It occurs with the combination of the following two recent commits:
- bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
- ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
I can confirm that reverting either patch makes the problem go away.
I'm planning on opening a gcc bug tomorrow.
> -static inline __u64 __be64_to_cpup(const __be64 *p)
> +static __always_inline __u64 __be64_to_cpup(const __be64 *p)
> {
> return __swab64p((__u64 *)p);
> }
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-13 3:36 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf
@ 2016-04-13 12:12 ` Denys Vlasenko
2016-04-13 12:36 ` Josh Poimboeuf
0 siblings, 1 reply; 45+ messages in thread
From: Denys Vlasenko @ 2016-04-13 12:12 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ingo Molnar, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel, Arnd Bergmann
On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
>> Sometimes gcc mysteriously doesn't inline
>> very small functions we expect to be inlined. See
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
>>
>> With this .config:
>> http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
>> the following functions get deinlined many times.
>> Examples of disassembly:
>>
>> <get_unaligned_be16> (12 copies, 51 calls):
>> 66 8b 07 mov (%rdi),%ax
>> 55 push %rbp
>> 48 89 e5 mov %rsp,%rbp
>> 86 e0 xchg %ah,%al
>> 5d pop %rbp
>> c3 retq
...
>> This patch fixes this via s/inline/__always_inline/.
>> Code size decrease after the patch is ~4.5k:
>>
>> text data bss dec hex filename
>> 92202377 20826112 36417536 149446025 8e85d89 vmlinux
>> 92197848 20826112 36417536 149441496 8e84bd8 vmlinux5_swap_after
>>
>> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Thomas Graf <tgraf@suug.ch>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> include/uapi/linux/byteorder/big_endian.h | 24 ++++++++++++------------
>> include/uapi/linux/byteorder/little_endian.h | 24 ++++++++++++------------
>> include/uapi/linux/swab.h | 10 +++++-----
>> 3 files changed, 29 insertions(+), 29 deletions(-)
>
> Hi,
>
> This patch seems to trigger a gcc bug which can produce corrupt code. I
> discovered it when investigating an objtool warning reported by kbuild
> bot:
>
> http://www.spinics.net/lists/linux-scsi/msg95481.html
>
> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
>
> 0000000000002f53 <qla2x00_get_host_fabric_name>:
> 2f53: 55 push %rbp
> 2f54: 48 89 e5 mov %rsp,%rbp
>
> 0000000000002f57 <qla2x00_get_fc_host_stats>:
> 2f57: 55 push %rbp
> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> 2f5d: 48 89 e5 mov %rsp,%rbp
> ...
>
> Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
> setting up the frame pointer. It falls through to the next function, which is
> very wrong.
Wow, that's ... interesting.
> I can recreate it with either gcc 5.3.1 or gcc 6.0 on linus/master with
> the .config from the above link.
>
> The call chain which appears to trigger the problem is:
>
> qla2x00_get_host_fabric_name()
> wwn_to_u64()
> get_unaligned_be64()
> be64_to_cpup()
> __be64_to_cpup() <- changed to __always_inline by this patch
>
> It occurs with the combination of the following two recent commits:
>
> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
>
> I can confirm that reverting either patch makes the problem go away.
> I'm planning on opening a gcc bug tomorrow.
Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
keywords are in fact __always_inline, so the bug must be triggering
even without the patch.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-13 12:12 ` Denys Vlasenko
@ 2016-04-13 12:36 ` Josh Poimboeuf
2016-04-13 15:15 ` Josh Poimboeuf
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-13 12:36 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel, Arnd Bergmann
On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> >> Sometimes gcc mysteriously doesn't inline
> >> very small functions we expect to be inlined. See
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> >>
> >> With this .config:
> >> http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
> >> the following functions get deinlined many times.
> >> Examples of disassembly:
> >>
> >> <get_unaligned_be16> (12 copies, 51 calls):
> >> 66 8b 07 mov (%rdi),%ax
> >> 55 push %rbp
> >> 48 89 e5 mov %rsp,%rbp
> >> 86 e0 xchg %ah,%al
> >> 5d pop %rbp
> >> c3 retq
> ...
> >> This patch fixes this via s/inline/__always_inline/.
> >> Code size decrease after the patch is ~4.5k:
> >>
> >> text data bss dec hex filename
> >> 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> >> 92197848 20826112 36417536 149441496 8e84bd8 vmlinux5_swap_after
> >>
> >> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> >> Cc: Ingo Molnar <mingo@kernel.org>
> >> Cc: Thomas Graf <tgraf@suug.ch>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: David Rientjes <rientjes@google.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >> include/uapi/linux/byteorder/big_endian.h | 24 ++++++++++++------------
> >> include/uapi/linux/byteorder/little_endian.h | 24 ++++++++++++------------
> >> include/uapi/linux/swab.h | 10 +++++-----
> >> 3 files changed, 29 insertions(+), 29 deletions(-)
> >
> > Hi,
> >
> > This patch seems to trigger a gcc bug which can produce corrupt code. I
> > discovered it when investigating an objtool warning reported by kbuild
> > bot:
> >
> > http://www.spinics.net/lists/linux-scsi/msg95481.html
> >
> > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> >
> > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > 2f53: 55 push %rbp
> > 2f54: 48 89 e5 mov %rsp,%rbp
> >
> > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > 2f57: 55 push %rbp
> > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > 2f5d: 48 89 e5 mov %rsp,%rbp
> > ...
> >
> > Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
> > setting up the frame pointer. It falls through to the next function, which is
> > very wrong.
>
> Wow, that's ... interesting.
>
>
> > I can recreate it with either gcc 5.3.1 or gcc 6.0 on linus/master with
> > the .config from the above link.
> >
> > The call chain which appears to trigger the problem is:
> >
> > qla2x00_get_host_fabric_name()
> > wwn_to_u64()
> > get_unaligned_be64()
> > be64_to_cpup()
> > __be64_to_cpup() <- changed to __always_inline by this patch
> >
> > It occurs with the combination of the following two recent commits:
> >
> > - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> > - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> >
> > I can confirm that reverting either patch makes the problem go away.
> > I'm planning on opening a gcc bug tomorrow.
>
>
> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> keywords are in fact __always_inline, so the bug must be triggering
> even without the patch.
Makes sense in theory, but the bug doesn't actually trigger when I
revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
Perhaps even more surprising, it doesn't trigger *with* the patch and
CONFIG_OPTIMIZE_INLINING=n.
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-13 12:36 ` Josh Poimboeuf
@ 2016-04-13 15:15 ` Josh Poimboeuf
2016-04-13 16:55 ` James Bottomley
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-13 15:15 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel, Arnd Bergmann, James Bottomley
On Wed, Apr 13, 2016 at 07:36:07AM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> > On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> > >> Sometimes gcc mysteriously doesn't inline
> > >> very small functions we expect to be inlined. See
> > >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > >>
> > >> With this .config:
> > >> http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_Os,
> > >> the following functions get deinlined many times.
> > >> Examples of disassembly:
> > >>
> > >> <get_unaligned_be16> (12 copies, 51 calls):
> > >> 66 8b 07 mov (%rdi),%ax
> > >> 55 push %rbp
> > >> 48 89 e5 mov %rsp,%rbp
> > >> 86 e0 xchg %ah,%al
> > >> 5d pop %rbp
> > >> c3 retq
> > ...
> > >> This patch fixes this via s/inline/__always_inline/.
> > >> Code size decrease after the patch is ~4.5k:
> > >>
> > >> text data bss dec hex filename
> > >> 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> > >> 92197848 20826112 36417536 149441496 8e84bd8 vmlinux5_swap_after
> > >>
> > >> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> > >> Cc: Ingo Molnar <mingo@kernel.org>
> > >> Cc: Thomas Graf <tgraf@suug.ch>
> > >> Cc: Peter Zijlstra <peterz@infradead.org>
> > >> Cc: David Rientjes <rientjes@google.com>
> > >> Cc: Andrew Morton <akpm@linux-foundation.org>
> > >> Cc: linux-kernel@vger.kernel.org
> > >> ---
> > >> include/uapi/linux/byteorder/big_endian.h | 24 ++++++++++++------------
> > >> include/uapi/linux/byteorder/little_endian.h | 24 ++++++++++++------------
> > >> include/uapi/linux/swab.h | 10 +++++-----
> > >> 3 files changed, 29 insertions(+), 29 deletions(-)
> > >
> > > Hi,
> > >
> > > This patch seems to trigger a gcc bug which can produce corrupt code. I
> > > discovered it when investigating an objtool warning reported by kbuild
> > > bot:
> > >
> > > http://www.spinics.net/lists/linux-scsi/msg95481.html
> > >
> > > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> > >
> > > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > > 2f53: 55 push %rbp
> > > 2f54: 48 89 e5 mov %rsp,%rbp
> > >
> > > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > > 2f57: 55 push %rbp
> > > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > > 2f5d: 48 89 e5 mov %rsp,%rbp
> > > ...
> > >
> > > Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
> > > setting up the frame pointer. It falls through to the next function, which is
> > > very wrong.
> >
> > Wow, that's ... interesting.
> >
> >
> > > I can recreate it with either gcc 5.3.1 or gcc 6.0 on linus/master with
> > > the .config from the above link.
> > >
> > > The call chain which appears to trigger the problem is:
> > >
> > > qla2x00_get_host_fabric_name()
> > > wwn_to_u64()
> > > get_unaligned_be64()
> > > be64_to_cpup()
> > > __be64_to_cpup() <- changed to __always_inline by this patch
> > >
> > > It occurs with the combination of the following two recent commits:
> > >
> > > - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> > > - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> > >
> > > I can confirm that reverting either patch makes the problem go away.
> > > I'm planning on opening a gcc bug tomorrow.
> >
> >
> > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > keywords are in fact __always_inline, so the bug must be triggering
> > even without the patch.
>
> Makes sense in theory, but the bug doesn't actually trigger when I
> revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
>
> Perhaps even more surprising, it doesn't trigger *with* the patch and
> CONFIG_OPTIMIZE_INLINING=n.
[ Adding James to CC since this bug affects scsi. ]
Here's the gcc bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-13 15:15 ` Josh Poimboeuf
@ 2016-04-13 16:55 ` James Bottomley
2016-04-13 17:10 ` Josh Poimboeuf
0 siblings, 1 reply; 45+ messages in thread
From: James Bottomley @ 2016-04-13 16:55 UTC (permalink / raw)
To: Josh Poimboeuf, Denys Vlasenko
Cc: Ingo Molnar, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel, Arnd Bergmann, linux-scsi
On Wed, 2016-04-13 at 10:15 -0500, Josh Poimboeuf wrote:
> On Wed, Apr 13, 2016 at 07:36:07AM -0500, Josh Poimboeuf wrote:
> > On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> > > On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > > > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> > > > > Sometimes gcc mysteriously doesn't inline
> > > > > very small functions we expect to be inlined. See
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > > > >
> > > > > With this .config:
> > > > > http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_O
> > > > > s,
> > > > > the following functions get deinlined many times.
> > > > > Examples of disassembly:
> > > > >
> > > > > <get_unaligned_be16> (12 copies, 51 calls):
> > > > > 66 8b 07 mov (%rdi),%ax
> > > > > 55 push %rbp
> > > > > 48 89 e5 mov %rsp,%rbp
> > > > > 86 e0 xchg %ah,%al
> > > > > 5d pop %rbp
> > > > > c3 retq
> > > ...
> > > > > This patch fixes this via s/inline/__always_inline/.
> > > > > Code size decrease after the patch is ~4.5k:
> > > > >
> > > > > text data bss dec hex filename
> > > > > 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> > > > > 92197848 20826112 36417536 149441496 8e84bd8
> > > > > vmlinux5_swap_after
> > > > >
> > > > > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > Cc: Thomas Graf <tgraf@suug.ch>
> > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > Cc: David Rientjes <rientjes@google.com>
> > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > ---
> > > > > include/uapi/linux/byteorder/big_endian.h | 24
> > > > > ++++++++++++------------
> > > > > include/uapi/linux/byteorder/little_endian.h | 24
> > > > > ++++++++++++------------
> > > > > include/uapi/linux/swab.h | 10 +++++-----
> > > > > 3 files changed, 29 insertions(+), 29 deletions(-)
> > > >
> > > > Hi,
> > > >
> > > > This patch seems to trigger a gcc bug which can produce corrupt
> > > > code. I
> > > > discovered it when investigating an objtool warning reported by
> > > > kbuild
> > > > bot:
> > > >
> > > > http://www.spinics.net/lists/linux-scsi/msg95481.html
> > > >
> > > > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> > > >
> > > > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > > > 2f53: 55 push %rbp
> > > > 2f54: 48 89 e5 mov %rsp,%rbp
> > > >
> > > > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > > > 2f57: 55 push %rbp
> > > > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > > > 2f5d: 48 89 e5 mov %rsp,%rbp
> > > > ...
> > > >
> > > > Note that qla2x00_get_host_fabric_name() is inexplicably
> > > > truncated after
> > > > setting up the frame pointer. It falls through to the next
> > > > function, which is
> > > > very wrong.
> > >
> > > Wow, that's ... interesting.
> > >
> > >
> > > > I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> > > > linus/master with
> > > > the .config from the above link.
> > > >
> > > > The call chain which appears to trigger the problem is:
> > > >
> > > > qla2x00_get_host_fabric_name()
> > > > wwn_to_u64()
> > > > get_unaligned_be64()
> > > > be64_to_cpup()
> > > > __be64_to_cpup() <- changed to __always_inline by this
> > > > patch
> > > >
> > > > It occurs with the combination of the following two recent
> > > > commits:
> > > >
> > > > - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> > > > inlining of some byteswap operations")
> > > > - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> > > > access")
> > > >
> > > > I can confirm that reverting either patch makes the problem go
> > > > away.
> > > > I'm planning on opening a gcc bug tomorrow.
> > >
> > >
> > > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > > keywords are in fact __always_inline, so the bug must be
> > > triggering
> > > even without the patch.
> >
> > Makes sense in theory, but the bug doesn't actually trigger when I
> > revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> >
> > Perhaps even more surprising, it doesn't trigger *with* the patch
> > and
> > CONFIG_OPTIMIZE_INLINING=n.
>
> [ Adding James to CC since this bug affects scsi. ]
>
> Here's the gcc bug:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>
Actually, adding me doesn't help, I've added linux-scsi. The summary
is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
this means we're going to have to ask the compiler version of reported
crashes.
James
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-13 16:55 ` James Bottomley
@ 2016-04-13 17:10 ` Josh Poimboeuf
2016-04-14 15:29 ` Denys Vlasenko
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-13 17:10 UTC (permalink / raw)
To: James Bottomley
Cc: Denys Vlasenko, Ingo Molnar, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi
On Wed, Apr 13, 2016 at 09:55:09AM -0700, James Bottomley wrote:
> On Wed, 2016-04-13 at 10:15 -0500, Josh Poimboeuf wrote:
> > On Wed, Apr 13, 2016 at 07:36:07AM -0500, Josh Poimboeuf wrote:
> > > On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> > > > On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > > > > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> > > > > > Sometimes gcc mysteriously doesn't inline
> > > > > > very small functions we expect to be inlined. See
> > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > > > > >
> > > > > > With this .config:
> > > > > > http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_O
> > > > > > s,
> > > > > > the following functions get deinlined many times.
> > > > > > Examples of disassembly:
> > > > > >
> > > > > > <get_unaligned_be16> (12 copies, 51 calls):
> > > > > > 66 8b 07 mov (%rdi),%ax
> > > > > > 55 push %rbp
> > > > > > 48 89 e5 mov %rsp,%rbp
> > > > > > 86 e0 xchg %ah,%al
> > > > > > 5d pop %rbp
> > > > > > c3 retq
> > > > ...
> > > > > > This patch fixes this via s/inline/__always_inline/.
> > > > > > Code size decrease after the patch is ~4.5k:
> > > > > >
> > > > > > text data bss dec hex filename
> > > > > > 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> > > > > > 92197848 20826112 36417536 149441496 8e84bd8
> > > > > > vmlinux5_swap_after
> > > > > >
> > > > > > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> > > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > > Cc: Thomas Graf <tgraf@suug.ch>
> > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > Cc: David Rientjes <rientjes@google.com>
> > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > ---
> > > > > > include/uapi/linux/byteorder/big_endian.h | 24
> > > > > > ++++++++++++------------
> > > > > > include/uapi/linux/byteorder/little_endian.h | 24
> > > > > > ++++++++++++------------
> > > > > > include/uapi/linux/swab.h | 10 +++++-----
> > > > > > 3 files changed, 29 insertions(+), 29 deletions(-)
> > > > >
> > > > > Hi,
> > > > >
> > > > > This patch seems to trigger a gcc bug which can produce corrupt
> > > > > code. I
> > > > > discovered it when investigating an objtool warning reported by
> > > > > kbuild
> > > > > bot:
> > > > >
> > > > > http://www.spinics.net/lists/linux-scsi/msg95481.html
> > > > >
> > > > > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> > > > >
> > > > > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > > > > 2f53: 55 push %rbp
> > > > > 2f54: 48 89 e5 mov %rsp,%rbp
> > > > >
> > > > > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > > > > 2f57: 55 push %rbp
> > > > > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > > > > 2f5d: 48 89 e5 mov %rsp,%rbp
> > > > > ...
> > > > >
> > > > > Note that qla2x00_get_host_fabric_name() is inexplicably
> > > > > truncated after
> > > > > setting up the frame pointer. It falls through to the next
> > > > > function, which is
> > > > > very wrong.
> > > >
> > > > Wow, that's ... interesting.
> > > >
> > > >
> > > > > I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> > > > > linus/master with
> > > > > the .config from the above link.
> > > > >
> > > > > The call chain which appears to trigger the problem is:
> > > > >
> > > > > qla2x00_get_host_fabric_name()
> > > > > wwn_to_u64()
> > > > > get_unaligned_be64()
> > > > > be64_to_cpup()
> > > > > __be64_to_cpup() <- changed to __always_inline by this
> > > > > patch
> > > > >
> > > > > It occurs with the combination of the following two recent
> > > > > commits:
> > > > >
> > > > > - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> > > > > inlining of some byteswap operations")
> > > > > - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> > > > > access")
> > > > >
> > > > > I can confirm that reverting either patch makes the problem go
> > > > > away.
> > > > > I'm planning on opening a gcc bug tomorrow.
> > > >
> > > >
> > > > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > > > keywords are in fact __always_inline, so the bug must be
> > > > triggering
> > > > even without the patch.
> > >
> > > Makes sense in theory, but the bug doesn't actually trigger when I
> > > revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> > >
> > > Perhaps even more surprising, it doesn't trigger *with* the patch
> > > and
> > > CONFIG_OPTIMIZE_INLINING=n.
> >
> > [ Adding James to CC since this bug affects scsi. ]
> >
> > Here's the gcc bug:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> >
>
>
> Actually, adding me doesn't help, I've added linux-scsi. The summary
> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
> this means we're going to have to ask the compiler version of reported
> crashes.
The bug isn't specific to a compiler version. I've seen it with gcc
5.3.1 and gcc 6.0. I haven't tried any older versions. And the gcc bug
hasn't been resolved (or even investigated) yet.
The bug is triggered by a combination of the above two commits from the
4.6 merge window, so presumably we'd need to revert one of them to avoid
crashes in 4.6.
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-13 17:10 ` Josh Poimboeuf
@ 2016-04-14 15:29 ` Denys Vlasenko
2016-04-14 15:57 ` Josh Poimboeuf
0 siblings, 1 reply; 45+ messages in thread
From: Denys Vlasenko @ 2016-04-14 15:29 UTC (permalink / raw)
To: Josh Poimboeuf, James Bottomley
Cc: Ingo Molnar, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, linux-kernel, Arnd Bergmann, linux-scsi
On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
>>>>>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
>>>>>>
>>>>>> 0000000000002f53 <qla2x00_get_host_fabric_name>:
>>>>>> 2f53: 55 push %rbp
>>>>>> 2f54: 48 89 e5 mov %rsp,%rbp
>>>>>>
>>>>>> 0000000000002f57 <qla2x00_get_fc_host_stats>:
>>>>>> 2f57: 55 push %rbp
>>>>>> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
>>>>>> 2f5d: 48 89 e5 mov %rsp,%rbp
>>>>>> ...
>>>>>>
>>>>>> Note that qla2x00_get_host_fabric_name() is inexplicably
>>>>>> truncated after
>>>>>> setting up the frame pointer. It falls through to the next
>>>>>> function, which is
>>>>>> very wrong.
>>>>>
>>>>> Wow, that's ... interesting.
>>>>>
>>>>>
>>>>>> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
>>>>>> linus/master with
>>>>>> the .config from the above link.
>>>>>>
>>>>>> The call chain which appears to trigger the problem is:
>>>>>>
>>>>>> qla2x00_get_host_fabric_name()
>>>>>> wwn_to_u64()
>>>>>> get_unaligned_be64()
>>>>>> be64_to_cpup()
>>>>>> __be64_to_cpup() <- changed to __always_inline by this
>>>>>> patch
>>>>>>
>>>>>> It occurs with the combination of the following two recent
>>>>>> commits:
>>>>>>
>>>>>> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
>>>>>> inlining of some byteswap operations")
>>>>>> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
>>>>>> access")
>>>>>>
>>>>>> I can confirm that reverting either patch makes the problem go
>>>>>> away.
>>>>>> I'm planning on opening a gcc bug tomorrow.
>>>>>
>>>>>
>>>>> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
>>>>> keywords are in fact __always_inline, so the bug must be
>>>>> triggering
>>>>> even without the patch.
>>>>
>>>> Makes sense in theory, but the bug doesn't actually trigger when I
>>>> revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
>>>>
>>>> Perhaps even more surprising, it doesn't trigger *with* the patch
>>>> and
>>>> CONFIG_OPTIMIZE_INLINING=n.
>>>
>>> [ Adding James to CC since this bug affects scsi. ]
>>>
>>> Here's the gcc bug:
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>>>
>>
>>
>> Actually, adding me doesn't help, I've added linux-scsi. The summary
>> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
>> this means we're going to have to ask the compiler version of reported
>> crashes.
>
> The bug isn't specific to a compiler version. I've seen it with gcc
> 5.3.1 and gcc 6.0. I haven't tried any older versions. And the gcc bug
> hasn't been resolved (or even investigated) yet.
>
> The bug is triggered by a combination of the above two commits from the
> 4.6 merge window, so presumably we'd need to revert one of them to avoid
> crashes in 4.6.
The bug is indeed in the compiler. 4.9 and all later versions are affected.
gcc bugzilla now has a reproducer. In abridged form:
static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
{
return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & (u64)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 56))) : __builtin_bswap64(*p));
}
static inline u64 wwn_to_u64(void *wwn)
{
return __swab64p(wwn);
}
static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
{
scsi_qla_host_t *vha = shost_priv(shost);
u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
u64 fabric_name = wwn_to_u64(node_name);
if (vha->device_flags & 0x1)
fabric_name = wwn_to_u64(vha->fabric_node_name);
(((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
}
Two (or more, there were more before simplification) levels of inlining
are necessary for bug to trigger in this example (folding to one level
makes it go away). "__attribute__((always_inline))" is necessary too.
Since we have lots of __always_inline anyway, this bug has a potential
to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
and with or without the patches mentioned above (they just happen
to create a reliable reproducer).
Since it was not detected for two years since gcc 4.9 release,
it must be triggering quite rarely.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-14 15:29 ` Denys Vlasenko
@ 2016-04-14 15:57 ` Josh Poimboeuf
2016-04-14 17:09 ` Denys Vlasenko
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-14 15:57 UTC (permalink / raw)
To: Denys Vlasenko
Cc: James Bottomley, Ingo Molnar, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi
On Thu, Apr 14, 2016 at 05:29:06PM +0200, Denys Vlasenko wrote:
> On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
> >>>>>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> >>>>>>
> >>>>>> 0000000000002f53 <qla2x00_get_host_fabric_name>:
> >>>>>> 2f53: 55 push %rbp
> >>>>>> 2f54: 48 89 e5 mov %rsp,%rbp
> >>>>>>
> >>>>>> 0000000000002f57 <qla2x00_get_fc_host_stats>:
> >>>>>> 2f57: 55 push %rbp
> >>>>>> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> >>>>>> 2f5d: 48 89 e5 mov %rsp,%rbp
> >>>>>> ...
> >>>>>>
> >>>>>> Note that qla2x00_get_host_fabric_name() is inexplicably
> >>>>>> truncated after
> >>>>>> setting up the frame pointer. It falls through to the next
> >>>>>> function, which is
> >>>>>> very wrong.
> >>>>>
> >>>>> Wow, that's ... interesting.
> >>>>>
> >>>>>
> >>>>>> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> >>>>>> linus/master with
> >>>>>> the .config from the above link.
> >>>>>>
> >>>>>> The call chain which appears to trigger the problem is:
> >>>>>>
> >>>>>> qla2x00_get_host_fabric_name()
> >>>>>> wwn_to_u64()
> >>>>>> get_unaligned_be64()
> >>>>>> be64_to_cpup()
> >>>>>> __be64_to_cpup() <- changed to __always_inline by this
> >>>>>> patch
> >>>>>>
> >>>>>> It occurs with the combination of the following two recent
> >>>>>> commits:
> >>>>>>
> >>>>>> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> >>>>>> inlining of some byteswap operations")
> >>>>>> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> >>>>>> access")
> >>>>>>
> >>>>>> I can confirm that reverting either patch makes the problem go
> >>>>>> away.
> >>>>>> I'm planning on opening a gcc bug tomorrow.
> >>>>>
> >>>>>
> >>>>> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> >>>>> keywords are in fact __always_inline, so the bug must be
> >>>>> triggering
> >>>>> even without the patch.
> >>>>
> >>>> Makes sense in theory, but the bug doesn't actually trigger when I
> >>>> revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> >>>>
> >>>> Perhaps even more surprising, it doesn't trigger *with* the patch
> >>>> and
> >>>> CONFIG_OPTIMIZE_INLINING=n.
> >>>
> >>> [ Adding James to CC since this bug affects scsi. ]
> >>>
> >>> Here's the gcc bug:
> >>>
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> >>>
> >>
> >>
> >> Actually, adding me doesn't help, I've added linux-scsi. The summary
> >> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
> >> this means we're going to have to ask the compiler version of reported
> >> crashes.
> >
> > The bug isn't specific to a compiler version. I've seen it with gcc
> > 5.3.1 and gcc 6.0. I haven't tried any older versions. And the gcc bug
> > hasn't been resolved (or even investigated) yet.
> >
> > The bug is triggered by a combination of the above two commits from the
> > 4.6 merge window, so presumably we'd need to revert one of them to avoid
> > crashes in 4.6.
>
> The bug is indeed in the compiler. 4.9 and all later versions are affected.
> gcc bugzilla now has a reproducer. In abridged form:
>
>
> static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
> {
> return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & (u64)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 56))) : __builtin_bswap64(*p));
> }
> static inline u64 wwn_to_u64(void *wwn)
> {
> return __swab64p(wwn);
> }
> static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
> {
> scsi_qla_host_t *vha = shost_priv(shost);
> u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
> u64 fabric_name = wwn_to_u64(node_name);
> if (vha->device_flags & 0x1)
> fabric_name = wwn_to_u64(vha->fabric_node_name);
> (((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
> }
Nice work with the reproducer!
> Two (or more, there were more before simplification) levels of inlining
> are necessary for bug to trigger in this example (folding to one level
> makes it go away). "__attribute__((always_inline))" is necessary too.
>
>
> Since we have lots of __always_inline anyway, this bug has a potential
> to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
> and with or without the patches mentioned above (they just happen
> to create a reliable reproducer).
Well, but setting !CONFIG_OPTIMIZE_INLINING makes the problem go away
for some reason. It seems like the bug requires wwn_to_u64() being
out-of-line and __swab64p() being in-line.
In fact, the following patch seems to fix it:
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index bf66ea6..56b9e81 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
return result;
}
-static inline u64 wwn_to_u64(u8 *wwn)
+static __always_inline u64 wwn_to_u64(u8 *wwn)
{
return get_unaligned_be64(wwn);
}
> Since it was not detected for two years since gcc 4.9 release,
> it must be triggering quite rarely.
Yeah, and according to objtool this is the only occurrence of this bug
in the entire kernel tree. Thanks to the kbuild robot randconfig
builds, I think we can be pretty confident that objtool will find any
more of them if they show up.
So what do you think about working around this bug by doing something
like the above patch?
--
Josh
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-14 15:57 ` Josh Poimboeuf
@ 2016-04-14 17:09 ` Denys Vlasenko
2016-04-15 5:45 ` Ingo Molnar
0 siblings, 1 reply; 45+ messages in thread
From: Denys Vlasenko @ 2016-04-14 17:09 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: James Bottomley, Ingo Molnar, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi
On 04/14/2016 05:57 PM, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2016 at 05:29:06PM +0200, Denys Vlasenko wrote:
>> On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
>>>>>>>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
>>>>>>>>
>>>>>>>> 0000000000002f53 <qla2x00_get_host_fabric_name>:
>>>>>>>> 2f53: 55 push %rbp
>>>>>>>> 2f54: 48 89 e5 mov %rsp,%rbp
>>>>>>>>
>>>>>>>> 0000000000002f57 <qla2x00_get_fc_host_stats>:
>>>>>>>> 2f57: 55 push %rbp
>>>>>>>> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
>>>>>>>> 2f5d: 48 89 e5 mov %rsp,%rbp
>>>>>>>> ...
>>>>>>>>
>>>>>>>> Note that qla2x00_get_host_fabric_name() is inexplicably
>>>>>>>> truncated after
>>>>>>>> setting up the frame pointer. It falls through to the next
>>>>>>>> function, which is
>>>>>>>> very wrong.
>>>>>>>
>>>>>>> Wow, that's ... interesting.
>>>>>>>
>>>>>>>
>>>>>>>> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
>>>>>>>> linus/master with
>>>>>>>> the .config from the above link.
>>>>>>>>
>>>>>>>> The call chain which appears to trigger the problem is:
>>>>>>>>
>>>>>>>> qla2x00_get_host_fabric_name()
>>>>>>>> wwn_to_u64()
>>>>>>>> get_unaligned_be64()
>>>>>>>> be64_to_cpup()
>>>>>>>> __be64_to_cpup() <- changed to __always_inline by this
>>>>>>>> patch
>>>>>>>>
>>>>>>>> It occurs with the combination of the following two recent
>>>>>>>> commits:
>>>>>>>>
>>>>>>>> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
>>>>>>>> inlining of some byteswap operations")
>>>>>>>> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
>>>>>>>> access")
>>>>>>>>
>>>>>>>> I can confirm that reverting either patch makes the problem go
>>>>>>>> away.
>>>>>>>> I'm planning on opening a gcc bug tomorrow.
>>>>>>>
>>>>>>>
>>>>>>> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
>>>>>>> keywords are in fact __always_inline, so the bug must be
>>>>>>> triggering
>>>>>>> even without the patch.
>>>>>>
>>>>>> Makes sense in theory, but the bug doesn't actually trigger when I
>>>>>> revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
>>>>>>
>>>>>> Perhaps even more surprising, it doesn't trigger *with* the patch
>>>>>> and
>>>>>> CONFIG_OPTIMIZE_INLINING=n.
>>>>>
>>>>> [ Adding James to CC since this bug affects scsi. ]
>>>>>
>>>>> Here's the gcc bug:
>>>>>
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>>>>>
>>>>
>>>>
>>>> Actually, adding me doesn't help, I've added linux-scsi. The summary
>>>> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
>>>> this means we're going to have to ask the compiler version of reported
>>>> crashes.
>>>
>>> The bug isn't specific to a compiler version. I've seen it with gcc
>>> 5.3.1 and gcc 6.0. I haven't tried any older versions. And the gcc bug
>>> hasn't been resolved (or even investigated) yet.
>>>
>>> The bug is triggered by a combination of the above two commits from the
>>> 4.6 merge window, so presumably we'd need to revert one of them to avoid
>>> crashes in 4.6.
>>
>> The bug is indeed in the compiler. 4.9 and all later versions are affected.
>> gcc bugzilla now has a reproducer. In abridged form:
>>
>>
>> static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
>> {
>> return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & (u64)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 56))) : __builtin_bswap64(*p));
>> }
>> static inline u64 wwn_to_u64(void *wwn)
>> {
>> return __swab64p(wwn);
>> }
>> static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
>> {
>> scsi_qla_host_t *vha = shost_priv(shost);
>> u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
>> u64 fabric_name = wwn_to_u64(node_name);
>> if (vha->device_flags & 0x1)
>> fabric_name = wwn_to_u64(vha->fabric_node_name);
>> (((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
>> }
>
> Nice work with the reproducer!
>
>> Two (or more, there were more before simplification) levels of inlining
>> are necessary for bug to trigger in this example (folding to one level
>> makes it go away). "__attribute__((always_inline))" is necessary too.
>>
>>
>> Since we have lots of __always_inline anyway, this bug has a potential
>> to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
>> and with or without the patches mentioned above (they just happen
>> to create a reliable reproducer).
>
> Well, but setting !CONFIG_OPTIMIZE_INLINING makes the problem go away
> for some reason. It seems like the bug requires wwn_to_u64() being
> out-of-line and __swab64p() being in-line.
By my reading of what gcc gurus are talking there,
gcc has some new-ish machinery for discarding unreachable code.
Akin to not continuing code generation after a call to never-returning
function like exit(), but smarter (it can detect much less obvious
cases when some code path is not possible).
And it has a subtle bug. In this case, it decided that both branches
of ternary op ?: in __swab64p() are impossible, and therefore
__swab64p() is impossible.
(Which is, of course, bogus, that's why it's a bug).
Then this bogus decision was propagated through inlining
and gcc decided that entire qla2x00_get_host_fabric_name()
function is an impossible (unreachable) code path, and...
eliminated it all. Good boy :D
> In fact, the following patch seems to fix it:
>
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> index bf66ea6..56b9e81 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> return result;
> }
>
> -static inline u64 wwn_to_u64(u8 *wwn)
> +static __always_inline u64 wwn_to_u64(u8 *wwn)
> {
> return get_unaligned_be64(wwn);
> }
It is not a guarantee.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-14 17:09 ` Denys Vlasenko
@ 2016-04-15 5:45 ` Ingo Molnar
2016-04-15 13:47 ` Josh Poimboeuf
2016-04-16 7:42 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann
0 siblings, 2 replies; 45+ messages in thread
From: Ingo Molnar @ 2016-04-15 5:45 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Josh Poimboeuf, James Bottomley, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi
* Denys Vlasenko <dvlasenk@redhat.com> wrote:
> > In fact, the following patch seems to fix it:
> >
> > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > index bf66ea6..56b9e81 100644
> > --- a/include/scsi/scsi_transport_fc.h
> > +++ b/include/scsi/scsi_transport_fc.h
> > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > return result;
> > }
> >
> > -static inline u64 wwn_to_u64(u8 *wwn)
> > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > {
> > return get_unaligned_be64(wwn);
> > }
>
> It is not a guarantee.
Of course it's a workaround - but is there any deterministic way to turn off this
GCC bug (by activating some GCC command line switch), or do we have to live with
objtool warning about this GCC?
Which, by the way, is pretty cool!
Thanks,
Ingo
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-15 5:45 ` Ingo Molnar
@ 2016-04-15 13:47 ` Josh Poimboeuf
2016-04-15 22:20 ` Josh Poimboeuf
2016-04-16 7:42 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann
1 sibling, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-15 13:47 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, James Bottomley, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi
On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
>
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> > > In fact, the following patch seems to fix it:
> > >
> > > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > > index bf66ea6..56b9e81 100644
> > > --- a/include/scsi/scsi_transport_fc.h
> > > +++ b/include/scsi/scsi_transport_fc.h
> > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > return result;
> > > }
> > >
> > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > {
> > > return get_unaligned_be64(wwn);
> > > }
> >
> > It is not a guarantee.
>
> Of course it's a workaround - but is there any deterministic way to turn off this
> GCC bug (by activating some GCC command line switch), or do we have to live with
> objtool warning about this GCC?
I don't think we know yet if there's a reliable way to turn the bug off.
Also, according to the gcc guys, this bug won't always result in a
truncated function, and may sometimes just make some inline function
call sites disappear:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
though I haven't been able to confirm that experimentally. But if it's
true, that means that objtool won't be able to detect all cases of the
bug and some function calls may just silently disappear!
There's a lot of activity in the bug now, so hopefully they'll be able
to tell us soon if there's a reliable way to avoid it and/or detect it.
BTW, Denys posted a workaround patch for the qla2xxxx code:
https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-15 13:47 ` Josh Poimboeuf
@ 2016-04-15 22:20 ` Josh Poimboeuf
2016-04-16 9:03 ` Ingo Molnar
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-15 22:20 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, James Bottomley, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi, jamborm
On Fri, Apr 15, 2016 at 08:47:45AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
> >
> > * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> >
> > > > In fact, the following patch seems to fix it:
> > > >
> > > > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > > > index bf66ea6..56b9e81 100644
> > > > --- a/include/scsi/scsi_transport_fc.h
> > > > +++ b/include/scsi/scsi_transport_fc.h
> > > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > > return result;
> > > > }
> > > >
> > > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > > {
> > > > return get_unaligned_be64(wwn);
> > > > }
> > >
> > > It is not a guarantee.
> >
> > Of course it's a workaround - but is there any deterministic way to turn off this
> > GCC bug (by activating some GCC command line switch), or do we have to live with
> > objtool warning about this GCC?
>
> I don't think we know yet if there's a reliable way to turn the bug off.
>
> Also, according to the gcc guys, this bug won't always result in a
> truncated function, and may sometimes just make some inline function
> call sites disappear:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
>
> though I haven't been able to confirm that experimentally. But if it's
> true, that means that objtool won't be able to detect all cases of the
> bug and some function calls may just silently disappear!
>
> There's a lot of activity in the bug now, so hopefully they'll be able
> to tell us soon if there's a reliable way to avoid it and/or detect it.
>
> BTW, Denys posted a workaround patch for the qla2xxxx code:
>
> https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
Martin Jambor wrote a succinct summary of the conditions needed for this
bug:
"This bug can occur when an inlineable function containing a call to
__builtin_constant_p, which checks a parameter or a value it
references and a (possibly indirect) caller of the function actually
passes a constant, but stores it using a type of a different size."
So to prevent it from happening elsewhere in the kernel, it sounds like
we'd have to either remove all uses of __builtin_constant_p() or disable
inlining completely.
There's also no reliable way to detect the bug has occurred, though
objtool will detect it in cases when the function gets truncated.
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-15 5:45 ` Ingo Molnar
2016-04-15 13:47 ` Josh Poimboeuf
@ 2016-04-16 7:42 ` Arnd Bergmann
2016-04-18 13:22 ` Josh Poimboeuf
1 sibling, 1 reply; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-16 7:42 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Josh Poimboeuf, James Bottomley, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, linux-kernel,
linux-scsi
On Friday 15 April 2016 07:45:19 Ingo Molnar wrote:
>
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> > > In fact, the following patch seems to fix it:
> > >
> > > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > > index bf66ea6..56b9e81 100644
> > > --- a/include/scsi/scsi_transport_fc.h
> > > +++ b/include/scsi/scsi_transport_fc.h
> > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > return result;
> > > }
> > >
> > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > {
> > > return get_unaligned_be64(wwn);
> > > }
> >
> > It is not a guarantee.
>
> Of course it's a workaround - but is there any deterministic way to turn off this
> GCC bug (by activating some GCC command line switch), or do we have to live with
> objtool warning about this GCC?
>
> Which, by the way, is pretty cool!
I have done a patch for the asm-generic/unaligned handling recently that
reworks the implementation to avoid an ARM specific bug (gcc uses certain
CPU instructions that require aligned data when we tell it that unaligned
data is not).
It changes the code enough that the gcc bug might not be triggered any more,
aside from generating far superior code in some cases.
I thought I had submitted that patch before, but I can't find a version
with a proper changelog any more now, so I probably haven't. However, I did
all the research to show that it only makes things better on ARM and x86
except in cases where the gcc inliner happens to pick a different set of
functions to be inline (these have a 50:50 chance of better vs worse, the
result on average seems to be the same).
Arnd
commit 752b719f6675be02a3dd29fe5d92b2f380b5743d
Author: Arnd Bergmann <arnd@arndb.de>
Date: Fri Mar 4 16:15:20 2016 +0100
asm-generic: always use struct based unaligned access
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index 1ac097279db1..e8f5523eeb0a 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -3,29 +3,19 @@
/*
* This is the most generic implementation of unaligned accesses
- * and should work almost anywhere.
+ * and should work almost anywhere, we trust that the compiler
+ * knows how to handle unaligned accesses.
*/
#include <asm/byteorder.h>
-/* Set by the arch if it can handle unaligned accesses in hardware. */
-#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-# include <linux/unaligned/access_ok.h>
-#endif
+#include <linux/unaligned/le_struct.h>
+#include <linux/unaligned/be_struct.h>
+#include <linux/unaligned/generic.h>
#if defined(__LITTLE_ENDIAN)
-# ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-# include <linux/unaligned/le_struct.h>
-# include <linux/unaligned/be_byteshift.h>
-# endif
-# include <linux/unaligned/generic.h>
# define get_unaligned __get_unaligned_le
# define put_unaligned __put_unaligned_le
#elif defined(__BIG_ENDIAN)
-# ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-# include <linux/unaligned/be_struct.h>
-# include <linux/unaligned/le_byteshift.h>
-# endif
-# include <linux/unaligned/generic.h>
# define get_unaligned __get_unaligned_be
# define put_unaligned __put_unaligned_be
#else
diff --git a/include/linux/unaligned/be_struct.h b/include/linux/unaligned/be_struct.h
index 132415836c50..9ab8c53bb3fe 100644
--- a/include/linux/unaligned/be_struct.h
+++ b/include/linux/unaligned/be_struct.h
@@ -2,35 +2,36 @@
#define _LINUX_UNALIGNED_BE_STRUCT_H
#include <linux/unaligned/packed_struct.h>
+#include <asm/byteorder.h>
static inline u16 get_unaligned_be16(const void *p)
{
- return __get_unaligned_cpu16((const u8 *)p);
+ return be16_to_cpu((__le16 __force)__get_unaligned_cpu16((const u8 *)p));
}
static inline u32 get_unaligned_be32(const void *p)
{
- return __get_unaligned_cpu32((const u8 *)p);
+ return be32_to_cpu((__le32 __force)__get_unaligned_cpu32((const u8 *)p));
}
static inline u64 get_unaligned_be64(const void *p)
{
- return __get_unaligned_cpu64((const u8 *)p);
+ return be64_to_cpu((__le64 __force)__get_unaligned_cpu64((const u8 *)p));
}
static inline void put_unaligned_be16(u16 val, void *p)
{
- __put_unaligned_cpu16(val, p);
+ __put_unaligned_cpu16((u16 __force)cpu_to_be16(val), p);
}
static inline void put_unaligned_be32(u32 val, void *p)
{
- __put_unaligned_cpu32(val, p);
+ __put_unaligned_cpu32((u32 __force)cpu_to_be32(val), p);
}
static inline void put_unaligned_be64(u64 val, void *p)
{
- __put_unaligned_cpu64(val, p);
+ __put_unaligned_cpu64((u64 __force)cpu_to_be64(val), p);
}
#endif /* _LINUX_UNALIGNED_BE_STRUCT_H */
diff --git a/include/linux/unaligned/le_struct.h b/include/linux/unaligned/le_struct.h
index 088c4572faa8..64171ad0b100 100644
--- a/include/linux/unaligned/le_struct.h
+++ b/include/linux/unaligned/le_struct.h
@@ -2,35 +2,36 @@
#define _LINUX_UNALIGNED_LE_STRUCT_H
#include <linux/unaligned/packed_struct.h>
+#include <asm/byteorder.h>
static inline u16 get_unaligned_le16(const void *p)
{
- return __get_unaligned_cpu16((const u8 *)p);
+ return le16_to_cpu((__le16 __force)__get_unaligned_cpu16((const u8 *)p));
}
static inline u32 get_unaligned_le32(const void *p)
{
- return __get_unaligned_cpu32((const u8 *)p);
+ return le32_to_cpu((__le32 __force)__get_unaligned_cpu32((const u8 *)p));
}
static inline u64 get_unaligned_le64(const void *p)
{
- return __get_unaligned_cpu64((const u8 *)p);
+ return le64_to_cpu((__le64 __force)__get_unaligned_cpu64((const u8 *)p));
}
static inline void put_unaligned_le16(u16 val, void *p)
{
- __put_unaligned_cpu16(val, p);
+ __put_unaligned_cpu16((u16 __force)cpu_to_le16(val), p);
}
static inline void put_unaligned_le32(u32 val, void *p)
{
- __put_unaligned_cpu32(val, p);
+ __put_unaligned_cpu32((u32 __force)cpu_to_le32(val), p);
}
static inline void put_unaligned_le64(u64 val, void *p)
{
- __put_unaligned_cpu64(val, p);
+ __put_unaligned_cpu64((u64 __force)cpu_to_le64(val), p);
}
#endif /* _LINUX_UNALIGNED_LE_STRUCT_H */
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-15 22:20 ` Josh Poimboeuf
@ 2016-04-16 9:03 ` Ingo Molnar
2016-04-18 13:39 ` Josh Poimboeuf
0 siblings, 1 reply; 45+ messages in thread
From: Ingo Molnar @ 2016-04-16 9:03 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Denys Vlasenko, James Bottomley, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi, jamborm
* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > I don't think we know yet if there's a reliable way to turn the bug off.
> >
> > Also, according to the gcc guys, this bug won't always result in a
> > truncated function, and may sometimes just make some inline function
> > call sites disappear:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> >
> > though I haven't been able to confirm that experimentally. But if it's
> > true, that means that objtool won't be able to detect all cases of the
> > bug and some function calls may just silently disappear!
> >
> > There's a lot of activity in the bug now, so hopefully they'll be able
> > to tell us soon if there's a reliable way to avoid it and/or detect it.
> >
> > BTW, Denys posted a workaround patch for the qla2xxxx code:
> >
> > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
>
> Martin Jambor wrote a succinct summary of the conditions needed for this
> bug:
>
> "This bug can occur when an inlineable function containing a call to
> __builtin_constant_p, which checks a parameter or a value it
> references and a (possibly indirect) caller of the function actually
> passes a constant, but stores it using a type of a different size."
>
> So to prevent it from happening elsewhere in the kernel, it sounds like
> we'd have to either remove all uses of __builtin_constant_p() or disable
> inlining completely.
>
> There's also no reliable way to detect the bug has occurred, though
> objtool will detect it in cases when the function gets truncated.
So it appears to me that due to the hard to detect nature of the GCC bug the fix
will probably be backported by them, so I think we should be fine with relying on
objtool to detect weird code sequences in the kernel, and should work around
specific instances of the bug.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-16 7:42 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann
@ 2016-04-18 13:22 ` Josh Poimboeuf
0 siblings, 0 replies; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-18 13:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ingo Molnar, Denys Vlasenko, James Bottomley, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, linux-kernel,
linux-scsi
On Sat, Apr 16, 2016 at 09:42:37AM +0200, Arnd Bergmann wrote:
> On Friday 15 April 2016 07:45:19 Ingo Molnar wrote:
> >
> > * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> >
> > > > In fact, the following patch seems to fix it:
> > > >
> > > > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > > > index bf66ea6..56b9e81 100644
> > > > --- a/include/scsi/scsi_transport_fc.h
> > > > +++ b/include/scsi/scsi_transport_fc.h
> > > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > > return result;
> > > > }
> > > >
> > > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > > {
> > > > return get_unaligned_be64(wwn);
> > > > }
> > >
> > > It is not a guarantee.
> >
> > Of course it's a workaround - but is there any deterministic way to turn off this
> > GCC bug (by activating some GCC command line switch), or do we have to live with
> > objtool warning about this GCC?
> >
> > Which, by the way, is pretty cool!
>
> I have done a patch for the asm-generic/unaligned handling recently that
> reworks the implementation to avoid an ARM specific bug (gcc uses certain
> CPU instructions that require aligned data when we tell it that unaligned
> data is not).
>
> It changes the code enough that the gcc bug might not be triggered any more,
> aside from generating far superior code in some cases.
I tried this patch, but unfortunately it doesn't make the gcc bug go
away.
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-16 9:03 ` Ingo Molnar
@ 2016-04-18 13:39 ` Josh Poimboeuf
2016-04-18 14:07 ` Arnd Bergmann
2016-04-19 8:52 ` Ingo Molnar
0 siblings, 2 replies; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-18 13:39 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, James Bottomley, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi, jamborm
On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> > > I don't think we know yet if there's a reliable way to turn the bug off.
> > >
> > > Also, according to the gcc guys, this bug won't always result in a
> > > truncated function, and may sometimes just make some inline function
> > > call sites disappear:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > >
> > > though I haven't been able to confirm that experimentally. But if it's
> > > true, that means that objtool won't be able to detect all cases of the
> > > bug and some function calls may just silently disappear!
> > >
> > > There's a lot of activity in the bug now, so hopefully they'll be able
> > > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > >
> > > BTW, Denys posted a workaround patch for the qla2xxxx code:
> > >
> > > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
> >
> > Martin Jambor wrote a succinct summary of the conditions needed for this
> > bug:
> >
> > "This bug can occur when an inlineable function containing a call to
> > __builtin_constant_p, which checks a parameter or a value it
> > references and a (possibly indirect) caller of the function actually
> > passes a constant, but stores it using a type of a different size."
> >
> > So to prevent it from happening elsewhere in the kernel, it sounds like
> > we'd have to either remove all uses of __builtin_constant_p() or disable
> > inlining completely.
> >
> > There's also no reliable way to detect the bug has occurred, though
> > objtool will detect it in cases when the function gets truncated.
>
> So it appears to me that due to the hard to detect nature of the GCC bug the fix
> will probably be backported by them, so I think we should be fine with relying on
> objtool to detect weird code sequences in the kernel, and should work around
> specific instances of the bug.
I agree. So how should we work around the bug in this case? There have
been several suggestions:
- change wwn_to_u64() to __always_inline
- change qla2x00_get_host_fabric_name() to skip the unnecessary call to
wwn_to_u64()
- revert one of the two commits:
bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-18 13:39 ` Josh Poimboeuf
@ 2016-04-18 14:07 ` Arnd Bergmann
2016-04-18 14:12 ` Josh Poimboeuf
2016-04-19 8:52 ` Ingo Molnar
1 sibling, 1 reply; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-18 14:07 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ingo Molnar, Denys Vlasenko, James Bottomley, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, linux-kernel,
linux-scsi, jamborm
On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
>
> I agree. So how should we work around the bug in this case? There have
> been several suggestions:
>
> - change wwn_to_u64() to __always_inline
>
> - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> wwn_to_u64()
>
> - revert one of the two commits:
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
What about the patch to change get_unaligned_be64() that I posted?
I think we want to merge that anyway, I just don't know if that helps
with this particular problem as well.
Arnd
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-18 14:07 ` Arnd Bergmann
@ 2016-04-18 14:12 ` Josh Poimboeuf
2016-04-18 14:21 ` Arnd Bergmann
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-18 14:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ingo Molnar, Denys Vlasenko, James Bottomley, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, linux-kernel,
linux-scsi, jamborm
On Mon, Apr 18, 2016 at 04:07:51PM +0200, Arnd Bergmann wrote:
> On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> >
> > I agree. So how should we work around the bug in this case? There have
> > been several suggestions:
> >
> > - change wwn_to_u64() to __always_inline
> >
> > - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> > wwn_to_u64()
> >
> > - revert one of the two commits:
> > bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> > ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
>
> What about the patch to change get_unaligned_be64() that I posted?
>
> I think we want to merge that anyway, I just don't know if that helps
> with this particular problem as well.
I replied to your other email about that -- it doesn't seem to help this
issue.
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-18 14:12 ` Josh Poimboeuf
@ 2016-04-18 14:21 ` Arnd Bergmann
0 siblings, 0 replies; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-18 14:21 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ingo Molnar, Denys Vlasenko, James Bottomley, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, linux-kernel,
linux-scsi, jamborm
On Monday 18 April 2016 09:12:41 Josh Poimboeuf wrote:
> On Mon, Apr 18, 2016 at 04:07:51PM +0200, Arnd Bergmann wrote:
> > On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> > >
> > > I agree. So how should we work around the bug in this case? There have
> > > been several suggestions:
> > >
> > > - change wwn_to_u64() to __always_inline
> > >
> > > - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> > > wwn_to_u64()
> > >
> > > - revert one of the two commits:
> > > bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> > > ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> >
> > What about the patch to change get_unaligned_be64() that I posted?
> >
> > I think we want to merge that anyway, I just don't know if that helps
> > with this particular problem as well.
>
> I replied to your other email about that -- it doesn't seem to help this
> issue.
>
Ok, I see. I had problems with my mail server last week, your reply
must have been a victim of that as I never saw it (found it on the
web archive now).
I'd vote for the wwn_to_u64 change then as it should prevent the
same thing from happining in other drivers. I would prefer not to
see ef3fb2422ffe reverted, as that works around another gcc-6 bug
on ARM.
Arnd
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)
2016-04-18 13:39 ` Josh Poimboeuf
2016-04-18 14:07 ` Arnd Bergmann
@ 2016-04-19 8:52 ` Ingo Molnar
2016-04-19 13:56 ` [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf
1 sibling, 1 reply; 45+ messages in thread
From: Ingo Molnar @ 2016-04-19 8:52 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Denys Vlasenko, James Bottomley, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, linux-kernel, Arnd Bergmann,
linux-scsi, jamborm
* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
> >
> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > > > I don't think we know yet if there's a reliable way to turn the bug off.
> > > >
> > > > Also, according to the gcc guys, this bug won't always result in a
> > > > truncated function, and may sometimes just make some inline function
> > > > call sites disappear:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > > >
> > > > though I haven't been able to confirm that experimentally. But if it's
> > > > true, that means that objtool won't be able to detect all cases of the
> > > > bug and some function calls may just silently disappear!
> > > >
> > > > There's a lot of activity in the bug now, so hopefully they'll be able
> > > > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > > >
> > > > BTW, Denys posted a workaround patch for the qla2xxxx code:
> > > >
> > > > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
> > >
> > > Martin Jambor wrote a succinct summary of the conditions needed for this
> > > bug:
> > >
> > > "This bug can occur when an inlineable function containing a call to
> > > __builtin_constant_p, which checks a parameter or a value it
> > > references and a (possibly indirect) caller of the function actually
> > > passes a constant, but stores it using a type of a different size."
> > >
> > > So to prevent it from happening elsewhere in the kernel, it sounds like
> > > we'd have to either remove all uses of __builtin_constant_p() or disable
> > > inlining completely.
> > >
> > > There's also no reliable way to detect the bug has occurred, though
> > > objtool will detect it in cases when the function gets truncated.
> >
> > So it appears to me that due to the hard to detect nature of the GCC bug the fix
> > will probably be backported by them, so I think we should be fine with relying on
> > objtool to detect weird code sequences in the kernel, and should work around
> > specific instances of the bug.
>
> I agree. So how should we work around the bug in this case? There have
> been several suggestions:
>
> - change wwn_to_u64() to __always_inline
>
> - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> wwn_to_u64()
>
> - revert one of the two commits:
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
The first option sounds like the best one by far: it does a change that is related
to the GCC bug (tweaks inlining), has near zero impact and does not revert other
useful progress.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-19 8:52 ` Ingo Molnar
@ 2016-04-19 13:56 ` Josh Poimboeuf
2016-04-22 23:17 ` Quinn Tran
2016-04-25 16:07 ` Josh Poimboeuf
0 siblings, 2 replies; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-19 13:56 UTC (permalink / raw)
To: James Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Denys Vlasenko, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, Arnd Bergmann,
jamborm, Ingo Molnar, Himanshu Madhani, qla2xxx-upstream
objtool reports [1] the following warning:
drivers/scsi/qla2xxx/qla_attr.o: warning: objtool: qla2x00_get_host_fabric_name() falls through to next function qla2x00_get_starget_port_name()
This warning is due to a gcc bug [2] which causes corrupt code:
0000000000002f53 <qla2x00_get_host_fabric_name>:
2f53: 55 push %rbp
2f54: 48 89 e5 mov %rsp,%rbp
0000000000002f57 <qla2x00_get_fc_host_stats>:
2f57: 55 push %rbp
2f58: b9 e8 00 00 00 mov $0xe8,%ecx
2f5d: 48 89 e5 mov %rsp,%rbp
...
Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
setting up the frame pointer. It falls through to the next function,
which is very bad.
It occurs with the combination of the following two recent commits:
bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
The call chain which appears to trigger the problem is:
qla2x00_get_host_fabric_name()
wwn_to_u64()
get_unaligned_be64()
be64_to_cpup()
__be64_to_cpup()
The bug requires very specific conditions to trigger. According to Martin
Jambor (from the gcc bugzilla):
"This bug can occur when an inlineable function containing a call to
__builtin_constant_p, which checks a parameter or a value it
references and a (possibly indirect) caller of the function actually
passes a constant, but stores it using a type of a different size."
There's no reliable way to avoid (or even detect) the bug. Until it
gets fixed in released versions of gcc, the least intrusive workaround
for this particular issue is to force the wwn conversion functions to be
inlined.
[1] https://lists.01.org/pipermail/kbuild-all/2016-April/019579.html
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
include/scsi/scsi_transport_fc.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index bf66ea6..1919cd4 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -796,12 +796,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
return result;
}
-static inline u64 wwn_to_u64(u8 *wwn)
+static __always_inline u64 wwn_to_u64(u8 *wwn)
{
return get_unaligned_be64(wwn);
}
-static inline void u64_to_wwn(u64 inm, u8 *wwn)
+static __always_inline void u64_to_wwn(u64 inm, u8 *wwn)
{
put_unaligned_be64(inm, wwn);
}
--
2.4.11
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-19 13:56 ` [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf
@ 2016-04-22 23:17 ` Quinn Tran
2016-04-25 16:07 ` Josh Poimboeuf
1 sibling, 0 replies; 45+ messages in thread
From: Quinn Tran @ 2016-04-22 23:17 UTC (permalink / raw)
To: Josh Poimboeuf, James Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Denys Vlasenko, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, Arnd Bergmann,
jamborm@gcc.gnu.org, Ingo Molnar, Himanshu Madhani,
Dept-Eng QLA2xxx Upstream
Current kernel (4.6.0-rc4+) + GCC 5.3.0 definitely truncated qla2x00_get_host_fabric_name() routine. Just like Josh indicated, we’re dropping down to the next routine.
root@mars:/sys/class/fc_host/host3 2016-04-22 16:07:30
> cat fabric_name
Killed
——
static void
qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
{
32d0: e8 00 00 00 00 callq 32d5 <qla2x00_get_host_fabric_name+0x5>
32d5: 55 push %rbp
32d6: 48 89 e5 mov %rsp,%rbp
32d9: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
00000000000032e0 <qla2x00_get_starget_node_name>:
qla2x00_get_starget_node_name():
/root/qt/linux.git/drivers/scsi/qla2xxx/qla_attr.c:1756
fc_host_port_type(shost) = port_type;
}
----
Apr 22 16:07:50 mars kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
Apr 22 16:07:50 mars kernel: IP: [<ffffffff813f72d7>] scsi_is_host_device+0x7/0x20
Apr 22 16:07:50 mars kernel: PGD 7fe1c8067 PUD 7f5c72067 PMD 0
Apr 22 16:07:50 mars kernel: Oops: 0000 [#1] SMP
Apr 22 16:07:50 mars kernel: Modules linked in: qla2xxx scsi_transport_fc ebtable_nat ebtables ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4
...
dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E) [last unloaded: qla2xxx]
Apr 22 16:07:50 mars kernel: CPU: 8 PID: 10452 Comm: cat Tainted: G E 4.6.0-rc4+ #2
Apr 22 16:07:50 mars kernel: Hardware name: HP ProLiant DL380 G7, BIOS P67 05/05/2011
Apr 22 16:07:50 mars kernel: task: ffff8807fcd1a880 ti: ffff8807ff128000 task.ti: ffff8807ff128000
Apr 22 16:07:50 mars kernel: RIP: 0010:[<ffffffff813f72d7>] [<ffffffff813f72d7>] scsi_is_host_device+0x7/0x20
Apr 22 16:07:50 mars kernel: RSP: 0018:ffff8807ff12bcf0 EFLAGS: 00010246
Apr 22 16:07:50 mars kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff880ffe8ade88
Apr 22 16:07:50 mars kernel: RDX: ffff8807f5db9000 RSI: ffff880ffed43340 RDI: 0000000000000000
Apr 22 16:07:50 mars kernel: RBP: ffff8807ff12bd08 R08: ffff88101f45ac38 R09: ffff8807fccef280
Apr 22 16:07:50 mars kernel: R10: 0000000000000000 R11: 0000000000000000 R12: ffff88101b7e6000
Apr 22 16:07:50 mars kernel: R13: ffff8807fdaf1f00 R14: ffff8800dad379c0 R15: 0000000000000001
Apr 22 16:07:50 mars kernel: FS: 00007fd569512700(0000) GS:ffff88081fc80000(0000) knlGS:0000000000000000
Apr 22 16:07:50 mars kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Apr 22 16:07:50 mars kernel: CR2: 0000000000000058 CR3: 00000007f23f4000 CR4: 00000000000006e0
Apr 22 16:07:50 mars kernel: Stack:
Apr 22 16:07:50 mars kernel: ffffffffa0759db5 ffff88101b7e6000 ffff8807f5db9000 ffff8807ff12bd10
Apr 22 16:07:50 mars kernel: ffff8807ff12bd30 ffffffffa065b1bb ffff880ffed43340 ffffffff8166d950
Apr 22 16:07:50 mars kernel: ffff8807ff12bd50 ffffffff813c9e30 ffffffff815ccfb2 ffff8800dad379c0
Apr 22 16:07:50 mars kernel: Call Trace:
Apr 22 16:07:50 mars kernel: [<ffffffffa0759db5>] ? qla2x00_get_starget_node_name+0x25/0x90 [qla2xxx]
Apr 22 16:07:50 mars kernel: [<ffffffffa065b1bb>] ? show_fc_host_fabric_name+0x4b/0x80 [scsi_transport_fc]
Apr 22 16:07:50 mars kernel: [<ffffffff813c9e30>] ? dev_attr_show+0x20/0x50
Regards,
Quinn Tran
-----Original Message-----
From: <linux-scsi-owner@vger.kernel.org> on behalf of Josh Poimboeuf <jpoimboe@redhat.com>
Date: Tuesday, April 19, 2016 at 6:56 AM
To: James Bottomley <James.Bottomley@HansenPartnership.com>, "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi <linux-scsi@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Denys Vlasenko <dvlasenk@redhat.com>, Thomas Graf <tgraf@suug.ch>, Peter Zijlstra <peterz@infradead.org>, David Rientjes <rientjes@google.com>, Andrew Morton <akpm@linux-foundation.org>, Arnd Bergmann <arnd@arndb.de>, "jamborm@gcc.gnu.org" <jamborm@gcc.gnu.org>, Ingo Molnar <mingo@kernel.org>, Himanshu Madhani <himanshu.madhani@qlogic.com>, Dept-Eng QLA2xxx Upstream <qla2xxx-upstream@qlogic.com>
Subject: [PATCH] scsi: fc: force inlining of wwn conversion functions
>objtool reports [1] the following warning:
>
> drivers/scsi/qla2xxx/qla_attr.o: warning: objtool: qla2x00_get_host_fabric_name() falls through to next function qla2x00_get_starget_port_name()
>
>This warning is due to a gcc bug [2] which causes corrupt code:
>
> 0000000000002f53 <qla2x00_get_host_fabric_name>:
> 2f53: 55 push %rbp
> 2f54: 48 89 e5 mov %rsp,%rbp
>
> 0000000000002f57 <qla2x00_get_fc_host_stats>:
> 2f57: 55 push %rbp
> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> 2f5d: 48 89 e5 mov %rsp,%rbp
> ...
>
>Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
>setting up the frame pointer. It falls through to the next function,
>which is very bad.
>
>It occurs with the combination of the following two recent commits:
>
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
>
>The call chain which appears to trigger the problem is:
>
> qla2x00_get_host_fabric_name()
> wwn_to_u64()
> get_unaligned_be64()
> be64_to_cpup()
> __be64_to_cpup()
>
>The bug requires very specific conditions to trigger. According to Martin
>Jambor (from the gcc bugzilla):
>
> "This bug can occur when an inlineable function containing a call to
> __builtin_constant_p, which checks a parameter or a value it
> references and a (possibly indirect) caller of the function actually
> passes a constant, but stores it using a type of a different size."
>
>There's no reliable way to avoid (or even detect) the bug. Until it
>gets fixed in released versions of gcc, the least intrusive workaround
>for this particular issue is to force the wwn conversion functions to be
>inlined.
>
>[1] https://lists.01.org/pipermail/kbuild-all/2016-April/019579.html
>[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>
>Reported-by: kbuild test robot <fengguang.wu@intel.com>
>Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>---
> include/scsi/scsi_transport_fc.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
>index bf66ea6..1919cd4 100644
>--- a/include/scsi/scsi_transport_fc.h
>+++ b/include/scsi/scsi_transport_fc.h
>@@ -796,12 +796,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
> return result;
> }
>
>-static inline u64 wwn_to_u64(u8 *wwn)
>+static __always_inline u64 wwn_to_u64(u8 *wwn)
> {
> return get_unaligned_be64(wwn);
> }
>
>-static inline void u64_to_wwn(u64 inm, u8 *wwn)
>+static __always_inline void u64_to_wwn(u64 inm, u8 *wwn)
> {
> put_unaligned_be64(inm, wwn);
> }
>--
>2.4.11
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-19 13:56 ` [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf
2016-04-22 23:17 ` Quinn Tran
@ 2016-04-25 16:07 ` Josh Poimboeuf
2016-04-26 2:40 ` Martin K. Petersen
1 sibling, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-25 16:07 UTC (permalink / raw)
To: James Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Denys Vlasenko, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, Arnd Bergmann,
jamborm, Ingo Molnar, Himanshu Madhani, qla2xxx-upstream
James,
Can you merge this patch for 4.6?
On Tue, Apr 19, 2016 at 08:56:00AM -0500, Josh Poimboeuf wrote:
> objtool reports [1] the following warning:
>
> drivers/scsi/qla2xxx/qla_attr.o: warning: objtool: qla2x00_get_host_fabric_name() falls through to next function qla2x00_get_starget_port_name()
>
> This warning is due to a gcc bug [2] which causes corrupt code:
>
> 0000000000002f53 <qla2x00_get_host_fabric_name>:
> 2f53: 55 push %rbp
> 2f54: 48 89 e5 mov %rsp,%rbp
>
> 0000000000002f57 <qla2x00_get_fc_host_stats>:
> 2f57: 55 push %rbp
> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> 2f5d: 48 89 e5 mov %rsp,%rbp
> ...
>
> Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
> setting up the frame pointer. It falls through to the next function,
> which is very bad.
>
> It occurs with the combination of the following two recent commits:
>
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
>
> The call chain which appears to trigger the problem is:
>
> qla2x00_get_host_fabric_name()
> wwn_to_u64()
> get_unaligned_be64()
> be64_to_cpup()
> __be64_to_cpup()
>
> The bug requires very specific conditions to trigger. According to Martin
> Jambor (from the gcc bugzilla):
>
> "This bug can occur when an inlineable function containing a call to
> __builtin_constant_p, which checks a parameter or a value it
> references and a (possibly indirect) caller of the function actually
> passes a constant, but stores it using a type of a different size."
>
> There's no reliable way to avoid (or even detect) the bug. Until it
> gets fixed in released versions of gcc, the least intrusive workaround
> for this particular issue is to force the wwn conversion functions to be
> inlined.
>
> [1] https://lists.01.org/pipermail/kbuild-all/2016-April/019579.html
> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> include/scsi/scsi_transport_fc.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> index bf66ea6..1919cd4 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -796,12 +796,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
> return result;
> }
>
> -static inline u64 wwn_to_u64(u8 *wwn)
> +static __always_inline u64 wwn_to_u64(u8 *wwn)
> {
> return get_unaligned_be64(wwn);
> }
>
> -static inline void u64_to_wwn(u64 inm, u8 *wwn)
> +static __always_inline void u64_to_wwn(u64 inm, u8 *wwn)
> {
> put_unaligned_be64(inm, wwn);
> }
> --
> 2.4.11
>
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-25 16:07 ` Josh Poimboeuf
@ 2016-04-26 2:40 ` Martin K. Petersen
2016-04-26 3:37 ` James Bottomley
0 siblings, 1 reply; 45+ messages in thread
From: Martin K. Petersen @ 2016-04-26 2:40 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: James Bottomley, Martin K. Petersen, linux-scsi, linux-kernel,
Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, Arnd Bergmann, jamborm, Ingo Molnar,
Himanshu Madhani, qla2xxx-upstream
>>>>> "Josh" == Josh Poimboeuf <jpoimboe@redhat.com> writes:
Josh> Can you merge this patch for 4.6?
I am really not a big fan of working around compiler bugs in a device
driver.
Are we sure there are no other get_unaligned_be64() calls in the kernel
that suffer the same fate?
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 2:40 ` Martin K. Petersen
@ 2016-04-26 3:37 ` James Bottomley
2016-04-26 7:22 ` Arnd Bergmann
0 siblings, 1 reply; 45+ messages in thread
From: James Bottomley @ 2016-04-26 3:37 UTC (permalink / raw)
To: Martin K. Petersen, Josh Poimboeuf
Cc: linux-scsi, linux-kernel, Denys Vlasenko, Thomas Graf,
Peter Zijlstra, David Rientjes, Andrew Morton, Arnd Bergmann,
jamborm, Ingo Molnar, Himanshu Madhani, qla2xxx-upstream
On Mon, 2016-04-25 at 22:40 -0400, Martin K. Petersen wrote:
> > > > > > "Josh" == Josh Poimboeuf <jpoimboe@redhat.com> writes:
>
> Josh> Can you merge this patch for 4.6?
>
> I am really not a big fan of working around compiler bugs in a device
> driver.
Me neither
> Are we sure there are no other get_unaligned_be64() calls in the
> kernel that suffer the same fate?
Agree, plus, as I've said before, we have 3-4 weeks before we go final,
so we still have some time before a decision has to be made. It looks
like the gcc people already have a patch for the compiler, so the
distributions could just push that out through channels.
James
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 3:37 ` James Bottomley
@ 2016-04-26 7:22 ` Arnd Bergmann
2016-04-26 8:35 ` Christoph Hellwig
2016-04-26 13:06 ` Martin K. Petersen
0 siblings, 2 replies; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-26 7:22 UTC (permalink / raw)
To: James Bottomley
Cc: Martin K. Petersen, Josh Poimboeuf, linux-scsi, linux-kernel,
Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, jamborm, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream
On Monday 25 April 2016 20:37:31 James Bottomley wrote:
> On Mon, 2016-04-25 at 22:40 -0400, Martin K. Petersen wrote:
> > > > > > > "Josh" == Josh Poimboeuf <jpoimboe@redhat.com> writes:
> >
> > Josh> Can you merge this patch for 4.6?
> >
> > I am really not a big fan of working around compiler bugs in a device
> > driver.
>
> Me neither
>
> > Are we sure there are no other get_unaligned_be64() calls in the
> > kernel that suffer the same fate?
>
> Agree, plus, as I've said before, we have 3-4 weeks before we go final,
> so we still have some time before a decision has to be made. It looks
> like the gcc people already have a patch for the compiler, so the
> distributions could just push that out through channels.
I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to compilers
that have not been released yet in order to build a linux-4.6 kernel.
Arnd
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 7:22 ` Arnd Bergmann
@ 2016-04-26 8:35 ` Christoph Hellwig
2016-04-26 10:05 ` Arnd Bergmann
2016-04-26 13:06 ` Martin K. Petersen
1 sibling, 1 reply; 45+ messages in thread
From: Christoph Hellwig @ 2016-04-26 8:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: James Bottomley, Martin K. Petersen, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, jamborm, Ingo Molnar,
Himanshu Madhani, qla2xxx-upstream
On Tue, Apr 26, 2016 at 09:22:46AM +0200, Arnd Bergmann wrote:
> > Agree, plus, as I've said before, we have 3-4 weeks before we go final,
> > so we still have some time before a decision has to be made. It looks
> > like the gcc people already have a patch for the compiler, so the
> > distributions could just push that out through channels.
>
> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to compilers
> that have not been released yet in order to build a linux-4.6 kernel.
Agreed. What about just removing the wrappers? They seem fairly
pointless to start with.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 8:35 ` Christoph Hellwig
@ 2016-04-26 10:05 ` Arnd Bergmann
0 siblings, 0 replies; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-26 10:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: James Bottomley, Martin K. Petersen, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, jamborm, Ingo Molnar,
Himanshu Madhani, qla2xxx-upstream
On Tuesday 26 April 2016 01:35:16 Christoph Hellwig wrote:
> On Tue, Apr 26, 2016 at 09:22:46AM +0200, Arnd Bergmann wrote:
> > > Agree, plus, as I've said before, we have 3-4 weeks before we go final,
> > > so we still have some time before a decision has to be made. It looks
> > > like the gcc people already have a patch for the compiler, so the
> > > distributions could just push that out through channels.
> >
> > I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> > gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to compilers
> > that have not been released yet in order to build a linux-4.6 kernel.
>
> Agreed. What about just removing the wrappers? They seem fairly
> pointless to start with.
I think at this point it's mainly a question of whether we want such a
big (however trivial) patch in v4.6. We can certainly do that for 4.7,
but as a fixup for the existing problem, either the __always_inline
hack or using a macro should be sufficient:
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index bf66ea6bed2b..51a98b182a67 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -796,15 +796,8 @@ fc_remote_port_chkready(struct fc_rport *rport)
return result;
}
-static inline u64 wwn_to_u64(u8 *wwn)
-{
- return get_unaligned_be64(wwn);
-}
-
-static inline void u64_to_wwn(u64 inm, u8 *wwn)
-{
- put_unaligned_be64(inm, wwn);
-}
+#define wwn_to_u64(wwn) get_unaligned_be64(wwn)
+#define u64_to_wwn(inm, wwn) put_unaligned_be64(inm, wwn)
/**
* fc_vport_set_state() - called to set a vport's state. Saves the old state,
Arnd
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 7:22 ` Arnd Bergmann
2016-04-26 8:35 ` Christoph Hellwig
@ 2016-04-26 13:06 ` Martin K. Petersen
2016-04-26 15:58 ` Arnd Bergmann
1 sibling, 1 reply; 45+ messages in thread
From: Martin K. Petersen @ 2016-04-26 13:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: James Bottomley, Martin K. Petersen, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, jamborm, Ingo Molnar,
Himanshu Madhani, qla2xxx-upstream
>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
Arnd> compilers that have not been released yet in order to build a
Arnd> linux-4.6 kernel.
I agree that compiler blacklisting is problematic and I'd like to avoid
it. The question is how far we go in the kernel to accommodate various
levels of brokenness.
In any case. Sticking compiler workarounds in device driver code is akin
to putting demolition orders on display on Alpha Centauri. At the very
minimum the patch should put a fat comment in the code stating that
these wrapper functions or #defines should not be changed in the future
because that'll break builds using gcc XYZ. But that does not solve the
problem for anybody else that might be doing something similar.
Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
seem like a rare and unusual programming pattern.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 13:06 ` Martin K. Petersen
@ 2016-04-26 15:58 ` Arnd Bergmann
2016-04-26 22:36 ` James Bottomley
2016-04-27 11:05 ` Martin Jambor
0 siblings, 2 replies; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-26 15:58 UTC (permalink / raw)
To: Martin K. Petersen
Cc: James Bottomley, Josh Poimboeuf, linux-scsi, linux-kernel,
Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
Andrew Morton, jamborm, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream
On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> >>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
>
> Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
> Arnd> compilers that have not been released yet in order to build a
> Arnd> linux-4.6 kernel.
>
> I agree that compiler blacklisting is problematic and I'd like to avoid
> it. The question is how far we go in the kernel to accommodate various
> levels of brokenness.
>
> In any case. Sticking compiler workarounds in device driver code is akin
> to putting demolition orders on display on Alpha Centauri. At the very
> minimum the patch should put a fat comment in the code stating that
> these wrapper functions or #defines should not be changed in the future
> because that'll break builds using gcc XYZ. But that does not solve the
> problem for anybody else that might be doing something similar.
> Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
> seem like a rare and unusual programming pattern.
It's not the driver really, it's the core scsi/fc layer, which makes
it a little dangerous that a random driver.
I agree that putting a comment in would also help. What I understand
from the bug report is that to trigger this bug you need these elements:
1. an inline function marked __always_inline
2. another inline function that is automatically inlined (not __always_inline)
3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
4. __builtin_compatible_p inside that inline function
The last point is what Denys introduced in the kernel with
bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some
byteswap operations"). So maybe it's better after all to revert that
patch, to have a higher confidence in the same bug not appearing
elsewhere. It's also really a workaround for another quirk of the
compiler, but that one only results in duplicated functions in object
code rather than functions that end in the middle.
Arnd
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 15:58 ` Arnd Bergmann
@ 2016-04-26 22:36 ` James Bottomley
2016-04-27 0:44 ` Martin K. Petersen
2016-04-27 11:05 ` Martin Jambor
1 sibling, 1 reply; 45+ messages in thread
From: James Bottomley @ 2016-04-26 22:36 UTC (permalink / raw)
To: Arnd Bergmann, Martin K. Petersen
Cc: Josh Poimboeuf, linux-scsi, linux-kernel, Denys Vlasenko,
Thomas Graf, Peter Zijlstra, David Rientjes, Andrew Morton,
jamborm, Ingo Molnar, Himanshu Madhani, qla2xxx-upstream
On Tue, 2016-04-26 at 17:58 +0200, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> > > > > > > "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> >
> > Arnd> I don't think we can realistically blacklist gcc
> > -4.9.{0,1,2,3},
> > Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade
> > to
> > Arnd> compilers that have not been released yet in order to build a
> > Arnd> linux-4.6 kernel.
> >
> > I agree that compiler blacklisting is problematic and I'd like to
> > avoid it. The question is how far we go in the kernel to
> > accommodate various levels of brokenness.
> >
> > In any case. Sticking compiler workarounds in device driver code is
> > akin to putting demolition orders on display on Alpha Centauri. At
> > the very minimum the patch should put a fat comment in the code
> > stating that these wrapper functions or #defines should not be
> > changed in the future because that'll break builds using gcc XYZ.
> > But that does not solve the problem for anybody else that might be
> > doing something similar. Converting between u64 and $RANDOM_TYPE in
> > an inline wrapper does not seem like a rare and unusual programming
> > pattern.
>
> It's not the driver really, it's the core scsi/fc layer, which makes
> it a little dangerous that a random driver.
Well, it's libfc; that's a fibre channel transport class mostly used by
FCoE drivers ... there's few enough of those to call it driver only.
> I agree that putting a comment in would also help. What I understand
> from the bug report is that to trigger this bug you need these
> elements:
>
> 1. an inline function marked __always_inline
> 2. another inline function that is automatically inlined (not
> __always_inline)
> 3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
> 4. __builtin_compatible_p inside that inline function
>
> The last point is what Denys introduced in the kernel with
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of
> some byteswap operations"). So maybe it's better after all to revert
> that patch, to have a higher confidence in the same bug not appearing
> elsewhere. It's also really a workaround for another quirk of the
> compiler, but that one only results in duplicated functions in object
> code rather than functions that end in the middle.
Yes, I think this is my preferred option. That patch is nothing more
than an attempt to force the compiler to do something it didn't do but
should have. If we apply the general rule that we shouldn't work
around compiler problems in the kernel code, then that should have been
disallowed first. Plus, as the root cause of all of this, reverting
that patch will ensure that nothing else picks up this problem (at
least from the route we got it).
James
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 22:36 ` James Bottomley
@ 2016-04-27 0:44 ` Martin K. Petersen
0 siblings, 0 replies; 45+ messages in thread
From: Martin K. Petersen @ 2016-04-27 0:44 UTC (permalink / raw)
To: James Bottomley
Cc: Arnd Bergmann, Martin K. Petersen, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, jamborm, Ingo Molnar,
Himanshu Madhani, qla2xxx-upstream
>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:
>> The last point is what Denys introduced in the kernel with
>> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of
>> some byteswap operations"). So maybe it's better after all to revert
>> that patch, to have a higher confidence in the same bug not appearing
>> elsewhere. It's also really a workaround for another quirk of the
>> compiler, but that one only results in duplicated functions in object
>> code rather than functions that end in the middle.
James> Yes, I think this is my preferred option.
Same here.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-26 15:58 ` Arnd Bergmann
2016-04-26 22:36 ` James Bottomley
@ 2016-04-27 11:05 ` Martin Jambor
2016-04-27 21:34 ` Arnd Bergmann
2016-04-27 22:00 ` [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
1 sibling, 2 replies; 45+ messages in thread
From: Martin Jambor @ 2016-04-27 11:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Martin K. Petersen, James Bottomley, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream, Jan Hubicka
Hi,
On Tue, Apr 26, 2016 at 05:58:20PM +0200, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> > >>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> >
> > Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> > Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
> > Arnd> compilers that have not been released yet in order to build a
> > Arnd> linux-4.6 kernel.
> >
> > I agree that compiler blacklisting is problematic and I'd like to avoid
> > it. The question is how far we go in the kernel to accommodate various
> > levels of brokenness.
> >
> > In any case. Sticking compiler workarounds in device driver code is akin
> > to putting demolition orders on display on Alpha Centauri. At the very
> > minimum the patch should put a fat comment in the code stating that
> > these wrapper functions or #defines should not be changed in the future
> > because that'll break builds using gcc XYZ. But that does not solve the
> > problem for anybody else that might be doing something similar.
> > Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
> > seem like a rare and unusual programming pattern.
>
> It's not the driver really, it's the core scsi/fc layer, which makes
> it a little dangerous that a random driver.
>
> I agree that putting a comment in would also help. What I understand
> from the bug report is that to trigger this bug you need these elements:
>
> 1. an inline function marked __always_inline
> 2. another inline function that is automatically inlined (not __always_inline)
> 3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
> 4. __builtin_compatible_p inside that inline function
The __always_inline requirement is not true. In fact, if you look at
the example testcase filed in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c7 you'll see it
uses __builtin_compatible_p in an __always inline function that is
called from one that is not tagged with that attribute.
And generally speaking, always inline is never a requirement, any call
or chain of calls that the inliner can decide to inline can lead to
the bug (if it complies with the condition below).
What is a requirement, though, is that __builtin_compatible_p is
called on something passed in an argument by reference or in an
aggregate (i.e. struct or array) argument.
So,
int foo1 (unsigned long *ref)
{
if (__builtin_constant (*ref))
...
else
/* wrongly unreachable code */
}
can lead to this issue, as can
int foo2 (struct S s)
{
if ((__builtin_constant (s.l))
...
else
/* wrongly unreachable code */
}
but
int foo3 (unsigned long val)
{
if (__builtin_constant (val))
...
else
/* all OK */
}
cannot, and is fine. But please note that wrapping a foo[12]-like
function into a dereferencing wrapper might not help if foo[12] would
be early-inlined into such wrapper (GCC has two inliners, a very
simple early-inliner that only handles simple cases and a full-blown
IPA inliner that contains the bug). I believe this can be ensured by
making the wrapper always_inline and never calling it indirectly (via
a pointer). Honza (CCed), you know inlining heuristics better, please
correct me if my last statement is somehow inaccurate (or indeed if
you have a better idea how kernel developers can make sure they do not
hit the bug).
Thanks,
Martin
>
> The last point is what Denys introduced in the kernel with
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some
> byteswap operations"). So maybe it's better after all to revert that
> patch, to have a higher confidence in the same bug not appearing
> elsewhere. It's also really a workaround for another quirk of the
> compiler, but that one only results in duplicated functions in object
> code rather than functions that end in the middle.
>
> Arnd
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-27 11:05 ` Martin Jambor
@ 2016-04-27 21:34 ` Arnd Bergmann
2016-04-28 14:58 ` Chris Metcalf
2016-04-27 22:00 ` [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
1 sibling, 1 reply; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-27 21:34 UTC (permalink / raw)
To: Martin Jambor
Cc: Martin K. Petersen, James Bottomley, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream, Jan Hubicka, Chris Metcalf
On Wednesday 27 April 2016 13:05:03 Martin Jambor wrote:
> On Tue, Apr 26, 2016 at 05:58:20PM +0200, Arnd Bergmann wrote:
> > On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> > > >>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> > >
> > > Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> > > Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
> > > Arnd> compilers that have not been released yet in order to build a
> > > Arnd> linux-4.6 kernel.
> > >
> > > I agree that compiler blacklisting is problematic and I'd like to avoid
> > > it. The question is how far we go in the kernel to accommodate various
> > > levels of brokenness.
> > >
> > > In any case. Sticking compiler workarounds in device driver code is akin
> > > to putting demolition orders on display on Alpha Centauri. At the very
> > > minimum the patch should put a fat comment in the code stating that
> > > these wrapper functions or #defines should not be changed in the future
> > > because that'll break builds using gcc XYZ. But that does not solve the
> > > problem for anybody else that might be doing something similar.
> > > Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
> > > seem like a rare and unusual programming pattern.
> >
> > It's not the driver really, it's the core scsi/fc layer, which makes
> > it a little dangerous that a random driver.
> >
> > I agree that putting a comment in would also help. What I understand
> > from the bug report is that to trigger this bug you need these elements:
> >
> > 1. an inline function marked __always_inline
> > 2. another inline function that is automatically inlined (not __always_inline)
> > 3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
> > 4. __builtin_compatible_p inside that inline function
>
> The __always_inline requirement is not true. In fact, if you look at
> the example testcase filed in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c7 you'll see it
> uses __builtin_compatible_p in an __always inline function that is
> called from one that is not tagged with that attribute.
>
> And generally speaking, always inline is never a requirement, any call
> or chain of calls that the inliner can decide to inline can lead to
> the bug (if it complies with the condition below).
Ok, thanks for the clarification, I thought you always had to have both
kinds of inline functions.
> What is a requirement, though, is that __builtin_compatible_p is
> called on something passed in an argument by reference or in an
> aggregate (i.e. struct or array) argument.
>
> So,
>
> int foo1 (unsigned long *ref)
> {
> if (__builtin_constant (*ref))
> ...
> else
> /* wrongly unreachable code */
> }
>
> }
>
> cannot, and is fine. But please note that wrapping a foo[12]-like
> function into a dereferencing wrapper might not help if foo[12] would
> be early-inlined into such wrapper (GCC has two inliners, a very
> simple early-inliner that only handles simple cases and a full-blown
> IPA inliner that contains the bug). I believe this can be ensured by
> making the wrapper always_inline and never calling it indirectly (via
> a pointer). Honza (CCed), you know inlining heuristics better, please
> correct me if my last statement is somehow inaccurate (or indeed if
> you have a better idea how kernel developers can make sure they do not
> hit the bug).
I guess that means that any user of this code in the kernel:
static inline __attribute_const__ __u64 __fswab64(__u64 val)
{
#ifdef __HAVE_BUILTIN_BSWAP64__
return __builtin_bswap64(val);
#elif defined (__arch_swab64)
return __arch_swab64(val);
#elif defined(__SWAB_64_THRU_32__)
__u32 h = val >> 32;
__u32 l = val & ((1ULL << 32) - 1);
return (((__u64)__fswab32(l)) << 32) | ((__u64)(__fswab32(h)));
#else
return ___constant_swab64(val);
#endif
}
#define __swab64(x) \
(__builtin_constant_p((__u64)(x)) ? \
___constant_swab64(x) : \
__fswab64(x))
static __always_inline __u64 __swab64p(const __u64 *p)
{
#ifdef __arch_swab64p
return __arch_swab64p(p);
#else
return __swab64(*p);
#endif
}
has a chance of running into the same problem, and we may want to solve
it at the root. For architectures that define __HAVE_BUILTIN_BSWAP64__
(i.e. ARM, MIPS, POWERPC, S390, and x86 with gcc-4.4 or higher, 4.8
for __HAVE_BUILTIN_BSWAP16__), we can probably just change the logic
to avoid __builtin_constant_p() and always use __builtin_bswap64().
This won't help on TILE, which is the one architecture that sets
ARCH_SUPPORTS_OPTIMIZED_INLINING but does not set ARCH_USE_BUILTIN_BSWAP.
Chris Metcalf should be able to figure out whether we can just
set ARCH_USE_BUILTIN_BSWAP for tile as well.
Arnd
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug
2016-04-27 11:05 ` Martin Jambor
2016-04-27 21:34 ` Arnd Bergmann
@ 2016-04-27 22:00 ` Arnd Bergmann
2016-04-27 22:11 ` Josh Poimboeuf
1 sibling, 1 reply; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-27 22:00 UTC (permalink / raw)
To: Martin Jambor
Cc: Martin K. Petersen, James Bottomley, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream, Jan Hubicka
This is another attempt to avoid a regression in wwn_to_u64()
after that started using get_unaligned_be64(), which in turn
ran into a bug on gcc-4.9 through 6.1.
As part of the problem is how __builtin_constant_p gets evaluated
on an argument passed by reference into an inline function, this
avoids the use of __builtin_constant_p() for all architectures
that set CONFIG_ARCH_USE_BUILTIN_BSWAP. Most architectures do not
set ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably
do not suffer from the problem in the qla2xxx driver, but they
might still run into it elsewhere.
I have not been able to reproduce the original problem, so I don't
know if this patch solves it, but at least it leads to simpler
code doing the same thing, so at least there should be no downsides.
Please test.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index 3f10e5317b46..de56fd54428d 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -45,9 +45,7 @@
static inline __attribute_const__ __u16 __fswab16(__u16 val)
{
-#ifdef __HAVE_BUILTIN_BSWAP16__
- return __builtin_bswap16(val);
-#elif defined (__arch_swab16)
+#if defined (__arch_swab16)
return __arch_swab16(val);
#else
return ___constant_swab16(val);
@@ -56,9 +54,7 @@ static inline __attribute_const__ __u16 __fswab16(__u16 val)
static inline __attribute_const__ __u32 __fswab32(__u32 val)
{
-#ifdef __HAVE_BUILTIN_BSWAP32__
- return __builtin_bswap32(val);
-#elif defined(__arch_swab32)
+#if defined(__arch_swab32)
return __arch_swab32(val);
#else
return ___constant_swab32(val);
@@ -67,9 +63,7 @@ static inline __attribute_const__ __u32 __fswab32(__u32 val)
static inline __attribute_const__ __u64 __fswab64(__u64 val)
{
-#ifdef __HAVE_BUILTIN_BSWAP64__
- return __builtin_bswap64(val);
-#elif defined (__arch_swab64)
+#if defined (__arch_swab64)
return __arch_swab64(val);
#elif defined(__SWAB_64_THRU_32__)
__u32 h = val >> 32;
@@ -102,28 +96,40 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
* __swab16 - return a byteswapped 16-bit value
* @x: value to byteswap
*/
+#ifdef __HAVE_BUILTIN_BSWAP16__
+#define __swab16(x) __builtin_bswap16((__u16)(x))
+#else
#define __swab16(x) \
(__builtin_constant_p((__u16)(x)) ? \
___constant_swab16(x) : \
__fswab16(x))
+#endif
/**
* __swab32 - return a byteswapped 32-bit value
* @x: value to byteswap
*/
+#ifdef __HAVE_BUILTIN_BSWAP32__
+#define __swab32(x) __builtin_bswap32((__u32)(x))
+#else
#define __swab32(x) \
(__builtin_constant_p((__u32)(x)) ? \
___constant_swab32(x) : \
__fswab32(x))
+#endif
/**
* __swab64 - return a byteswapped 64-bit value
* @x: value to byteswap
*/
+#ifdef __HAVE_BUILTIN_BSWAP64__
+#define __swab64(x) __builtin_bswap64((__u64)(x))
+#else
#define __swab64(x) \
(__builtin_constant_p((__u64)(x)) ? \
___constant_swab64(x) : \
__fswab64(x))
+#endif
/**
* __swahw32 - return a word-swapped 32-bit value
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug
2016-04-27 22:00 ` [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
@ 2016-04-27 22:11 ` Josh Poimboeuf
2016-04-28 16:27 ` Quinn Tran
0 siblings, 1 reply; 45+ messages in thread
From: Josh Poimboeuf @ 2016-04-27 22:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Martin Jambor, Martin K. Petersen, James Bottomley, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream, Jan Hubicka
On Thu, Apr 28, 2016 at 12:00:36AM +0200, Arnd Bergmann wrote:
> This is another attempt to avoid a regression in wwn_to_u64()
> after that started using get_unaligned_be64(), which in turn
> ran into a bug on gcc-4.9 through 6.1.
>
> As part of the problem is how __builtin_constant_p gets evaluated
> on an argument passed by reference into an inline function, this
> avoids the use of __builtin_constant_p() for all architectures
> that set CONFIG_ARCH_USE_BUILTIN_BSWAP. Most architectures do not
> set ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably
> do not suffer from the problem in the qla2xxx driver, but they
> might still run into it elsewhere.
>
> I have not been able to reproduce the original problem, so I don't
> know if this patch solves it, but at least it leads to simpler
> code doing the same thing, so at least there should be no downsides.
>
> Please test.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Nice patch. I can confirm it fixes the issue with gcc 5.3.1.
Tested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
> index 3f10e5317b46..de56fd54428d 100644
> --- a/include/uapi/linux/swab.h
> +++ b/include/uapi/linux/swab.h
> @@ -45,9 +45,7 @@
>
> static inline __attribute_const__ __u16 __fswab16(__u16 val)
> {
> -#ifdef __HAVE_BUILTIN_BSWAP16__
> - return __builtin_bswap16(val);
> -#elif defined (__arch_swab16)
> +#if defined (__arch_swab16)
> return __arch_swab16(val);
> #else
> return ___constant_swab16(val);
> @@ -56,9 +54,7 @@ static inline __attribute_const__ __u16 __fswab16(__u16 val)
>
> static inline __attribute_const__ __u32 __fswab32(__u32 val)
> {
> -#ifdef __HAVE_BUILTIN_BSWAP32__
> - return __builtin_bswap32(val);
> -#elif defined(__arch_swab32)
> +#if defined(__arch_swab32)
> return __arch_swab32(val);
> #else
> return ___constant_swab32(val);
> @@ -67,9 +63,7 @@ static inline __attribute_const__ __u32 __fswab32(__u32 val)
>
> static inline __attribute_const__ __u64 __fswab64(__u64 val)
> {
> -#ifdef __HAVE_BUILTIN_BSWAP64__
> - return __builtin_bswap64(val);
> -#elif defined (__arch_swab64)
> +#if defined (__arch_swab64)
> return __arch_swab64(val);
> #elif defined(__SWAB_64_THRU_32__)
> __u32 h = val >> 32;
> @@ -102,28 +96,40 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
> * __swab16 - return a byteswapped 16-bit value
> * @x: value to byteswap
> */
> +#ifdef __HAVE_BUILTIN_BSWAP16__
> +#define __swab16(x) __builtin_bswap16((__u16)(x))
> +#else
> #define __swab16(x) \
> (__builtin_constant_p((__u16)(x)) ? \
> ___constant_swab16(x) : \
> __fswab16(x))
> +#endif
>
> /**
> * __swab32 - return a byteswapped 32-bit value
> * @x: value to byteswap
> */
> +#ifdef __HAVE_BUILTIN_BSWAP32__
> +#define __swab32(x) __builtin_bswap32((__u32)(x))
> +#else
> #define __swab32(x) \
> (__builtin_constant_p((__u32)(x)) ? \
> ___constant_swab32(x) : \
> __fswab32(x))
> +#endif
>
> /**
> * __swab64 - return a byteswapped 64-bit value
> * @x: value to byteswap
> */
> +#ifdef __HAVE_BUILTIN_BSWAP64__
> +#define __swab64(x) __builtin_bswap64((__u64)(x))
> +#else
> #define __swab64(x) \
> (__builtin_constant_p((__u64)(x)) ? \
> ___constant_swab64(x) : \
> __fswab64(x))
> +#endif
>
> /**
> * __swahw32 - return a word-swapped 32-bit value
>
--
Josh
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-27 21:34 ` Arnd Bergmann
@ 2016-04-28 14:58 ` Chris Metcalf
2016-04-28 15:23 ` Arnd Bergmann
0 siblings, 1 reply; 45+ messages in thread
From: Chris Metcalf @ 2016-04-28 14:58 UTC (permalink / raw)
To: Arnd Bergmann, Martin Jambor
Cc: Martin K. Petersen, James Bottomley, Josh Poimboeuf, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, Ingo Molnar, Himanshu Madhani,
qla2xxx-upstream, Jan Hubicka
(Resending as text/plain)
On 4/27/2016 5:34 PM, Arnd Bergmann wrote:
> This won't help on TILE, which is the one architecture that sets
> ARCH_SUPPORTS_OPTIMIZED_INLINING but does not set ARCH_USE_BUILTIN_BSWAP.
> Chris Metcalf should be able to figure out whether we can just
> set ARCH_USE_BUILTIN_BSWAP for tile as well.
We certainly could enable ARCH_USE_BUILTIN_BSWAP. The only problem is
that we never added explicit support for bswap16() in gcc, which is
efficiently done on tilegx via the "revbytes" instruction and a 48-bit
right-shift. So gcc instead does a generic thing with four
instructions in three bundles, so really not as good as our asm/swab.h.
I'm not sure how to weigh the implications of converting to
builtin_bswap16 (and possibly upstreaming a better implementation to
gcc), vs. disabling ARCH_SUPPORTS_OPTIMIZED_INLINING (which no one
else but x86 uses anyway), vs. just ignoring the compiler bug and
hoping it's not an issue in practice :-)
--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-28 14:58 ` Chris Metcalf
@ 2016-04-28 15:23 ` Arnd Bergmann
2016-04-28 15:48 ` Chris Metcalf
0 siblings, 1 reply; 45+ messages in thread
From: Arnd Bergmann @ 2016-04-28 15:23 UTC (permalink / raw)
To: Chris Metcalf
Cc: Martin Jambor, Martin K. Petersen, James Bottomley,
Josh Poimboeuf, linux-scsi, linux-kernel, Denys Vlasenko,
Thomas Graf, Peter Zijlstra, David Rientjes, Andrew Morton,
Ingo Molnar, Himanshu Madhani, qla2xxx-upstream, Jan Hubicka
On Thursday 28 April 2016 10:58:43 Chris Metcalf wrote:
> (Resending as text/plain)
>
> On 4/27/2016 5:34 PM, Arnd Bergmann wrote:
> > This won't help on TILE, which is the one architecture that sets
> > ARCH_SUPPORTS_OPTIMIZED_INLINING but does not set ARCH_USE_BUILTIN_BSWAP.
> > Chris Metcalf should be able to figure out whether we can just
> > set ARCH_USE_BUILTIN_BSWAP for tile as well.
>
> We certainly could enable ARCH_USE_BUILTIN_BSWAP. The only problem is
> that we never added explicit support for bswap16() in gcc, which is
> efficiently done on tilegx via the "revbytes" instruction and a 48-bit
> right-shift. So gcc instead does a generic thing with four
> instructions in three bundles, so really not as good as our asm/swab.h.
>
> I'm not sure how to weigh the implications of converting to
> builtin_bswap16 (and possibly upstreaming a better implementation to
> gcc), vs. disabling ARCH_SUPPORTS_OPTIMIZED_INLINING (which no one
> else but x86 uses anyway), vs. just ignoring the compiler bug and
> hoping it's not an issue in practice
How about figuring out whether you hit the gcc bug on tile as a
first step?
Another idea would be to adapt this section in include/linux/compiler-gcc.h:
#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
!defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
#define inline inline __attribute__((always_inline)) notrace
#define __inline__ __inline__ __attribute__((always_inline)) notrace
#define __inline __inline __attribute__((always_inline)) notrace
#else
/* A lot of inline functions can cause havoc with function tracing */
#define inline inline notrace
#define __inline__ __inline__ notrace
#define __inline __inline notrace
#endif
to work around the issue. We already check for gcc before 4.0, and
we could also check for the affected releases (4.9, 5.x, 6.1) in the
same place, possibly conditional on ARCH_USE_BUILTIN_BSWAP with
a comment pointing to the gcc bug tracker.
Arnd
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH] scsi: fc: force inlining of wwn conversion functions
2016-04-28 15:23 ` Arnd Bergmann
@ 2016-04-28 15:48 ` Chris Metcalf
0 siblings, 0 replies; 45+ messages in thread
From: Chris Metcalf @ 2016-04-28 15:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Martin Jambor, Martin K. Petersen, James Bottomley,
Josh Poimboeuf, linux-scsi, linux-kernel, Denys Vlasenko,
Thomas Graf, Peter Zijlstra, David Rientjes, Andrew Morton,
Ingo Molnar, Himanshu Madhani, qla2xxx-upstream, Jan Hubicka
On 4/28/2016 11:23 AM, Arnd Bergmann wrote:
> On Thursday 28 April 2016 10:58:43 Chris Metcalf wrote:
>> (Resending as text/plain)
>>
>> On 4/27/2016 5:34 PM, Arnd Bergmann wrote:
>>> This won't help on TILE, which is the one architecture that sets
>>> ARCH_SUPPORTS_OPTIMIZED_INLINING but does not set ARCH_USE_BUILTIN_BSWAP.
>>> Chris Metcalf should be able to figure out whether we can just
>>> set ARCH_USE_BUILTIN_BSWAP for tile as well.
>> We certainly could enable ARCH_USE_BUILTIN_BSWAP. The only problem is
>> that we never added explicit support for bswap16() in gcc, which is
>> efficiently done on tilegx via the "revbytes" instruction and a 48-bit
>> right-shift. So gcc instead does a generic thing with four
>> instructions in three bundles, so really not as good as our asm/swab.h.
>>
>> I'm not sure how to weigh the implications of converting to
>> builtin_bswap16 (and possibly upstreaming a better implementation to
>> gcc), vs. disabling ARCH_SUPPORTS_OPTIMIZED_INLINING (which no one
>> else but x86 uses anyway), vs. just ignoring the compiler bug and
>> hoping it's not an issue in practice
> How about figuring out whether you hit the gcc bug on tile as a
> first step?
I don't have an affected build of gcc handy (just 4.8 and 4.4). I will pass
this to our compiler folks and see what they know.
> Another idea would be to adapt this section in include/linux/compiler-gcc.h:
>
> #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
> #define inline inline __attribute__((always_inline)) notrace
> #define __inline__ __inline__ __attribute__((always_inline)) notrace
> #define __inline __inline __attribute__((always_inline)) notrace
> #else
> /* A lot of inline functions can cause havoc with function tracing */
> #define inline inline notrace
> #define __inline__ __inline__ notrace
> #define __inline __inline notrace
> #endif
>
> to work around the issue. We already check for gcc before 4.0, and
> we could also check for the affected releases (4.9, 5.x, 6.1) in the
> same place, possibly conditional on ARCH_USE_BUILTIN_BSWAP with
> a comment pointing to the gcc bug tracker.
This does seem like a more robust solution anyway, since more instances of the
bad inline pattern might get introduced in the future in other places.
I wouldn't make it conditional on ARCH_USE_BUILTIN_BSWAP for the same reason.
--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug
2016-04-27 22:11 ` Josh Poimboeuf
@ 2016-04-28 16:27 ` Quinn Tran
0 siblings, 0 replies; 45+ messages in thread
From: Quinn Tran @ 2016-04-28 16:27 UTC (permalink / raw)
To: Josh Poimboeuf, Arnd Bergmann
Cc: Martin Jambor, Martin K. Petersen, James Bottomley, linux-scsi,
linux-kernel, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
David Rientjes, Andrew Morton, Ingo Molnar, Himanshu Madhani,
Dept-Eng QLA2xxx Upstream, Jan Hubicka
-----Original Message-----
From: <linux-scsi-owner@vger.kernel.org> on behalf of Josh Poimboeuf <jpoimboe@redhat.com>
Date: Wednesday, April 27, 2016 at 3:11 PM
To: Arnd Bergmann <arnd@arndb.de>
Cc: Martin Jambor <mjambor@suse.cz>, "Martin K. Petersen" <martin.petersen@oracle.com>, James Bottomley <James.Bottomley@hansenpartnership.com>, linux-scsi <linux-scsi@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Denys Vlasenko <dvlasenk@redhat.com>, Thomas Graf <tgraf@suug.ch>, Peter Zijlstra <peterz@infradead.org>, David Rientjes <rientjes@google.com>, Andrew Morton <akpm@linux-foundation.org>, Ingo Molnar <mingo@kernel.org>, Himanshu Madhani <himanshu.madhani@qlogic.com>, Dept-Eng QLA2xxx Upstream <qla2xxx-upstream@qlogic.com>, Jan Hubicka <hubicka@ucw.cz>
Subject: Re: [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug
>On Thu, Apr 28, 2016 at 12:00:36AM +0200, Arnd Bergmann wrote:
>> This is another attempt to avoid a regression in wwn_to_u64()
>> after that started using get_unaligned_be64(), which in turn
>> ran into a bug on gcc-4.9 through 6.1.
>>
>> As part of the problem is how __builtin_constant_p gets evaluated
>> on an argument passed by reference into an inline function, this
>> avoids the use of __builtin_constant_p() for all architectures
>> that set CONFIG_ARCH_USE_BUILTIN_BSWAP. Most architectures do not
>> set ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably
>> do not suffer from the problem in the qla2xxx driver, but they
>> might still run into it elsewhere.
>>
>> I have not been able to reproduce the original problem, so I don't
>> know if this patch solves it, but at least it leads to simpler
>> code doing the same thing, so at least there should be no downsides.
>>
>> Please test.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
>Nice patch. I can confirm it fixes the issue with gcc 5.3.1.
>
>Tested-by: Josh Poimboeuf <jpoimboe@redhat.com>
>Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
>
>> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
>> index 3f10e5317b46..de56fd54428d 100644
>> --- a/include/uapi/linux/swab.h
>> +++ b/include/uapi/linux/swab.h
>> @@ -45,9 +45,7 @@
>>
>> static inline __attribute_const__ __u16 __fswab16(__u16 val)
>> {
>> -#ifdef __HAVE_BUILTIN_BSWAP16__
>> - return __builtin_bswap16(val);
>> -#elif defined (__arch_swab16)
>> +#if defined (__arch_swab16)
>> return __arch_swab16(val);
>> #else
>> return ___constant_swab16(val);
>> @@ -56,9 +54,7 @@ static inline __attribute_const__ __u16 __fswab16(__u16 val)
>>
>> static inline __attribute_const__ __u32 __fswab32(__u32 val)
>> {
>> -#ifdef __HAVE_BUILTIN_BSWAP32__
>> - return __builtin_bswap32(val);
>> -#elif defined(__arch_swab32)
>> +#if defined(__arch_swab32)
>> return __arch_swab32(val);
>> #else
>> return ___constant_swab32(val);
>> @@ -67,9 +63,7 @@ static inline __attribute_const__ __u32 __fswab32(__u32 val)
>>
>> static inline __attribute_const__ __u64 __fswab64(__u64 val)
>> {
>> -#ifdef __HAVE_BUILTIN_BSWAP64__
>> - return __builtin_bswap64(val);
>> -#elif defined (__arch_swab64)
>> +#if defined (__arch_swab64)
>> return __arch_swab64(val);
>> #elif defined(__SWAB_64_THRU_32__)
>> __u32 h = val >> 32;
>> @@ -102,28 +96,40 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>> * __swab16 - return a byteswapped 16-bit value
>> * @x: value to byteswap
>> */
>> +#ifdef __HAVE_BUILTIN_BSWAP16__
>> +#define __swab16(x) __builtin_bswap16((__u16)(x))
>> +#else
>> #define __swab16(x) \
>> (__builtin_constant_p((__u16)(x)) ? \
>> ___constant_swab16(x) : \
>> __fswab16(x))
>> +#endif
>>
>> /**
>> * __swab32 - return a byteswapped 32-bit value
>> * @x: value to byteswap
>> */
>> +#ifdef __HAVE_BUILTIN_BSWAP32__
>> +#define __swab32(x) __builtin_bswap32((__u32)(x))
>> +#else
>> #define __swab32(x) \
>> (__builtin_constant_p((__u32)(x)) ? \
>> ___constant_swab32(x) : \
>> __fswab32(x))
>> +#endif
>>
>> /**
>> * __swab64 - return a byteswapped 64-bit value
>> * @x: value to byteswap
>> */
>> +#ifdef __HAVE_BUILTIN_BSWAP64__
>> +#define __swab64(x) __builtin_bswap64((__u64)(x))
>> +#else
>> #define __swab64(x) \
>> (__builtin_constant_p((__u64)(x)) ? \
>> ___constant_swab64(x) : \
>> __fswab64(x))
>> +#endif
>>
>> /**
>> * __swahw32 - return a word-swapped 32-bit value
>>
The patch works. Thanks.
>
>--
>Josh
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2016-04-28 16:27 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-04 19:45 [PATCH] asm-generic: force inlining of some atomic_long operations Denys Vlasenko
2016-02-04 19:45 ` [PATCH] force inlining of some byteswap operations Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
2016-04-13 3:36 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf
2016-04-13 12:12 ` Denys Vlasenko
2016-04-13 12:36 ` Josh Poimboeuf
2016-04-13 15:15 ` Josh Poimboeuf
2016-04-13 16:55 ` James Bottomley
2016-04-13 17:10 ` Josh Poimboeuf
2016-04-14 15:29 ` Denys Vlasenko
2016-04-14 15:57 ` Josh Poimboeuf
2016-04-14 17:09 ` Denys Vlasenko
2016-04-15 5:45 ` Ingo Molnar
2016-04-15 13:47 ` Josh Poimboeuf
2016-04-15 22:20 ` Josh Poimboeuf
2016-04-16 9:03 ` Ingo Molnar
2016-04-18 13:39 ` Josh Poimboeuf
2016-04-18 14:07 ` Arnd Bergmann
2016-04-18 14:12 ` Josh Poimboeuf
2016-04-18 14:21 ` Arnd Bergmann
2016-04-19 8:52 ` Ingo Molnar
2016-04-19 13:56 ` [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf
2016-04-22 23:17 ` Quinn Tran
2016-04-25 16:07 ` Josh Poimboeuf
2016-04-26 2:40 ` Martin K. Petersen
2016-04-26 3:37 ` James Bottomley
2016-04-26 7:22 ` Arnd Bergmann
2016-04-26 8:35 ` Christoph Hellwig
2016-04-26 10:05 ` Arnd Bergmann
2016-04-26 13:06 ` Martin K. Petersen
2016-04-26 15:58 ` Arnd Bergmann
2016-04-26 22:36 ` James Bottomley
2016-04-27 0:44 ` Martin K. Petersen
2016-04-27 11:05 ` Martin Jambor
2016-04-27 21:34 ` Arnd Bergmann
2016-04-28 14:58 ` Chris Metcalf
2016-04-28 15:23 ` Arnd Bergmann
2016-04-28 15:48 ` Chris Metcalf
2016-04-27 22:00 ` [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
2016-04-27 22:11 ` Josh Poimboeuf
2016-04-28 16:27 ` Quinn Tran
2016-04-16 7:42 ` This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann
2016-04-18 13:22 ` Josh Poimboeuf
2016-02-04 19:45 ` [PATCH] force inlining of unaligned byteswap operations Denys Vlasenko
2016-02-05 7:28 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).