Initial support for poll-based backend #1687

jasta · 2023-07-11T03:49:26Z

With mio_unsupported_force_poll_poll, mio will use the legacy poll(2) syscall instead of epoll or kqueue. While this is not intended to replace either of those more modern and efficient alternatives, it is a suitable avenue to enable a lot of other somewhat more obscure platforms such as the esp32 family or Haiku OS. esp-idf support is my primary purpose and I believe the esp-rs community can get behind this as a way to better interop with a large number of Tokio-based async crates out there, but this PR is intended only to provide the barebones technical support for now with minimal invasion into existing production code paths.

This work is largely based off #1602 with improvements to fix tests and provide a more consistent implementation. Specific changes include:

Closed fds are now automatically removed as is the case with the Windows implementation
Waker is now a pass through for the internal waker that the poll Selector needs
Haiku and esp-idf specific changes were removed. IMO, those should be introduced as part of separate PRs for each respective platform.

Closes #1604

tests/tcp_stream.rs

jasta · 2023-07-11T04:02:38Z

tests/unix_stream.rs

@@ -451,6 +451,8 @@ where

    assert!(stream.take_error().unwrap().is_none());

+    assert_would_block(stream.read(&mut buf));


This is the same behaviour that tcp_stream has and is functionally important for the edge triggered emulation to work. Basically if you only read exactly the data you need then nothing would trigger the EAGAIN case and the fd wouldn't be re-registered with POLLIN. Technically this doesn't feel like a big issue in practice though since in any real world application you'd have to read all available buffered data up to EAGAIN to know for sure that subsequent calls to epoll would actually tell you if data was ready. There could be contrived and very bizarre usages though that would break...

jasta · 2023-07-11T04:05:46Z

Also, I wonder if maybe the WakerRegistrar thing would be better served by copying the kqueue design pattern whereby the Selector provides factory-type methods to create and use a waker. I didn't notice that it already is working pretty similarly to what I need until just now...

jasta · 2023-07-11T21:23:17Z

Indeed, I think I found a much cleaner way to detangle Waker behaviour from the Selector so that we can have the concept of a WakerDriver (a low-level description of how to call eventfd, pipe, pipe2, or kevent) vs a Waker which is created by the selector and introduces some customization of the WakerDriver behaviour based on the selector's quirks. For example, the poll selector can use either eventfd or pipe2 WakerDriver, but the Waker it creates requires reregistering the interest with poll before wake can be called. The epoll selector can use either eventfd or pipe2 WakerDriver, but the Waker it creates has a no-op before each call to wake.

This seems to elegantly resolve one existing gotcha and two that I created:

pipe::Waker::wake no longer needs an illumos-specific cfg check. Instead, the epoll selector will have that check since it is epoll-specific behaviour on that platform that's the issue.
I can drop my WakerRegistrar as I can now have the poll selector directly control reregistration of whatever WakerDriver is used
The poll selector won't need another eventfd/pipe impl living in side the poll selector for the purposes of internally waking a blocking poll call. We can just use the WakerDriver internally.

This also fixes the FreeBSD failing tests (which are actually just because WakerRegistrar is unused in the kqueue path)

Thomasdezeeuw · 2023-07-12T12:54:03Z

I don't really have a lot of time to review a pr this big. Can you maybe remove the changes to the waker to reduce the size of the pr? The pipe implementation should work on any Unix right?

Thomasdezeeuw · 2023-07-12T12:58:56Z

src/sys/unix/selector/adapters/level_triggered/io_source_state.rs

