Add support for level-triggered epoll

[WhySoBad]

Hi,

As a basis for implementing a poll shim, this pull request adds support for level-triggered epolls. Besides needing to switch from a ready set to a ready queue, the changes in the epoll shim are relatively small.

To test the functionality of the level-triggered epoll I've added level_triggered and edge_triggered revisions to the epoll libc tests. At the moment I can't come up with any level-triggered edge cases which aren't covered by this testing method.

As mentioned in #4725, the non-blocking epoll tests didn't use the shared check_epoll_wait_noblock helper but used a custom helper instead. I've changed that in the first commit (a tiny bit also slipped through into the second one).

Closes #3448

[rustbot]

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

[WhySoBad]

As far as I can tell, this should also close #3448 (and by extension #602?).

[RalfJung]

Looks good overall. :-)
@rustbot author

View changes since this review

[RalfJung]

The name isn't correct as we only collect level-triggered events here.

What about the following:
in the loop we directly do ready_events.push_back,
and also instead of array_iter we iterate over something like

// We will fill at most the first `ready_events.len()` slots of the array.
// Bounding the iterator this way ensures that we can re-add events
// to the end of the queue during the loop without having them show up in the array.
let mut array_iter = array_iter.take(ready_events.len());

[WhySoBad]

array_iter isn't a real Iterator, so take() doesn't exist. However, array_iter.next() gives a tuple where the first element is the index, so I've now added a check to the while let Some(...) loop with idx < ready_events.len() which achieves the same.

[RalfJung]

I think this can be simplified. You can get rid of got_ordered, emit a panic if position returns None, and after the loop also panic if there's anything left in got.

[WhySoBad]

I initially implemented it this way, however, I think especially for epoll events it would be nice to see the actual output compared to the expected output.
With the suggested implementation, we would also only get a panic for the first missing event which can be annoying to debug when multiple events are missing.

[RalfJung]

You can put whatever details into the panic message that you'd like to have there. But that's not an argument for making the logic more complicated than it has to be.

[RalfJung]

Another implementation strategy could be

• replace &[Ev] by [Ev; N]
• sort both expected and got, and then assert_eq! them

[RalfJung]

As you said there's a footgun here where one makes this N not big enough to get all the events we are expecting... so maybe we should just remove this N and have the function allocate a vec![..., expected.len()+1] to ensure we always know there would have been no more events?

[WhySoBad]

I thought about adding this, however, this test would break because we want to test what happens when we don't read all events:

miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs

Lines 514 to 515 in b4a15e0

	// Two notification should be received. But we only provide buffer for one event.
	check_epoll_wait_noblock::<1>(epfd, &[Ev { events: EPOLLOUT, data: fds[0] }]);

Maybe we want to add and expose a check_epoll_wait_unsafe(epfd, expected, max_events, timeout) and check_epoll_wait and check_epoll_wait_unblock call this method with max_events = expected.len() + 1?

[RalfJung]

I'd call it check_epoll_wait_partial or so.

Maybe it can then also be used for multiple_events_wake_multiple_threads where for some reason we still call epoll_wait directly?

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[RalfJung]

You can put whatever details into the panic message that you'd like to have there. But that's not an argument for making the logic more complicated than it has to be.

[RalfJung]

I'd call it check_epoll_wait_partial or so.

Maybe it can then also be used for multiple_events_wake_multiple_threads where for some reason we still call epoll_wait directly?

[WhySoBad]

I'd propose the following solution to the check_epoll_wait problem:

/// Call `epoll_wait` on `epfd` with the provided `timeout`.
/// It fetches at most `N` events from `epfd` and ensures that
/// the returned events match the `expected` events.
#[track_caller]
pub fn check_epoll_wait_explicit<const N: usize, const M: usize>(
    epfd: i32,
    mut expected: [Ev; M],
    timeout: i32,
) {
    assert!(N >= M, "Max event count should be at least as big as expected event count");
    let mut array: [libc::epoll_event; N] = [libc::epoll_event { events: 0, u64: 0 }; N];
    let num = errno_result(unsafe {
        libc::epoll_wait(epfd, array.as_mut_ptr(), N.try_into().unwrap(), timeout)
    })
    .expect("epoll_wait returned an error");
    let got = &mut array[..num.try_into().unwrap()];
    let mut got = got
        .iter()
        .map(|e| Ev { events: e.events.cast_signed(), data: e.u64.try_into().unwrap() })
        .collect::<Vec<_>>();
    // We sort `expected` and `got` to prevent assertion
    // failures due to wrong ordering.
    expected.sort();
    got.sort();
    assert_eq!(got, expected, "got wrong epoll events")
}

/// Call `epoll_wait` on `epfd` with the provided `timeout` and ensure
/// that the returned events match the `expected` events.
/// This function checks whether `expected` is equal to **all** ready events
/// of `epfd`. If you only want to ensure that `expected` matches a subset
/// of the ready events [`check_epoll_wait_explicit`] should be used instead.
#[track_caller]
pub fn check_epoll_wait<const N: usize>(epfd: i32, expected: [Ev; N], timeout: i32) {
    check_epoll_wait_explicit::<{ N + 1 }, N>(epfd, expected, timeout);
}

/// This does the same as [`check_epoll_wait`] just without blocking (zero `timeout`).
#[track_caller]
pub fn check_epoll_wait_noblock<const N: usize>(epfd: i32, expected: [Ev; N]) {
    check_epoll_wait::<N>(epfd, expected, 0);
}

The problem is that this now changes the function signature; should I still add this change as part of this PR or would you like to have this in a separate PR?

[RalfJung]

I don't think you can do arithmetic in const generics currently, so check_epoll_wait_explicit::<{ N + 1 }, N>(...) doesn't work I think. I'd suggest making N a runtime parameter rather than a const generic.

We can delay the signature change to another PR, but the current got_ordered thing is too confusing to be landed IMO.

And also... given that we now have an explicit queue, I am getting second thoughts about ignoring the order. Apparently order matters, otherwise we wouldn't use a queue. Where do we currently run into issues if we don't ignore the order?

[WhySoBad]

I don't think you can do arithmetic in const generics currently, so check_epoll_wait_explicit::<{ N + 1 }, N>(...) doesn't work I think. I'd suggest making N a runtime parameter rather than a const generic.

Oh yeah, sadly it's a nightly feature.

And also... given that we now have an explicit queue, I am getting second thoughts about ignoring the order. Apparently order matters, otherwise we wouldn't use a queue. Where do we currently run into issues if we don't ignore the order?

The test which previously failed on native hosts now also fails on Miri due to the queue. I've now changed the event order in this test and it now works on Miri as well as native hosts.

@rustbot ready

[RalfJung]

This looks great, thanks! Please squash the commits. You can squash manually if there are multiple independent commits you want to preserve, or use ./miri squash (make sure to pick a suitable commit message). Then write @rustbot ready after you force-pushed the squashed PR.

@rustbot author

[WhySoBad]

@rustbot ready