Implement epoll_wait #2764

DebugSteven · 2023-01-23T16:14:50Z

This PR continues working on #602.

This is an implementation for sleep, though admittedly not a good one. It just does busy waiting.

src/shims/unix/linux/fd.rs

oli-obk · 2023-01-25T08:46:47Z

src/shims/unix/linux/fd/event.rs

+        bytes: &[u8],
+    ) -> InterpResult<'tcx, io::Result<usize>> {
+        let v1 = self.val.get();
+        let v2 = v1.checked_add(u64::from_be_bytes(bytes.try_into().unwrap())).unwrap();


Leave a FIXME to handle these error cases to behave as described in the doc comment

FYI this case is hit by the test suites of a handful of crates. So far I know of aqueue 1.2.10, async-io-typed 3.0.0, terminus-store 0.20.0, quic-rpc 0.3.2, and snocat 0.8.0-alpha.1.

src/shims/unix/linux/foreign_items.rs

oli-obk · 2023-01-28T12:10:57Z

I guess the important thing is to test that sleep doesn't sleep less than the specified time, so maybe just compare the lower end of the range?

Once/if we figure out how to do tokio tests in isolation mode, we can make the test more reliable with an upper limit

oli-obk · 2023-02-03T17:47:03Z

@bors r+

bors · 2023-02-03T17:47:06Z

📌 Commit e87acf1 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-02-03T17:47:13Z

⌛ Testing commit e87acf1 with merge 7fc42bf...

bors · 2023-02-03T18:21:00Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 7fc42bf to master...

RalfJung · 2023-02-13T21:47:32Z

src/shims/unix/linux/fd.rs

+            let _epfd = epfd.as_epoll_handle()?;
+
+            // FIXME return number of events ready when scheme for marking events ready exists
+            Ok(Scalar::from_i32(numevents))


Doesn't this tell the caller that numevents events are ready without actually initializing the buffer? That sounds pretty bad, the user will then see UB when trying to access the info in the buffer.

RalfJung · 2023-02-13T21:49:22Z

src/shims/unix/linux/fd/event.rs

+    /// supplied buffer is less than 8 bytes, or if an attempt is
+    /// made to write the value 0xffffffffffffffff.
+    ///
+    /// FIXME: use endianness


Please don't land code that just silently does the wrong thing on some targets. At the very least it must ICE. Nothing is worse than if Miri pretends to be able to execute something but then executes it incorrectly. That must never happen.

RalfJung · 2023-02-13T21:50:04Z

src/shims/unix/linux/fd/event.rs

+        // or fail with EAGAIN if the file descriptor is nonblocking.
+        let v2 = v1.checked_add(u64::from_be_bytes(bytes.try_into().unwrap())).unwrap();
+        self.val.set(v2);
+        assert_eq!(8, bytes.len());


This should be checked before converting the value to an integer.

The unwrap two lines above already does this check, we can remove the assert

RalfJung · 2023-02-13T21:52:32Z

src/shims/unix/linux/fd.rs

+    /// ready during the requested timeout milliseconds. On failure,
+    /// `epoll_wait()` returns -1 and errno is set to indicate the error.
+    ///
+    /// <https://man7.org/linux/man-pages/man2/epoll_wait.2.html>


Our implementation is very, very far from this documentation, right? How's that not a huge problem? If a random program calls epoll_wait (keep in mind that tokio is not the only thing that can call this function), then one of two things must happen:

either it behaves in some reasonable way

or it stops execution (ideally via throw_unsupported, or an ICE if need be)

But here we just silently ignore the timeout and a bunch of other things, so execution will just silently go wrong, right? That's not something we want Miri to ever do.

This is all an ongoing implementation where each new test fleshes out the implementation details. I don't think it would be good to require testing and implementing the entire thing in one go, as that PR would become unreviewable.

We could hide all of it behind a -Zmiri-experimental-epoll to make it clear this is unfinished

This is all an ongoing implementation where each new test fleshes out the implementation details. I don't think it would be good to require testing and implementing the entire thing in one go, as that PR would become unreviewable.

That's not what I asked for.

I think ideally, the supported part of the function is explicitly carved out, by aggressively doing throw_unsup for any parameter combination that is potentially not implemented correctly yet. That doesn't mean implementing the entire function, not at all! It could mean doing throw_unsup on literally every input, for instance. That is how we have worked so far for other shims and I think it has worked well. Wouldn't that work here as well?

I find this helps quite a bit with reviewing, since then one can immediately see which things the function intends to support, and can check if those make sense. Right now that seems to be a secret between you and @DebugSteven, and it's not even documented anywhere what the current status is. That is not great. It should be clear from the code where the gaps are, and which things are expected to work correctly. Right now I look at this and I can't see how this works correctly for any input (given that it doesn't fill the events buffer), but I can't even tell if that is a bug since I don't know for which inputs it is supposed to work.

We could hide all of it behind a -Zmiri-experimental-epoll to make it clear this is unfinished

That would be a backup plan, but from my perspective it would mean that when reviewing the PR which removes this flag, I do have to review the entire functionality of all gated shims at once, since incremental review of the previous PRs is not possible. I would prefer to avoid that.

RalfJung · 2023-02-13T21:54:34Z

src/shims/unix/linux/fd/event.rs

+    /// stored in the counter is the largest unsigned 64-bit value
+    /// minus 1 (i.e., 0xfffffffffffffffe).  If the addition would
+    /// cause the counter's value to exceed the maximum, then the
+    /// write either blocks until a read is performed on the


This mention of reads is confusing. Reads don't seem to do anything with the counter?

RalfJung · 2023-02-13T21:55:08Z

src/shims/unix/linux/fd/event.rs

-    pub val: u32,
+    /// The object contains an unsigned 64-bit integer (uint64_t) counter that is maintained by the
+    /// kernel. This counter is initialized with the value specified in the argument initval.
+    pub val: Cell<u64>,


But what does the value mean? This documentation is rather mysterious.

The original POSIX documentation is not very clear either ^^ the value can mean all kinds of things depending on the usage pattern. All that the storage should know is that it's a 64 bit int

Might be good to have an example then for how the Miri-implemented shims use this. POSIX docs being bad is no excuse for our own docs being bad. ;)

RalfJung · 2023-02-13T21:56:12Z

src/shims/unix/linux/fd/event.rs

+    ///
+    /// FIXME: use endianness
+    fn write<'tcx>(
+        &self,


Is it possible to change write to take &mut self and then avoid the Cell?

Nope, we looked into this, and the issue is that preexisting code needs to write access memory while using the file descriptors. It would be possible, but it would make all code using writes very roundabout. Also I think other write impls already use interior mutability.

I can open a PR if you'd like to see the workarounds needed

Also I think other write impls already use interior mutability.

I don't recall that.

But if you tried the alternative and considered it worse than this, that's enough of an argument. :) Thanks!

So... one write impl that "accidentally" uses interior mutability is our actual File handles. Write is implemented for &File.

The reason we can't easily switch to &mut is this piece of code:

https://github.com/rust-lang/miri/blob/master/src/shims/unix/fs.rs#L814-L819

We could handle all of this correctly, but it would require cloning somewhere, as we can't hold the mutable reference to the file descriptor while also reading from memory.

Yeah we're doing something rather clever there, directly copying from the machine memory to the syscall.

RalfJung · 2023-04-21T13:15:12Z

tests/pass-dep/tokio/tokio_mvp.rs

@@ -1,5 +1,5 @@
 // Need to disable preemption to stay on the supported MVP codepath in mio.
-//@compile-flags: -Zmiri-disable-isolation -Zmiri-permissive-provenance -Zmiri-preemption-rate=0
+//@compile-flags: -Zmiri-disable-isolation -Zmiri-permissive-provenance


Removing the -Zmiri-preemption-rate=0 here was premature, this is causing issues again. I'll add the flag back.

