waitqueue lockdep annotation

waitqueue lockdep annotation

[Christoph Hellwig]

Hi all,

this series adds a strategic lockdep_assert_held to __wake_up_common
to ensure callers really do hold the wait_queue_head lock when calling
the unlocked wake_up variants.  It turns out epoll did not do this
for a fairly common path (hit all the time by systemd during bootup),
so the second patch fixed this instance as well.

[PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Christoph Hellwig]

The eoll code currently always uses the unlocked waitqueue helpers for
ep->wq, but instead of holding the lock inside the waitqueue around these
calls, as expected by the epoll code uses its own lock.  Given that the
waitqueue is not exposed to the rest of the kernel this actually works
ok at the moment, but prevents the epoll locking rules from being
enforced using lockdep.  Remove ep->lock and use the waitqueue lock
to not only reduce the size of struct eventpoll but also make sure we
can assert locking invariations in the waitqueue code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/eventpoll.c | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index afd548ebc328..2b2c5ac80e26 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -182,11 +182,10 @@ struct epitem {
  * This structure is stored inside the "private_data" member of the file
  * structure and represents the main data structure for the eventpoll
  * interface.
+ *
+ * Access to it is protected by the lock inside wq.
  */
 struct eventpoll {
-	/* Protect the access to this structure */
-	spinlock_t lock;
-
 	/*
 	 * This mutex is used to ensure that files are not removed
 	 * while epoll is using them. This is held during the event
@@ -210,7 +209,7 @@ struct eventpoll {
 	/*
 	 * This is a single linked list that chains all the "struct epitem" that
 	 * happened while transferring ready events to userspace w/out
-	 * holding ->lock.
+	 * holding ->wq.lock.
 	 */
 	struct epitem *ovflist;
 
@@ -686,17 +685,17 @@ static int ep_scan_ready_list(struct eventpoll *ep,
 	 * because we want the "sproc" callback to be able to do it
 	 * in a lockless way.
 	 */
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 	list_splice_init(&ep->rdllist, &txlist);
 	ep->ovflist = NULL;
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	/*
 	 * Now call the callback function.
 	 */
 	error = (*sproc)(ep, &txlist, priv);
 
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 	/*
 	 * During the time we spent inside the "sproc" callback, some
 	 * other events might have been queued by the poll callback.
@@ -738,7 +737,7 @@ static int ep_scan_ready_list(struct eventpoll *ep,
 		if (waitqueue_active(&ep->poll_wait))
 			pwake++;
 	}
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	if (!ep_locked)
 		mutex_unlock(&ep->mtx);
@@ -782,10 +781,10 @@ static int ep_remove(struct eventpoll *ep, struct epitem *epi)
 
 	rb_erase_cached(&epi->rbn, &ep->rbr);
 
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 	if (ep_is_linked(&epi->rdllink))
 		list_del_init(&epi->rdllink);
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	wakeup_source_unregister(ep_wakeup_source(epi));
 	/*
@@ -1015,7 +1014,6 @@ static int ep_alloc(struct eventpoll **pep)
 	if (unlikely(!ep))
 		goto free_uid;
 
-	spin_lock_init(&ep->lock);
 	mutex_init(&ep->mtx);
 	init_waitqueue_head(&ep->wq);
 	init_waitqueue_head(&ep->poll_wait);
@@ -1119,7 +1117,7 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
 	struct eventpoll *ep = epi->ep;
 	int ewake = 0;
 
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 
 	ep_set_busy_poll_napi_id(epi);
 
@@ -1196,7 +1194,7 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
 		pwake++;
 
 out_unlock:
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	/* We have to call this outside the lock */
 	if (pwake)
@@ -1480,7 +1478,7 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
 		goto error_remove_epi;
 
 	/* We have to drop the new item inside our item list to keep track of it */
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 
 	/* record NAPI ID of new item if present */
 	ep_set_busy_poll_napi_id(epi);
@@ -1497,7 +1495,7 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
 			pwake++;
 	}
 
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	atomic_long_inc(&ep->user->epoll_watches);
 
@@ -1523,10 +1521,10 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
 	 * list, since that is used/cleaned only inside a section bound by "mtx".
 	 * And ep_insert() is called with "mtx" held.
 	 */
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 	if (ep_is_linked(&epi->rdllink))
 		list_del_init(&epi->rdllink);
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	wakeup_source_unregister(ep_wakeup_source(epi));
 
@@ -1593,7 +1591,7 @@ static int ep_modify(struct eventpoll *ep, struct epitem *epi, struct epoll_even
 	 * list, push it inside.
 	 */
 	if (revents & event->events) {
-		spin_lock_irq(&ep->lock);
+		spin_lock_irq(&ep->wq.lock);
 		if (!ep_is_linked(&epi->rdllink)) {
 			list_add_tail(&epi->rdllink, &ep->rdllist);
 			ep_pm_stay_awake(epi);
@@ -1604,7 +1602,7 @@ static int ep_modify(struct eventpoll *ep, struct epitem *epi, struct epoll_even
 			if (waitqueue_active(&ep->poll_wait))
 				pwake++;
 		}
-		spin_unlock_irq(&ep->lock);
+		spin_unlock_irq(&ep->wq.lock);
 	}
 
 	/* We have to call this outside the lock */
@@ -1754,7 +1752,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 		 * caller specified a non blocking operation.
 		 */
 		timed_out = 1;
-		spin_lock_irqsave(&ep->lock, flags);
+		spin_lock_irqsave(&ep->wq.lock, flags);
 		goto check_events;
 	}
 
@@ -1763,7 +1761,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	if (!ep_events_available(ep))
 		ep_busy_loop(ep, timed_out);
 
-	spin_lock_irqsave(&ep->lock, flags);
+	spin_lock_irqsave(&ep->wq.lock, flags);
 
 	if (!ep_events_available(ep)) {
 		/*
@@ -1805,11 +1803,11 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 				break;
 			}
 
-			spin_unlock_irqrestore(&ep->lock, flags);
+			spin_unlock_irqrestore(&ep->wq.lock, flags);
 			if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS))
 				timed_out = 1;
 
-			spin_lock_irqsave(&ep->lock, flags);
+			spin_lock_irqsave(&ep->wq.lock, flags);
 		}
 
 		__remove_wait_queue(&ep->wq, &wait);
@@ -1819,7 +1817,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	/* Is it worth to try to dig for events ? */
 	eavail = ep_events_available(ep);
 
-	spin_unlock_irqrestore(&ep->lock, flags);
+	spin_unlock_irqrestore(&ep->wq.lock, flags);
 
 	/*
 	 * Try to transfer events to user space. In case we get 0 events and
-- 
2.14.2

[PATCH 2/2] sched/wait: assert the wait_queue_head lock is held in __wake_up_common

[Christoph Hellwig]

Better ensure we actually hold the lock using lockdep than just commenting
on it.  Due to the various exported _locked interfaces it is far too easy
to get the locking wrong.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/sched/wait.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 98feab7933c7..347c06c8222e 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -76,6 +76,8 @@ static int __wake_up_common(struct wait_queue_head *wq_head, unsigned int mode,
 	wait_queue_entry_t *curr, *next;
 	int cnt = 0;
 
+	lockdep_assert_held(&wq_head->lock);
+
 	if (bookmark && (bookmark->flags & WQ_FLAG_BOOKMARK)) {
 		curr = list_next_entry(bookmark, entry);
 
-- 
2.14.2

Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Ingo Molnar]

* Christoph Hellwig <hch@lst.de> wrote:

> The eoll code currently always uses the unlocked waitqueue helpers for

s/eoll
 /epoll

> ep->wq, but instead of holding the lock inside the waitqueue around these
> calls, as expected by the epoll code uses its own lock.

Hm, that reads a bit weirdly. How about:

 The epoll code currently uses the unlocked waitqueue helpers for managing
 ep->wq, but instead of holding the waitqueue lock around these calls, it
 uses its own ep->lock spinlock.


> Given that the
> waitqueue is not exposed to the rest of the kernel this actually works
> ok at the moment, but prevents the epoll locking rules from being
> enforced using lockdep.  Remove ep->lock and use the waitqueue lock
> to not only reduce the size of struct eventpoll but also make sure we
> can assert locking invariations in the waitqueue code.

s/but also make sure
  but also to make sure

s/invariations
 /invariants

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/eventpoll.c | 46 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index afd548ebc328..2b2c5ac80e26 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -182,11 +182,10 @@ struct epitem {
>   * This structure is stored inside the "private_data" member of the file
>   * structure and represents the main data structure for the eventpoll
>   * interface.
> + *
> + * Access to it is protected by the lock inside wq.
>   */
>  struct eventpoll {
> -	/* Protect the access to this structure */
> -	spinlock_t lock;
> -
>  	/*
>  	 * This mutex is used to ensure that files are not removed
>  	 * while epoll is using them. This is held during the event
> @@ -210,7 +209,7 @@ struct eventpoll {
>  	/*
>  	 * This is a single linked list that chains all the "struct epitem" that
>  	 * happened while transferring ready events to userspace w/out
> -	 * holding ->lock.
> +	 * holding ->wq.lock.
>  	 */

Neat trick!

This exposes some waitqueue internals, but AFAICS the FUSE code already does a 
similar trick with fiq->waitq.lock so there's precedent.

Peter, what do you think?

Thanks,

	Ingo

Re: [PATCH 2/2] sched/wait: assert the wait_queue_head lock is held in __wake_up_common

[Ingo Molnar]

* Christoph Hellwig <hch@lst.de> wrote:

> Better ensure we actually hold the lock using lockdep than just commenting
> on it.  Due to the various exported _locked interfaces it is far too easy
> to get the locking wrong.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  kernel/sched/wait.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index 98feab7933c7..347c06c8222e 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -76,6 +76,8 @@ static int __wake_up_common(struct wait_queue_head *wq_head, unsigned int mode,
>  	wait_queue_entry_t *curr, *next;
>  	int cnt = 0;
>  
> +	lockdep_assert_held(&wq_head->lock);
> +
>  	if (bookmark && (bookmark->flags & WQ_FLAG_BOOKMARK)) {
>  		curr = list_next_entry(bookmark, entry);

Makes sense. Would you like to carry this patch together with the epoll patch, to 
be able to test them both? If yes then:

  Acked-by: Ingo Molnar <mingo@kernel.org>

... otherwise I can pick this up into the scheduler tree as well.

Thanks,

	Ingo

Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Andreas Dilger]

> On Dec 6, 2017, at 17:49, Ingo Molnar <mingo@kernel.org> wrote:
> 
> This exposes some waitqueue internals, but AFAICS the FUSE code already does a 
> similar trick with fiq->waitq.lock so there's precedent.

What about waitqueue_lock() and waitqueue_unlock() helpers that
lock and unlock, to avoid exposing the internals?  Or would that add
confusion by making users think they need their own waitqueue locking?

Alternately, a helper that returns the pointer to the lock:

#define waitqueue_lockp(wq) &((wq)->lock)

Used like the following:
        spin_lock_irqsave(waitqueue_lockp(&ep->wq), flags);

Cheers, Andreas

Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Ingo Molnar]

* Andreas Dilger <adilger@dilger.ca> wrote:

> > On Dec 6, 2017, at 17:49, Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > This exposes some waitqueue internals, but AFAICS the FUSE code already does a 
> > similar trick with fiq->waitq.lock so there's precedent.
> 
> What about waitqueue_lock() and waitqueue_unlock() helpers that
> lock and unlock, to avoid exposing the internals?  Or would that add
> confusion by making users think they need their own waitqueue locking?

Right now there are just two users (FUSE and epoll), and both are well-maintained, 
essentially core kernel code - I'd rather prefer the readability of explicitly 
writing out the locking/unlocking pattern.

So while it's a mild layering violation, it's also a valid looking optimization.

Thanks,

	Ingo

Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Jason Baron]

On 12/06/2017 06:52 PM, Christoph Hellwig wrote:
> The eoll code currently always uses the unlocked waitqueue helpers for
> ep->wq, but instead of holding the lock inside the waitqueue around these
> calls, as expected by the epoll code uses its own lock.  Given that the
> waitqueue is not exposed to the rest of the kernel this actually works
> ok at the moment, but prevents the epoll locking rules from being
> enforced using lockdep.  Remove ep->lock and use the waitqueue lock
> to not only reduce the size of struct eventpoll but also make sure we
> can assert locking invariations in the waitqueue code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Probably should also fix the locking comments at the top of
fs/eventpoll.c that refer to ep->lock...

The rest looks good.

Reviewed-by: Jason Baron <jbaron@akamai.com>

Thanks,

-Jason


> ---
>  fs/eventpoll.c | 46 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index afd548ebc328..2b2c5ac80e26 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -182,11 +182,10 @@ struct epitem {
>   * This structure is stored inside the "private_data" member of the file
>   * structure and represents the main data structure for the eventpoll
>   * interface.
> + *
> + * Access to it is protected by the lock inside wq.
>   */
>  struct eventpoll {
> -	/* Protect the access to this structure */
> -	spinlock_t lock;
> -
>  	/*
>  	 * This mutex is used to ensure that files are not removed
>  	 * while epoll is using them. This is held during the event
> @@ -210,7 +209,7 @@ struct eventpoll {
>  	/*
>  	 * This is a single linked list that chains all the "struct epitem" that
>  	 * happened while transferring ready events to userspace w/out
> -	 * holding ->lock.
> +	 * holding ->wq.lock.
>  	 */
>  	struct epitem *ovflist;
>  
> @@ -686,17 +685,17 @@ static int ep_scan_ready_list(struct eventpoll *ep,
>  	 * because we want the "sproc" callback to be able to do it
>  	 * in a lockless way.
>  	 */
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  	list_splice_init(&ep->rdllist, &txlist);
>  	ep->ovflist = NULL;
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	/*
>  	 * Now call the callback function.
>  	 */
>  	error = (*sproc)(ep, &txlist, priv);
>  
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  	/*
>  	 * During the time we spent inside the "sproc" callback, some
>  	 * other events might have been queued by the poll callback.
> @@ -738,7 +737,7 @@ static int ep_scan_ready_list(struct eventpoll *ep,
>  		if (waitqueue_active(&ep->poll_wait))
>  			pwake++;
>  	}
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	if (!ep_locked)
>  		mutex_unlock(&ep->mtx);
> @@ -782,10 +781,10 @@ static int ep_remove(struct eventpoll *ep, struct epitem *epi)
>  
>  	rb_erase_cached(&epi->rbn, &ep->rbr);
>  
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  	if (ep_is_linked(&epi->rdllink))
>  		list_del_init(&epi->rdllink);
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	wakeup_source_unregister(ep_wakeup_source(epi));
>  	/*
> @@ -1015,7 +1014,6 @@ static int ep_alloc(struct eventpoll **pep)
>  	if (unlikely(!ep))
>  		goto free_uid;
>  
> -	spin_lock_init(&ep->lock);
>  	mutex_init(&ep->mtx);
>  	init_waitqueue_head(&ep->wq);
>  	init_waitqueue_head(&ep->poll_wait);
> @@ -1119,7 +1117,7 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
>  	struct eventpoll *ep = epi->ep;
>  	int ewake = 0;
>  
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  
>  	ep_set_busy_poll_napi_id(epi);
>  
> @@ -1196,7 +1194,7 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
>  		pwake++;
>  
>  out_unlock:
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	/* We have to call this outside the lock */
>  	if (pwake)
> @@ -1480,7 +1478,7 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
>  		goto error_remove_epi;
>  
>  	/* We have to drop the new item inside our item list to keep track of it */
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  
>  	/* record NAPI ID of new item if present */
>  	ep_set_busy_poll_napi_id(epi);
> @@ -1497,7 +1495,7 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
>  			pwake++;
>  	}
>  
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	atomic_long_inc(&ep->user->epoll_watches);
>  
> @@ -1523,10 +1521,10 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
>  	 * list, since that is used/cleaned only inside a section bound by "mtx".
>  	 * And ep_insert() is called with "mtx" held.
>  	 */
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  	if (ep_is_linked(&epi->rdllink))
>  		list_del_init(&epi->rdllink);
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	wakeup_source_unregister(ep_wakeup_source(epi));
>  
> @@ -1593,7 +1591,7 @@ static int ep_modify(struct eventpoll *ep, struct epitem *epi, struct epoll_even
>  	 * list, push it inside.
>  	 */
>  	if (revents & event->events) {
> -		spin_lock_irq(&ep->lock);
> +		spin_lock_irq(&ep->wq.lock);
>  		if (!ep_is_linked(&epi->rdllink)) {
>  			list_add_tail(&epi->rdllink, &ep->rdllist);
>  			ep_pm_stay_awake(epi);
> @@ -1604,7 +1602,7 @@ static int ep_modify(struct eventpoll *ep, struct epitem *epi, struct epoll_even
>  			if (waitqueue_active(&ep->poll_wait))
>  				pwake++;
>  		}
> -		spin_unlock_irq(&ep->lock);
> +		spin_unlock_irq(&ep->wq.lock);
>  	}
>  
>  	/* We have to call this outside the lock */
> @@ -1754,7 +1752,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  		 * caller specified a non blocking operation.
>  		 */
>  		timed_out = 1;
> -		spin_lock_irqsave(&ep->lock, flags);
> +		spin_lock_irqsave(&ep->wq.lock, flags);
>  		goto check_events;
>  	}
>  
> @@ -1763,7 +1761,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  	if (!ep_events_available(ep))
>  		ep_busy_loop(ep, timed_out);
>  
> -	spin_lock_irqsave(&ep->lock, flags);
> +	spin_lock_irqsave(&ep->wq.lock, flags);
>  
>  	if (!ep_events_available(ep)) {
>  		/*
> @@ -1805,11 +1803,11 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  				break;
>  			}
>  
> -			spin_unlock_irqrestore(&ep->lock, flags);
> +			spin_unlock_irqrestore(&ep->wq.lock, flags);
>  			if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS))
>  				timed_out = 1;
>  
> -			spin_lock_irqsave(&ep->lock, flags);
> +			spin_lock_irqsave(&ep->wq.lock, flags);
>  		}
>  
>  		__remove_wait_queue(&ep->wq, &wait);
> @@ -1819,7 +1817,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  	/* Is it worth to try to dig for events ? */
>  	eavail = ep_events_available(ep);
>  
> -	spin_unlock_irqrestore(&ep->lock, flags);
> +	spin_unlock_irqrestore(&ep->wq.lock, flags);
>  
>  	/*
>  	 * Try to transfer events to user space. In case we get 0 events and
> 

Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Christoph Hellwig]

On Thu, Dec 07, 2017 at 11:09:11AM -0500, Jason Baron wrote:
> On 12/06/2017 06:52 PM, Christoph Hellwig wrote:
> > The eoll code currently always uses the unlocked waitqueue helpers for
> > ep->wq, but instead of holding the lock inside the waitqueue around these
> > calls, as expected by the epoll code uses its own lock.  Given that the
> > waitqueue is not exposed to the rest of the kernel this actually works
> > ok at the moment, but prevents the epoll locking rules from being
> > enforced using lockdep.  Remove ep->lock and use the waitqueue lock
> > to not only reduce the size of struct eventpoll but also make sure we
> > can assert locking invariations in the waitqueue code.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Probably should also fix the locking comments at the top of
> fs/eventpoll.c that refer to ep->lock...

Done.  Note that while doing this I noticed that the epoll code
seems to have sketchy workarounds for the fact that it abused ep->poll
as the waitqueue lock that might be able to be removed now.
But I don't really dare to touch the guts of this code.

Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq

[Christoph Hellwig]

> Hm, that reads a bit weirdly. How about:
> 
>  The epoll code currently uses the unlocked waitqueue helpers for managing
>  ep->wq, but instead of holding the waitqueue lock around these calls, it
>  uses its own ep->lock spinlock.

Thanks, fixed.

Re: [PATCH 2/2] sched/wait: assert the wait_queue_head lock is held in __wake_up_common

[Christoph Hellwig]

On Thu, Dec 07, 2017 at 01:50:33AM +0100, Ingo Molnar wrote:
> Makes sense. Would you like to carry this patch together with the epoll patch, to 
> be able to test them both? If yes then:

It would be good to merge them all together.  I still thing the sched
tree might be the best place for all of them, but I'll let you fight that
out with Andrew once the next iteration is posted :)