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

feature(process): Use pidfd on Linux for tokio::process::Child::wait #6152

Merged
merged 17 commits into from
Jan 11, 2024

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Nov 14, 2023

Resolve #6141

Motivation

tokio::process::Child::wait currently uses signal to decide whether or not the child might have exited.
Since signal can be dropped at arbitrary time, any SIGCHLD signal received would cause all tokio::process::Child::wait() future to be awakened and execute std::process::Child::try_wait to decide whether it's ready.

Solution

On Linux, a better solution would be to use pidfd and fallback to waiting on SIGCHLD signal.

Since libstd support for pidfd is still unstable rust-lang/rust#82971, this PR uses pidfd_open in parent to open the pid, this isn't 100% race free but it might be a good start, it will be simple to implement and vfork can continue to be used.

The pidfd returned can be epolled and it will awake only one future when the process has exited, and then std::process::Child::try_wait is called to get the exit status since holding a pidfd prevents the pid from being recycled.

@NobodyXu NobodyXu force-pushed the feature/pidfd branch 3 times, most recently from bd8371b to 92b8831 Compare November 15, 2023 12:05
@NobodyXu
Copy link
Contributor Author

cc @Darksonn @ipetkov

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-process Module: tokio/process labels Nov 24, 2023
tokio/Cargo.toml Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Darksonn commented Dec 3, 2023

So, I think the situation for this PR is as follows: I think this is a good idea, and I would be happy to see it happen. However, right now, I just don't have the bandwidth to take on this PR.

To move forward with this PR, you will need to find a different maintainer who is willing to review it.

Sorry.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Dec 3, 2023

Thank you for reviewing my PR!

I would ping others to review this, or I can wait until someone is free to do this.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Dec 3, 2023

Pinging @taiki-e , can you review this please?
Sorry if I was coming as rude.

fallback to signal-based `Reaper` if pidfd cannot be created.

Signed-off-by: Jiahao XU <[email protected]>
Since miri does not support process spawning

Signed-off-by: Jiahao XU <[email protected]>
Only Linux kernel >= 5.10 support pidfd

Signed-off-by: Jiahao XU <[email protected]>
And fixed `test::is_pidfd_available`

Signed-off-by: Jiahao XU <[email protected]>
To avoid potential headaches such as:

 - The drop being run outside of a Tokio runtime context
 - The runtime being present but not actually running

Since this is the initial implementation, I chose to use the
`OrphanQueue` also used by the signal-driven `Reaper` impl.

It's simple and easy to implement without having to add runtime driver
and guaranteed to work.

Further improvement can be done on the basis of this PR.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Contributor Author

@Darksonn Do you know which maintainer might be willing to pick this PR?
I'm not familiar with tokio maintainers and not sure who's willing to pick this up,

@Darksonn
Copy link
Contributor

Could I get you to open a thread for discussing this in #tokio-dev on our Discord? Preferably after new years so that the others are around to respond.

@NobodyXu
Copy link
Contributor Author

Could I get you to open a thread for discussing this in #tokio-dev on our Discord? Preferably after new years so that the others are around to respond.

Sure I will do that tomorrow.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jan 1, 2024

@Darksonn It seems that there's already a thread there (seems to be opened by you).

Sorry it's my first time trying to open a thread on discord and I thought commenting on tokio-dev would automatically makes a thread.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. I left some smallish comments, but this should be pretty close to mergable.

tokio/src/process/unix/mod.rs Show resolved Hide resolved
tokio/Cargo.toml Outdated Show resolved Hide resolved
Instead of automatically enabling feature `rt` if `process` is enabled,
even if the target os is not `linux`.

Signed-off-by: Jiahao XU <[email protected]>
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM, I'll give @Darksonn an opportunity to provide final thoughts before merging.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Overall looks pretty reasonable. A few comments:

tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved
tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved
Also remove invocation of one `unsafe` function `errno_location`.

Signed-off-by: Jiahao XU <[email protected]>
Move `err.kind() == io::ErrorKind::Other` checking into
`is_rt_shutdown_err`

Signed-off-by: Jiahao XU <[email protected]>
tokio/tests/process_change_of_runtime.rs Outdated Show resolved Hide resolved
tokio/src/process/unix/pidfd_reaper.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you.

@Darksonn Darksonn merged commit e4f9bcb into tokio-rs:master Jan 11, 2024
71 checks passed
@NobodyXu NobodyXu deleted the feature/pidfd branch January 11, 2024 13:00
@sunshowers
Copy link
Contributor

