Use pidfd_spawn for faster process spawning when a PidFd is requested #126827
Conversation
| rustbot has assigned @workingjubilee. Use |
rustbot added O-unix 
This comment has been minimized.
This comment has been minimized.
| // The child has been spawned and we are holding its pidfd. | ||
| // But we cannot obtain its pid even though pidfd_getpid support was verified earlier. | ||
| // This might happen if libc can't open procfs because the file descriptor limit has been reached. | ||
| libc::close(pidfd); | ||
| return Err(Error::new( | ||
| e.kind(), | ||
| "pidfd_spawnp succeeded but the child's PID could not be obtained", | ||
| )); |
There was a problem hiding this comment.
In their unfathomable wisdom the glibc developers have not designed pidfd_spawnp to return the pid and pidfd at the same time even though clone3 does support it, which means we can get into this awkward halfway state.
I'm not sure if this is the best solution for the edge-case. An alternative is to change it to Process { pid: Option<NonZero<pid_t>> } and delay the error reporting by only panicking if someone tries to call Child.id() if the when a pid couldn't be obtained.
There was a problem hiding this comment.
I think for right now this is best because I think assuming calling .id() won't panic is pretty reasonable and this will let us find out if we do in fact have to make that change. At a glance, it seems like a bit of a "it almost-never really happens and there's no reasonable response" scenario, in which case failing fast is better?
| if self.get_create_pidfd() { | ||
| unreachable!("only implemented on linux") | ||
| } |
There was a problem hiding this comment.
Hmm. This currently returns false for all other platforms?
There was a problem hiding this comment.
yes, pidfd is a linux feature
| if #[cfg(target_os = "linux")] { | ||
| weak! { | ||
| fn pidfd_spawnp( | ||
| *mut libc::c_int, | ||
| *const libc::c_char, | ||
| *const libc::posix_spawn_file_actions_t, | ||
| *const libc::posix_spawnattr_t, | ||
| *const *mut libc::c_char, | ||
| *const *mut libc::c_char | ||
| ) -> libc::c_int | ||
| } | ||
| | ||
| weak! { fn pidfd_getpid(libc::c_int) -> libc::c_int } | ||
| | ||
| static PIDFD_SPAWN_SUPPORTED: AtomicU8 = AtomicU8::new(0); | ||
| const UNKNOWN: u8 = 0; | ||
| const YES: u8 = 1; | ||
| // NO currently forces a fallback to fork/exec. We could be more nuanced here and keep using spawn | ||
| // if we know pidfd's aren't supported at all and the fallback would be futile. | ||
| const NO: u8 = 2; | ||
| | ||
| if self.get_create_pidfd() { | ||
| let flag = PIDFD_SPAWN_SUPPORTED.load(Ordering::Relaxed); | ||
| if flag == NO || pidfd_spawnp.get().is_none() || pidfd_getpid.get().is_none() { | ||
| return Ok(None); | ||
| } | ||
| if flag == UNKNOWN { | ||
| let mut support = NO; | ||
| let our_pid = crate::process::id(); | ||
| let pidfd = | ||
| unsafe { libc::syscall(libc::SYS_pidfd_open, our_pid, 0) } as libc::c_int; | ||
| if pidfd >= 0 { | ||
| let pid = unsafe { pidfd_getpid.get().unwrap()(pidfd) } as u32; | ||
| unsafe { libc::close(pidfd) }; | ||
| if pid == our_pid { | ||
| support = YES | ||
| }; | ||
| } | ||
| PIDFD_SPAWN_SUPPORTED.store(support, Ordering::Relaxed); | ||
| if support != YES { | ||
| return Ok(None); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| if self.get_create_pidfd() { | ||
| unreachable!("only implemented on linux") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it possible to outline this code into a support function and just check for and propagate the Ok(None) return?
There was a problem hiding this comment.
The weak symbols are needed later in the same function
There was a problem hiding this comment.
I'll try moving things into a module
There was a problem hiding this comment.
Tried moving it to a module but weak! makes it really unergonomic since it generates lets. Turning things into items requires repeating the function signature several times.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
we now distinguish between pidfd_spawn support, pidfd-via-fork/exec and not-supported
workingjubilee left a comment
workingjubilee left a comment There was a problem hiding this comment.
This looks fine to me.
| // The child has been spawned and we are holding its pidfd. | ||
| // But we cannot obtain its pid even though pidfd_getpid support was verified earlier. | ||
| // This might happen if libc can't open procfs because the file descriptor limit has been reached. | ||
| libc::close(pidfd); | ||
| return Err(Error::new( | ||
| e.kind(), | ||
| "pidfd_spawnp succeeded but the child's PID could not be obtained", | ||
| )); |
There was a problem hiding this comment.
I think for right now this is best because I think assuming calling .id() won't panic is pretty reasonable and this will let us find out if we do in fact have to make that change. At a glance, it seems like a bit of a "it almost-never really happens and there's no reasonable response" scenario, in which case failing fast is better?
| let spawn_res = pidfd_spawnp.get().unwrap()( | ||
| &mut pidfd, | ||
| self.get_program_cstr().as_ptr(), | ||
| file_actions.0.as_ptr(), | ||
| attrs.0.as_ptr(), | ||
| self.get_argv().as_ptr() as *const _, | ||
| envp as *const _, | ||
| ); |
There was a problem hiding this comment.
Ah, I see why they didn't add another argument to pidfd_spawnp: this way it is "exactly the same" arg/ret as posix_spawnp instead of adding another one. They are 1:1 except for "you get a pidfd, not a pid", right?
| } | ||
| _ => {} | ||
| } | ||
| PIDFD_SUPPORTED.store(support, Ordering::Relaxed); |
There was a problem hiding this comment.
Relaxed store because we don't care if we accidentally probe twice in a hypothetical "multiple threads spawn processes with similar Commands" cases, right?
| @bors r+ |
bors added S-waiting-on-bors 
5 participants
glibc 2.39 added
pidfd_spawnpandpidfd_getpidwhich makes it possible to get pidfds while staying on the CLONE_VFORK path.verified that vfork gets used with strace:
Tracking issue: #82971