Re: page_waitqueue() considered harmful

On Wed, Sep 28, 2016 at 4:11 AM, Peter Zijlstra [off-list ref] wrote:

-void unlock_page(struct page *page)
+void __unlock_page(struct page *page)
 {
+       struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(&page->flags, PG_locked);
+       wait_queue_head_t *wq = page_waitqueue(page);
+
+       if (waitqueue_active(wq))
+               __wake_up(wq, TASK_NORMAL, 1, &key);
+       else
+               ClearPageContended(page);
 }
+EXPORT_SYMBOL(__unlock_page);

I think the above needs to be protected. Something like

    spin_lock_irqsave(&q->lock, flags);
    if (waitqueue_active(wq))
          __wake_up_locked(wq, TASK_NORMAL, 1, &key);
    else
          ClearPageContended(page);
    spin_unlock_irqrestore(&q->lock, flags);

because otherwise a new waiter could come in and add itself to the
wait-queue, and then set the bit, and now we clear it (because we
didn't see the new waiter).

The *waiter* doesn't need any extra locking, because doing

    add_wait_queue(..);
    SetPageContended(page);

is not racy (the add_wait_queue() will now already guarantee that
nobody else clears the bit).

Hmm?

                  Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>