Collection: add Variables field to interact with global variables

[ti-mo]

See individual commits for more detailed information.

• Extend Global Variables documentation

[ti-mo]

Gave this a rebase. I made sure we allocate an extra page to guarantee we find a usable page boundary and added some memory coherence notes on Memory. Documentation to be done after autumn break.

[lmb]

I'll make some time next week to review this.

[ti-mo]

I've added a small docs section on Variable and Collection.Variables, should be enough for now. More will be added with the MemoryPointer and VariablePointer follow-up PR.

[lmb]

I really like how modifying the variable via the getter and setter work, I feel like it integrates with the rest of the API very well.

As you can imagine I'm a little bit hesitant about Memory. From reading the discussion on Slack it seems like the core idea behind it is to (at some later point) facilitate atomic access for things like uint64 and to maybe make arena memory usable like Go memory.

The latter is an intriguing idea but I worry that it'll cause us to paint ourselves into a corner. Once we add an API which "transmutes" the off-heap BPF arena memory into a Go pointer "tracked" by the GC we lose all ability to fix things later on. There is pretty much just a single way of implementing this. And while the implementation works right now (on the platforms we test) there is no guarantee that it will continue to do so. Worse, if the package is considered sufficiently important it might even prevent or constrain future improvements on the Go runtime side. We've seen this happen with the recent go:linkname debacle and I think we need to take this into account. Because it's not at all clear what sharing an arena is useful for I think we shouldn't shape our implementation based on this.

This leaves the atomic access to variables. I can't find the old branch, but I think in a previous iteration there were a bunch of wrapper types like ebpf.Uint64 etc. that abstracted the atomic access away. That added a lot of boilerplate and probably also makes code generation via bpf2go or similar more complicated. Now the plan is (if I understand correctly) to just directly access typed memory via the atomic package. I think there might be a third way, by mediating atomic access via Get and Set.

func (v *Variable) Set(value any) error {
	switch value := value.(type) {
	case uint64:
		if v.size != unsafe.Sizeof(value) && v.offset % unsafe.Alignof(value) {
			return errors.New("bad!")
		}
		atomic.StoreUint64(v.addr(), value)
		return nil

	// ...
	}
}

This only covers the most basic atomic accesses, but that is probably fine for 90% of the cases? If we ever need more access we could add a function like the following:

// VariableMemory allows accessing a variable as a pointer.
//
// It is not valid to access ptr outside of the closure it is passed into.
//
// Returns an error if T is not compatible with the variable.
func VariableMemory[T any](v *Variable, func(ptr *T) error) error

Behind the scenes each Variable would refer to a Memory (though I'd probably unexport that for now) which is used to unmap the memory with a finalizer.

[mejedi]

@lmb

As you can imagine I'm a little bit hesitant about Memory. From reading the discussion on Slack it seems like the core idea behind it is to (at some later point) facilitate atomic access for things like uint64 and to maybe make arena memory usable like Go memory.

For me, the most appealing aspect of it was compound types, not the arena. If a variable is a structure with fields, how would one access those individually from golang? I tend to agree with you that the hack is risky; I'll try to write a proposal and if it goes well, we might get gcmmap in golang in a year or two. Based on what I learned about golang internals, the implementation should be rather straight-forward.

I'd like to have something less arcane than the proposed VariableMemory though:

func VariableMemory[T any](v *Variable, func(ptr *T) error) error

A callback can leak the pointer, hence this design doesn't buy us much in terms of safety. Would it be possible to have an honest .UnsafePointer() on a variable?

[ti-mo]

I've addressed all feedback, I think I'll merge this so we can move on. It feels like we need a bit more support from the runtime to make native variable pointers a reality, but in the meantime, the branch with Variable/MemoryPointer and heap mmapping lives here: https://github.com/ti-mo/ebpf/commits/tb/atomic-memory.

I've experimented with the closure suggestion and it's indeed incredibly clunky, though it does move us closer to our goal. It does guarantee the Variable/Memory lifetime for the duration of the variable access, and it's not terribly prone to misuse, even though it can be. It's not a solution I'm comfortable pushing towards our users, though. For now, I think the current proposal is shippable and adds good value, so let's not hold it up any further.

I've created a Go proposal (golang/go#70224) and it's received at least some form of positive traction. Let's see how it plays out.

Feedback addressed