Implement epoll shim

[tiif]

This PR:

• implemented non-blocking epoll for #3448 . The design for this PR is documented in https://hackmd.io/@tiif/SJatftrH0 .
• renamed FileDescriptor to FileDescriptionRef
• assigned an ID to every file description

[tiif]

@rustbot author

[tiif]

The overall implementation is done. I will add more documentation and update the design docs in the next few days, and there are stuff that can be done to improve the code, but I appreciate an earlier feedback that potentially involves major changes.

@rustbot ready

[RalfJung]

@rustbot author

[bors]

:umbrella: The latest upstream changes (presumably #3743) made this pull request unmergeable. Please resolve the merge conflicts.

[tiif]

The PR has been squashed because there is a merge conflict in master and rebase through the 100+ commits is too painful.

The current commit structure is:

• First review: the set of commits approved by oli + a few minor fix
• Second review: the commits pushed after Ralf's review
• Add global file description ID for each file description: The changes needed to assign a global file description ID

I will resolve other comments after refactoring as it might not be applicable after the new changes.

[tiif]

@oli-obk The latest commit contains the changes to add a global epoll events map and eliminate epoll_events field. ~It is worth mentioning that currently we still need to add an id field inside the file description because update_readiness needs to know the identity of file description so it can update the correct set of Vec<EpollEvent>.~

Edit: We no longer need to manually add an id field to any file description struct.

[tiif]

Let's move that file description commit to its own PR. Even if we don't use the ids yet (so you can just leave out the id field in fdtable and in the wrapper struct), the change seems good on its own

Sure. I am currently trying to find a way to duplicate the change to a new PR. Since the global epoll event map (specifically this commit ) needs the file description id, I don't think it could be entirely removed from this PR.

[oli-obk]

Since the global epoll event map (specifically this commit ) needs the file description id, I don't think it could be entirely removed from this PR.

yea, epoll and id changes can be kept here. Just remove everything that doesn't make sense in isolation from the other PR and keep just the basic refactoring

my recommendation for doing this is to take this entire PR, duplicate it, git reset --soft HEAD~1 away all the commits and then look at it in your diff viewer and revert all changes that are epoll specific

[tiif]

my recommendation for doing this is to take this entire PR, duplicate it, git reset --soft HEAD~1 away all the commits and then look at it in your diff viewer and revert all changes that are epoll specific

Yup it works wonder! The isolated PR is currently in #3763 .

[bors]

:umbrella: The latest upstream changes (presumably #3766) made this pull request unmergeable. Please resolve the merge conflicts.

[tiif]

I have renamed FileDescriptor to FileDescriptionRef here, but there are still a lot of variables that still being named as file_descriptor. I will open an issue for this refactor after this landed.

[oli-obk]

you have some clippy or rustfmt changes to address

[bors]

:umbrella: The latest upstream changes (presumably #3770) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

:umbrella: The latest upstream changes (presumably #3778) made this pull request unmergeable. Please resolve the merge conflicts.

[tiif]

The majority of the change discussed is completed, but I will do a few more passes to catch some minor errors. So to reflect the current status,

@rustbot ready

[tiif]

Weird... ./miri clippy -- -D warnings works fine locally but failed on CI?

[oli-obk]

Maybe you need to rebase

[tiif]

https://github.com/rust-lang/miri/pull/3712/commits/80d407e80996284a6bc7fda4c4bf8cb729c49412 is an attempt to pass in FileDescriptionRef to check_and_update_readiness, but it ICEs for eventfd test. I haven't fully understood why it happens, but my current guess is it forms a kind of circular reference because we are doing this:

1. FileDescription::read calls check_and_update_readiness
2. Inside check_and_update_readiness, we borrow_mut the file description again.

I am not sure if it is possible to make it work, but I am going to revert this an try to pass FdId to read/write first.

ICE error message

```bash thread 'rustc' panicked at src/shims/unix/fd.rs:236:45: already borrowed: BorrowMutError stack backtrace: 0: 0x74cf3217ddd5 - std::backtrace_rs::backtrace::libunwind::trace::he59335385543b4e7 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/../../backtrace/src/backtrace/libunwind.rs:116:5 1: 0x74cf3217ddd5 - std::backtrace_rs::backtrace::trace_unsynchronized::h7c84e90889d9a1bb at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5 2: 0x74cf3217ddd5 - std::sys::backtrace::_print_fmt::h4d4fd356397c3378 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:66:9 3: 0x74cf3217ddd5 - ::fmt::hde110f186e471219 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:39:26 4: 0x74cf321cecfb - core::fmt::rt::Argument::fmt::h08e1f010bbba14d0 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/fmt/rt.rs:173:76 5: 0x74cf321cecfb - core::fmt::write::hb225859127e7765b at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/fmt/mod.rs:1178:21 6: 0x74cf3217261f - std::io::Write::write_fmt::h210b8145a2e5358b at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/io/mod.rs:1823:15 7: 0x74cf32180671 - std::sys::backtrace::BacktraceLock::print::h2cba555a5ecfba6e at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:42:9 8: 0x74cf32180671 - std::panicking::default_hook::{{closure}}::h908da943d0764776 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:266:22 9: 0x74cf3218034c - std::panicking::default_hook::h78057f076c8a8ad8 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:293:9 10: 0x74cf35644af9 - std[47b6bda5310033f4]::panicking::update_hook::>::{closure#0} 11: 0x74cf3218103f - as core::ops::function::Fn>::call::h0f07d18a9e7ba2b2 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2162:9 12: 0x74cf3218103f - std::panicking::rust_panic_with_hook::h8cf4d1eff41758c5 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:805:13 13: 0x74cf32180c67 - std::panicking::begin_panic_handler::{{closure}}::hc381813fafeb3969 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:671:13 14: 0x74cf3217e299 - std::sys::backtrace::__rust_end_short_backtrace::h688ac9f52e29476a at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:170:18 15: 0x74cf321808f4 - rust_begin_unwind at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:662:5 16: 0x74cf321cb2c3 - core::panicking::panic_fmt::hf0000f31cd37d561 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/panicking.rs:74:14 17: 0x74cf321c8103 - core::cell::panic_already_borrowed::hce2c3af5b8e750d8 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/cell.rs:787:5 18: 0x61f340c419d0 - core::cell::RefCell::borrow_mut::hc8491909e95877c9 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/cell.rs:1078:25 19: 0x61f340c419d0 - miri::shims::unix::fd::FileDescriptionRef::borrow_mut::ha6b56c880ba131fa at /home/byt/Documents/miri/src/shims/unix/fd.rs:236:21 20: 0x61f340c419d0 - miri::shims::unix::linux::epoll::EvalContextExt::check_and_update_readiness::hb3bfc5ed26545b74 at /home/byt/Documents/miri/src/shims/unix/linux/epoll.rs:449:38 21: 0x61f340ce34db - ::read::h95373e54e5d409b5 at /home/byt/Documents/miri/src/shims/unix/linux/eventfd.rs:97:13 22: 0x61f340c2f14e - miri::shims::unix::fd::EvalContextExt::read::h6387a12f7f75f421 at /home/byt/Documents/miri/src/shims/unix/fd.rs:576:21 23: 0x61f340c2481d - miri::shims::unix::foreign_items::EvalContextExt::emulate_foreign_item_inner::hdf8f4f8957eade9c at /home/byt/Documents/miri/src/shims/unix/foreign_items.rs:95:30 24: 0x61f340c6f090 - miri::shims::foreign_items::EvalContextExtPriv::emulate_foreign_item_inner::hf33eb5da0886c6d4 at /home/byt/Documents/miri/src/shims/foreign_items.rs:952:25 25: 0x61f340c6b84a - miri::shims::foreign_items::EvalContextExt::emulate_foreign_item::hb3d83275619b51de at /home/byt/Documents/miri/src/shims/foreign_items.rs:70:15 26: 0x61f340bbd4b5 - ::find_mir_or_eval_fn::h12f89890355ac47f at /home/byt/Documents/miri/src/machine.rs:984:20 27: 0x61f340bbd4b5 - rustc_const_eval::interpret::terminator::>::eval_fn_call::h5139575d80e9aa6a at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/terminator.rs:621:46 28: 0x61f340bfb821 - rustc_const_eval::interpret::terminator::>::eval_terminator::h282a1bcffca2f0eb at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/terminator.rs:133:17 29: 0x61f340bfb821 - rustc_const_eval::interpret::step::>::terminator::hc66a0ed88e387be6 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/step.rs:383:9 30: 0x61f340bfb821 - rustc_const_eval::interpret::step::>::step::h71c560bbe4128a31 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/step.rs:51:9 31: 0x61f340bfb821 - miri::concurrency::thread::EvalContextExt::run_threads::h4b8d54facb86cd82 at /home/byt/Documents/miri/src/concurrency/thread.rs:1197:25 32: 0x61f340b1917e - miri::eval::eval_entry::{{closure}}::h0a868308d84278ae at /home/byt/Documents/miri/src/eval.rs:445:49 33: 0x61f340b1917e - core::ops::function::FnOnce::call_once::h8458d316094a605b at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/ops/function.rs:250:5 34: 0x61f340b1917e - as core::ops::function::FnOnce<()>>::call_once::h6dcd64c25e38427b at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/panic/unwind_safe.rs:272:9 35: 0x61f340b1917e - std::panicking::try::do_call::h2a8bc7bd42ad739a at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:554:40 36: 0x61f340b1917e - std::panicking::try::h3e09c3f57cb7121b at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:518:19 37: 0x61f340b1917e - std::panic::catch_unwind::hd082b45a3f557876 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panic.rs:345:14 38: 0x61f340b1917e - miri::eval::eval_entry::h9606b7c20aa35ad7 at /home/byt/Documents/miri/src/eval.rs:445:9 39: 0x61f340a8e349 - ::after_analysis::{{closure}}::h4ed3975824bb13a7 at /home/byt/Documents/miri/src/bin/miri.rs:114:40 40: 0x61f340a8e349 - rustc_middle::ty::context::GlobalCtxt::enter::{{closure}}::h993e552016cccdcd at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context.rs:1331:37 41: 0x61f340a8e349 - rustc_middle::ty::context::tls::enter_context::{{closure}}::h520a6e8d4a69a77c at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context/tls.rs:82:9 42: 0x61f340a8e349 - std::thread::local::LocalKey::try_with::h821dca7f769b72a2 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/thread/local.rs:283:12 43: 0x61f340a8e349 - std::thread::local::LocalKey::with::h577858e84b543777 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/thread/local.rs:260:9 44: 0x61f340a8e349 - rustc_middle::ty::context::tls::enter_context::h359a4877bd951015 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context/tls.rs:79:9 45: 0x61f340a8e349 - rustc_middle::ty::context::GlobalCtxt::enter::h9aabebea09eac648 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context.rs:1331:9 46: 0x61f340a92b19 - rustc_interface::queries::QueryResult<&rustc_middle::ty::context::GlobalCtxt>::enter::h6667d8429b7307c4 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_interface/src/queries.rs:65:9 47: 0x61f340a92b19 - ::after_analysis::h4dd7be76087912d2 at /home/byt/Documents/miri/src/bin/miri.rs:74:9 48: 0x74cf379f5dce - rustc_interface[2879afccd290b137]::interface::run_compiler::, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1} 49: 0x74cf379db709 - std[47b6bda5310033f4]::sys::backtrace::__rust_begin_short_backtrace::, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>> 50: 0x74cf379db4ba - <::spawn_unchecked_, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#1} as core[88f6a1db45837a48]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0} 51: 0x74cf3218afbb - as core::ops::function::FnOnce>::call_once::hc2643f81a499fd0c at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2148:9 52: 0x74cf3218afbb - as core::ops::function::FnOnce>::call_once::ha8d56f3a1fc33872 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2148:9 53: 0x74cf3218afbb - std::sys::pal::unix::thread::Thread::new::thread_start::h9d7a26a6515c8c04 at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/pal/unix/thread.rs:105:17 54: 0x74cf31e94ac3 - start_thread at ./nptl/pthread_create.c:442:8 55: 0x74cf31f26850 - __GI___clone3 at ./misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 56: 0x0 - error: the compiler unexpectedly panicked. this is a bug. note: we would appreciate a bug report: https://github.com/rust-lang/miri/issues/new note: please make sure that you have updated to the latest nightly note: please attach the file at `/home/byt/Documents/miri/rustc-ice-2024-08-09T07_29_32-2219326.txt` to your bug report query stack during panic: end of query stack Miri caused an ICE during evaluation. Here's the interpreter backtrace at the time of the panic: note: the place in the program where the ICE was triggered --> ./tests/pass-dep/libc/libc-epoll.rs:478:29 | 478 | let res: i32 = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), 8).try_into().unwrap() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: BACKTRACE: = note: inside `test_event_overwrite` at ./tests/pass-dep/libc/libc-epoll.rs:478:29: 478:71 note: inside `main` --> ./tests/pass-dep/libc/libc-epoll.rs:8:5 | 8 | test_event_overwrite(); | ^^^^^^^^^^^^^^^^^^^^^^ = note: inside `>::call_once - shim(fn())` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71 = note: inside `std::sys::backtrace::__rust_begin_short_backtrace::` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/backtrace.rs:154:18: 154:21 = note: inside closure at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:164:18: 164:75 = note: inside `std::ops::function::impls:: for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ops/function.rs:284:13: 284:31 = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:554:40: 554:43 = note: inside `std::panicking::r#try:: i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:518:19: 518:88 = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panic.rs:345:14: 345:33 = note: inside closure at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:143:48: 143:73 = note: inside `std::panicking::r#try::do_call::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:554:40: 554:43 = note: inside `std::panicking::r#try::` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:518:19: 518:88 = note: inside `std::panic::catch_unwind::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panic.rs:345:14: 345:33 = note: inside `std::rt::lang_start_internal` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:143:20: 143:98 = note: inside `std::rt::lang_start::<()>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:163:17: 168:6 error: test failed, to rerun pass `--test ui` ```

[tiif]

All the comments were addressed, this is ready for another round of review. ^^

[tiif]

https://github.com/rust-lang/miri/pull/3712/commits/4812fa670538bf5c0e53fd7bb0b72e02e424b2ec ensures that notification will be provided if read emptied the buffer.

Related discussion is in https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/epoll.20notification.20on.20socketpair.20write.20unblock

[RalfJung]

Do we really want to emulate this behavior? I think epoll totally permits us to wake up these threads even when there is just one byte of space available for writing.

If we do, I think we shouldn't use "empty" as the threshold. More like, if at least 1KiB can be written, or at least half the buffer can be written, or something like that. I wonder what threshold the real implementation uses, but I doubt it is "empty" since that risks starving the readers -- the goal here is presumably to keep a good balance between readers and writers, so "half the buffer" seems like the most naive option to me. This should come with a comment indicating that we do this to avoid waking up threads when they could only write a few bytes.

[tiif]

Do we really want to emulate this behavior? I think epoll totally permits us to wake up these threads even when there is just one byte of space available for writing.

I actually prefer to not emulate this behavior and instead provide epoll notification every time read is called, because as long as socketpair reads more than 0 bytes, it will became writable. For this particular behavior, no matter how we approach it, there will always be cases where our emulation differs from the real system, so we might as well just choose one that makes sense. In my opinion, spuriously waking up threads is better than letting the thread blocks due to lack of notification. But I am not sure if I have missed something—let me know if anyone thinks otherwise.

[RalfJung]

I am completely fine with waking threads up immediately when there's just one byte writeable, even if the real implementation behaves differently for optimization reasons. There should be a comment in the code explaining that.

Not sure if @oli-obk has a preference either way.

[oli-obk]

I too prefer just notifying whenever anything becomes writable/readable, irrespective of the size.

[tiif]

Okay updated, thanks for helping figure this out!

[oli-obk]

[bors]

:pushpin: Commit 5702761073f602033654be6d353215457d645d96 has been approved by oli-obk

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 5702761073f602033654be6d353215457d645d96 with merge cd5d255937739cc1bff06ca88b016d9c8b33cd85...

[bors]

:sunny: Test successful - checks-actions Approved by: oli-obk Pushing cd5d255937739cc1bff06ca88b016d9c8b33cd85 to master...