[PATCH 1/2] fs/epoll: rename check_events label to send_events

[PATCH 1/2] fs/epoll: rename check_events label to send_events

[Davidlohr Bueso]

It is currently called check_events because it, well, did
exactly that. However, since the lockless ep_events_available()
call, the label no longer checks, but just sends the events.
Rename as such.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---

Both patch 1 and 2 apply on top of the mmots.

 fs/eventpoll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index ec14e5bcdaa9..7e2b5f3d6b3e 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1774,7 +1774,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 		eavail = ep_events_available(ep);
 		spin_unlock_irq(&ep->wq.lock);
 
-		goto check_events;
+		goto send_events;
 	}
 
 fetch_events:
@@ -1784,7 +1784,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 
 	eavail = ep_events_available(ep);
 	if (eavail)
-		goto check_events;
+		goto send_events;
 
 	/*
 	 * Busy poll timed out.  Drop NAPI ID for now, we can add
@@ -1840,7 +1840,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	__remove_wait_queue(&ep->wq, &wait);
 	spin_unlock_irq(&ep->wq.lock);
 
-check_events:
+send_events:
 	/*
 	 * Try to transfer events to user space. In case we get 0 events and
 	 * there's still timeout left over, we go trying again in search of
-- 
2.16.4

[PATCH 2/2] fs/epoll: deal with wait_queue only once

[Davidlohr Bueso]

There is no reason why we rearm the waitiqueue upon every
fetch_events retry (for when events are found yet send_events()
fails). If nothing else, this saves four lock operations per
retry, and furthermore reduces the scope of the lock even
further.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 fs/eventpoll.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 7e2b5f3d6b3e..25b0c94cc091 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1749,6 +1749,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 {
 	int res = 0, eavail, timed_out = 0;
 	u64 slack = 0;
+	bool waiter = false;
 	wait_queue_entry_t wait;
 	ktime_t expires, *to = NULL;
 
@@ -1786,6 +1787,15 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	if (eavail)
 		goto send_events;
 
+	if (!waiter) {
+		waiter = true;
+		init_waitqueue_entry(&wait, current);
+
+		spin_lock_irq(&ep->wq.lock);
+		__add_wait_queue_exclusive(&ep->wq, &wait);
+		spin_unlock_irq(&ep->wq.lock);
+	}
+
 	/*
 	 * Busy poll timed out.  Drop NAPI ID for now, we can add
 	 * it back in when we have moved a socket with a valid NAPI
@@ -1798,10 +1808,6 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	 * We need to sleep here, and we will be wake up by
 	 * ep_poll_callback() when events will become available.
 	 */
