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?

[ianlancetaylor]

I guess I don't see why we want or need a recursive pinning function. Callers who need memory pinned recursively can pin each pointer separately. We have to assume that these are rare cases. We don't want to casually pin a complex data structure.

I'm now more concerned about unpinning, as @seebs discusses above. If pinning is needed to pass a pointer to the kernel, then that pointer needs to be kept live and pinned until the kernel no longer needs it. But we have no idea when that is. For cgo code, we define pointers to be pinned when the cgo call starts and unpinned when it ends. But the examples here are not using cgo.

So if can pin a pointer, how do we unpin it?

Any pinned pointer is a potential problem for a future garbage collector implementation.

I'm starting to think that mmap is a better choice after all.

[seebs]

So, I've been thinking about this. I would like to state for the record that recursive pinning is almost certainly an awful idea. My observation that pinning a thing does not imply pinning sub-things is not an argument for recursive pinning, it's (maybe) an argument for having an explicit pin that isn't just "in an argument to cgo".

Thinking about the underlying issue more: This is one of those conversations which is innately premature, but where that's the best time to have it. So far as I know, there are no moving-things GC implementations out there, so we don't know what they're like, or what they will be like, so we are sort of guessing. At the same time, now is when we have to make sure that the specification allows them to exist without breaking things.

For KeepAlive, it's reasonably practical to express "this object is still in use up through here, don't mark it as free." The object is obviously alive before we try to do that; we only need to specify the end-of-lifetime. For pinning, we need a way to express "as of now, this object's address is known to something outside runtime, so it needs to stay at that address", and later a way to express "the thing outside runtime is no longer using that address".

Honestly, while I'm not sure about mmap in particular, I think it may make sense to make the rule be "if you need a thing pinned for a duration longer than the duration of a single cgo call that it's a parameter to, you have to do a special thing when allocating it, and then that thing just stays pinned its entire life".

Rationale: This replaces some kind of elaborate pin/unpun mechanism with the already well understood object lifetime rules, and lets KeepAlive do its thing the way it already does that thing; it just adds a category to the GC for "regions that we allocated, but which we can't move". In short, instead of runtime.Pin(uintptr/unsafe.Pointer/whatever), maybe it should be runtime.Pinned(???) ([]byte, error). And the obvious thing to put in there for parameters would be "a type", but it turns out I don't think we can do that. (To do that, it'd have to be a magic builtin, like make, and I think this is too specialized a case for that to be a good choice.)

At the risk of it being possibly inefficient, it might make sense to use the same idiom as unsafe.Sizeof(); you give it an object and somehow this works and produces storage suitable to hold that object. (Possibly with some way to force stricter alignment; some things that share memory are planning to do things using SIMD instructions, for instance, which may have extra requirements.)

So basically, I think the original proposal's idea is wrong: KeepAlive shouldn't imply pinning. Pinning should be a distinct operation which applies for the lifetime of an object we anticipate being shared, and if you're not sure which objects get shared, you probably copy them into a pinned object before sharing them. Clearly, whoever wrote that original proposal had not thought about this enough.

[RLH]

There is tension between languages with shared mutable state and enforcing recursive constraints such as pinning. If a list is recursively pinned then any pointer write to one of the nodes in the list would have to pin the referent. Furthermore two linked list can share a node, thus making unpinning tricky since a node that is reachable from multiple paths can’t be unpinned unless all paths are considered. Sprinkle in atomicity and what recursive pinning means in a shared mutable state language and any implementation becomes expensive and fragile.

…

On Sun, Aug 18, 2019 at 2:26 PM seebs ***@***.***> wrote: So, I've been thinking about this. I would like to state for the argument that recursive pinning is almost certainly an *awful* idea. My observation that pinning a thing does not imply pinning sub-things is not an argument for recursive pinning, it's (maybe) an argument for having an explicit pin that isn't just "in an argument to cgo". Thinking about the underlying issue more: This is one of those conversations which is innately premature, but where that's the best time to have it. So far as I know, there are no moving-things GC implementations out there, so we don't know what they're like, or what they will be like, so we are sort of guessing. At the same time, now is when we have to make sure that the specification *allows* them to exist without breaking things. For KeepAlive, it's reasonably practical to express "this object is still in use up through here, don't mark it as free." The object is obviously alive before we try to do that; we only need to specify the end-of-lifetime. For pinning, we need a way to express "as of now, this object's address is known to something outside runtime, so it needs to stay at that address", and later a way to express "the thing outside runtime is no longer using that address". Honestly, while I'm not sure about mmap in particular, I think it may make sense to make the rule be "if you need a thing pinned for a duration longer than the duration of a single cgo call that it's a parameter to, you have to do a special thing when allocating it, and then that thing just stays pinned its entire life". Rationale: This replaces some kind of elaborate pin/unpun mechanism with the *already well understood* object lifetime rules, and lets KeepAlive do its thing the way it already does that thing; it just adds a category to the GC for "regions that we allocated, but which we can't move". In short, instead of runtime.Pin(uintptr/unsafe.Pointer/whatever), maybe it should be runtime.Pinned(???) ([]byte, error). And the obvious thing to put in there for parameters would be "a type", but it turns out I don't think we can do that. (To do that, it'd have to be a magic builtin, like make, and I think this is too specialized a case for that to be a good choice.) At the risk of it being possibly inefficient, it might make sense to use the same idiom as unsafe.Sizeof(); you give it an object and somehow this works and produces storage suitable to hold that object. (Possibly with some way to force stricter alignment; some things that share memory are planning to do things using SIMD instructions, for instance, which may have extra requirements.) So basically, I think the original proposal's idea is wrong: KeepAlive shouldn't imply pinning. Pinning should be a distinct operation which applies for the lifetime of an object we anticipate being shared, and if you're not sure which objects get shared, you probably copy them into a pinned object before sharing them. Clearly, whoever wrote that original proposal had not thought about this enough. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#32115?email_source=notifications&email_token=AAHNNH4HK3RCKE6D5UYXIZ3QFGH5TA5CNFSM4HNXDUOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4RFNCI#issuecomment-522344073>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHNNHZ7UY5OSEJ33F6XIYDQFGH5TANCNFSM4HNXDUOA> .

[rsc]

It sounds to me like @seebs is recanting this proposal and maybe at some point in the future might want to make a different one (on a new issue)?

[seebs]

I'd sorta dislike losing the history, but I think it would make more sense to edit it or alter it to reflect the observation that KeepAlive doing pinning is probably wrong. Basically, I don't think my original idea was good, but this discussion still has some potentially-useful annotations on why pinning is an issue at all.

[rsc]

Based on the discussion above, it seems like this is a likely decline.
Leaving open for a week for final comments.

The issue will still be searchable for finding these thoughts in any future pinning discussion.

[rsc]

No change in consensus, so declined.