[patch] fs, epoll: short circuit fetching events if thread has been killed

[patch] fs, epoll: short circuit fetching events if thread has been killed

[David Rientjes]

We've encountered zombies that are waiting for a thread to exit that are
looping in ep_poll() almost endlessly although there is a pending SIGKILL
as a result of a group exit.

This happens because we always find ep_events_available() and fetch more
events and never are able to check for signal_pending() that would break
from the loop and return -EINTR.

Special case fatal signals and break immediately to guarantee that we
loop to fetch more events and delay making a timely exit.

It would also be possible to simply move the check for signal_pending()
higher than checking for ep_events_available(), but there have been no
reports of delayed signal handling other than SIGKILL preventing zombies
from exiting that would be fixed by this.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 fs/eventpoll.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1748,6 +1748,16 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 			 * to TASK_INTERRUPTIBLE before doing the checks.
 			 */
 			set_current_state(TASK_INTERRUPTIBLE);
+			/*
+			 * Always short-circuit for fatal signals to allow
+			 * threads to make a timely exit without the chance of
+			 * finding more events available and fetching
+			 * repeatedly.
+			 */
+			if (fatal_signal_pending(current)) {
+				res = -EINTR;
+				break;
+			}
 			if (ep_events_available(ep) || timed_out)
 				break;
 			if (signal_pending(current)) {

Re: [patch] fs, epoll: short circuit fetching events if thread has been killed

[Andrew Morton]

On Wed, 3 May 2017 17:22:53 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:

> We've encountered zombies that are waiting for a thread to exit that are
> looping in ep_poll() almost endlessly although there is a pending SIGKILL
> as a result of a group exit.
> 
> This happens because we always find ep_events_available() and fetch more
> events and never are able to check for signal_pending() that would break
> from the loop and return -EINTR.
> 
> Special case fatal signals and break immediately to guarantee that we
> loop to fetch more events and delay making a timely exit.
> 
> It would also be possible to simply move the check for signal_pending()
> higher than checking for ep_events_available(), but there have been no
> reports of delayed signal handling other than SIGKILL preventing zombies
> from exiting that would be fixed by this.

Any thoughts on the priority of this?  -stable?  If so, why?

Re: [patch] fs, epoll: short circuit fetching events if thread has been killed

[David Rientjes]

On Tue, 9 May 2017, Andrew Morton wrote:

> > We've encountered zombies that are waiting for a thread to exit that are
> > looping in ep_poll() almost endlessly although there is a pending SIGKILL
> > as a result of a group exit.
> > 
> > This happens because we always find ep_events_available() and fetch more
> > events and never are able to check for signal_pending() that would break
> > from the loop and return -EINTR.
> > 
> > Special case fatal signals and break immediately to guarantee that we
> > loop to fetch more events and delay making a timely exit.
> > 
> > It would also be possible to simply move the check for signal_pending()
> > higher than checking for ep_events_available(), but there have been no
> > reports of delayed signal handling other than SIGKILL preventing zombies
> > from exiting that would be fixed by this.
> 
> Any thoughts on the priority of this?  -stable?  If so, why?
> 

It fixes an issue for us where we have witnessed zombies sticking around 
for at least O(minutes), but considering the code has been like this 
forever and nobody else has complained that I have found, I would simply 
queue it up for 4.12.

Re: [patch] fs, epoll: short circuit fetching events if thread has been killed

[Michal Hocko]

On Wed 03-05-17 17:22:53, David Rientjes wrote:
[...]
> @@ -1748,6 +1748,16 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  			 * to TASK_INTERRUPTIBLE before doing the checks.
>  			 */
>  			set_current_state(TASK_INTERRUPTIBLE);
> +			/*
> +			 * Always short-circuit for fatal signals to allow
> +			 * threads to make a timely exit without the chance of
> +			 * finding more events available and fetching
> +			 * repeatedly.
> +			 */
> +			if (fatal_signal_pending(current)) {
> +				res = -EINTR;
> +				break;
> +			}
>  			if (ep_events_available(ep) || timed_out)
>  				break;
>  			if (signal_pending(current)) {

I am wondering. Is there any specific reason why we do not break out of
the loop before checking ep_events_available on any pending signal? Is
there any advantage to preempt signal handling by too many events?

I've tried to dig it out from the full history git tree but it goes all
the way down to "[PATCH] epoll update r3".

-- 
Michal Hocko
SUSE Labs