libct/utils: drop go:linkname from UnsafeCloseFrom#5252
libct/utils: drop go:linkname from UnsafeCloseFrom#5252kolyshkin wants to merge 1 commit intoopencontainers:mainfrom
Conversation
kolyshkin
force-pushed
the
hall-of-shame
branch
from
f1fa8a5 to
88aa6a1
Compare
|
I'm not quite sure whether this is 1.5 or 1.6 material. On one hand, the change is small and focused and including it into 1.5 will save us half a year. OTOH it could cause some kind of breakage. I am also not very happy about the overhead. |
| func runtime_IsPollDescriptor(fd uintptr) bool //nolint:revive | ||
| func isEpollFd(fd int) bool { | ||
| target, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(fd)) | ||
| return err == nil && strings.HasPrefix(target, "anon_inode:[event") |
There was a problem hiding this comment.
nit: err check can be dropped entirely
|
Honestly I don't really mind being in the Go "hall of shame" for this, FWIW. Yes, epoll fds are not something you can exploit to keep with |
This stems from the discussion at go.dev/issue/67639, which proposed adding a public API to query if an FD is internal to Go. It was rejected and the proposal is to not close epoll FDs instead. Let's do just that. As a result, we can drop the last use of go:linkname and thus runc can be removed from the Go hall of shame (see go.dev/issue/67401 and golang/go@5fc5555feb0). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
hall-of-shame
branch
from
88aa6a1 to
7ba03c3
Compare
|
Quite agree with cyphar. The If we want a clean long-term solution, the right path is to continue pushing Go upstream to expose a public API (like Moreover, this is fundamentally a responsibility issue: these fds are opened by the Go runtime itself, not by user code. The runtime knows exactly which fds it owns -- we don't. It is therefore the runtime's responsibility to expose a public API so that callers can identify and skip those fds (as proposed in golang/go#67639). Asking runc to guess which fds the runtime is using -- via heuristics over |
I agree (and this is why I opened golang/go#67639 in the first place, but Go upstream essentially says "go check fd type before closing it". One other alternative would be to stop go runtime entirely (we're about to exec and don't need any of it). This was suggested here and I guess I can try to propose it. |
Alas this won't work; in case we've closed the netpoll fds, we can "stop the world" (including sysmon which calls netpoll) but we can not "restart" it -- in case execve fails. Since netpoll fds are closed already, it will panic upon restart. So this would be a one-way operation. Not that we can do much in case execve fails; but I doubt this proposal will be accepted. |
3 participants
This stems from the discussion at go.dev/issue/67639, which proposed adding a public API to query if an FD is internal to Go. It was rejected and the proposal is to not close epoll FDs instead.
Let's do just that.
As a result, we can drop the last use of go:linkname and thus runc can be removed from the Go hall of shame (see go.dev/issue/67401 and golang/go@5fc5555feb0).