+use std::sync::Arc;
+use crate::sys::unix::selector::poll::RegistrationRecord;
+
+struct InternalState {


This is a lot of state per fd.

The wasi implementation is based of Posix poll (Wasi's poll_oneoff) and doesn't need to hold any state, but requires proper (de)registering, can we follow that implementation more closely?

This is basically a copy of how the Windows impl does it, and was the same in #1602. I don't think the wasi approach will work, and actually I'm not really sure the wasi implementation is even correct. It's certainly missing a lot of features and doesn't behave the same as epoll/kqueue. The poll behaviour in this PR is trying to be as close as possible to a correct epoll emulation with only one leak that I'm aware of (see the change to tcp_stream test)

jasta · 2023-07-12T18:04:33Z

@Thomasdezeeuw some important context here is that I didn't change much from #1602 which was already pretty heavily reviewed and discussed. The main changes are mentioned in the PR summary, and all changes were done directly because of legitimately failing tests (i.e. unintended changes in behaviour).

While I admit I took an approach that moves around a lot more code than is needed, the reason I did this was two fold:

The way Waker and Selector interact with each other for poll vs epoll is pretty significant and this would require an ugly hack like this (and it would be copy/pasted for eventfd and pipe):

pub struct Waker {
  selector: Option<Selector>,
  ...
}
impl Waker {
  pub fn new(selector: &Selector, token: Token) -> io::Result<Self> {
    ...
    cfg_poll_selector! {
      Ok(Waker { selector: Some(selector.try_clone()?, token, fd: file })
    }
    cfg_epoll_selector! {
      Ok(Waker { selector: None, token, fd: file })
    }
  }
  pub fn wake(&self) -> io::Result<()> {
    cfg_poll_selector! {
      self.selector.unwrap().reregister(self.fd.as_raw_fd(), self.token, Interest::READABLE)?;
    }
    ...
  }

The poll-based selector needs an internal Waker (calling register needs to interrupt poll ofc), so this creates a tear where you can have a totally separate waker behaviour for when register is called vs when you use the public Waker API. For example the original PR used pipe with a blocking write-side fd for the internal register waker, but used eventfd for the Waker API. Dropping the refactor will mean that this inconsistency must remain and I'd argue this lowers confidence that the Linux-based tests are even a good proxy for the platforms we're trying to support. One of the main sticking points identified in Poll selector #1602 (comment)

Happy to proceed with whatever you recommend however, though if we drop the refactor I could use some help finding a cleaner way to do (1) or at least an understanding that it's acceptable to not do it cleanly :\

Also yes, in theory the poll+pipe implementation should work on any UNIX, I'm right now working with a FreeBSD VM to try to get that to build and work nicely but it currently does not.

jasta · 2023-07-12T18:12:40Z

Oh and also I could break up the refactor into a separate PR so it's easier to swallow and confirm that it's correct without introducing the poll backend.

jasta · 2023-07-12T19:13:26Z

I was wrong re my statement that any UNIX should be able to support the poll+pipe backend. While technically true that it will "work", it's hopeless to get all current tests to pass because libc::POLLRDHUP is Linux-specific and without it tests like tests/tcp.rs:write_shutdown will fail. There are about a dozen tests like this. This does mean that mio still works of course, just that shutdown(Read/Write) will not behave as expected and the caller will need to detect these shutdown events by trying to read or write.

jasta · 2023-07-12T19:32:03Z

Also while I'm waiting for review it looks like you can kick the CI tests so I can confirm everything is working correctly. Cirrus is running, but not GitHub Actions.

Thomasdezeeuw · 2023-07-17T09:24:20Z

I wanted to review this in the weekend, but I didn't. I think this still has a lot of changes that are not strictly necessary for poll(2) support. From the changes to IoSourceState (can be contained to src/sys/unix/poll.rs), to the macros (not required), but also the waker (this should just be an pipe/eventfd fd with IoSourceState). Also unnecessary name changes Waker to WakerDriver?

Maybe I have time next weekend, but not promises, I do want to reduce the number of changes though. I really like to see a minimal set of changes to implement this.

jasta · 2023-07-18T00:39:18Z

From the changes to IoSourceState (can be contained to src/sys/unix/poll.rs)

Ackd, I'll avoid breaking things out into separate files.

...to the macros (not required)

I only did this because the original PR reviewer asked for it because the PR will require copy/pasting the cfg criteria quite a few times. I can drop it though, but see my comments below so we're on the same page as to how many times this will need to get repeated.

...but also the waker (this should just be an pipe/eventfd fd with IoSourceState)

I'm not sure how this can be achieved. The critical difference between epoll vs poll is that for poll you either need to read the pipe/eventfd when it wakes (what the original PR did) or you need to reregister the interests again before calling wake (what my PR did). In either case, there needs to be code that allows the poll Selector to interact with Waker internals. IoSourceState is also but separately interacting with Selector internals.

Fwiw the reason I opted for a different strategy was so that I didn't need to deepen the circular relationship between Selector and Waker but could rather describe the dependencies as such:

Selector -> Waker -> WakerDriver
Waker -> Selector

Also unnecessary name changes Waker to WakerDriver?

There wasn't a rename here as such, but rather WakerDriver was added to be used by Waker. This is because as I mentioned above in a previous comment, we need to use pipe or eventfd to internally wake poll() when register() is called. But Waker as written right now calls register in the constructor making it a bit fragile to reuse internally. So I stripped WakerDriver away which is just a pure side effect free API and then added Waker back inside the Selector with the register side effect. This allowed the poll Selector to internally use WakerDriver and this neither duplicated code nor, as the original PR did, unintentionally use pipe for the internal register() wakeup case but eventfd for the Waker case.

Maybe I have time next weekend, but not promises, I do want to reduce the number of changes though. I really like to see a minimal set of changes to implement this.

Understood, I'm willing to rework the PR with reduced churn, it's just not 100% clear where the tradeoffs should be. See my comment above about how clumsy it gets to try to conditionally call reregister() inside wake() only for the poll Selector case. If that clumsiness is preferred, that's what I'll do.

It is, I admit, less intrusive to use the original PR's strategy for reading the pipe/eventfd on wake tho since you only need to add new constructor APIs that don't do the register (e.g. new_internal), then add a new clear() API that resets them as needed. I'll play around with this approach some more and see if I can't get it to look a little less clumsy than the original PR had it.

jasta · 2023-07-18T03:05:33Z

Also wanted to thank you for taking the time to review and discuss. I know this poll support is probably weighing on you as a reviewer a bit after so much churn and iteration. This will be such a huge help for the esp32 community once we're able to get it over the line though!

Thomasdezeeuw · 2023-07-18T10:29:44Z

...to the macros (not required)

I only did this because the original PR reviewer asked for it because the PR will require copy/pasting the cfg criteria quite a few times. I can drop it though, but see my comments below so we're on the same page as to how many times this will need to get repeated.

I'm personally a bit torn on the macros. For things like cfg_net they help, but for the selector I'm no so sure, I think the code/cfgs is easier to follow without th emacro.

...but also the waker (this should just be an pipe/eventfd fd with IoSourceState)

I'm not sure how this can be achieved. The critical difference between epoll vs poll is that for poll you either need to read the pipe/eventfd when it wakes (what the original PR did) or you need to reregister the interests again before calling wake (what my PR did). In either case, there needs to be code that allows the poll Selector to interact with Waker internals. IoSourceState is also but separately interacting with Selector internals.

I was thinking an eventfd/pipe who's fd is always passed to poll (assuming the waker was added), preferably in a fixed position in the pollfds. Then we always check the waker's pollfd and drain it's readiness if we get an event (e.g. read from the pipe). For the Waker we can't use the kqueue implementation as it has no fd, but the eventfd and pipe should continue working as normal, I would expect. The only change in the Waker code would be a special call to register, something like selector.register_waker, which for poll(2) would mean to duplicate the fd and use it as described above. For epoll we can just call register.register (as we currently do).

Fwiw the reason I opted for a different strategy was so that I didn't need to deepen the circular relationship between Selector and Waker but could rather describe the dependencies as such:
Selector -> Waker -> WakerDriver
Waker -> Selector
Also unnecessary name changes Waker to WakerDriver?

There wasn't a rename here as such, but rather WakerDriver was added to be used by Waker. This is because as I mentioned above in a previous comment, we need to use pipe or eventfd to internally wake poll() when register() is called. But Waker as written right now calls register in the constructor making it a bit fragile to reuse internally. So I stripped WakerDriver away which is just a pure side effect free API and then added Waker back inside the Selector with the register side effect. This allowed the poll Selector to internally use WakerDriver and this neither duplicated code nor, as the original PR did, unintentionally use pipe for the internal register() wakeup case but eventfd for the Waker case.

That makes more sense. If my idea describe above doesn't work we can go with something like this. But I'm not sure about the "driver" name, but we can bikeshed this later.

Maybe I have time next weekend, but not promises, I do want to reduce the number of changes though. I really like to see a minimal set of changes to implement this.

Understood, I'm willing to rework the PR with reduced churn, it's just not 100% clear where the tradeoffs should be. See my comment above about how clumsy it gets to try to conditionally call reregister() inside wake() only for the poll Selector case. If that clumsiness is preferred, that's what I'll do.

See above, but I prefer to limit the scope of this pr, affecting the minimal amount of non-poll(2) code would be ideal. That makes it easier to review and less risky to merge as other code isn't affected.

It is, I admit, less intrusive to use the original PR's strategy for reading the pipe/eventfd on wake tho since you only need to add new constructor APIs that don't do the register (e.g. new_internal), then add a new clear() API that resets them as needed. I'll play around with this approach some more and see if I can't get it to look a little less clumsy than the original PR had it.

Also wanted to thank you for taking the time to review and discuss. I know this poll support is probably weighing on you as a reviewer a bit after so much churn and iteration. This will be such a huge help for the esp32 community once we're able to get it over the line though!

I appreciate this. I very much want to help the esp32 community, but my main priority is to keep the existing code working and maintainable (for my own sanity).

jasta · 2023-07-18T18:01:31Z

I was thinking an eventfd/pipe who's fd is always passed to poll (assuming the waker was added), preferably in a fixed position in the pollfds. Then we always check the waker's pollfd and drain it's readiness if we get an event (e.g. read from the pipe). For the Waker we can't use the kqueue implementation as it has no fd, but the eventfd and pipe should continue working as normal, I would expect. The only change in the Waker code would be a special call to register, something like selector.register_waker, which for poll(2) would mean to duplicate the fd and use it as described above. For epoll we can just call register.register (as we currently do).

Looking into more detail here and this is basically how the original PR (#1602) worked but it needed unsafe handling of the waker RawFd and a change to all sys::Selector impls. I also realized that your suggestion to dup the fd won't work directly directly because esp32 doesn't implement dup but it can work indirectly by just internally re-using the waker we know the poll-impl already has:

#[cfg(mio_unsupported_force_poll_poll)]
mod poll {
  pub struct Waker {
    selector: sys::Selector,
  }

  impl Waker {
    pub fn new(selector: &Selector, token: Token) -> io::Result<Waker> {
      Ok(Waker { selector: selector.try_clone()? })
    }

    pub fn wake(&self) -> io::Result<()> {
      // Since this is the poll-only impl, the Selector would need only to call self.notify_waker.wake() and everything would
      // just work the exact same way it does when you call Selector::register.
      select.selector.wake_internal()
    }
  }
}

This would then save on an fd and make the code path far, far clearer how it's being used. So, the only real issue then is how can we get the poll impl to re-use the code we have in eventfd::Waker and pipe::Waker. I'd propose that once again the WakerDriver idea helps us here (pending a rename perhaps):

Expose pub(crate) versions of eventfd::Waker and pipe::Waker as a new type (for argument let's say WakerImpl) that doesn't interact with Selector at all (like what my PR did with WakerDriver). This can be a simple rename mostly, but unlike my PR today will not include other Waker implementations and is only meant to be an internal sys/unix utility to re-use eventfd/poll impl code.
Convert the eventfd::Waker and pipe::Waker types to be simple pass-throughs to eventfd::WakerImpl and pipe::WakerImpl with the extra register behaviour on top.
Add a new poll::Waker as written above that is a simple pass-through to poll::Selector::wake_internal.

Thoughts? I'm working on this locally now in parallel but I want to be respectful of your suggestions and not blindside you with changes to the PR that aren't quite like we discussed already.

Introduces a new backend for UNIX using the level triggered poll() syscall instead of epoll or kqueue. This support is crucial for embedded systems like the esp32 family but also for alternative operating systems like Haiku. This diff does not introduce any new platform support targets itself but provides the core technical implementation necessary to support these other targets. Future PRs will introduce specific platform support however due to reasons outlined in tokio-rs#1602 (many thanks for this initial effort BTW!) it is not possible to automate tests for those platforms. We will instead rely on the fact that Linux can serve as a proxy to prove that the mio code is working nominally. Note that only Linux has a sufficiently complex implementation to pass all tests. This is due to SIGRDHUP missing on other platforms and is required for about a dozen or so tests that check is_read_closed().

jasta · 2023-07-18T20:56:01Z

Force pushed a reworked version of the PR that I think is much cleaner based on your feedback and on my comment above. Heads up that there is a functional change here to re-use the internal Waker which both simplifies the implementation and makes it much clearer why we had to slightly refactor eventfd::Waker and pipe::Waker.

jasta · 2023-07-25T17:00:25Z

Silly mistake had the CI checks not running, now that they're up and failing I'll go ahead and fix.

EDIT: I see now you have to manually run them each time I push commits. It is possible to relax this in the repo's Settings under Actions -> General, Require approval for first-time contributors who are new to GitHub. Up to you ofc if you want to enable this.

Co-authored-by: Thomas de Zeeuw <[email protected]>

…g for an expired timeout

jasta · 2023-07-26T19:45:46Z

Applied most of the suggested changes (see comments above on the others) and confirmed tests still pass. If you prefer I can squash all commits to prepare to merge. Also once again, huge thanks for your time and patience on this PR. I know it's big and I really appreciate your attention to detail.

Co-authored-by: Thomas de Zeeuw <[email protected]>

jasta · 2023-07-30T19:29:21Z

Ok applied a few more suggestions and left a tracking issue for the optimizations. Again, I can squash all these commits if you prefer, just lmk how you want to proceed to merge.

jasta · 2023-07-30T19:37:02Z

Just caught an issue from CI that needs to be addressed, working on fixing up a bad assumption that IoSourceState in sys/shell/mod could assume non-Windows was UNIX which breaks for wasm32-wasi

Thomasdezeeuw · 2023-07-31T12:28:03Z

Thanks @jasta for pulling this over the finish line!

jasta · 2023-07-31T15:35:03Z

Thanks @jasta for pulling this over the finish line!

Don't you mean polling? Har har har :) But really a big thanks to you as well for taking the time and effort to make sure the crates you maintain continue to meet a high quality bar. The rust community is lucky to have you!

jasta commented Jul 11, 2023

View reviewed changes

tests/tcp_stream.rs Outdated Show resolved Hide resolved

jasta commented Jul 11, 2023

View reviewed changes

jasta requested a review from carllerche as a code owner July 12, 2023 02:48

Thomasdezeeuw reviewed Jul 12, 2023

View reviewed changes

jasta mentioned this pull request Jul 12, 2023

error when compiling mio ivmarkov/rust-esp32-std-demo#9

Closed

This was referenced Jul 13, 2023

Possibly fragile coordination between mio and tokio for Windows event-triggered simulation tokio-rs/tokio#5866

Closed

Support ESP-IDF framework (esp32 family of hardware devices with std support) tokio-rs/tokio#5867

Closed

jasta force-pushed the poll2 branch from dda0f69 to a6edda7 Compare July 18, 2023 20:53

This was referenced Jul 18, 2023

Add ESP-IDF framework support #1691

Closed

Add ESP-IDF framework support #1692

Merged

jasta requested a review from Thomasdezeeuw July 19, 2023 08:16

Fixed syntax error in ci workflow

496bc0e

jasta added 3 commits July 25, 2023 10:17

Fix build break with --no-default-features --features net

2889557

Fix mio_unsupported_force_poll_poll release builds

1813a63

Fix non-conforming usage of log::trace

cfc13bc

jasta and others added 7 commits July 26, 2023 12:24

poll: Slight change to error message consistency

037a538

Co-authored-by: Thomas de Zeeuw <[email protected]>

poll: Fix event copy/paste debug_details formatting

5fda03c

Co-authored-by: Thomas de Zeeuw <[email protected]>

poll: Manually fix missing rustfmt case

f7d0885

Co-authored-by: Thomas de Zeeuw <[email protected]>

poll: Fix try_clone not carrying has_waker state forward

a4c72a8

poll: Simplify and optimize pending_wake_tokens to use a singular Token

1de715c

poll: No longer special case poll returning 0 and redundantly checkin…

0ce239e

…g for an expired timeout

poll: Copy timeout determination logic from epoll impl

e15d449

jasta and others added 4 commits July 26, 2023 12:50

poll: Clarify nested poll loop behaviour slightly

c61d221

Co-authored-by: Thomas de Zeeuw <[email protected]>

poll: Further clarify nested poll loop behaviour

fa88f19

Co-authored-by: Thomas de Zeeuw <[email protected]>

poll: rename notified_events for consistency

185dbf5

poll: Minor documentation for pending_wake_token behaviour

71c635f

This was referenced Jul 27, 2023

Poll selector #1602

Closed

Has waker check doesn't work with try_cloned Registries #1698

Closed

jasta added 2 commits July 30, 2023 12:16

poll: Document design choice quirks in comments

0970313

poll: Avoid early return on error in deregister_all

fc0a153

jasta mentioned this pull request Jul 30, 2023

poll: Optimize locks and allocations in the critical path #1701

Open

poll: Rerun rustfmt

8570ec9

jasta added 2 commits July 30, 2023 12:42

poll: Fix target_os="wasi" compilation error

189a117

poll: Fix MSRV by not using std::os::fd

a7c41ee

jasta requested a review from Thomasdezeeuw July 30, 2023 22:34

Thomasdezeeuw approved these changes Jul 31, 2023

View reviewed changes

Thomasdezeeuw merged commit 60a86ea into tokio-rs:master Jul 31, 2023

This was referenced Jul 31, 2023

Poll(2) implementation follow-ups #1702

Open

Support AIX operating system #1620

Closed

Thomasdezeeuw mentioned this pull request Aug 4, 2023

Reduce allocations in poll(2) implementation #1705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for poll-based backend #1687

Initial support for poll-based backend #1687

jasta commented Jul 11, 2023 •

edited

Loading

jasta Jul 11, 2023

jasta commented Jul 11, 2023

jasta commented Jul 11, 2023

Thomasdezeeuw commented Jul 12, 2023

Thomasdezeeuw Jul 12, 2023

jasta Jul 12, 2023 •

edited

Loading

jasta commented Jul 12, 2023 •

edited

Loading

jasta commented Jul 12, 2023

jasta commented Jul 12, 2023

jasta commented Jul 12, 2023

Thomasdezeeuw commented Jul 17, 2023

jasta commented Jul 18, 2023

jasta commented Jul 18, 2023

Thomasdezeeuw commented Jul 18, 2023

jasta commented Jul 18, 2023

jasta commented Jul 18, 2023

jasta commented Jul 25, 2023 •

edited

Loading

jasta commented Jul 26, 2023 •

edited

Loading

jasta commented Jul 30, 2023

jasta commented Jul 30, 2023

Thomasdezeeuw commented Jul 31, 2023

jasta commented Jul 31, 2023

		@@ -451,6 +451,8 @@ where

		assert!(stream.take_error().unwrap().is_none());

		assert_would_block(stream.read(&mut buf));

Initial support for poll-based backend #1687

Initial support for poll-based backend #1687

Conversation

jasta commented Jul 11, 2023 • edited Loading

jasta Jul 11, 2023

Choose a reason for hiding this comment

jasta commented Jul 11, 2023

jasta commented Jul 11, 2023

Thomasdezeeuw commented Jul 12, 2023

Thomasdezeeuw Jul 12, 2023

Choose a reason for hiding this comment

jasta Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

jasta commented Jul 12, 2023 • edited Loading

jasta commented Jul 12, 2023

jasta commented Jul 12, 2023

jasta commented Jul 12, 2023

Thomasdezeeuw commented Jul 17, 2023

jasta commented Jul 18, 2023

jasta commented Jul 18, 2023

Thomasdezeeuw commented Jul 18, 2023

jasta commented Jul 18, 2023

jasta commented Jul 18, 2023

jasta commented Jul 25, 2023 • edited Loading

jasta commented Jul 26, 2023 • edited Loading

jasta commented Jul 30, 2023

jasta commented Jul 30, 2023

Thomasdezeeuw commented Jul 31, 2023

jasta commented Jul 31, 2023

jasta commented Jul 11, 2023 •

edited

Loading

jasta Jul 12, 2023 •

edited

Loading

jasta commented Jul 12, 2023 •

edited

Loading

jasta commented Jul 25, 2023 •

edited

Loading

jasta commented Jul 26, 2023 •

edited

Loading