TCP window full with EPOLLWAKEUP

-next regression In review Jiri Slaby 3 participants 9 messages May 19, 2012 – May 22, 2012 VFS

lore

Share & tools

lore

Jiri Slaby

author

2012-05-19 22:07 UTC #0 lore raw

Hi,

a bisection shows that with the following commit from -next:
commit 4d7e30d98939a0340022ccd49325a3d70f7e0238

Author: Arve Hjønnevåg <[email protected]>
Date:   Tue May 1 21:33:34 2012 +0200

epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll
events are ready

====

one of mono programs I use stops receiving data from the network. Wireshark shows that the TCP window of a connection is filled. This means the program does not read the data fast enough after requesting the data.

If I revert that commit on the top of -next (20120518), everything works as expected.

regards,

Signature · 2 lines

js
suse labs

Rafael J. Wysocki

2012-05-20 12:18 UTC #1 lore raw

Quoted 12 lines · Jiri Slaby

Hi,
a bisection shows that with the following commit from -next:
commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
Author: Arve Hjønnevåg <[email protected]>
Date:   Tue May 1 21:33:34 2012 +0200
epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll
events are ready
====

Quoted 7 lines

one of mono programs I use stops receiving data from the network. Wireshark shows that the TCP window of a connection is filled. This means the program does not read the data fast enough after requesting the data.
If I revert that commit on the top of -next (20120518), everything works as expected.

Hmm. I suppose that the failing program doesn't set EPOLLWAKEUP by mistake, does it?

Rafael

Rafael J. Wysocki

2012-05-20 12:48 UTC #2 lore raw

Quoted 18 lines · Rafael J. Wysocki

Quoted 17 lines · Jiri Slaby
Hi,
a bisection shows that with the following commit from -next:
commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
Author: Arve Hjønnevåg <[email protected]>
Date:   Tue May 1 21:33:34 2012 +0200
epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll
events are ready
====
one of mono programs I use stops receiving data from the network. Wireshark shows that the TCP window of a connection is filled. This means the program does not read the data fast enough after requesting the data.

Quoted 5 lines

Quoted 2 lines
If I revert that commit on the top of -next (20120518), everything works as expected.
Hmm. I suppose that the failing program doesn't set EPOLLWAKEUP by mistake, does it?

If it doesn't, we can assume that epi-ws is always NULL and all of the added overhead comes from the function calls. So, I wonder if the appended patch makes any difference?

Rafael

---
 fs/eventpoll.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

fs/eventpoll.c +24 ~~-11~~

Index: linux/fs/eventpoll.c
===================================================================
@@ -597,7 +597,8 @@ static int ep_scan_ready_list(struct eve
 		 */
 		if (!ep_is_linked(&epi->rdllink)) {
 			list_add_tail(&epi->rdllink, &ep->rdllist);
-			__pm_stay_awake(epi->ws);
+			if (epi->ws)
+				__pm_stay_awake(epi->ws);
 		}
 	}
 	/*

@@ -611,7 +612,8 @@ static int ep_scan_ready_list(struct eve
 	 * Quickly re-inject items left on "txlist".
 	 */
 	list_splice(&txlist, &ep->rdllist);
-	__pm_relax(ep->ws);
+	if (ep->ws)
+		__pm_relax(ep->ws);
 
 	if (!list_empty(&ep->rdllist)) {
 		/*

@@ -750,7 +752,9 @@ static int ep_read_events_proc(struct ev
 			 * callback, but it's not actually ready, as far as
 			 * caller requested events goes. We can remove it here.
 			 */
-			__pm_relax(epi->ws);
+			if (epi->ws)
+				__pm_relax(epi->ws);
+
 			list_del_init(&epi->rdllink);
 		}
 	}

@@ -956,7 +960,8 @@ static int ep_poll_callback(wait_queue_t
 	/* If this file is already in the ready list we exit soon */
 	if (!ep_is_linked(&epi->rdllink)) {
 		list_add_tail(&epi->rdllink, &ep->rdllist);
-		__pm_stay_awake(epi->ws);
+		if (epi->ws)
+			__pm_stay_awake(epi->ws);
 	}
 
 	/*

@@ -1219,7 +1224,8 @@ static int ep_insert(struct eventpoll *e
 	/* If the file is already "ready" we drop it inside the ready list */
 	if ((revents & event->events) && !ep_is_linked(&epi->rdllink)) {
 		list_add_tail(&epi->rdllink, &ep->rdllist);
-		__pm_stay_awake(epi->ws);
+		if (epi->ws)
+			__pm_stay_awake(epi->ws);
 
 		/* Notify waiting tasks that events are available */
 		if (waitqueue_active(&ep->wq))

@@ -1309,7 +1315,8 @@ static int ep_modify(struct eventpoll *e
 		spin_lock_irq(&ep->lock);
 		if (!ep_is_linked(&epi->rdllink)) {
 			list_add_tail(&epi->rdllink, &ep->rdllist);
-			__pm_stay_awake(epi->ws);
+			if (epi->ws)
+				__pm_stay_awake(epi->ws);
 
 			/* Notify waiting tasks that events are available */
 			if (waitqueue_active(&ep->wq))

@@ -1357,9 +1364,12 @@ static int ep_send_events_proc(struct ev
 		 * instead, but then epi->ws would temporarily be out of sync
 		 * with ep_is_linked().
 		 */
-		if (epi->ws && epi->ws->active)
-			__pm_stay_awake(ep->ws);
-		__pm_relax(epi->ws);
+		if (epi->ws) {
+			if (epi->ws->active)
+				__pm_stay_awake(ep->ws);
+
+			__pm_relax(epi->ws);
+		}
 		list_del_init(&epi->rdllink);
 
 		pt._key = epi->event.events;

@@ -1376,7 +1386,9 @@ static int ep_send_events_proc(struct ev
 			if (__put_user(revents, &uevent->events) ||
 			    __put_user(epi->event.data, &uevent->data)) {
 				list_add(&epi->rdllink, head);
-				__pm_stay_awake(epi->ws);
+				if (epi->ws)
+					__pm_stay_awake(epi->ws);
+
 				return eventcnt ? eventcnt : -EFAULT;
 			}
 			eventcnt++;

@@ -1396,7 +1408,8 @@ static int ep_send_events_proc(struct ev
 				 * poll callback will queue them in ep->ovflist.
 				 */
 				list_add_tail(&epi->rdllink, &ep->rdllist);
-				__pm_stay_awake(epi->ws);
+				if (epi->ws)
+					__pm_stay_awake(epi->ws);
 			}
 		}
 	}

Rafael J. Wysocki

2012-05-20 18:29 UTC #3 lore raw

Quoted 22 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Jiri Slaby
Hi,
a bisection shows that with the following commit from -next:
commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
Author: Arve Hjønnevåg <[email protected]>
Date:   Tue May 1 21:33:34 2012 +0200
epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll
events are ready
====
one of mono programs I use stops receiving data from the network. Wireshark shows that the TCP window of a connection is filled. This means the program does not read the data fast enough after requesting the data.
If I revert that commit on the top of -next (20120518), everything works as expected.

Quoted 6 lines

Quoted 2 lines
Hmm. I suppose that the failing program doesn't set EPOLLWAKEUP by mistake, does it?
If it doesn't, we can assume that epi-ws is always NULL and all of the added overhead comes from the function calls. So, I wonder if the appended patch makes any difference?

Having thought more about this I have to say this doesn't seem to make much sense, because in that case you'd see some progress, although probably a bit slower than before.

So, I think what happens is that the application tries to set EPOLLWAKEUP, but doesn't have the capability, so the entire operation fails for it, but it doesn't check the return value.

I wonder if the following helps, then.

Thanks,
Rafael

---
 fs/eventpoll.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/eventpoll.c +1 -1

Index: linux/fs/eventpoll.c
===================================================================
@@ -1711,7 +1711,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, in
 
 	/* Check if EPOLLWAKEUP is allowed */
 	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_EPOLLWAKEUP))
-		goto error_tgt_fput;
+		epds.events &= ~EPOLLWAKEUP;
 
 	/*
 	 * We have to check that the file structure underneath the file descriptor

Jiri Slaby

author

2012-05-21 13:55 UTC #4 lore raw

Quoted 53 lines · Rafael J. Wysocki

Quoted 22 lines · Rafael J. Wysocki
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Jiri Slaby
Hi,
a bisection shows that with the following commit from -next:
commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
Author: Arve Hjønnevåg <[email protected]>
Date:   Tue May 1 21:33:34 2012 +0200
epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll
events are ready
====
one of mono programs I use stops receiving data from the network. Wireshark shows that the TCP window of a connection is filled. This means the program does not read the data fast enough after requesting the data.
If I revert that commit on the top of -next (20120518), everything works as expected.
Quoted 6 lines
Quoted 2 lines
Hmm. I suppose that the failing program doesn't set EPOLLWAKEUP by mistake, does it?
If it doesn't, we can assume that epi-ws is always NULL and all of the added overhead comes from the function calls. So, I wonder if the appended patch makes any difference?
Having thought more about this I have to say this doesn't seem to make much sense, because in that case you'd see some progress, although probably a bit slower than before.
So, I think what happens is that the application tries to set EPOLLWAKEUP, but doesn't have the capability, so the entire operation fails for it, but it doesn't check the return value.
I wonder if the following helps, then.
Thanks,
Rafael
---
 fs/eventpoll.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/fs/eventpoll.c
===================================================================

fs/eventpoll.c SYSCALL_DEFINE4(epoll_ctl, int, epfd, in · line 1714

Quoted 5 lines

fs/eventpoll.c +1 -1

@@ -1711,7 +1711,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, in
 
 	/* Check if EPOLLWAKEUP is allowed */
 	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_EPOLLWAKEUP))
-		goto error_tgt_fput;
+		epds.events &= ~EPOLLWAKEUP;

Yes, this fixed the issue.

thanks,

Signature · 2 lines

js
suse labs

Rafael J. Wysocki

2012-05-21 19:06 UTC #5 lore raw

Quoted 55 lines · Jiri Slaby

Quoted 54 lines · Rafael J. Wysocki
Quoted 22 lines · Rafael J. Wysocki
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Jiri Slaby
Hi,
a bisection shows that with the following commit from -next:
commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
Author: Arve Hjønnevåg <[email protected]>
Date:   Tue May 1 21:33:34 2012 +0200
epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll
events are ready
====
one of mono programs I use stops receiving data from the network. Wireshark shows that the TCP window of a connection is filled. This means the program does not read the data fast enough after requesting the data.
If I revert that commit on the top of -next (20120518), everything works as expected.
Quoted 6 lines
Quoted 2 lines
Hmm. I suppose that the failing program doesn't set EPOLLWAKEUP by mistake, does it?
If it doesn't, we can assume that epi-ws is always NULL and all of the added overhead comes from the function calls. So, I wonder if the appended patch makes any difference?
Having thought more about this I have to say this doesn't seem to make much sense, because in that case you'd see some progress, although probably a bit slower than before.
So, I think what happens is that the application tries to set EPOLLWAKEUP, but doesn't have the capability, so the entire operation fails for it, but it doesn't check the return value.
I wonder if the following helps, then.
Thanks,
Rafael
---
 fs/eventpoll.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/fs/eventpoll.c
===================================================================
fs/eventpoll.c +0 -0
@@ -1711,7 +1711,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, in
 

Quoted 6 lines

fs/eventpoll.c SYSCALL_DEFINE4(epoll_ctl, int, epfd, in · line 1714
Quoted 4 lines
fs/eventpoll.c +1 -1
 	/* Check if EPOLLWAKEUP is allowed */
 	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_EPOLLWAKEUP))
-		goto error_tgt_fput;
+		epds.events &= ~EPOLLWAKEUP;
Yes, this fixed the issue.

Cool, thanks for testing!

I'll resend it in a while with a changelog and tags.

Thanks,
Rafael

Rafael J. Wysocki

2012-05-21 19:23 UTC #6 lore raw

PATCH 1/1 epoll: Fix user space breakage related to EPOLLWAKEUP (was: Re: [-next regression] TCP window full with EPOLLWAKEUP)

From: Rafael J. Wysocki <[email protected]>

Commit 4d7e30d (epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll events are ready) caused some applications to malfunction, because they set the bit corresponding to the new EPOLLWAKEUP flag in their eventpoll flags and they don't have the new CAP_EPOLLWAKEUP capability.

To prevent that from happening, change epoll_ctl() to clear EPOLLWAKEUP in epds.events if the caller doesn't have the CAP_EPOLLWAKEUP capability instead of failing and returning an error code, which allows the affected applications to function normally.

Reported-and-tested-by Jiri Slaby <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 fs/eventpoll.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/eventpoll.c +1 -1

Index: linux/fs/eventpoll.c
===================================================================
@@ -1711,7 +1711,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, in
 
 	/* Check if EPOLLWAKEUP is allowed */
 	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_EPOLLWAKEUP))
-		goto error_tgt_fput;
+		epds.events &= ~EPOLLWAKEUP;
 
 	/*
 	 * We have to check that the file structure underneath the file descriptor

Arve Hjønnevåg

2012-05-21 22:11 UTC #7 lore raw

Re: [PATCH] epoll: Fix user space breakage related to EPOLLWAKEUP (was: Re: [-next regression] TCP window full with EPOLLWAKEUP)

Quoted 25 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Commit 4d7e30d (epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll events are ready) caused some applications to malfunction, because they set the bit corresponding to the new EPOLLWAKEUP flag in their eventpoll flags and they don't have the new CAP_EPOLLWAKEUP capability.
To prevent that from happening, change epoll_ctl() to clear EPOLLWAKEUP in epds.events if the caller doesn't have the CAP_EPOLLWAKEUP capability instead of failing and returning an error code, which allows the affected applications to function normally.
Reported-and-tested-by Jiri Slaby <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
fs/eventpoll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/fs/eventpoll.c
===================================================================

fs/eventpoll.c SYSCALL_DEFINE4(epoll_ctl, int, epfd, in · line 1717

Quoted 8 lines

fs/eventpoll.c +1 -1

@@ -1711,7 +1711,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, in
 
        /* Check if EPOLLWAKEUP is allowed */
        if ((epds.events & EPOLLWAKEUP) && !capable(CAP_EPOLLWAKEUP))
-               goto error_tgt_fput;
+               epds.events &= ~EPOLLWAKEUP;
 
        /*
         * We have to check that the file structure underneath the file descriptor

Is there any way for the application to detect that it did not get the EPOLLWAKEUP feature?

Signature · 1 line

Arve Hjønnevåg

Rafael J. Wysocki

2012-05-22 15:51 UTC #8 lore raw

Re: [PATCH] epoll: Fix user space breakage related to EPOLLWAKEUP (was: Re: [-next regression] TCP window full with EPOLLWAKEUP)

Quoted 32 lines · Arve Hjønnevåg

Quoted 31 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Commit 4d7e30d (epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll events are ready) caused some applications to malfunction, because they set the bit corresponding to the new EPOLLWAKEUP flag in their eventpoll flags and they don't have the new CAP_EPOLLWAKEUP capability.
To prevent that from happening, change epoll_ctl() to clear EPOLLWAKEUP in epds.events if the caller doesn't have the CAP_EPOLLWAKEUP capability instead of failing and returning an error code, which allows the affected applications to function normally.
Reported-and-tested-by Jiri Slaby <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 fs/eventpoll.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/fs/eventpoll.c
===================================================================
fs/eventpoll.c +1 -1
@@ -1711,7 +1711,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, in
 
       /* Check if EPOLLWAKEUP is allowed */
       if ((epds.events & EPOLLWAKEUP) && !capable(CAP_EPOLLWAKEUP))
-               goto error_tgt_fput;
+               epds.events &= ~EPOLLWAKEUP;
 

Quoted 5 lines

fs/eventpoll.c SYSCALL_DEFINE4(epoll_ctl, int, epfd, in · line 1717
Quoted 2 lines
fs/eventpoll.c +0 -0
       /*
        * We have to check that the file structure underneath the file descriptor
Is there any way for the application to detect that it did not get the EPOLLWAKEUP feature?

Well, it should check its capabilities beforehand if it really cares ...

Moreover, if the creation of the wakeup source in ep_modify() fails, the application won't be notified either.

Thanks,
Rafael