miri icon indicating copy to clipboard operation
miri copied to clipboard

Implement epoll shim

Open tiif opened this issue 1 year ago • 5 comments

This PR implements non-blocking epoll for #3448 . The design for this PR is documented in https://hackmd.io/@tiif/SJatftrH0 .

tiif avatar Jun 26 '24 02:06 tiif

@rustbot author

tiif avatar Jun 26 '24 15:06 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

tiif avatar Jul 13 '24 13:07 tiif

@rustbot author

RalfJung avatar Jul 22 '24 08:07 RalfJung

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

bors avatar Jul 22 '24 16:07 bors

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 avatar Jul 23 '24 14:07 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 avatar Jul 24 '24 16:07 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.

tiif avatar Jul 24 '24 17:07 tiif

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

oli-obk avatar Jul 25 '24 09:07 oli-obk

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 .

tiif avatar Jul 25 '24 14:07 tiif

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

bors avatar Jul 26 '24 14:07 bors

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.

tiif avatar Jul 27 '24 17:07 tiif

you have some clippy or rustfmt changes to address

oli-obk avatar Jul 28 '24 09:07 oli-obk

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

bors avatar Jul 30 '24 06:07 bors

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

bors avatar Jul 31 '24 09:07 bors

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 avatar Jul 31 '24 18:07 tiif

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

tiif avatar Aug 08 '24 15:08 tiif

Maybe you need to rebase

oli-obk avatar Aug 08 '24 15:08 oli-obk

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
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 - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::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::<alloc[8f32396a97214ba5]::boxed::Box<rustc_driver_impl[bad6c552c0296adc]::install_ice_hook::{closure#0}>>::{closure#0}
  11:     0x74cf3218103f - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::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<T>::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 - <miri::shims::unix::linux::eventfd::Event as miri::shims::unix::fd::FileDescription>::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 - <miri::machine::MiriMachine as rustc_const_eval::interpret::machine::Machine>::find_mir_or_eval_fn::h12f89890355ac47f
                               at /home/byt/Documents/miri/src/machine.rs:984:20
  27:     0x61f340bbd4b5 - rustc_const_eval::interpret::terminator::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::eval_fn_call::h5139575d80e9aa6a
                               at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/terminator.rs:621:46
  28:     0x61f340bfb821 - rustc_const_eval::interpret::terminator::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::eval_terminator::h282a1bcffca2f0eb
                               at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/terminator.rs:133:17
  29:     0x61f340bfb821 - rustc_const_eval::interpret::step::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::terminator::hc66a0ed88e387be6
                               at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/step.rs:383:9
  30:     0x61f340bfb821 - rustc_const_eval::interpret::step::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::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 - <core::panic::unwind_safe::AssertUnwindSafe<F> 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 - <miri::MiriCompilerCalls as rustc_driver_impl::Callbacks>::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<T>::try_with::h821dca7f769b72a2
                               at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/thread/local.rs:283:12
  43:     0x61f340a8e349 - std::thread::local::LocalKey<T>::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 - <miri::MiriCompilerCalls as rustc_driver_impl::Callbacks>::after_analysis::h4dd7be76087912d2
                               at /home/byt/Documents/miri/src/bin/miri.rs:74:9
  48:     0x74cf379f5dce - rustc_interface[2879afccd290b137]::interface::run_compiler::<core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1}
  49:     0x74cf379db709 - std[47b6bda5310033f4]::sys::backtrace::__rust_begin_short_backtrace::<rustc_interface[2879afccd290b137]::util::run_in_thread_with_globals<rustc_interface[2879afccd290b137]::util::run_in_thread_pool_with_globals<rustc_interface[2879afccd290b137]::interface::run_compiler<core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>, 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 - <<std[47b6bda5310033f4]::thread::Builder>::spawn_unchecked_<rustc_interface[2879afccd290b137]::util::run_in_thread_with_globals<rustc_interface[2879afccd290b137]::util::run_in_thread_pool_with_globals<rustc_interface[2879afccd290b137]::interface::run_compiler<core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>, 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 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::hc2643f81a499fd0c
                               at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2148:9
  52:     0x74cf3218afbb - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::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 - <unknown>

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 `<fn() as std::ops::FnOnce<()>>::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::<fn(), ()>` 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::<impl std::ops::FnOnce<()> 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, &dyn std::ops::Fn() -> 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::<isize, {closure@std::rt::lang_start_internal::{closure#2}}>` 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 avatar Aug 09 '24 07:08 tiif

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

tiif avatar Aug 09 '24 21:08 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

tiif avatar Aug 10 '24 16:08 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.

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.

RalfJung avatar Aug 10 '24 17:08 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.

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.

tiif avatar Aug 10 '24 18:08 tiif

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.

RalfJung avatar Aug 10 '24 19:08 RalfJung

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

oli-obk avatar Aug 10 '24 20:08 oli-obk

Okay updated, thanks for helping figure this out!

tiif avatar Aug 11 '24 08:08 tiif

bors r plus

oli-obk avatar Aug 14 '24 13:08 oli-obk

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

It is now in the queue for this repository.

bors avatar Aug 14 '24 13:08 bors

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

bors avatar Aug 14 '24 13:08 bors

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

bors avatar Aug 14 '24 13:08 bors