Messages in this thread | | | Date | Wed, 7 Jul 2004 20:20:25 +0200 | From | Andrea Arcangeli <> | Subject | Re: Unnecessary barrier in sync_page()? |
| |
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |