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

Implement blocking support for eventfd and socketpair #3665

Open
RalfJung opened this issue Jun 10, 2024 · 8 comments
Open

Implement blocking support for eventfd and socketpair #3665

RalfJung opened this issue Jun 10, 2024 · 8 comments
Assignees
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 10, 2024

Our socketpair and eventfd implementations currently do not support blocking.

To fix that will require a bit of re-structuring to enable FileDescription::{read,write} to block. In that case we can't use the convenient bytes: &mut [u8]/bytes: &[u8] arguments, we need the underlying Miri place, so that we can store it in a callback to be used on unblocking. (Incidentally that would also simplify eventfd read/write a bit since we could use the normal memory APIs to reading/writing u64 in an endianess-aware way, rather than having to implement that on a byte slice.) The functions also need access to the return place so they can write the return value later, when they are unblocked.

Basically, a bunch of the convenience that is currently implemented here can't be used when blocking is involved. We have to make read/write lower-level functions that look more like the usual Miri hooks, and possibly add some helper functions to make it still easy to implement the current behavior. (Those helper functions are then likely useful with other file system functions as well.)

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-shims Area: This affects the external function shims E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available labels Jun 10, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Jun 10, 2024

Alternatively we could implement it similar to sleep, but instead of waking on the next instruction, just reexecute the current one.

That does mean we cannot mutate any state before blocking or we'd get incorrect behavior

@RalfJung
Copy link
Member Author

sleep uses the same blocking machinery as everything else. We don't currently use the "re-execute instruction" approach for anything and TBH I think that is a bit too hacky for my taste. For instance if an intrinsic argument is *ptr then we don't want to re-load it from memory each time... though I guess if the value would change that would be a data race so it doesn't actually make a difference?

@adwinwhite
Copy link
Contributor

adwinwhite commented Jun 28, 2024

May I work on this issue? Or it belongs to a larger project? I don't want to block others' work :⁠-⁠)

@tiif
Copy link
Contributor

tiif commented Jun 29, 2024

I think my work on #3448 might potentially block this. The current specification for epoll is only for non-blocking eventfd and socketpair. I wish to let that land then incorporate blocking mechanism to it.

@adwinwhite
Copy link
Contributor

I see. Thanks for letting me know. I'll look for something else then.

@RalfJung
Copy link
Member Author

FWIW we could have blocking socketpair without that being supported by epoll.

But also we currently don't have enough review capacity for another large projects like this. I can't even keep up with the PRs we already have. So picking something smaller is probably better.

@tiif
Copy link
Contributor

tiif commented Aug 10, 2024

I'd like to work on this.

@rustbot claim

@RalfJung
Copy link
Member Author

A good preparation for this would be to change FileDescription::read/write to take a dest: &MPlace<'tcx> rather than returning an io::Result. That's the most conflict-heavy part and can be done in a separate PR.

bors added a commit that referenced this issue Sep 16, 2024
Refactor fd read/write

This PR passed the responsibility of reading to user supplied buffer and dest place to each implementation of ``FileDescription::read/write/pread/pwrite``.

This is part of #3665.
bors added a commit that referenced this issue Sep 22, 2024
Refactor fd read/write

This PR passed the responsibility of reading to user supplied buffer and dest place to each implementation of ``FileDescription::read/write/pread/pwrite``.

This is part of #3665.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available
Projects
None yet
Development

No branches or pull requests

4 participants