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

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