Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable Miri weak memory emulation for deque tests #829

Closed
wants to merge 1 commit into from

Conversation

cbeuw
Copy link
Contributor

@cbeuw cbeuw commented May 16, 2022

(Don't merge yet) Once rust-lang/miri#1963 is merged into Miri to support weak memory emulation, crossbeam-deque/tests/injector.rs::mpmc() test will hang forever on Miri CI. This PR disables weak memory emulation for deque tests to prevent this. This is why we need to do this:

Miri README says

Threading support is not finished yet. E.g., weak memory effects are not emulated and spin loops (without syscalls) just loop forever. There is no threading support on Windows.

This is because Miri's scheduler is not pre-emptive. It runs the current thread until it terminates or yield_now() is called explicitly.

The test has a spin loop without yielding. When run natively it relies on the OS to have a pre-emptive scheduler:

loop {
if let Success(n) = q.steal() {
v[n].fetch_add(1, SeqCst);
break;
}
}

This currently passes due to the fact that all the pushing threads run and terminate before any stealing threads gets to run. .steal() never returns anything other than success.

However, if a pushing thread's push() call causes it to yield, then Miri could end up in a stealing thread. If this happens multiple times then Miri could land in a stealing spin loop with nothing in the queue since there aren't enough successful pushes. The test ends up hanging.

push() could yield the thread by a compare exchange failure:

match self.tail.index.compare_exchange_weak(
tail,
new_tail,
Ordering::SeqCst,
Ordering::Acquire,
) {
Ok(_) => unsafe {
// If we've reached the end of the block, install the next one.
if offset + 1 == BLOCK_CAP {
let next_block = Box::into_raw(next_block.unwrap());
let next_index = new_tail.wrapping_add(1 << SHIFT);
self.tail.block.store(next_block, Ordering::Release);
self.tail.index.store(next_index, Ordering::Release);
(*block).next.store(next_block, Ordering::Release);
}
// Write the task into the slot.
let slot = (*block).slots.get_unchecked(offset);
slot.task.get().write(MaybeUninit::new(task));
slot.state.fetch_or(WRITE, Ordering::Release);
return;
},
Err(t) => {
tail = t;
block = self.tail.block.load(Ordering::Acquire);
backoff.spin();
}
}

Without weak memory behaviour or thread interleaving, the previously loaded tail value is always the most recent one which is what the compare exchange will observe, so the only way it could fail is spuriously. This is why currently we turned off spurious failure, and the thread never yields:

MIRIFLAGS="-Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-ignore-leaks -Zmiri-compare-exchange-weak-failure-rate=1.0" \

(yes, -Zmiri-compare-exchange-weak-failure-rate=1.0 implies that the exchange always fails. However it actually means never failing because a comparison in Miri was written backwards. This will be fixed rust-lang/miri#2105)

However, once weak memory is added, tail doesn't have to be the most recent value, meaning the compare exchange could fail and the pushing thread yields, landing us in a stealing spin loop.

let mut tail = self.tail.index.load(Ordering::Acquire);

The best way to solve this for Miri to have a pre-emptive scheduler. But to keep the CI green this workaround is fine.

@RalfJung
Copy link
Contributor

The test has a spin loop without yielding.

What about adding a yield call under cfg(miri) in that loop?
That's how we fixed the standard library test suite to work with Miri.

@cbeuw
Copy link
Contributor Author

cbeuw commented May 17, 2022

Ah yes I forgot about cfg. That's indeed a better way to do this, and it's already been used in crossbeam https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-queue/tests/array_queue.rs

Though in std we used std::hint::spin_loop() instead of std::thread::yield_now(). I didn't know the hint has effect on Miri's scheduler. I feel like that's better than yield_now(), and it has been stabilised since rust-lang/miri#1388 was opened

@RalfJung
Copy link
Contributor

They should both have the same effect on Miri.

bors bot added a commit that referenced this pull request May 19, 2022
831: Add spin loop hints in tests for Miri r=taiki-e a=cbeuw

This is a better way to do #829 

Miri does not have a pre-emptive scheduler, so once the execution falls into a spin loop it'll hang forever: rust-lang/miri#1388

Similar measures (`yield_now()`) are already present in [some other tests](https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-queue/tests/array_queue.rs), but it's missing here

832: Fix links to macro rules in docs r=taiki-e a=alygin

The fix provides explicit links to macro rules in docs because they [are not resolved globally anymore](rust-lang/rust#96676).


Co-authored-by: Andy Wang <cbeuw.andy@gmail.com>
Co-authored-by: Andrew Lygin <alygin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants