* [PATCH 0/2] fs/epoll: restore user-visible behavior upon event ready
@ 2021-04-05 23:10 Davidlohr Bueso
2021-04-05 23:10 ` [PATCH 1/2] kselftest: introduce new epoll test case Davidlohr Bueso
2021-04-05 23:10 ` [PATCH 2/2] fs/epoll: restore waking from ep_done_scan() Davidlohr Bueso
0 siblings, 2 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2021-04-05 23:10 UTC (permalink / raw)
To: akpm; +Cc: jbaron, rpenyaev, viro, linux-fsdevel, linux-kernel, dave
Hi,
This series tries to address a change in user visible behavior,
reported in:
https://bugzilla.kernel.org/show_bug.cgi?id=208943
Epoll does not report an event to all the threads running epoll_wait()
on the same epoll descriptor. Unsurprisingly, this was bisected back to
339ddb53d373 (fs/epoll: remove unnecessary wakeups of nested epoll), which
has had various problems in the past, beyond only nested epoll usage.
Thanks!
Davidlohr Bueso (2):
kselftest: introduce new epoll test case
fs/epoll: restore waking from ep_done_scan()
fs/eventpoll.c | 6 +++
.../filesystems/epoll/epoll_wakeup_test.c | 44 +++++++++++++++++++
2 files changed, 50 insertions(+)
--
2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] kselftest: introduce new epoll test case
2021-04-05 23:10 [PATCH 0/2] fs/epoll: restore user-visible behavior upon event ready Davidlohr Bueso
@ 2021-04-05 23:10 ` Davidlohr Bueso
2021-04-05 23:10 ` [PATCH 2/2] fs/epoll: restore waking from ep_done_scan() Davidlohr Bueso
1 sibling, 0 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2021-04-05 23:10 UTC (permalink / raw)
To: akpm
Cc: jbaron, rpenyaev, viro, linux-fsdevel, linux-kernel, dave,
Davidlohr Bueso
This incorporates the testcase originally reported in:
https://bugzilla.kernel.org/show_bug.cgi?id=208943
Which ensures an event is reported to all threads blocked on the
same epoll descriptor, otherwise only a single thread will receive
the wakeup once the event become ready.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
.../filesystems/epoll/epoll_wakeup_test.c | 44 +++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c b/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c
index ad7fabd575f9..65ede506305c 100644
--- a/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c
+++ b/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c
@@ -3449,4 +3449,48 @@ TEST(epoll63)
close(sfd[1]);
}
+/*
+ * t0 t1
+ * (ew) \ / (ew)
+ * e0
+ * | (lt)
+ * s0
+ */
+TEST(epoll64)
+{
+ pthread_t waiter[2];
+ struct epoll_event e;
+ struct epoll_mtcontext ctx = { 0 };
+
+ signal(SIGUSR1, signal_handler);
+
+ ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, ctx.sfd), 0);
+
+ ctx.efd[0] = epoll_create(1);
+ ASSERT_GE(ctx.efd[0], 0);
+
+ e.events = EPOLLIN;
+ ASSERT_EQ(epoll_ctl(ctx.efd[0], EPOLL_CTL_ADD, ctx.sfd[0], &e), 0);
+
+ /*
+ * main will act as the emitter once both waiter threads are
+ * blocked and expects to both be awoken upon the ready event.
+ */
+ ctx.main = pthread_self();
+ ASSERT_EQ(pthread_create(&waiter[0], NULL, waiter_entry1a, &ctx), 0);
+ ASSERT_EQ(pthread_create(&waiter[1], NULL, waiter_entry1a, &ctx), 0);
+
+ usleep(100000);
+ ASSERT_EQ(write(ctx.sfd[1], "w", 1), 1);
+
+ ASSERT_EQ(pthread_join(waiter[0], NULL), 0);
+ ASSERT_EQ(pthread_join(waiter[1], NULL), 0);
+
+ EXPECT_EQ(ctx.count, 2);
+
+ close(ctx.efd[0]);
+ close(ctx.sfd[0]);
+ close(ctx.sfd[1]);
+}
+
TEST_HARNESS_MAIN
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] fs/epoll: restore waking from ep_done_scan()
2021-04-05 23:10 [PATCH 0/2] fs/epoll: restore user-visible behavior upon event ready Davidlohr Bueso
2021-04-05 23:10 ` [PATCH 1/2] kselftest: introduce new epoll test case Davidlohr Bueso
@ 2021-04-05 23:10 ` Davidlohr Bueso
2021-04-06 1:50 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Davidlohr Bueso @ 2021-04-05 23:10 UTC (permalink / raw)
To: akpm
Cc: jbaron, rpenyaev, viro, linux-fsdevel, linux-kernel, dave,
Davidlohr Bueso
339ddb53d373 (fs/epoll: remove unnecessary wakeups of nested epoll) changed
the userspace visible behavior of exclusive waiters blocked on a common
epoll descriptor upon a single event becoming ready. Previously, all tasks
doing epoll_wait would awake, and now only one is awoken, potentially causing
missed wakeups on applications that rely on this behavior, such as Apache Qpid.
While the aforementioned commit aims at having only a wakeup single path in
ep_poll_callback (with the exceptions of epoll_ctl cases), we need to restore
the wakeup in what was the old ep_scan_ready_list() such that the next thread
can be awoken, in a cascading style, after the waker's corresponding ep_send_events().
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
fs/eventpoll.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 73138ea68342..1e596e1d0bba 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -657,6 +657,12 @@ static void ep_done_scan(struct eventpoll *ep,
*/
list_splice(txlist, &ep->rdllist);
__pm_relax(ep->ws);
+
+ if (!list_empty(&ep->rdllist)) {
+ if (waitqueue_active(&ep->wq))
+ wake_up(&ep->wq);
+ }
+
write_unlock_irq(&ep->lock);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs/epoll: restore waking from ep_done_scan()
2021-04-05 23:10 ` [PATCH 2/2] fs/epoll: restore waking from ep_done_scan() Davidlohr Bueso
@ 2021-04-06 1:50 ` Andrew Morton
2021-04-06 3:22 ` Davidlohr Bueso
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2021-04-06 1:50 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: jbaron, rpenyaev, viro, linux-fsdevel, linux-kernel,
Davidlohr Bueso
On Mon, 5 Apr 2021 16:10:25 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
> 339ddb53d373 (fs/epoll: remove unnecessary wakeups of nested epoll) changed
> the userspace visible behavior of exclusive waiters blocked on a common
> epoll descriptor upon a single event becoming ready. Previously, all tasks
> doing epoll_wait would awake, and now only one is awoken, potentially causing
> missed wakeups on applications that rely on this behavior, such as Apache Qpid.
>
> While the aforementioned commit aims at having only a wakeup single path in
> ep_poll_callback (with the exceptions of epoll_ctl cases), we need to restore
> the wakeup in what was the old ep_scan_ready_list() such that the next thread
> can be awoken, in a cascading style, after the waker's corresponding ep_send_events().
>
Tricky. 339ddb53d373 was merged in December 2019. So do we backport
this fix? Could any userspace code be depending upon the
post-339ddb53d373 behaviour?
Or do we just leave the post-339ddb53d373 code as-is? Presumably the
issue is very rarely encountered, and changeing it back has its own
risks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs/epoll: restore waking from ep_done_scan()
2021-04-06 1:50 ` Andrew Morton
@ 2021-04-06 3:22 ` Davidlohr Bueso
2021-04-06 5:09 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Davidlohr Bueso @ 2021-04-06 3:22 UTC (permalink / raw)
To: Andrew Morton
Cc: jbaron, rpenyaev, viro, linux-fsdevel, linux-kernel,
Davidlohr Bueso
On Mon, 05 Apr 2021, Andrew Morton wrote:
>Tricky. 339ddb53d373 was merged in December 2019. So do we backport
>this fix? Could any userspace code be depending upon the
>post-339ddb53d373 behavior?
As with previous trouble caused by this commit, I vote for restoring the behavior
backporting the fix, basically the equivalent of adding (which was my intention):
Fixes: 339ddb53d373 ("fs/epoll: remove unnecessary wakeups of nested epoll")
>
>Or do we just leave the post-339ddb53d373 code as-is? Presumably the
>issue is very rarely encountered, and changeing it back has its own
>risks.
While I also consider this scenario rare (normally new ready events can become
ready and trigger new wakeups), I'm seeing reports in real applications of task
hangs due to this change of semantics. Alternatively, users can update their code
to timeout in such scenarios, but it is ultimately the kernel's fault. Furthermore
it hasn't really been all _that_ long since the commit was merged, so I don't think
it merits a change in behavior.
As for the risks of restoring the behavior, afaict this only fixed a double wakeup
in an obscure nested epoll scenario, so I'm not too worried there sacrificing
performance for functionality. That said, there are fixes, for example 65759097d80
(epoll: call final ep_events_available() check under the lock) that would perhaps
be rendered unnecessary.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs/epoll: restore waking from ep_done_scan()
2021-04-06 3:22 ` Davidlohr Bueso
@ 2021-04-06 5:09 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2021-04-06 5:09 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: jbaron, rpenyaev, viro, linux-fsdevel, linux-kernel,
Davidlohr Bueso
On Mon, 5 Apr 2021 20:22:26 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Mon, 05 Apr 2021, Andrew Morton wrote:
>
> >Tricky. 339ddb53d373 was merged in December 2019. So do we backport
> >this fix? Could any userspace code be depending upon the
> >post-339ddb53d373 behavior?
>
> As with previous trouble caused by this commit, I vote for restoring the behavior
> backporting the fix, basically the equivalent of adding (which was my intention):
>
> Fixes: 339ddb53d373 ("fs/epoll: remove unnecessary wakeups of nested epoll")
OK, I added the Fixes: line and the cc:stable line.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-06 5:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-05 23:10 [PATCH 0/2] fs/epoll: restore user-visible behavior upon event ready Davidlohr Bueso
2021-04-05 23:10 ` [PATCH 1/2] kselftest: introduce new epoll test case Davidlohr Bueso
2021-04-05 23:10 ` [PATCH 2/2] fs/epoll: restore waking from ep_done_scan() Davidlohr Bueso
2021-04-06 1:50 ` Andrew Morton
2021-04-06 3:22 ` Davidlohr Bueso
2021-04-06 5:09 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).