Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli <andrea@suse.de>]

Hi Marcelo,

On Wed, Jul 07, 2004 at 03:58:14PM -0300, Marcelo Tosatti wrote:
> I think this comment on i386 bitops.h can lead to 
> misunderstanding and should be changed:
> 
> /**
>  * set_bit - Atomically set a bit in memory
>  * @nr: the bit to set
>  * @addr: the address to start counting from
>  *
>  * This function is atomic and may not be reordered. 
> 
> "This function is atomic and may not be reordered (other architectures MAY reorder it)"
> 
> Or something like this. The comment as it is leads people to
> write nonportable code which assumes set_bit() cant be reordered. Like naive me did.

agreed. (as usual trusting comments more than code carries some
risk, last time I was fooled by a comment was only a few months ago too
in some device driver ;)

btw, for completeness, test_bit (the one running before sync_page) can
be reordered even in x86.

The only one that enforces ordering everywhere is test_and_set_bit (as
Linus recently reminded on the list too). And it only enforces ordering
if it returns 0. If it returns 1 no ordering is enforced (for example on
alpha) because no change was made to the memory and supposedly the code
will not be allowed to access any critical section (and in turn no need
of any barrier).

> Andrew, what you think about removing that barrier from sync_page() 
> on -mm? 
> 
> And what about changing the "may not reordered" comments on i386 bitops.h
> to "may not be reordered on i386 only, other arches do reorder it." ?

both looks correct to me, thanks.