Re: [PATCH 6/8] i386: bitops: Don't mark memory as clobbered unnecessarily

[Nick Piggin <nickpiggin@yahoo.com.au>]

Satyam Sharma wrote:
> On Tue, 24 Jul 2007, Nick Piggin wrote:
> 
> 
>>Satyam Sharma wrote:
>>
>>>From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
>>>
>>>[6/8] i386: bitops: Don't mark memory as clobbered unnecessarily
>>>
>>>The goal is to let gcc generate good, beautiful, optimized code.
>>>
>>>But test_and_set_bit, test_and_clear_bit, __test_and_change_bit,
>>>and test_and_change_bit unnecessarily mark all of memory as clobbered,
>>>thereby preventing gcc from doing perfectly valid optimizations.
>>>
>>>The case of __test_and_change_bit() is particularly surprising, given
>>>that it's a variant where we don't make any guarantees at all.
>>
>>__test_and_change_bit is one that you could remove the memory clobber
>>from.
> 
> 
> Yes, for the atomic versions we don't care if we're asking gcc to
> generate trashy code (even though I'd have wanted to only disallow
> problematic optimizations -- ones involving the passed bit-string
> address -- there, and allow other memory references to be optimized
> as and how the compiler feels like it) because the atomic variants
> are slow anyway and we probably want to be extra-safe there.
> 
> But for the non-atomic variants, it does make sense to remove the
> memory clobber (and the unneeded __asm__ __volatile__ that another
> patch did -- for the non-atomic variants, again).

No. It has nothing to do with atomicity and all to do with ordering.
For example test_bit, clear_bit, set_bit, etc are all atomic but none
place any restrictions on ordering.

__test_and_change_bit has no restriction on ordering, so as long as
the correct operands are clobbered, a "memory" clobber to enforce a
compiler barrier is not needed.


> OTOH, as per Linus' review it seems we can drop the "memory" clobber
> and specify the output operand for the extended asm as "+m". But I
> must admit I didn't quite understand that at all.

Yes, but that is for cases where "memory" is being used to say that
some otherwise unspecified memory is actually being changed, rather
than to provide a compiler barrier as is the case for test_and_set_bit
and co.


> [ I should probably start reading gcc sources, the docs are said to
>   be insufficient/out-of-date, as per the reviews of the patches. ]

You should read Documentation/atomic_ops.txt and memory-barriers.txt,
which are quite useful and should be uptodate.

-- 
SUSE Labs, Novell Inc.