disable preemption in tokoo tests again The comment even still says we need preemption disabled, but the flag got lost in #2764.

disable preemption in tokio tests again The comment even still says we need preemption disabled, but the flag got lost in #2764.

disable preemption in tokio tests again The comment even still says we need preemption disabled, but the flag got lost in rust-lang/miri#2764.

DebugSteven force-pushed the sleep branch 2 times, most recently from bdbf151 to 8286c13 Compare January 23, 2023 22:35

oli-obk reviewed Jan 25, 2023

View reviewed changes

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

oli-obk reviewed Jan 25, 2023

View reviewed changes

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

DebugSteven force-pushed the sleep branch from 8286c13 to 067dde2 Compare January 27, 2023 21:55

DebugSteven mentioned this pull request Jan 27, 2023

Do the tokio tests work with the default preemption rate? #2770

Closed

oli-obk approved these changes Jan 30, 2023

View reviewed changes

busy waiting implementation for sleep

e87acf1

DebugSteven force-pushed the sleep branch from a1ad9fb to e87acf1 Compare January 31, 2023 22:28

bors merged commit 7fc42bf into rust-lang:master Feb 3, 2023

taiki-e mentioned this pull request Feb 6, 2023

chore: test more features with Miri tokio-rs/tokio#5317

Closed

RalfJung reviewed Feb 13, 2023

View reviewed changes

RalfJung mentioned this pull request Feb 24, 2023

epoll_wait implementation can silently produce wrong results #2800

Closed

RalfJung reviewed Apr 21, 2023

View reviewed changes

RalfJung mentioned this pull request Apr 21, 2023

disable preemption in tokio tests again #2848

Merged

bors added a commit that referenced this pull request Apr 21, 2023

Auto merge of #2848 - RalfJung:tokio, r=RalfJung

6dbd3ca

disable preemption in tokoo tests again The comment even still says we need preemption disabled, but the flag got lost in #2764.

bors added a commit that referenced this pull request Apr 21, 2023

Auto merge of #2848 - RalfJung:tokio, r=RalfJung

821ab05

disable preemption in tokoo tests again The comment even still says we need preemption disabled, but the flag got lost in #2764.

bors added a commit that referenced this pull request Apr 21, 2023

Auto merge of #2848 - RalfJung:tokio, r=RalfJung

cd3de05

disable preemption in tokio tests again The comment even still says we need preemption disabled, but the flag got lost in #2764.

def- mentioned this pull request Jun 16, 2023

miri: Enable tests using epoll_wait MaterializeInc/materialize#19956

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement epoll_wait #2764

Implement epoll_wait #2764

DebugSteven commented Jan 23, 2023

oli-obk Jan 25, 2023

saethlin Feb 8, 2023

oli-obk commented Jan 28, 2023

oli-obk commented Feb 3, 2023

bors commented Feb 3, 2023

bors commented Feb 3, 2023

bors commented Feb 3, 2023

RalfJung Feb 13, 2023

RalfJung Feb 13, 2023

RalfJung Feb 13, 2023

oli-obk Feb 13, 2023

RalfJung Feb 13, 2023 •

edited

Loading

oli-obk Feb 13, 2023

RalfJung Feb 14, 2023 •

edited

Loading

RalfJung Feb 13, 2023

RalfJung Feb 13, 2023

oli-obk Feb 13, 2023

RalfJung Feb 14, 2023 •

edited

Loading

RalfJung Feb 13, 2023

oli-obk Feb 13, 2023

RalfJung Feb 14, 2023

oli-obk Feb 14, 2023

RalfJung Feb 14, 2023

RalfJung Apr 21, 2023

Implement epoll_wait #2764

Implement epoll_wait #2764

Conversation

DebugSteven commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jan 28, 2023

oli-obk commented Feb 3, 2023

bors commented Feb 3, 2023

bors commented Feb 3, 2023

bors commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 13, 2023 •

edited

Loading

RalfJung Feb 14, 2023 •

edited

Loading

RalfJung Feb 14, 2023 •

edited

Loading