Re: page_waitqueue() considered harmful

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, Sep 28, 2016 at 4:11 AM, Peter Zijlstra <peterz@infradead.org> 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>