-	init_waitqueue_entry(&wait, current);
-	spin_lock_irq(&ep->wq.lock);
-	__add_wait_queue_exclusive(&ep->wq, &wait);
-	spin_unlock_irq(&ep->wq.lock);
 
 	for (;;) {
 		/*
@@ -1836,10 +1842,6 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 
 	__set_current_state(TASK_RUNNING);
 
-	spin_lock_irq(&ep->wq.lock);
-	__remove_wait_queue(&ep->wq, &wait);
-	spin_unlock_irq(&ep->wq.lock);
-
 send_events:
 	/*
 	 * Try to transfer events to user space. In case we get 0 events and
@@ -1850,6 +1852,12 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	    !(res = ep_send_events(ep, events, maxevents)) && !timed_out)
 		goto fetch_events;
 
+	if (waiter) {
+		spin_lock_irq(&ep->wq.lock);
+		__remove_wait_queue(&ep->wq, &wait);
+		spin_unlock_irq(&ep->wq.lock);
+	}
+
 	return res;
 }
 
-- 
2.16.4

Re: [PATCH 2/2] fs/epoll: deal with wait_queue only once

[Andrew Morton]

On Wed, 14 Nov 2018 10:25:32 -0800 Davidlohr Bueso <dave@stgolabs.net> wrote:

> There is no reason why we rearm the waitiqueue upon every
> fetch_events retry (for when events are found yet send_events()
> fails). If nothing else, this saves four lock operations per
> retry, and furthermore reduces the scope of the lock even
> further.
> 
> ..
>
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1749,6 +1749,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  {
>  	int res = 0, eavail, timed_out = 0;
>  	u64 slack = 0;
> +	bool waiter = false;
>  	wait_queue_entry_t wait;
>  	ktime_t expires, *to = NULL;
>  
> @@ -1786,6 +1787,15 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  	if (eavail)
>  		goto send_events;
>  
> +	if (!waiter) {
> +		waiter = true;
> +		init_waitqueue_entry(&wait, current);
> +
> +		spin_lock_irq(&ep->wq.lock);
> +		__add_wait_queue_exclusive(&ep->wq, &wait);
> +		spin_unlock_irq(&ep->wq.lock);
> +	}
> +
>  	/*
>  	 * Busy poll timed out.  Drop NAPI ID for now, we can add
>  	 * it back in when we have moved a socket with a valid NAPI
> @@ -1798,10 +1808,6 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  	 * We need to sleep here, and we will be wake up by
>  	 * ep_poll_callback() when events will become available.
>  	 */
> -	init_waitqueue_entry(&wait, current);
> -	spin_lock_irq(&ep->wq.lock);
> -	__add_wait_queue_exclusive(&ep->wq, &wait);
> -	spin_unlock_irq(&ep->wq.lock);

Why was this moved to before the ep_reset_busy_poll_napi_id() call? 
That movement placed the code ahead of the block comment which serves
to explain its function.


This?  Which also fixes that comment and reflows it to use 80 cols.

--- a/fs/eventpoll.c~fs-epoll-deal-with-wait_queue-only-once-fix
+++ a/fs/eventpoll.c
@@ -1787,15 +1787,6 @@ fetch_events:
 	if (eavail)
 		goto send_events;
 
-	if (!waiter) {
-		waiter = true;
-		init_waitqueue_entry(&wait, current);
-
-		spin_lock_irq(&ep->wq.lock);
-		__add_wait_queue_exclusive(&ep->wq, &wait);
-		spin_unlock_irq(&ep->wq.lock);
-	}
-
 	/*
 	 * Busy poll timed out.  Drop NAPI ID for now, we can add
 	 * it back in when we have moved a socket with a valid NAPI
@@ -1804,10 +1795,18 @@ fetch_events:
 	ep_reset_busy_poll_napi_id(ep);
 
 	/*
-	 * We don't have any available event to return to the caller.
-	 * We need to sleep here, and we will be wake up by
-	 * ep_poll_callback() when events will become available.
+	 * We don't have any available event to return to the caller.  We need
+	 * to sleep here, and we will be woken by ep_poll_callback() when events
+	 * become available.
 	 */
+	if (!waiter) {
+		waiter = true;
+		init_waitqueue_entry(&wait, current);
+
+		spin_lock_irq(&ep->wq.lock);
+		__add_wait_queue_exclusive(&ep->wq, &wait);
+		spin_unlock_irq(&ep->wq.lock);
+	}
 
 	for (;;) {
 		/*
_

Re: [PATCH 2/2] fs/epoll: deal with wait_queue only once

[Davidlohr Bueso]

On Wed, 14 Nov 2018, Andrew Morton wrote:

>Why was this moved to before the ep_reset_busy_poll_napi_id() call?
>That movement placed the code ahead of the block comment which serves
>to explain its function.

Yikes, that was a brain fart.

>
>
>This?  Which also fixes that comment and reflows it to use 80 cols.

Looks good, thanks.

>
>--- a/fs/eventpoll.c~fs-epoll-deal-with-wait_queue-only-once-fix
>+++ a/fs/eventpoll.c
>@@ -1787,15 +1787,6 @@ fetch_events:
> 	if (eavail)
> 		goto send_events;
>
>-	if (!waiter) {
>-		waiter = true;
>-		init_waitqueue_entry(&wait, current);
>-
>-		spin_lock_irq(&ep->wq.lock);
>-		__add_wait_queue_exclusive(&ep->wq, &wait);
>-		spin_unlock_irq(&ep->wq.lock);
>-	}
>-
> 	/*
> 	 * Busy poll timed out.  Drop NAPI ID for now, we can add
> 	 * it back in when we have moved a socket with a valid NAPI
>@@ -1804,10 +1795,18 @@ fetch_events:
> 	ep_reset_busy_poll_napi_id(ep);
>
> 	/*
>-	 * We don't have any available event to return to the caller.
>-	 * We need to sleep here, and we will be wake up by
>-	 * ep_poll_callback() when events will become available.
>+	 * We don't have any available event to return to the caller.  We need
>+	 * to sleep here, and we will be woken by ep_poll_callback() when events
>+	 * become available.
> 	 */
>+	if (!waiter) {
>+		waiter = true;
>+		init_waitqueue_entry(&wait, current);
>+
>+		spin_lock_irq(&ep->wq.lock);
>+		__add_wait_queue_exclusive(&ep->wq, &wait);
>+		spin_unlock_irq(&ep->wq.lock);
>+	}
>
> 	for (;;) {
> 		/*
>_
>