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
os: make use of pidfd for linux #62654
Comments
Change https://go.dev/cl/528436 mentions this issue: |
I don't think there is an API change here (before optional item 7) so taking this out of the proposal process. CC @golang/runtime |
@ianlancetaylor can you please elaborate? Is this about FindProcess returning ErrProcessGone, or something else? If this is about FindProcess returning an error, as I pointed out in 4 above, this is entirely optional. We can make it succeed every time. |
What I mean is that we only need a proposal for an API change or a significant functionality change. This issue doesn't seem to be either, so we should treat it as an ordinary feature request issue, rather than running it through the proposal process. I hope that makes sense. Thanks. |
Thanks for the explanation, makes sense.I will keep working on implementation. |
Change https://go.dev/cl/528438 mentions this issue: |
Change https://go.dev/cl/528798 mentions this issue: |
I'm a bit unhappy w/ #4: ErrProcessGone could happen on any OS. Just practically only happens on Linux for now. So, developers always need to be prepared for this. |
@golang/runtime could you please take a look at this proposal as well as CLs implementing it? I would appreciate any feedback. |
Hi @kolyshkin, Sorry for the delay. I took a look at your CLs. I'm leaving some feedback here since it is about potential API changes.
|
@prattmic thanks for review comments, I updated the code based on your suggestions (except for the item 3, see below). In the process, I found a bug in my older PidFD implementation (see https://go.dev/cl/542698 for the fix). The series is now ready for (re-)review, I'd be grateful about any feedback. As for using GODEBUG, I feel the overall change is major and might break some setups (in particular, new syscalls are used, which might be blocked on some older systems, and the runtime workarounds may be too complex (see e.g. What do you think? CC @golang/runtime |
I agree, this seems reasonable. |
CL 520266 added pidfd_send_signal linux syscall numbers to the syscall package for the sake of a unit test. As pidfd_send_signal will be used from the os package, let's revert the changes to syscall package, add the pidfd_send_signal syscall numbers and the implementation to internal/syscall/unix, and change the above test to use it. Updates #51246. For #62654. Change-Id: I862174c3c1a64baf1080792bdb3a1c1d1b417bb4 Reviewed-on: https://go-review.googlesource.com/c/go/+/528436 Run-TryBot: Kirill Kolyshkin <kolyshkin@gmail.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com>
@prattmic Implemented as discussed; PTAL https://go.dev/cl/528438 |
Use Process.handle field to store pidfd, and make use of it. Only use pidfd functionality if all the needed syscalls are available. 1. StartProcess: obtain the pidfd from the kernel, if available, using the functionality added by CL 520266. Note we could not modify syscall.StartProcess to return pidfd directly because it is a public API and its callers do not expect it, so we have to use ensurePidfd and getPidfd. 2. (*Process).Kill: use pidfdSendSignal, if the syscall is available and pidfd is known. This is slightly more complicated than it should be, since the syscall can be blocked by e.g. seccomp security policy, therefore the need for a function to check if it's actually working, and a soft fallback to kill. Perhaps this precaution is not really needed. 3. (*Process).Wait: use pidfdWait, if available, otherwise fall back to using waitid/wait4. This is also more complicated than expected due to struct siginfo_t idiosyncrasy. NOTE pidfdSendSignal and pidfdWait are used without a race workaround (blockUntilWaitable and sigMu, added by CL 23967) because with pidfd, PID recycle issue doesn't exist (IOW, pidfd, unlike PID, is guaranteed to refer to one particular process) and thus the race doesn't exist either. For #62654. Updates #13987. Change-Id: I22ebcc7142b16a3a94c422d2f32504d1a80e8a8f Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/528438 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change https://go.dev/cl/566179 mentions this issue: |
Change https://go.dev/cl/566180 mentions this issue: |
Change https://go.dev/cl/566181 mentions this issue: |
Change https://go.dev/cl/566182 mentions this issue: |
Seeing the main CL land in master, I was a bit surprised to not see any GODEBUG support included, given the discussion here. For those reading along like me, it seems like it was discarded in favor of a fallback to the old mechanism, per https://go-review.googlesource.com/c/go/+/528438/comment/12693995_7cc3bdc5/. |
Change https://go.dev/cl/566476 mentions this issue: |
Change https://go.dev/cl/566477 mentions this issue: |
This reverts CL 542699. Reason for revert: Some applications assume FindProcess does not return errors. For #62654. Fixes #65866. Change-Id: Ic185a6253c8e508b08150b618c39a9905f6cdd60 Reviewed-on: https://go-review.googlesource.com/c/go/+/566476 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Michael Pratt <mpratt@google.com>
This reverts CL 528438. Reason for revert: Implicated in "bad FD" test failures. Full extent of issue still unclear. For #62654. Fixes #65857. Change-Id: I066e38040544c506917e90255bd0e330964a0276 Reviewed-on: https://go-review.googlesource.com/c/go/+/566477 Auto-Submit: Michael Pratt <mpratt@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@mvdan Note that CL 542699 (now rolled back) did use a GODEBUG setting for the behavior change in |
Change https://go.dev/cl/570036 mentions this issue: |
Change https://go.dev/cl/570681 mentions this issue: |
Based on a discussion in #51246 (in particular, #51246 (comment)), this is a proposal to implement pidfd support for process methods in the os package -- StartProcess, FindProcess, Kill, Wait.
Reuse the
handle
field ofstruct Process
to keep pidfd. Since0
is a valid value, use^uintptr(0)
as a default value (if pidfd is not known).Instrument
syscall.StartProcess
os.StartProcess
to getpidfd
from the kernel and return it as ahandle
. The initial idea was to modifysyscall.StartProcess
as it returns ahandle
(which we're reusing for pidfd) but that would be a change is not backward-compatible and can result in leaking pidfd as callers are not expecting it.Use pidfdSendSignal from
(*process).Signal
, if handle is known and the syscall is available; fallback to old code usingsyscall.Kill
if syscall is not available (e.g. disabled by seccomp).Use
waitid(P_PIDFD, ...)
in Wait, if pidfd is set; fall back to old code if P_PIDFD is not supported.ErrProcessGone
, and modify the documentation accordingly (adding that it can returnErrProcessGone
on Linux).(*Process).Handle() uintptr
method to return process handle on Windows and pidfd on Linux. This might be useful for low-level operations that require handle/pidfd (e.g. pidfd_getfd on Linux), or to ensure that pidfd (rather than pid) is being used for kill/wait.The text was updated successfully, but these errors were encountered: