Skip to content

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 7, 2025

This carries the first commit from #4517, and adds another patch on top of it.

The sole reason is to use pidfd_send_signal under the hood, if that's available.

@kolyshkin kolyshkin requested review from lifubang and rata March 7, 2025 21:17
Because we should switch to unix.PidFDSendSignal in new kernels, it has
been supported in go runtime. We don't need to add fall back to
unix.Kill code here.

Signed-off-by: lifubang <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin force-pushed the pidfd_send_signal branch from c7ba051 to 71f1362 Compare March 7, 2025 22:07
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Just curious, which go version is the min for this to use pidfd?

@kolyshkin
Copy link
Contributor Author

LGTM, thanks!

Just curious, which go version is the min for this to use pidfd?

This is in Go 1.23 (see golang/go#62654 for a high-level overview).

@kolyshkin
Copy link
Contributor Author

LGTM, thanks!
Just curious, which go version is the min for this to use pidfd?

This is in Go 1.23 (see golang/go#62654 for a high-level overview).

The gist of the implementation is in https://go-review.googlesource.com/c/go/+/570036; pidfd will be used when the kernel has all needed support (and it is not blocked by e.g. selinux) -- this means Linux v5.4 or so.

@kolyshkin kolyshkin force-pushed the pidfd_send_signal branch from 71f1362 to 3058ed0 Compare March 8, 2025 00:31
This way, given a recent Go and Linux version, pidfd_send_signal will
be used under the hood.

Keep unix.Signal and unix.SignalName for logging (it is way more
readable than what os.Signal.String() provides).

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin force-pushed the pidfd_send_signal branch from 3058ed0 to 1afa1b8 Compare March 8, 2025 00:48
@lifubang lifubang merged commit 854fb52 into opencontainers:main Mar 8, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants