proposal: runtime: KeepAlive should perhaps keep things from being moved

[seebs]

What version of Go are you using (go version)?

N/A

What did you do?

This comes out of some discussion about ioctl syscalls on golang-nuts:

https://groups.google.com/forum/#!topic/golang-nuts/FfasFTZvU_o

Basically: In some cases, there are things that might be pointer-like, and which need to be passed to a syscall. But worse, there are cases where a syscall takes a parameter which is then interpreted by the kernel as a structure containing a pointer.

It seems to me that even if the pointer is in a go structure which the runtime knows contains a pointer (and would thus, say, be able to update to point to the right new location if the pointer needed to be moved), it's not as clear that the runtime will be fully aware of the implications of a structure-containing-a-pointer for purposes of the extra promises made around arguments to syscalls, and the insane API of ioctl makes it very likely that people will want to be able to do this with data that is insufficiently-clearly known to the runtime to be a pointer. (Sadly, a complete fix requires a time machine.)

What we sort of end up wanting is the ability to say not just "don't deallocate this", but "don't deallocate this or move it".

The more I think about this, the more I think this might be a reasonable additional requirement to express with runtime.KeepAlive. It's already supposed to keep pointer values from being invalidated, by hinting to the runtime that they aren't done being used even if it seems as though they should be done being used. Possibly making that hint stronger, and explicitly including protection against being moved around by a future compacting-type implementation, would be enough.

I was originally planning to write this as a proposal for a new runtime.KeepAround or something that would have these semantics, but looking at the cases where KeepAlive is currently useful, it seems to me that in most of these cases, we would actually be very sad if the object were moved to a new location at some unspecified time before the KeepAlive, but not deallocated; in practice, KeepAlive means "I am doing unspecified black magic which involves the belief that this address points to this data, please don't yank this rug out from under me until here."

[ianlancetaylor]

If we do this, I would lean toward a separate function. One of the main uses I see of runtime.KeepAlive is when working with a memory allocated using cgo that is freed by a finalizer set by runtime.SetFinalizer. This looks something like

type Wrapper struct {
    cptr *C.CType
}

func New() *Wrapper {
    cp := C.AllocateType()
    w := &Wrapper{cp}
    runtime.SetFinalizer(cp, Release)
    return w
}

func Release(w *Wrapper) {
    C.FreeType(w.cptr)
    w.cptr = nil
}

func DoSomething(w *Wrapper) {
    C.DoSomething(w.cptr)
    runtime.KeepAlive(w)
}

Here the runtime.KeepAlive is required because otherwise the Go object might be freed, and the finalizer run, and the C object free, all while running inside C.DoSomething. But there is no reason that w can't be moved. Clearly w.cptr can't be moved, but that won't happen because it isn't a Go pointer anyhow.

[seebs]

Oh, good point, I'd forgotten about the Finalizer use case for KeepAlive. So, yeah, it makes sense to distinguish those. So imagining that we had a thing like this, only it were for some reason expressed in the struct as a Go pointer, to a Go object, but we still wanted the "don't move it" behavior... I think I'd probably be expecting something like runtime.KeepAround(w.cptr), I guess?

This might also be relevant/applicable to cases where the formal boundaries of what has to happen in a single line of code (involving unsafe.Pointer, or uintptr, or Syscall) make it hard to express something clearly, and we want a way to mark clearly that we need a thing to stay put.

[ianlancetaylor]

One name for what you are describing is pinning the pointer, so runtime.Pin would be a possibility.

One consideration is that pinned pointers limit what the GC can do. The current GC does not move heap pointers (stack pointers are moved in some cases) but we don't necessarily want to restrict future GC work. If programs can make arbitrary calls to runtime.Pin, that can arbitrarily restrict the GC.

One of the compromises of the cgo pointer passing discussion was to agree that while clearly pointers passed to cgo have to be pinned for the duration of the call, we can limit the number of such pointers. Specifically, as discussed at https://github.com/golang/proposal/blob/master/design/12416-cgo-pointers.md, we can have a known fixed number of pinned pointers, basically the number of concurrent cgo calls times the number of pointers passed to those calls. runtime.Pin would remove that limit.

[seebs]

Hmm. Part of the problem is that KeepAlive doesn't care about when the thing starts being alive, so you don't have to think about when it becomes-alive. By contrast, Pin actually does need to know when to start pinning, not just where to end it. And you can't necessarily assume that the pinning ends when the call does; you might need enough time to go delving back into some weird union-representation thing to find and extract a pointer, to know which pointer you're reading from. But you also want to be sure that these definitely get released.

So as a starting point: Possibly a Pin should be inherently limited to "until the end of the scope in which it is called". (Or possibly the function? It could be like an implicit defer runtime.Unpin(p), I guess.)

But that still means that anything doing this has to be treated comparably to a cgo call. They could consume some finite resource (like a semaphore) but this seems like it creates opportunities for deadlock, if two cgo calls have a dependency (invisible to go) such that one of them has to at least begin before the other can complete, but the pinning surrounding the first one can prevent the second from happening. (Which I guess would be theoretically possible with the current implementation, but much more likely if there's more pinning going on, and some of the pinning lasts longer than the cgo calls.)

On the other hand, "you have to use mmap" seems awful, and makes it harder to analyze things clearly; knowing that a thing is a pinned pointer, and when it was pinned, and when it becomes unpinned, is a lot more desireable than having a gazillion mmapped pointers floating around some of which are actually pinned, and some of which don't really care.

On second thought, maybe we should just build the time machine and prevent bad APIs from being implemented in the first place.

[rsc]

Why do we need to pin anything? I looked at the code snippet here and it looks like it does the right thing without any custom functions.

Specifically, here is the code:

	ifgrs := make([]wgh.Ifgreq, ifg.Len/sizeofIfgreq)
	*(*uintptr)(unsafe.Pointer(&ifg.Ifgru[0])) = uintptr(unsafe.Pointer(&ifgrs[0]))

	// Now actually fetch the device names.
	if err := ioctl(c.fd, wgh.SIOCGIFGMEMB, unsafe.Pointer(&ifg)); err != nil {
		return nil, err
	}

	// Keep this alive until we're done doing the ioctl dance.
	runtime.KeepAlive(&ifg)

Passing ifg to ioctl should keep it pinned already. It's unclear what more is needed here. We should answer that question - what more is needed in the code? - before designing a new mechanism.

[seebs]

Is &ifgrs[0] pinned? Sure, ifg is pinned. But I don't know whether pinning is recursive; nothing I've seen immediately makes me think it must be. And the type of ifg.Ifgru isn't pointer-like -- so even if pinning is recursive on pointer types, it might not catch that one.

Unfortunately, in the absence of an existing implementation which habitually moves heap-allocated things, it's hard to be entirely sure what the actual bounds of pinning/not-pinning really are. We can't easily test to see whether things get moved, or could in principle get moved in a future implementation.

[mdlayher]

Is &ifgrs[0] pinned?

Yep, this is my question as well.

I was able to create a workable solution with Cgo as well, but I also came up with an alternative based on some further insight from @ianlancetaylor on the golang-nuts thread:

It would be OK to pass a pointer to a struct to ioctl if the struct contains a pointer to other Go memory, but the struct field must have pointer type.

To do this, I defined my own C structure that replaces the union with a pointer to another type: https://github.com/WireGuard/wgctrl-go/blob/5b564c0ffc6585320e444fdfebae8d545ac50133/internal/wgopenbsd/internal/wgh/defs.go#L14-L24.

This produces code which now has a Go struct field with pointer type, versus a byte array: https://github.com/WireGuard/wgctrl-go/blob/5b564c0ffc6585320e444fdfebae8d545ac50133/internal/wgopenbsd/internal/wgh/defs_openbsd_amd64.go#L14-L24.

And finally, I'm able to use that new structure and store a Go pointer within that field: https://github.com/WireGuard/wgctrl-go/blob/5b564c0ffc6585320e444fdfebae8d545ac50133/internal/wgopenbsd/client_openbsd.go#L78-L88.

As far as I can tell, this follows the rule that Ian mentioned and I was able to eliminate the need to allocate memory with C.malloc and similar.

This seems to work as intended and IMO is much clearer than the workaround I was doing before to shove a pointer into the union byte array, but it could still perhaps be useful to have a pinning API for cases like this.

[rsc]

Re my earlier comment, it's true - implicit cgo pinning of function arguments is not recursive. I misspoke.

[rsc]

@ianlancetaylor, @thanm, @cherrymui, @aclements, what are your thoughts about whether we want to support a recursive pinning operation? Obviously that would be a no-op for the main toolchain but it might be expensive in gccgo/gollvm.

[cherrymui]

Pinning operation means telling the runtime not to move it? Currently both gccgo and gollvm do not move anything (for stacks, it uses split stack). If in the future it gets a moving GC, presumably the implementation would be similar to the main toolchain so I guess the pinning operation would be similar as well?