Don't unconditionally set CLOEXEC twice on every fd we open on Linux #50638
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
tbu-
force-pushed
the
pr_open_cloexec_once
branch
from
cb7da53 to
0754652
Compare
|
Source: Before: After: |
tbu-
changed the title
tbu-
force-pushed
the
pr_open_cloexec_once
branch
from
0754652 to
0a6b1a8
Compare
|
@bors r+ |
|
📌 Commit 0a6b1a8 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- Failed in rollup #50704 (comment). |
bors
added
S-waiting-on-author
kennytm
reviewed
May 13, 2018
src/libstd/sys/unix/fs.rs
Outdated
| @@ -18,6 +18,7 @@ use mem; | |||
| use path::{Path, PathBuf}; | |||
| use ptr; | |||
| use sync::Arc; | |||
| use sync::atomic::{AtomicUsize, Ordering}; | |||
There was a problem hiding this comment.
[00:24:14] error: unused imports: `AtomicUsize`, `Ordering`
[00:24:14] --> libstd/sys/unix/fs.rs:21:20
[00:24:14] |
[00:24:14] 21 | use sync::atomic::{AtomicUsize, Ordering};
[00:24:14] | ^^^^^^^^^^^ ^^^^^^^^
[00:24:14] = note: `-D unused-imports` implied by `-D warnings`
[00:24:14]
[00:24:17] error: aborting due to previous error
0a6b1a8 to
f3956e9
Compare
|
@kennytm Sorry for the compilation failure. Second one in a row due to only checking the Linux build. :( I moved the imports into the function. |
|
@bors r+ |
|
📌 Commit f3956e9 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit f3956e9ff16dc78031dc93c0cf11c275e6cbd1e1 with merge bbd6f38235858820ce7b1fdfe78acf7d064560d0... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
added
S-waiting-on-author
kennytm
reviewed
May 14, 2018
src/libstd/sys/unix/fs.rs
Outdated
| } | ||
|
|
||
| #[cfg(not(target_os = "linux"))] | ||
| fn ensure_cloexec(fd: &FileDesc) -> io::Result<()> { |
There was a problem hiding this comment.
Replace the fd by _ to fix the warning 🤷
Previously, every `open64` was accompanied by a `ioctl(…, FIOCLEX)`, because some old Linux version would ignore the `O_CLOEXEC` flag we pass to the `open64` function. Now, we check whether the `CLOEXEC` flag is set on the first file we open – if it is, we won't do extra syscalls for every opened file. If it is not set, we fall back to the old behavior of unconditionally calling `ioctl(…, FIOCLEX)` on newly opened files. On old Linuxes, this amounts to one extra syscall per process, namely the `fcntl(…, F_GETFD)` call to check the `CLOEXEC` flag. On new Linuxes, this reduces the number of syscalls per opened file by one, except for the first file, where it does the same number of syscalls as before (`fcntl(…, F_GETFD)` to check the flag instead of `ioctl(…, FIOCLEX)` to set it).
tbu-
force-pushed
the
pr_open_cloexec_once
branch
from
f3956e9 to
6d1da82
Compare
|
:( Fixed. |
|
@bors r+ |
|
📌 Commit 6d1da82 has been approved by |
bors
added
S-waiting-on-bors
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
May 14, 2018
Don't unconditionally set CLOEXEC twice on every fd we open on Linux Previously, every `open64` was accompanied by a `ioctl(…, FIOCLEX)`, because some old Linux version would ignore the `O_CLOEXEC` flag we pass to the `open64` function. Now, we check whether the `CLOEXEC` flag is set on the first file we open – if it is, we won't do extra syscalls for every opened file. If it is not set, we fall back to the old behavior of unconditionally calling `ioctl(…, FIOCLEX)` on newly opened files. On old Linuxes, this amounts to one extra syscall per process, namely the `fcntl(…, F_GETFD)` call to check the `CLOEXEC` flag. On new Linuxes, this reduces the number of syscalls per opened file by one, except for the first file, where it does the same number of syscalls as before (`fcntl(…, F_GETFD)` to check the flag instead of `ioctl(…, FIOCLEX)` to set it).
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
May 15, 2018
Don't unconditionally set CLOEXEC twice on every fd we open on Linux Previously, every `open64` was accompanied by a `ioctl(…, FIOCLEX)`, because some old Linux version would ignore the `O_CLOEXEC` flag we pass to the `open64` function. Now, we check whether the `CLOEXEC` flag is set on the first file we open – if it is, we won't do extra syscalls for every opened file. If it is not set, we fall back to the old behavior of unconditionally calling `ioctl(…, FIOCLEX)` on newly opened files. On old Linuxes, this amounts to one extra syscall per process, namely the `fcntl(…, F_GETFD)` call to check the `CLOEXEC` flag. On new Linuxes, this reduces the number of syscalls per opened file by one, except for the first file, where it does the same number of syscalls as before (`fcntl(…, F_GETFD)` to check the flag instead of `ioctl(…, FIOCLEX)` to set it).
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
May 16, 2018
Don't unconditionally set CLOEXEC twice on every fd we open on Linux Previously, every `open64` was accompanied by a `ioctl(…, FIOCLEX)`, because some old Linux version would ignore the `O_CLOEXEC` flag we pass to the `open64` function. Now, we check whether the `CLOEXEC` flag is set on the first file we open – if it is, we won't do extra syscalls for every opened file. If it is not set, we fall back to the old behavior of unconditionally calling `ioctl(…, FIOCLEX)` on newly opened files. On old Linuxes, this amounts to one extra syscall per process, namely the `fcntl(…, F_GETFD)` call to check the `CLOEXEC` flag. On new Linuxes, this reduces the number of syscalls per opened file by one, except for the first file, where it does the same number of syscalls as before (`fcntl(…, F_GETFD)` to check the flag instead of `ioctl(…, FIOCLEX)` to set it).
bors
added a commit
that referenced
this pull request
May 17, 2018
Rollup of 17 pull requests Successful merges: - #50170 (Implement From for more types on Cow) - #50638 (Don't unconditionally set CLOEXEC twice on every fd we open on Linux) - #50656 (Fix `fn main() -> impl Trait` for non-`Termination` trait) - #50669 (rustdoc: deprecate `#![doc(passes, plugins, no_default_passes)]`) - #50726 (read2: Use inner function instead of closure) - #50728 (Fix rustdoc panic with `impl Trait` in type parameters) - #50736 (env: remove unwrap in examples in favor of try op) - #50740 (Remove LazyBTreeMap.) - #50752 (Add missing error codes in libsyntax-ext asm) - #50779 (Make mutable_noalias and arg_align_attributes be tracked) - #50787 (Fix run-make wasm tests) - #50788 (Fix an ICE when casting a nonexistent const) - #50789 (Ensure libraries built in stage0 have unique metadata) - #50793 (tidy: Add a check for empty UI test files) - #50797 (fix a typo in signed-integer::from_str_radix()) - #50808 (Stabilize num::NonZeroU*) - #50809 (GitHub: Stop treating Cargo.lock as a generated file.) Failed merges:
|
Just wondering: how come this isn't implemented using |
|
From the documentation of
The implementation in this PR never blocks, that means that opening of files never blocks on this cached value. As an effect of this, the initialization routine might be called multiple times, but this is no problem. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, every
open64was accompanied by aioctl(…, FIOCLEX),because some old Linux version would ignore the
O_CLOEXECflag we passto the
open64function.Now, we check whether the
CLOEXECflag is set on the first file weopen – if it is, we won't do extra syscalls for every opened file. If it
is not set, we fall back to the old behavior of unconditionally calling
ioctl(…, FIOCLEX)on newly opened files.On old Linuxes, this amounts to one extra syscall per process, namely
the
fcntl(…, F_GETFD)call to check theCLOEXECflag.On new Linuxes, this reduces the number of syscalls per opened file by
one, except for the first file, where it does the same number of
syscalls as before (
fcntl(…, F_GETFD)to check the flag instead ofioctl(…, FIOCLEX)to set it).