Re: d1fc031747 ("sched/wait: assert the wait_queue_head lock is .."): EIP: __wake_up_common

[Matthew Wilcox]

On Thu, Dec 14, 2017 at 02:10:37PM +0100, Christoph Hellwig wrote:
> On Thu, Dec 14, 2017 at 04:58:09AM -0800, Matthew Wilcox wrote:
> > Looks pretty clear to me that userfaultfd is also abusing the wake_up_locked
> > interfaces:
> > 
> >         spin_lock(&ctx->fault_pending_wqh.lock);
> >         __wake_up_locked_key(&ctx->fault_pending_wqh, TASK_NORMAL, &range);
> >         __wake_up_locked_key(&ctx->fault_wqh, TASK_NORMAL, &range);
> >         spin_unlock(&ctx->fault_pending_wqh.lock);
> > 
> > Sure, it's locked, but not by the lock you thought it was going to be.
> > 
> > There doesn't actually appear to be a bug here; fault_wqh is always serialised
> > by fault_pending_wqh.lock, but lockdep can't know that.  I think this patch
> > will solve the problem.
> 
> Or userfaultfd could just always use the waitqueue lock, similar to what
> we are doing in epoll.
> 
> But unless someone care about micro-optimizatations I'm tempted to
> add your patch to the next iteration of the series.

userfaultfd is using the waitqueue lock -- it just has two waitqueues
that it's protecting with the same lock.

If the patch goes through as-is, try this changelog:

[PATCH] userfaultfd: Use fault_wqh lock

userfaultfd was using the fault_pending_wq lock to protect both
fault_pending_wq and fault_wqh.  With Christoph's addition of a lockdep
assert to the wait queue code, that will trigger warnings (although there
is no bug).  Remove the warning by using __wake_up which will take the
fault_wqh lock.  This lock now nests inside the fault_pending_wqh lock,
but that's not a problem since it was entireyl unused before.

Signed-off-by: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>