+ x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile.patch added to -mm tree

+ x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile.patch added to -mm tree

[akpm]

The patch titled
     x86: avoid 'constant_test_bit()' misoptimization due to cast to non-volatile
has been added to the -mm tree.  Its filename is
     x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: x86: avoid 'constant_test_bit()' misoptimization due to cast to non-volatile
From: Led <led@altlinux.ru>

While debugging bit_spin_lock() hang, it was tracked down to gcc-4.4
misoptimization of constant_test_bit() when 'const volatile unsigned long *addr'
cast to 'unsigned long *' with subsequent unconditional jump to pause
(and not to the test) leading to hang.

Compiling with gcc-4.3 or disabling CONFIG_OPTIMIZE_INLINING yields inlined
constant_test_bit() and correct jump.

Other arches than asm-x86 may implement this slightly differently; 2.6.29
mitigates the misoptimization by changing the function prototype in commit
c4295fbb6048 ("x86: make 'constant_test_bit()' take an unsigned bit
number") but probably fixing the issue itself is better.

Cc: Michael Shigorin <mike@osdn.org.ua>
Cc: Volodymyr G. Lukiianyk <volodymyrgl@gmail.com>
Cc: Alexander Chumachenko <ledest@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Glauber de Oliveira Costa <gcosta@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/x86/include/asm/bitops.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -puN arch/x86/include/asm/bitops.h~x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile arch/x86/include/asm/bitops.h
--- a/arch/x86/include/asm/bitops.h~x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile
+++ a/arch/x86/include/asm/bitops.h
@@ -308,8 +308,7 @@ static inline int test_and_change_bit(in
 
 static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
 {
-	return ((1UL << (nr % BITS_PER_LONG)) &
-		(((unsigned long *)addr)[nr / BITS_PER_LONG])) != 0;
+	return ((1UL << (nr % BITS_PER_LONG)) & addr[nr / BITS_PER_LONG]) != 0;
 }
 
 static inline int variable_test_bit(int nr, volatile const unsigned long *addr)
_

Patches currently in -mm which might be from led@altlinux.ru are

x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile.patch

Re: + x86-avoid-constant_test_bit-misoptimization-due-to-cast-to-non-volatile.patch added to -mm tree

[H. Peter Anvin]

On 09/23/2010 05:08 PM, Linus Torvalds wrote:
> On Thu, Sep 23, 2010 at 4:51 PM,  <akpm@linux-foundation.org> wrote:
>>
>> Subject: x86: avoid 'constant_test_bit()' misoptimization due to cast to non-volatile
>> From: Led <led@altlinux.ru>
>>
>> While debugging bit_spin_lock() hang, it was tracked down to gcc-4.4
>> misoptimization of constant_test_bit() when 'const volatile unsigned long *addr'
>> cast to 'unsigned long *' with subsequent unconditional jump to pause
>> (and not to the test) leading to hang.
> 
> Ack on the patch, however I think the commit message shouldn't make
> this sound so much like a compiler bug. I think the cast to "unsigned
> long *" is simply wrong, exactly because it makes it valid for the
> compiler to merge multiple bit tests. And like it or not, our historic
> semantics for our bitops are that they are valid on volatile data.
> 
> That said, it's really sad how this will make 'test_bit()' potentially
> suck horribly and cause reloads when not necessary. We should probably
> (re-)introduce a __test_bit() operation that - like __set_bit and
> __clear_bit() works on things that are otherwise locked and can avoid
> reloading the value.
> 
> I dunno. Maybe we don't have a lot of users of 'test_bit()' that would
> actually care. How much does it cost us to have that volatile access?
> 

Somewhat offtopic...

On the general subject of bit operators, I'm wondering if we should
change the bit index to "unsigned long" like it already is on sparc64;
most other architectures have it as "int".  This already causes failures
if we have more than 16 TiB bytes of RAM in a single node -- not exactly
urgent stuff but something that might be an issue long term, especially
for a gigantic all-interleaved-memory machine.  I did try this on x86 a
while ago and found that it did added less than a kilobyte to the size
of the allyesconfig x86-64 kernel (unless my memory fails me.)

	-hpa