replacing runtime.KeepAlive with runtime.Pinner #1226

kwakubiney · 2023-11-17T10:24:19Z

Benchmark comparison.

Looks like the runtime.Pin allocations adds some extra latency to the map operation benchmarks. See benchmarks below :

root@ubuntu-s-2vcpu-4gb-sfo3-01:~/ebpf# uname -a
Linux ubuntu-s-2vcpu-4gb-sfo3-01 5.15.0-86-generic #96-Ubuntu SMP Wed Sep 20 08:23:49 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Without runtime.Pinner

BenchmarkMap/Lookup-2         	  775972	      1466 ns/op	       0 B/op	       0 allocs/op
BenchmarkMap/Update-2         	  804397	      1422 ns/op	       0 B/op	       0 allocs/op
BenchmarkMap/NextKey-2        	 1000000	      1051 ns/op	       0 B/op	       0 allocs/op
BenchmarkMap/Delete-2         	  656106	      1976 ns/op	      64 B/op	       2 allocs/op
BenchmarkIterate/MapIterator-2         	     312	   3969072 ns/op	   56086 B/op	    3004 allocs/op
BenchmarkIterate/MapIteratorDelete-2   	     212	   5791723 ns/op	   56086 B/op	    3004 allocs/op

With runtime.Pinner

BenchmarkMap/Lookup-2         	  551258	      1993 ns/op	      32 B/op	       1 allocs/op
BenchmarkMap/Update-2         	  544279	      2062 ns/op	      32 B/op	       1 allocs/op
BenchmarkMap/NextKey-2        	  670621	      1637 ns/op	      24 B/op	       1 allocs/op
BenchmarkMap/Delete-2         	  491283	      2285 ns/op	      96 B/op	       3 allocs/op
BenchmarkIterate/MapIterator-2         	     214	   5172339 ns/op	  112119 B/op	    5005 allocs/op
BenchmarkIterate/MapIteratorDelete-2   	     138	   8156222 ns/op	  144121 B/op	    6005 allocs/op

lmb

This looks great! Left comments for some small stuff. There is one subtlety which still needs to be ironed out: we're currently not pinning the "assorted" structs, aka anything that is not passed directly to a syscall. Take a look at sys.ObjInfo for example: we allocate a second struct that is referenced by ObjGetInfoByFdAttr. That struct also needs to be pinned recursively. I think the way to do this is to change the sys.Info.info method to take a runtime.Pinner as well. There might be others which need this treatment.

lmb · 2023-11-17T14:55:53Z

internal/pinning.go

@@ -27,7 +28,8 @@ func Pin(currentPath, newPath string, fd *sys.FD) error {
 		return fmt.Errorf("%s is not on a bpf filesystem", newPath)
 	}

-	defer runtime.KeepAlive(fd)


I think that here runtime.KeepAlive is sufficient: we want to prevent the GC from invoking the finalizer on sys.FD, but we don't care whether the value is moved since we don't pass the pointer to sys.FD to the kernel. Instead we pass the fd number.

lmb · 2023-11-17T15:00:22Z

internal/sys/types.go

@@ -490,18 +491,32 @@ type BtfInfo struct {
 	KernelBtf uint32
 }

+func (s *BtfInfo) Pin(p *runtime.Pinner) {
+	p.Pin(s)
+	p.Pin(&s.Btf)


Ah, this is subtle. We don't want to pin &s.Btf which is the address of the Btf member of this struct. Instead we need to pin the pointer it contains, which is in s.Btf.ptr.

How about implementing a pin function on Pointer? func (p Pointer) Pin(*runtime.Pinner).

lmb · 2023-11-17T15:01:52Z

prog.go

 		targetID, err := findTargetInProgram(spec.AttachTarget, spec.AttachTo, spec.Type, spec.AttachType)
 		if err != nil {
 			return nil, fmt.Errorf("attach %s/%s: %w", spec.Type, spec.AttachType, err)
 		}

 		attr.AttachBtfId = targetID
 		attr.AttachBtfObjFd = uint32(spec.AttachTarget.FD())
-		defer runtime.KeepAlive(spec.AttachTarget)
+		pinner.Pin(spec.AttachTarget)


Same as for the other sys.FD case: KeepAlive is appropriate here.

lmb · 2023-11-17T15:08:04Z

internal/cmd/gentypes/main.go

 		switch s.ret {
 		case retError:
-			fmt.Fprintf(w, "func %s(attr *%s) error { _, err := BPF(%s, unsafe.Pointer(attr), unsafe.Sizeof(*attr)); return err }\n\n", s.goType, goAttrType, s.cmd)
+			fmt.Fprintf(w, "func %s(attr *%s) error {\n"+


Nit: maybe it's nicer to use a multi line string for these?

lmb · 2023-11-17T15:11:33Z

internal/sys/types.go

@@ -624,7 +677,14 @@ type BtfLoadAttr struct {
 	BtfLogTrueSize uint32
 }

+func (s *BtfLoadAttr) Pin(p *runtime.Pinner) {
+	p.Pin(s)


This is missing pins for Btf and BtfLogBuf?

lmb · 2023-11-17T15:24:24Z

internal/cmd/gentypes/main.go

@@ -173,6 +175,7 @@ import (
 				replace(btfID, "btf_id", "attach_btf_obj_id"),
 				replace(typeID, "attach_btf_id"),
 			},
+			[]string{"XlatedProgInsns", "MapIds", "LineInfo", "FuncInfo"},


I worry that maintaining these by hand is going to be quite error prone. How about this:

Split outputPatchedStruct into patchStruct and outputStruct or similar.

Write a new function findPointerTypes which takes the patched result of patchStruct and iterates the members to find any where Member.Type == pointer. Return Member.Name as []string as now.

Feed the patched struct and the []string of the pointer types to outputStruct.

Realized there's no distinct btf pointer type so I type asserted against btf.Int and checked if the size == 8

True that there is no generally valid pointer, but for our purposes comparing against pointer should work fine.

Interesting. Maybe I am missing something here but the replace() function takes in a pointer which is pointer := &btf.Int{Size: 8} and the Type field of the Member is an interface so type asserting against btf.Pointer doesn't match any member.

I mean literally comparing the variables: Member.Type == pointer. This asserts that the variable Member.Type matches the pointer variable. Since both are an interface the comparison boils down to types equal && pointers equal.

lmb · 2023-11-17T15:31:42Z

Can you figure out where the extra allocation is coming from?

kwakubiney · 2023-11-20T14:17:57Z

Can you figure out where the extra allocation is coming from?

Looks like the extra allocation is coming from the var pinner runtime.Pinner declaration we make on each call.

Edit: In much more detail, I ran a cpuprofile and memprofile on the benchmarks and noticed a runtime.newobject() call for every (*Map).<operation> call. Digging down with memprofile, I realized that the allocation was coming from attr variable declarations. I am inclined to believe that the attr variable escapes to the heap because of the pinning semantics we have introduced.

View this document for details.

We currently use runtime.KeepAlive to prevent Go objects from being collected by the Go GC but in some cases this is not sufficient. This is because these objects can be moved by the GC and it becomes important to pin in situations where pointers of such objects are passed into a syscall, for example. Signed-off-by: kwakubiney <kebiney@hotmail.com>

lmb · 2023-11-21T17:25:48Z

Great sleuthing, seems like this is more subtle than I initially thought!

Here is the syscall implementation we end up calling: https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/syscall/syscall_linux.go;l=65

//go:uintptrkeepalive
//go:nosplit
//go:linkname Syscall
func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err Errno) {

From https://go.dev/src/runtime/HACKING

go:uintptrkeepalive

The //go:uintptrkeepalive directive must be followed by a function declaration.

It specifies that the function’s uintptr arguments may be pointer values that have been converted to uintptr and must be kept alive for the duration of the call, even though from the types alone it would appear that the object is no longer needed during the call.

This directive is similar to //go:uintptrescapes, but it does not force arguments to escape. [...]

So the various attr we use are in fact not allocated on the heap but the stack! (Kinda obvious that they have to be if we currently do map ops without heap allocations but oh well...) I guess this works because the stack of the goroutine performing a syscall is "frozen" for the duration of the syscall. This is a big win for performance and not something we can give up "just" to get move-safety.

If Go started to move memory we'd have all hell break loose:

Direct syscall arguments are currently allocated on the stack due to runtime magic, but there is no way to guarantee this. What if we change something in the way we invoke the syscall which causes attr to escape to the heap? For example we return the attr to the caller for some reason. Now we're in a situation where the syscall argument escapes but is not pinned and therefore may move in a future version of Go. But we can't unconditionally pin the memory since that forces escaping. We'd need a "pin if it escapes" API or a way to force only stack allocated arguments to syscalls.
The direct syscall arguments might refer to heap allocated memory. For example in the ObjInfo case or to point at buffers we've allocated for instructions or the verifier log. That memory might move in time.

Overall, I'm not sure any more whether runtime.Pinner is really applicable to our problem: from reading the proposal the problem is about dealing with memory that has to be heap allocated since it is stuffed into C memory. This isn't true in our case. Seems like even people on the Go team have similar questions: https://go-review.googlesource.com/c/sys/+/465676/5/unix/syscall_freebsd.go#286

Basically, I think that we're lacking guidance from the Go team on how to future proof syscall code for moving GC. If you're up for it you could write this up on an issue on the Go bug tracker asking for documentation. (We could do draft in a Gdoc for example.)

kwakubiney · 2023-11-22T02:58:55Z

Thanks for the links provided and very good spot with the //go:uintptrkeepalive directive. I currently cannot find any good documentation stating that pointer members of these implicitly pinned higher level objects are pinned as well, I assume that is the case, and we do not need to explicitly pin them either. This has been a very interesting adventure and i have learnt a lot about unsafe(I guess I now know why it has that name xD) and some nuances wrt the way Go handles memory management.

lmb · 2023-11-22T12:31:26Z

If you're up for it you could write this up on an issue on the Go bug tracker asking for documentation. (We could do draft in a Gdoc for example.)

Let me know if this would be interesting to you, otherwise I'll draft something at some point.

kwakubiney · 2023-11-22T14:02:36Z

If you're up for it you could write this up on an issue on the Go bug tracker asking for documentation. (We could do draft in a Gdoc for example.)

Let me know if this would be interesting to you, otherwise I'll draft something at some point.

Interested, will draft and let you know when I am done

Edit: Unfortunately won't be able to address this now. @lmb

lmb · 2023-11-30T09:35:48Z

See https://groups.google.com/g/golang-dev/c/_r_GkDSHDeY

kwakubiney force-pushed the evaluate-runtime-pin branch from c22a8c4 to 366d952 Compare November 17, 2023 10:26

kwakubiney requested a review from lmb November 17, 2023 10:27

kwakubiney force-pushed the evaluate-runtime-pin branch 4 times, most recently from 542a9db to 147b704 Compare November 17, 2023 14:13

lmb requested changes Nov 17, 2023

View reviewed changes

kwakubiney changed the title ~~test run: generate runtime.pinner method for each struct~~ replacing runtime.KeepAlive with runtime.Pinner Nov 17, 2023

kwakubiney force-pushed the evaluate-runtime-pin branch 4 times, most recently from 28428f8 to 5e27b28 Compare November 20, 2023 19:16

kwakubiney force-pushed the evaluate-runtime-pin branch from 5e27b28 to 65d2ea3 Compare November 20, 2023 19:20

kwakubiney mentioned this pull request Nov 28, 2023

internal: use runtime.Pinner instead of runtime.KeepAlive (Go 1.21) #1184

Closed

lmb closed this Nov 30, 2023

kwakubiney deleted the evaluate-runtime-pin branch November 30, 2023 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replacing runtime.KeepAlive with runtime.Pinner #1226

replacing runtime.KeepAlive with runtime.Pinner #1226

kwakubiney commented Nov 17, 2023 •

edited

lmb left a comment

lmb Nov 17, 2023

lmb Nov 17, 2023

lmb Nov 17, 2023

lmb Nov 17, 2023

lmb Nov 17, 2023

lmb Nov 17, 2023

kwakubiney Nov 20, 2023

lmb Nov 21, 2023

kwakubiney Nov 21, 2023

lmb Nov 21, 2023

lmb commented Nov 17, 2023

kwakubiney commented Nov 20, 2023 •

edited

lmb commented Nov 21, 2023

kwakubiney commented Nov 22, 2023 •

edited

lmb commented Nov 22, 2023

kwakubiney commented Nov 22, 2023 •

edited

lmb commented Nov 30, 2023

replacing runtime.KeepAlive with runtime.Pinner #1226

replacing runtime.KeepAlive with runtime.Pinner #1226

Conversation

kwakubiney commented Nov 17, 2023 • edited

Benchmark comparison.

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb commented Nov 17, 2023

kwakubiney commented Nov 20, 2023 • edited

lmb commented Nov 21, 2023

kwakubiney commented Nov 22, 2023 • edited

lmb commented Nov 22, 2023

kwakubiney commented Nov 22, 2023 • edited

lmb commented Nov 30, 2023

kwakubiney commented Nov 17, 2023 •

edited

kwakubiney commented Nov 20, 2023 •

edited

kwakubiney commented Nov 22, 2023 •

edited

kwakubiney commented Nov 22, 2023 •

edited