Repository: ceph/go-ceph
Pull request #435 (2021-01-15 12:56:34)

cutil: add PtrGuard for storing Go pointers in C memory
PtrGuard is a type that allows to store a Go pointer (that is a pointer pointing to memory allocated by the Go runtime) in C memory (that is memory allocated by C, with malloc() for example). The pointer passing rules of cgo don't allow Go code to store Go pointer in C memory. However, there is a compiler directive for functions that tells the GC not to touch a pointer stored in a uintptr argument until the function returns. PtrGuard takes advantage of this by calling such a function in a Goroutine that stores the uintptr containing the Go pointer in the provided C memory and then waits for a release signal (implemented via a mutex). After the release signal is received, the C memory is overwritten with NULL and the function returns, which makes the pointer accessible to the GC again.

Spin-off from #419 
Depends on #428 

## Checklist
- [X] Added tests for features and functional changes
- [X] Public functions and types are documented
- [X] Standard formatting is applied to Go code


ansiwen: Setting do-not-merge label, until dependencies got merged.
phlogistonjohn: > This looks good to me, but I have one doubt about it. The use of a go-routine per pointer (iiuc?) seems a lot. The routine would be blocked/sleeping, but depending on the implementation in Golang, it may still take up a (Linux kernel) task. Especially in container environments, the number of available tasks are limited.
> 
> Maybe my concerns are not too relevant, so a clarification would help me :)

@ansiwen and I discussed this over voice, but it wasn't broadcast: We plan on merging both pointer guard and a copy based implementation and allow switching between them with build tags. Once the code is in place we will be able to write and run benchmarks. We can see the perf vs. memory vs. threads/stack (?) tradeoffs there and that will help inform our future default choices. In the short term we'll default to copying.
@ansiwen please chime in if I missed anything.
phlogistonjohn: The above is why I often miss mailing lists. :-)
ansiwen: > @ansiwen and I discussed this over voice, but it wasn't broadcast: We plan on merging both pointer guard and a copy based implementation and allow switching between them with build tags. Once the code is in place we will be able to write and run benchmarks. We can see the perf vs. memory vs. threads/stack (?) tradeoffs there and that will help inform our future default choices. In the short term we'll default to copying.
> @ansiwen please chime in if I missed anything.

@phlogistonjohn I think you didn't miss anything. The followup is already there and waiting for reviews: #441 :wink: 
ansiwen: > This looks good to me, but I have one doubt about it. The use of a go-routine per pointer (iiuc?) seems a lot. The routine would be blocked/sleeping, but depending on the implementation in Golang, it may still take up a (Linux kernel) task. Especially in container environments, the number of available tasks are limited.
> 
> Maybe my concerns are not too relevant, so a clarification would help me :)

@nixpanic Goroutines are especially designed to be very cheap. When designing them they had in mind that a service handles every single request or message in it's own goroutine (maybe inspired by the erlang processes). They are entirely managed by the runtime and magnitudes cheaper than threads. Here is a quote from an article that I just quickly found:

> Even if tens of thousands of goroutines have been spawned, it’s not a waste of system resources if most of them are blocked on one of these since the runtime schedules another goroutine instead.
> 
> In simple terms, goroutines are a lightweight abstraction over threads. A Go programmer does not deal with threads, and similarly the OS is not aware of the existence of goroutines. From the OS’s perspective, a Go program will behave like an event-driven C program.

https://blog.nindalf.com/posts/how-goroutines-work/

There _could_ be an impact, because we are calling C functions and blocking inside them, however, the blocking itself happens with Go primitives again, so I assume there is still no thread created. I will check that to make sure.


ansiwen: @nixpanic Ok, kudos for your comment, as it made me double check and realize, that Cgo indeed creates one thread per Goroutine! Wow, what a pi** on my parade... :disappointed: Unfortunately we can't avoid that, because the whole point is _not_ returning from the C function. It might still be no problem, but we have to be aware of that for sure. And more motivation to do that without the silly C call.

So, I would suggest that we still merge that to get some insight, and at the same time I will try to find a way to do it without the Cgo call. (I looked into that already before and might see a chance.)
phlogistonjohn: > @nixpanic Ok, kudos for your comment, as it made me double check and realize, that Cgo indeed creates one thread per Goroutine! (As soon as I directly call the blocking go function, no threads are created.) Wow, what a pi** on my parade... disappointed Unfortunately we can't avoid that, because the whole point is _not_ returning from the C function. It might still be no problem, but we have to be aware of that for sure. And more motivation to do that without the silly C call.

Oh, I had already assumed you had considered that. That's why when we spoke I wanted to benchmark it because we trade off memory copies versus goroutines (threads, stack, etc) when we use ptrguard. It may be worth the trade off. This is also one reason I want to eventually raise the issue with the go maintainers because it would be nice to have the pointer guard behavior w/o these resources used.

> 
> So, I would suggest that we still merge that to get some insight, and at the same time I will try to find a way to do it without the Cgo call. (I looked into that already before and might see a chance.)

Indeed. Let's land it, default to copies and build up a decent body of data that shows us where the costs falls hardest and use that information to help make the next set of decisions.
ansiwen: > Oh, I had already assumed you had considered that. That's why when we spoke I wanted to benchmark it because we trade off memory copies versus goroutines (threads, stack, etc) when we use ptrguard. It may be worth the trade off. This is also one reason I want to eventually raise the issue with the go maintainers because it would be nice to have the pointer guard behavior w/o these resources used.

Yes, I considered it, that's why I called it _double_ check. I didn't come to the right conclusions, when I discovered there is a thread limit of 10000 (as I told you). Somehow I didn't realize, that this should be goroutines, and that this means that too many threads are actually created.

But the real good news is, that since we spoke last time, I discovered a pure Go method to "pin" a Go pointer, so that the GC doesn't touch it for a while. And as it turned out it's quite easy to use. See `//go:uintptrescapes` in the [docs](https://golang.org/cmd/compile/#hdr-Compiler_Directives) and the source code comment [here](https://github.com/golang/go/blob/d2d155d1ae8c704a37f42fd3ebb1f3846f78e4d4/src/cmd/compile/doc.go#L216-L223).
ansiwen: > Seems OK from the docs, etc. But looking thru the go codebase I see the directive is used twice!
> It makes me a tad nervous to be poking at these esoteric things. But, so as long as we don't enable this by default I'm still good with landing it and then building up a good base of experience and perf tests around it later.

Hmm, I think you missed some uses. IIUC it was first only used internally in syscall/dll_windows.go, but now it is used as well in syscall/syscall_linux.go and - yes - runtime/cgocall.go, which means we are using it anyway already and it's not more esoteric than cgo itself. :wink:  Also you can see that it's tested quite extensively.
ansiwen: @nixpanic I'm kindly asking for your re-review at your earliest convenience. :wink: 
phlogistonjohn: @nixpanic ping.
ansiwen: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 


Patch:
diff --git a/internal/cutil/ptrguard.go b/internal/cutil/ptrguard.go
new file mode 100644
index 0000000..545e0c0
--- /dev/null
+++ b/internal/cutil/ptrguard.go
@@ -0,0 +1,79 @@
+package cutil
+
+import (
+	"sync"
+	"unsafe"
+)
+
+// PtrGuard respresents a guarded Go pointer (pointing to memory allocated by Go
+// runtime) stored in C memory (allocated by C)
+type PtrGuard struct {
+	// These mutexes will be used as binary semaphores for signalling events from
+	// one thread to another, which - in contrast to other languages like C++ - is
+	// possible in Go, that is a Mutex can be locked in one thread and unlocked in
+	// another.
+	stored, release sync.Mutex
+	released        bool
+}
+
+// WARNING: using binary semaphores (mutexes) for signalling like this is quite
+// a delicate task in order to avoid deadlocks or panics. Whenever changing the
+// code logic, please review at least three times that there is no unexpected
+// state possible. Usually the natural choice would be to use channels instead,
+// but these can not easily passed to C code because of the pointer-to-pointer
+// cgo rule, and would require the use of a Go object registry.
+
+// NewPtrGuard writes the goPtr (pointing to Go memory) into C memory at the
+// position cPtr, and returns a PtrGuard object.
+func NewPtrGuard(cPtr CPtr, goPtr unsafe.Pointer) *PtrGuard {
+	var v PtrGuard
+	// Since the mutexes are used for signalling, they have to be initialized to
+	// locked state, so that following lock attempts will block.
+	v.release.Lock()
+	v.stored.Lock()
+	// Start a background go routine that lives until Release is called. This
+	// calls a special function that makes sure the garbage collector doesn't touch
+	// goPtr, stores it into C memory at position cPtr and then waits until it
+	// reveices the "release" signal, after which it nulls out the C memory at
+	// cPtr and then exits.
+	go func() {
+		storeUntilRelease(&v, (*CPtr)(cPtr), uintptr(goPtr))
+	}()
+	// Wait for the "stored" signal from the go routine when the Go pointer has
+	// been stored to the C memory. <--(1)
+	v.stored.Lock()
+	return &v
+}
+
+// Release removes the guarded Go pointer from the C memory by overwriting it
+// with NULL.
+func (v *PtrGuard) Release() {
+	if !v.released {
+		v.released = true
+		v.release.Unlock() // Send the "release" signal to the go routine. -->(2)
+		v.stored.Lock()    // Wait for the second "stored" signal when the C memory
+		//                    has been nulled out. <--(3)
+
+	}
+}
+
+//go:uintptrescapes
+
+// From https://golang.org/src/cmd/compile/internal/gc/lex.go:
+// For the next function declared in the file any uintptr arguments may be
+// pointer values converted to uintptr. This directive ensures that the
+// referenced allocated object, if any, is retained and not moved until the call
+// completes, even though from the types alone it would appear that the object
+// is no longer needed during the call. The conversion to uintptr must appear in
+// the argument list.
+// Also see https://golang.org/cmd/compile/#hdr-Compiler_Directives
+
+func storeUntilRelease(v *PtrGuard, cPtr *CPtr, goPtr uintptr) {
+	uip := (*uintptr)(unsafe.Pointer(cPtr))
+	*uip = goPtr      // store Go pointer in C memory at c_ptr
+	v.stored.Unlock() // send "stored" signal to main thread -->(1)
+	v.release.Lock()  // wait for "release" signal from main thread when
+	//                   Release() has been called. <--(2)
+	*uip = 0          // reset C memory to NULL
+	v.stored.Unlock() // send second "stored" signal to main thread -->(3)
+}


Test patch:
diff --git a/internal/cutil/ptrguard_test.go b/internal/cutil/ptrguard_test.go
new file mode 100644
index 0000000..b7e9042
--- /dev/null
+++ b/internal/cutil/ptrguard_test.go
@@ -0,0 +1,75 @@
+package cutil
+
+import (
+	"math/rand"
+	"testing"
+	"unsafe"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestPtrGuard(t *testing.T) {
+	t.Run("storeAndRelease", func(t *testing.T) {
+		s := "string"
+		goPtr := (unsafe.Pointer)(&s)
+		cPtr := Malloc(PtrSize)
+		defer Free(cPtr)
+		pg := NewPtrGuard(cPtr, goPtr)
+		assert.Equal(t, *(*unsafe.Pointer)(cPtr), goPtr)
+		pg.Release()
+		assert.Zero(t, *(*unsafe.Pointer)(cPtr))
+	})
+
+	t.Run("multiRelease", func(t *testing.T) {
+		s := "string"
+		goPtr := (unsafe.Pointer)(&s)
+		cPtr := Malloc(PtrSize)
+		defer Free(cPtr)
+		pg := NewPtrGuard(cPtr, goPtr)
+		assert.Equal(t, *(*unsafe.Pointer)(cPtr), goPtr)
+		pg.Release()
+		pg.Release()
+		pg.Release()
+		pg.Release()
+		assert.Zero(t, *(*unsafe.Pointer)(cPtr))
+	})
+
+	t.Run("stressTest", func(t *testing.T) {
+		// Because the default thread limit of the Go runtime is 10000, creating
+		// 20000 parallel PtrGuards asserts, that Go routines of PtrGuards don't
+		// create threads.
+		const N = 20000  // Number of parallel PtrGuards
+		const M = 100000 // Number of loops
+		var ptrGuards [N]*PtrGuard
+		cPtrArr := (*[N]CPtr)(unsafe.Pointer(Malloc(N * PtrSize)))
+		defer Free(CPtr(&cPtrArr[0]))
+		toggle := func(i int) {
+			if ptrGuards[i] == nil {
+				goPtr := unsafe.Pointer(&(struct{ byte }{42}))
+				cPtrPtr := CPtr(&cPtrArr[i])
+				ptrGuards[i] = NewPtrGuard(cPtrPtr, goPtr)
+				assert.Equal(t, (unsafe.Pointer)(cPtrArr[i]), goPtr)
+			} else {
+				ptrGuards[i].Release()
+				ptrGuards[i] = nil
+				assert.Zero(t, cPtrArr[i])
+			}
+		}
+		for i := range ptrGuards {
+			toggle(i)
+		}
+		for n := 0; n < M; n++ {
+			i := rand.Intn(N)
+			toggle(i)
+		}
+		for i := range ptrGuards {
+			if ptrGuards[i] != nil {
+				ptrGuards[i].Release()
+				ptrGuards[i] = nil
+			}
+		}
+		for i := uintptr(0); i < N; i++ {
+			assert.Zero(t, cPtrArr[i])
+		}
+	})
+}