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

Complete and fix our eventfd implementation #3445

Closed
RalfJung opened this issue Apr 3, 2024 · 8 comments · Fixed by #3650
Closed

Complete and fix our eventfd implementation #3445

RalfJung opened this issue Apr 3, 2024 · 8 comments · Fixed by #3650
Assignees
Labels
A-files Area: related to files, paths, sockets, file descriptors, or handles A-shims Area: This affects the external function shims C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps 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 Apr 3, 2024

eventfd is documented reasonably well in its man page. The current implementation in Miri is incomplete, doesn't have tests (it is only indirect exercised via the tokio MVP test but that's not a good way to test a specific low-level API), and is partially wrong.

It is incomplete in that read is not implemented, which means they can't actually be used to do anything.
That's also where EFD_NONBLOCK would come in.

It is partially wrong in that

  • write panics in all sorts of situations that should be handled gracefully
  • dup creates a new independent event object, rather than a second FD that refers to the same event object (this got fixed in the mean time)

Work on this should IMO follow the proposed "project" process.

@RalfJung RalfJung added the A-shims Area: This affects the external function shims label Apr 3, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Apr 3, 2024

Also the check here is bogus:

if flags & (efd_cloexec | efd_nonblock | efd_semaphore) == 0 {
throw_unsup_format!("{flags} is unsupported");
}

That should be flags & !(efd_cloexec | efd_nonblock | efd_semaphore) != 0.

EDIT: but turns out we have the flags tokio needs.

@RalfJung RalfJung added C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps E-good-first-issue A good way to start contributing, mentoring is available labels Apr 4, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2024

dup creates a new independent event object, rather than a second FD that refers to the same event object

This should probably be fixed by implementing a proper notion of "file description", and making the file descriptor table a simple indirection for file descriptions. That would fix dup for all FDs once and for all, and it will anyway be needed for epoll.

@m-rph
Copy link

m-rph commented Apr 5, 2024

Hey @RalfJung I would like to give this a shot. May I just @rustbot claim it?

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2024

Please coordinate with @oli-obk -- I saw something about a potential GSoC project for epoll support, and I assume this would be part of it.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 5, 2024

cc @tiif

There's probably some parallel development that can be done, but the file descriptor table refactor is likely gonna block other work, so the earlier it is done the better.

@m-rph
Copy link

m-rph commented Apr 5, 2024

I think it's probably for the best if I pick something simpler and isolated to start with.
Thank you!

@tiif
Copy link
Contributor

tiif commented May 5, 2024

@rustbot claim

@RalfJung RalfJung added E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available and removed E-good-first-issue A good way to start contributing, mentoring is available labels May 5, 2024
@RalfJung RalfJung added the A-files Area: related to files, paths, sockets, file descriptors, or handles label May 5, 2024
@tiif
Copy link
Contributor

tiif commented Jun 2, 2024

This hackmd contains the details for eventfd implementation: https://hackmd.io/@tiif/rk9hlmP4R

@tiif tiif mentioned this issue Jun 6, 2024
@bors bors closed this as completed in c51b733 Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-files Area: related to files, paths, sockets, file descriptors, or handles A-shims Area: This affects the external function shims C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps 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

Successfully merging a pull request may close this issue.

4 participants