Implement blocking eventfd #3939

tiif · 2024-10-04T10:48:47Z

This PR

Implemented blocking for both read and write of eventfd
Added test for eventfd blocking read and write
Removed eventfd blocking tests from fail-dep
Added a new BlockReason::Eventfd

cc #3665

tiif · 2024-10-04T10:49:24Z

@rustbot author

tiif · 2024-10-05T10:16:30Z

Hmm...why is rustfmt complaining? Let me try rebase it instead.

src/shims/unix/linux/eventfd.rs

bors · 2024-10-08T13:47:06Z

☔ The latest upstream changes (presumably #3951) made this pull request unmergeable. Please resolve the merge conflicts.

tests/fail-dep/libc/eventfd_block_twice.stderr

src/shims/unix/linux/eventfd.rs

tiif · 2024-10-17T15:36:00Z

src/shims/unix/linux/eventfd.rs

+    let mut waiter = Vec::new();
+    let mut blocked_write_tid = eventfd.blocked_write_tid.borrow_mut();
+    while let Some(tid) = blocked_write_tid.pop() {
+        waiter.push(tid);
+    }
+    drop(blocked_write_tid);
+
+    waiter.sort();
+    waiter.dedup();
+    for thread_id in waiter {
+        ecx.unblock_thread(thread_id, BlockReason::Eventfd)?;
+    }


I couldn't think of a testcase that can actually produce thread id duplication in blocked_write/read_id, but I will just keep the dedup here first.

we'll get a panic if that happens, so maybe just don't sort and dedup and wait until we get a test case. I don't think it can happen though, as a thread would need to get blocked again without having been removed and unblocked.

Also: the unblocking order is user-visible, and tho I assume that eventfd does not specify which thread gets woken first, we'd probably want a random (deterministic, but depending on the seed) order of unblocking to happen

cc @RalfJung I think we need a new system for unblocking multiple threads at the same time. So far we only ever unblocked one thread, and "unblock N threads, then randomly run them with the normal rules for picking the next thread" seems like a recurring thing.

Also I wonder if the normal thread unblocking is subtly wrong, too. Or just wrong in this PR:

When a thread gets unblocked and immediately performs some operation that is visible from other threads, then that means when the current thread continues after unblocking the other thread, it may behave as if the unblocked thread already had a few CPU cycles to do something. While that may be expected behaviour depending on preemption, the behaviour here does not depend on preemption. So, to avoid such a footgun, thread unblocking should not immediately execute the unblocking operation, but just unblock the thread and make the thread execute the unblocking operation when it gets scheduled next.

tiif · 2024-10-18T05:10:07Z

@rustbot ready

oli-obk

not finished with my review, but we need to figure out a few thread unblocking things first

src/shims/unix/linux/eventfd.rs

oli-obk · 2024-10-26T11:10:43Z

src/shims/unix/linux/eventfd.rs

+    let mut waiter = Vec::new();
+    let mut blocked_write_tid = eventfd.blocked_write_tid.borrow_mut();
+    while let Some(tid) = blocked_write_tid.pop() {
+        waiter.push(tid);
+    }
+    drop(blocked_write_tid);
+
+    waiter.sort();
+    waiter.dedup();
+    for thread_id in waiter {
+        ecx.unblock_thread(thread_id, BlockReason::Eventfd)?;
+    }


we'll get a panic if that happens, so maybe just don't sort and dedup and wait until we get a test case. I don't think it can happen though, as a thread would need to get blocked again without having been removed and unblocked.

src/shims/unix/linux/eventfd.rs

oli-obk · 2024-10-26T11:19:31Z

src/shims/unix/linux/eventfd.rs

+    let mut waiter = Vec::new();
+    let mut blocked_write_tid = eventfd.blocked_write_tid.borrow_mut();
+    while let Some(tid) = blocked_write_tid.pop() {
+        waiter.push(tid);
+    }
+    drop(blocked_write_tid);
+
+    waiter.sort();
+    waiter.dedup();
+    for thread_id in waiter {
+        ecx.unblock_thread(thread_id, BlockReason::Eventfd)?;
+    }


Also: the unblocking order is user-visible, and tho I assume that eventfd does not specify which thread gets woken first, we'd probably want a random (deterministic, but depending on the seed) order of unblocking to happen

cc @RalfJung I think we need a new system for unblocking multiple threads at the same time. So far we only ever unblocked one thread, and "unblock N threads, then randomly run them with the normal rules for picking the next thread" seems like a recurring thing.

Also I wonder if the normal thread unblocking is subtly wrong, too. Or just wrong in this PR:

When a thread gets unblocked and immediately performs some operation that is visible from other threads, then that means when the current thread continues after unblocking the other thread, it may behave as if the unblocked thread already had a few CPU cycles to do something. While that may be expected behaviour depending on preemption, the behaviour here does not depend on preemption. So, to avoid such a footgun, thread unblocking should not immediately execute the unblocking operation, but just unblock the thread and make the thread execute the unblocking operation when it gets scheduled next.

src/shims/unix/linux/eventfd.rs

oli-obk · 2024-10-26T15:57:17Z

@RalfJung we raced on the review, do you have any thoughts on #3939 (comment)

RalfJung · 2024-10-26T19:42:36Z

I think for now making wakeup deterministic with a queue (via VecDeque) is fine. We can leave an issue to randomize this. Regarding the unblocking callback, it is required for correctness right now to run immediately. As long as this does not perform atomic ops (that can be observed from other threads), I think that is fine? What exact issue are you concerned about?

oli-obk · 2024-10-27T07:18:07Z

What exact issue are you concerned about?

well, in this case, that we perform the reads and writes of the unblocked thread immediately, but then continue on the current thread. So unblocking a thread affects the current thread's next operations (there can now be more space to write to, even tho no thread switch happened). This makes no-preemption tests harder to write, but is not a fundamental issue I guess, just weird and not behavior a real system would have

tiif · 2024-10-27T09:12:20Z

that we perform the reads and writes of the unblocked thread immediately, but then continue on the current thread.

Oh that's surprising. I always assumed the current thread should finish first before executing other newly unblocked threads.

But I think I observed what is mentioned here before, and I was pretty confused by the execution sequence.

src/shims/unix/linux/eventfd.rs

oli-obk · 2024-11-12T20:33:38Z

tests/fail-dep/libc/eventfd_block_write_twice.rs

+// 1. Thread 1 blocks.
+// 2. Thread 2 blocks.
+// 3. Thread 3 unblocks both thread 1 and thread 2.
+// 4. Either thread 1 or thread 2 writes u64::MAX.


Due to the preemption rate being zero, this is deterministic no matter the seed, right?

Why is it thread 3 that gets to write first and not thread 2? How does that ordering of the unblocks happen?

Yes it is deterministic, it consistently blocked on thread2 when running with many-seeds . The unblock sequence is related to the unblocking order below:

let waiting_threads = std::mem::take(&mut *eventfd.blocked_write_tid.borrow_mut()); for thread_id in waiting_threads { ecx.unblock_thread(thread_id, BlockReason::Eventfd)?; }

In the test, thread1 gets blocked before thread2, so during the unblock, thread1 gets unblocked before thread2. Hence thread1 gets to return from eventfd_write first, while thread2 hits the blocking condition again.

oli-obk · 2024-11-12T20:35:29Z

Please squash the changes

tiif · 2024-11-13T05:50:18Z

Thanks! I left FIXME for randomizing unblock sequence, and slightly changed the comments for expected execution in eventfd_blocked_read_twice and eventfd_blocked_write_twice since it is deterministic.

RalfJung · 2024-11-21T21:22:51Z

src/shims/unix/linux/eventfd.rs

+            // When any of the event happened, we check and update the status of all supported event
+            // types for current file description.
+            ecx.check_and_update_readiness(&eventfd_ref)?;


This used to be done in both match arms. The comment even says that. Now it was moved to only run in one match arm. The comment is outdated now, and also -- why was it moved?

I guess it is because in the other arm, nothing actually changes, we just block?

I guess it is because in the other arm, nothing actually changes, we just block?

Yes exactly. There are four scenarios that could happen here:

It succeed without blocking, we do check_and_update_readiness.

It blocked and eventually unblock because some event happened, it will hit the succeed path that contains check_and_update_readiness.

It errors out with ErrorKind::WouldBlock.

It blocked and never get unblocked, in that case, that's a deadlock.

RalfJung · 2024-11-21T21:26:10Z

tests/pass-dep/libc/libc-eventfd.rs

+    thread1.thread().unpark();
+    thread2.thread().unpark();
+    thread3.thread().unpark();


What is the reason for this parking business? That should be explained in comments.

I used it to get the execution sequence I want under -Zmiri-preemption-rate=0. It's not ideal as it is pretty hard to reason from reading the test alone (right now I verify them manually using Zmiri-report-progress). I am trying to figure out a better way to write clearer blocking test case.

In particular, I want to find a way to execute another thread only after a thread is blocked. But when a thread is blocked, I couldn't find a way for it to signal "hey I am blocked now, so you should start doing something".

We have plenty of tests that need a particular execution sequence. None of them use thread parking. So either your test is quite special, or you can achieve the same thing in a much less complicated way. I think it's the latter.

The scheduler with -Zmiri-preemption-rate=0 is quite simple: the current thread keeps going until it blocks or yields. Then the next thread goes -- in the order that threads were spawned in. So please rewrite these tests to not use thread parking any more.

Then the next thread goes -- in the order that threads were spawned in.

This information is useful. I will take a look at the test again and open a pr to simplify the test here and #4033.

rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Oct 4, 2024

tiif force-pushed the blockeventfd branch from 7bbf6fb to fee11aa Compare October 5, 2024 10:17

oli-obk requested changes Oct 5, 2024

View reviewed changes

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

tiif force-pushed the blockeventfd branch from fee11aa to 37328f8 Compare October 13, 2024 15:51

tiif commented Oct 13, 2024

View reviewed changes

tests/fail-dep/libc/eventfd_block_twice.stderr Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

tiif commented Oct 14, 2024

View reviewed changes

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

tiif force-pushed the blockeventfd branch from bc3f54d to 18f9e84 Compare October 14, 2024 07:18

tiif commented Oct 17, 2024

View reviewed changes

tiif marked this pull request as ready for review October 18, 2024 05:09

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Oct 18, 2024

oli-obk requested changes Oct 26, 2024

View reviewed changes

RalfJung reviewed Oct 26, 2024

View reviewed changes

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 26, 2024

View reviewed changes

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 26, 2024

View reviewed changes

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

tiif mentioned this pull request Nov 4, 2024

Avoid eagerly executing unblock callback in unblock_thread #4011

Closed

oli-obk requested changes Nov 11, 2024

View reviewed changes

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

src/shims/unix/linux/eventfd.rs Outdated Show resolved Hide resolved

oli-obk reviewed Nov 12, 2024

View reviewed changes

oli-obk approved these changes Nov 12, 2024

View reviewed changes

Implement blocking eventfd

eabee96

tiif force-pushed the blockeventfd branch from 4429518 to eabee96 Compare November 13, 2024 05:47

oli-obk added this pull request to the merge queue Nov 13, 2024

Merged via the queue into rust-lang:master with commit 9d9da34 Nov 13, 2024
7 checks passed

bors mentioned this pull request Nov 13, 2024

Added epoll and eventfd for Android #4016

Merged

tiif deleted the blockeventfd branch November 13, 2024 09:23

tiif mentioned this pull request Nov 13, 2024

Add test for epoll #4033

Merged

RalfJung reviewed Nov 21, 2024

View reviewed changes

tiif mentioned this pull request Nov 25, 2024

Simplify thread blocking tests #4059

Merged

Implement blocking eventfd #3939

Implement blocking eventfd #3939

Uh oh!

Conversation

tiif commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiif commented Oct 4, 2024

Uh oh!

tiif commented Oct 5, 2024

Uh oh!

Uh oh!

Uh oh!

bors commented Oct 8, 2024

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

tiif Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiif commented Oct 18, 2024

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oli-obk commented Oct 26, 2024

Uh oh!

RalfJung commented Oct 26, 2024 via email

Uh oh!

oli-obk commented Oct 27, 2024

Uh oh!

tiif commented Oct 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiif Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Nov 12, 2024

Uh oh!

tiif commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiif commented Oct 4, 2024 •

edited

Loading

tiif Oct 17, 2024 •

edited

Loading

tiif Nov 13, 2024 •

edited

Loading

tiif commented Nov 13, 2024 •

edited

Loading

RalfJung Nov 23, 2024 •

edited

Loading