On 09/23/2010 05:08 PM, Linus Torvalds wrote: > On Thu, Sep 23, 2010 at 4:51 PM, <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: >> >> Subject: x86: avoid 'constant_test_bit()' misoptimization due to cast to non-volatile >> From: Led <led@xxxxxxxxxxx> >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
