Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli <andrea@suse.de>]

On Wed, Jul 07, 2004 at 02:57:24PM -0300, Marcelo Tosatti wrote:
> 
> Hi Chris,
> 
> I was talking to Andrew about this memory barrier 
> 
> static inline int sync_page(struct page *page)
> {
>         struct address_space *mapping;
>                                                                                         
>         /*
>          * FIXME, fercrissake.  What is this barrier here for?
>          */
>         smp_mb();
>         mapping = page_mapping(page);
>         if (mapping && mapping->a_ops && mapping->a_ops->sync_page)
>                 return mapping->a_ops->sync_page(page);
>         return 0;
> }
> 
> And does not seem to be a reason for it. The callers are:
> 
> void fastcall wait_on_page_bit(struct page *page, int bit_nr)
> {
>         wait_queue_head_t *waitqueue = page_waitqueue(page);
>         DEFINE_PAGE_WAIT(wait, page, bit_nr);
>                                                                                         
>         do {
>                 prepare_to_wait(waitqueue, &wait.wait, TASK_UNINTERRUPTIBLE);
>                 if (test_bit(bit_nr, &page->flags)) {
>                         sync_page(page);
>                         io_schedule();
>                 }
>         } while (test_bit(bit_nr, &page->flags));
>         finish_wait(waitqueue, &wait.wait);
> } 
> 
> void fastcall __lock_page(struct page *page)
> {
>         wait_queue_head_t *wqh = page_waitqueue(page);
>         DEFINE_PAGE_WAIT_EXCLUSIVE(wait, page, PG_locked);
>                                                                                         
>         while (TestSetPageLocked(page)) {
>                 prepare_to_wait_exclusive(wqh, &wait.wait, TASK_UNINTERRUPTIBLE);
>                 if (PageLocked(page)) {
>                         sync_page(page);
>                         io_schedule();
>                 }
>         }
>         finish_wait(wqh, &wait.wait);
> }
> 
> Both callers call set_bit (atomic operation which cannot be reordered) before 

set_bit is atomic but it _can_ be reordered just fine. atomic !=
barrier (they're the same only in x86 due the lack of specific smp-aware
opcodes).

however the smp_mb() isn't needed in sync_page, simply because it's
perfectly ok if we start running sync_page before reading pagelocked.
All we care about is to run sync_page _before_ io_schedule() and that we
read PageLocked _after_ prepare_to_wait_exclusive.

So the locking in between PageLocked and sync_page is _absolutely_
worthless and the smp_mb() can go away.