sunshowers commented Feb 4, 2024

Thanks for working on this!

this PR uses pidfd_open in parent to open the pid, this isn't 100% race free but it might be a good start, it will be simple to implement and vfork can continue to be used.

For nextest it would be a pretty big problem if we can't use posix_spawn/vfork -- in some situations it would lead to a performance loss of 100-200% along with some pretty bad race conditions. I see that in rust-lang/rust#82971 there's a discussion about pidfd_spawn, which would be ideal. But if not, it would be nice to not get forced down the fork/exec path, especially on machines that have glibc older than 2.39 (when pidfd_spawn was added).

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Feb 4, 2024

@sunshowers This PR calls pidfd_open after spawning in parent, it doesn't use the unstable pidfd API provided by libstd, so it would continue to use vfork if it originally uses vfork.

This PR would improve Child::wait() performance and also make sure the pid doesn't get recycled given that the child isn't reaped before pidfd_open() is called, this is why I called it racy.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Feb 4, 2024

@sunshowers would be great if you could upgrade nextest to tokio v1.36.0 and see if there's any performance improve on Linux.

@sunshowers
Copy link
Contributor

@sunshowers This PR calls pidfd_open after spawning in parent, it doesn't use the unstable pidfd API provided by libstd, so it would continue to use vfork if it originally uses vfork.

Right, and thanks for that! Just wanted to mention this for future work -- the fallback to pidfd_spawn ideally wouldn't be fork/exec, it would be this implementation.

@sunshowers would be great if you could upgrade nextest to tokio v1.36.0 and see if there's any performance improve on Linux.

Not seeing any perf difference with a standard release build, though I need to do an LTO build to really measure it.

kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Feb 5, 2024
Bumps tokio from 1.35.1 to 1.36.0.

Release notes
Sourced from tokio's releases.

Tokio v1.36.0
1.36.0 (February 2nd, 2024)
Added

io: add tokio::io::Join (#6220)
io: implement AsyncWrite for Empty (#6235)
net: add support for anonymous unix pipes (#6127)
net: add UnixSocket (#6290)
net: expose keepalive option on TcpSocket (#6311)
sync: add {Receiver,UnboundedReceiver}::poll_recv_many (#6236)
sync: add Sender::{try_,}reserve_many (#6205)
sync: add watch::Receiver::mark_unchanged (#6252)
task: add JoinSet::try_join_next (#6280)

Changed

io: make copy cooperative (#6265)
io: make repeat and sink cooperative (#6254)
io: simplify check for empty slice (#6293)
process: use pidfd on Linux when available (#6152)
sync: use AtomicBool in broadcast channel future (#6298)

Documented

io: clarify clear_ready docs (#6304)
net: document that *Fd traits on TcpSocket are unix-only (#6294)
sync: document FIFO behavior of tokio::sync::Mutex (#6279)
chore: typographic improvements (#6262)
runtime: remove obsolete comment (#6303)
task: fix typo (#6261)

#6220: tokio-rs/tokio#6220
#6235: tokio-rs/tokio#6235
#6127: tokio-rs/tokio#6127
#6290: tokio-rs/tokio#6290
#6311: tokio-rs/tokio#6311
#6236: tokio-rs/tokio#6236
#6205: tokio-rs/tokio#6205
#6252: tokio-rs/tokio#6252
#6280: tokio-rs/tokio#6280
#6265: tokio-rs/tokio#6265
#6254: tokio-rs/tokio#6254
#6293: tokio-rs/tokio#6293
#6238: tokio-rs/tokio#6238
#6152: tokio-rs/tokio#6152
#6298: tokio-rs/tokio#6298
#6262: tokio-rs/tokio#6262
#6303: tokio-rs/tokio#6303
#6261: tokio-rs/tokio#6261


... (truncated)


Commits

eaf81ed chore: prepare Tokio v1.36.0 (#6312)
53f9e5a ci: make sure dictionary words are sorted and unique (#6316)
9077762 net: expose keepalive option on TcpSocket (#6311)
131e7b4 ci: add spellchecking (#6297)
e53b92a io: clarify clear_ready docs (#6304)
7536132 sync: use AtomicBool in broadcast channel future (#6298)
b6d0c90 macros: fix trait_method breaking change detection (#6308)
4846959 runtime: remove obsolete comment (#6303)
ec30383 net: add UnixSocket (#6290)
f80bbec io: simplify check for empty slice (#6293)
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-process Module: tokio/process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use pidfd on Linux for tokio::process::Child::wait
5 participants