Skip to content

Add epoll integration for network sockets#4973

Open
WhySoBad wants to merge 13 commits intorust-lang:masterfrom
WhySoBad:network-socket-epoll-new
Open

Add epoll integration for network sockets#4973
WhySoBad wants to merge 13 commits intorust-lang:masterfrom
WhySoBad:network-socket-epoll-new

Conversation

@WhySoBad
Copy link
Copy Markdown
Contributor

Hi,

This pull request adds integration for Miri's epoll shim to the TCP socket shim.

Due to the platform specific differences of mio's poll, the blocking I/O manager had to be refactored to a high degree. Instead of having sources only registered in the poll when we're currently interested in an event, we now have the sources registered in the poll with all available interests for their entire lifespan. We now also store the current readiness for every source in the blocking I/O manager.
When for example a thread should be blocked until some I/O interest is fulfilled on a source, we just add the receiver to the blocking I/O manager without changing anything in the poll itself. In every call to poll we now return all interest receivers whose interests are currently fulfilled -- no longer only those which are newly fulfilled.
Once we hit an EWOULDBLOCK in a source operation (e.g. read, write, accept, ...) we need to falsify the corresponding readiness in the blocking I/O manager to keep it up to date.

With this refactor, the actual epoll implementation is pretty straightforward -- we just need to return the readiness stored in the blocking I/O manager from the epoll_active_events method of host-backed source file descriptions.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 20, 2026

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.

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Apr 20, 2026
@rustbot

This comment has been minimized.

@WhySoBad WhySoBad force-pushed the network-socket-epoll-new branch from 6d96f3e to db7c84b Compare April 23, 2026 13:08
@rustbot

This comment has been minimized.

@RalfJung
Copy link
Copy Markdown
Member

Feedback was given IRL.
@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Apr 27, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 27, 2026

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

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Apr 27, 2026
@WhySoBad
Copy link
Copy Markdown
Contributor Author

I noticed that I wrote the review comments of today's meeting in the PR on my fork, but every of those comments should now be resolved anyways.

The BlockingIoManager:poll now updates the epoll readiness of all sources. I've still kept the InterestReceivers even though we only need it for unblocking threads at the moment (maybe there will be something in the future which also needs to be added to the blocking I/O manager).
I've also renamed the EpollEvents struct to EpollReadiness. This is more in line with the new BlockingIoSourceReadiness struct of the blocking I/O manager.

Also, as you requested, the epoll test cases should now fail/block indefinitely when we forget to register a socket or when we forget to remove the readiness after receiving EWOULDBLOCK.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Apr 27, 2026
@RalfJung
Copy link
Copy Markdown
Member

The BlockingIoManager:poll now updates the epoll readiness of all sources. I've still kept the InterestReceivers even though we only need it for unblocking threads at the moment (maybe there will be something in the future which also needs to be added to the blocking I/O manager).

I prefer clean code over premature generalization. So unless we have at least a concrete idea for what other interest receivers there may be, I think it'd be less confusing to just hard-code for now that these are all blocked threads.

@WhySoBad
Copy link
Copy Markdown
Contributor Author

Okay, so I'll change it that we just register thread id's together with an interest.
The unblocking can then also be done in the poll, what about the deregistering? Should we implicitly deregister the thread id's once we unblocked the thread in the blocking I/O manager (how it's on current master) or should it always happen explicitly in the unblock callback (how it's in this PR)?

@RalfJung
Copy link
Copy Markdown
Member

Both are fine for me.

@WhySoBad
Copy link
Copy Markdown
Contributor Author

Both are fine for me.

I've now implemented it such that we always need to deregister the interests explicitly.

@WhySoBad WhySoBad force-pushed the network-socket-epoll-new branch from 1ee8f6c to 0a49628 Compare April 29, 2026 06:50
@rustbot

This comment has been minimized.

Copy link
Copy Markdown
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, very nice work. :) But I do of course have some comments. ;)

@rustbot author

View changes since this review


pub fn fulfills_interest(&self, interest: &BlockingIoInterest) -> bool {
match interest {
BlockingIoInterest::Read => self.readable || self.error,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some documentation somewhere saying that "error" sources should be "ready" for reads and writes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mio we have this: https://docs.rs/mio/latest/mio/struct.Poll.html#readiness-operations
Should I link this here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not say that read implies error. It says that read may imply error.

I guess this is actually mostly a decision for us -- should we wake up threads on an error signal? That probably makes sense, but then we should explain why.

Comment thread src/concurrency/blocking_io.rs Outdated
Comment thread src/concurrency/blocking_io.rs
Comment thread src/concurrency/blocking_io.rs Outdated
Comment thread src/concurrency/blocking_io.rs
Comment thread tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs
Comment thread tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs
Comment thread tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs Outdated
Comment thread tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs Outdated
Comment thread tests/utils/libc.rs Outdated
@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Apr 30, 2026
@rustbot

This comment has been minimized.

@WhySoBad WhySoBad force-pushed the network-socket-epoll-new branch from ebd1e70 to 1da89ec Compare May 1, 2026 15:13
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 1, 2026

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@WhySoBad
Copy link
Copy Markdown
Contributor Author

WhySoBad commented May 1, 2026

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Waiting for a review to complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants