This PR has been rebased on https://github.com/ti-mo/ebpf/tree/tb/ebpf-variables.
After discussing, @ti-mo proceeds with the Map.Memory() api and the definition of ebpf.Variable, while me (this PR) should provide the needed support for bpf2go.

1. 59808a1 is required but should be introduced in ti-mo updates (open for discussion).
2. f6a94c5 is the commit under review.

It probably makes sense to land basic bpf2go variable support first:

• VariableSpecs included in generated xxxSpecs; do we need to segregate them similar to today's xxxProgramSpecs/xxxMapSpecs?
• Variables included in generated xxxObjects.

I see huge value in a type-safe get/set API you are proposing. It would be nice to have not only Variables covered, but probably VariableSpecs, and Maps, and MapSpecs as well.

Is it possible to devise an approach that works with Variables initially, but can be extended for other object kinds later? Can we generate less code by using generics?

It probably makes sense to land basic bpf2go variable support first:

• VariableSpecs included in generated xxxSpecs; do we need to segregate them similar to today's xxxProgramSpecs/xxxMapSpecs?
• Variables included in generated xxxObjects.

I see huge value in a type-safe get/set API you are proposing. It would be nice to have not only Variables covered, but probably VariableSpecs, and Maps, and MapSpecs as well.

Is it possible to devise an approach that works with Variables initially, but can be extended for other object kinds later? Can we generate less code by using generics?

Do you suggest to change this PR to provide basic VariableSpecs support in bpf2go or should I open a new one?

Concerning the type-safe API, I can keep exploring solutions. I tried with generics with no luck, but maybe there's still a way...AFAIU CollectionSpec would also need to be a generic holding maps and variables each of different generic types.

@smagnani96

Do you suggest to change this PR to provide basic VariableSpecs support in bpf2go or should I open a new one?

It is probably worthwhile to keep this one around as a RFC and to showcase the end goal. One way to handle it is to structure commits such that basic support is introduced first, followed by type-safe APIs. Once @ti-mo changes are merged, I'd suggest doing a separate PR to introduce basic variable support in bpf2go. Keep rebasing this one as needed.

Concerning the type-safe API, I can keep exploring solutions. I tried with generics with no luck, but maybe there's still a way...

A clever use of reflect.Value.ConvertibleTo by the way:)

It looks like bpf2go won't have to generate Set and Get methods if we could rely on generic TypedVariable in ebpf package, e.g.:

// in ebpf package
type TypedVariable[T any] Variable
func (v *TypedVariable[T]) Set(val T) error { return (*Variable)(v).Set(val) }
...

AFAIU CollectionSpec would also need to be a generic holding maps and variables each of different generic types.

What I had in mind was xxxSpecs bpf2go generates. It is similar to xxxObjects, but for the specs. VariableSpecs should be included in generated xxxSpecs. They are useful to initialise constants, and it would be nice to do it in a type-safe manner as well.

It is probably worthwhile to keep this one around as a RFC and to showcase the end goal. One way to handle it is to structure commits such that basic support is introduced first, followed by type-safe APIs. Once @ti-mo changes are merged, I'd suggest doing a separate PR to introduce basic variable support in bpf2go. Keep rebasing this one as needed.

Perfect, I'll keep this one open just as reference and will track #1572. Once merged, I'll push basic support for VariableSpecs and Variables.

It looks like bpf2go won't have to generate Set and Get methods if we could rely on generic TypedVariable in ebpf package, e.g.:

// in ebpf package
type TypedVariable[T any] Variable
func (v *TypedVariable[T]) Set(val T) error { return (*Variable)(v).Set(val) }
...

A wrapper would certainly be a good choice, so we can statically define and test methods rather than generating them at runtime. An approach could be as follows, and then in bpf2go we just output the type accordingly.

type ROVariable[T any] struct {
	Variable
}

type RWStruct[T any] struct {
	ROVariable[T]
}

type AtomicVariable[T int32 | int64 | uint64 | uint32] struct {
	RWVariable[T]
}

func (a *ROVariable[T]) Get() T {
	...
}

func (a *RWVariable[T]) Set(val T) {
	...
}

func (a *AtomicVariable[T]) AtomicReference() atomic.T { //somehow
	...
}

Would you consider such a wrapper also for VariableSpecs (and in case it can be applied also to MapSpecs) as well?

@smagnani96 I've pushed up a few changes to #1572 today. Note that all 'atomic' methods are gone. It wasn't a great design to begin with and @mejedi suggested mmapping over the Go heap so we can hand out arbitrary pointers instead. The pointer stuff is not part of the initial proposal, this work is currently in a separate branch here: https://github.com/ti-mo/ebpf/commits/tb/atomic-memory.

tl;dr:

// VariablePointer returns a pointer to a value of type T that has its
// underlying memory mapped to the memory representing the Variable. Accessing a
// constant Variable is not supported.
//
// T must be a fixed-size type according to [binary.Size]. Types containing Go
// pointers are not valid.
//
// When accessing structs, embedding [structs.HostLayout] may help ensure the
// layout of the Go struct matches the one in the BPF C program.
func VariablePointer[T any](v *Variable) (*T, error)

This works for atomics, structs, scalars, etc. Currently no types containing pointers are accepted.

Note that currently, the plan is to not populate Collection.Variables in kernels <5.5, which would be a problem if the 'all objects' struct generated by bpf2go includes Variables by default. We can decide to keep these in a separate object, but that would mean the caller needs to load the whole Collection and then Assign() the variables in a separate step, which is not ideal. Curious to hear your opinions on this.

The alternative is to emit Variables, but disable Set, Get and VariablePointer (make them return errors), which is also far from ideal. If we go down this route, we'd need to introduce a compatibility layer that implements Get() using a map lookup, but then we need to carry a *Map in Variable. This makes the implementation quite a bit more complex.

@ti-mo

Note that currently, the plan is to not populate Collection.Variables in kernels <5.5, which would be a problem if the 'all objects' struct generated by bpf2go includes Variables by default. We can decide to keep these in a separate object, but that would mean the caller needs to load the whole Collection and then Assign() the variables in a separate step, which is not ideal. Curious to hear your opinions on this.

Globals landed in 5.2. Call me biased, but my preference would be to inconvenience (a presumably small number of) users requiring at least 5.2 that can't bump the minimum version to 5.5. As of today, bpf2go emits xxxObjects made of xxxPograms and xxxMaps. Should xxxVariables join the party, they can declare and use their own xxxMyObjects embedding just xxxPrograms and xxxMaps.

@mejedi That sounds reasonable, thanks for sharing your perspective. The downside is this is kind of a breaking change; rebuilding the same code base with a newer version of b2g and ebpf-go will no longer work as-is on some systems.

@smagnani96
Not sure about inheritance, will make the job of CollectionSpec.LoadAndAssign harder. With the latest changes from @ti-mo every rw Variable is capable of giving out the pointer, so there are fewer variations left:

• ROVariable — Get
• Variable — Get, Set, Addr

(Not sure how we should name these generics, TypedVariable or VariableT or something else?)

The overlap is just the Get method, we can afford to implement it twice for 2 distinct types.

type ROVariableT[T any] Variable
type VariableT[T any] Variable

In return, CollectionSpec.LoadAndAssign can use reflect.Value.ConvertibleTo and avoid further complication.

Note: .Addr is how reflect names a method to get a pointer to the Value. If you ask me, .AtomicReference is way too long.

Couple further questions to consider:

1. Should Get return T or (T, error)? Similarly for Set.

   It will be more convenient if user didn't have to handle errors. Errors happen because T fails to marshal but in bpf2go we are generating these types and we know that they will marshal cleanly.

2. A subset of types has a matching type in sync/atomic, such as atomic.Uin64, etc. Should .Addr return a pointer to type from sync/atomic if available?

var objects struct { foo *VariableT[uint64, atomic.Uint64] `ebpf:"foo"` }
objects.foo.Addr().Load()

v.s.

var objects struct { foo *VariableT[uint64] `ebpf:"foo"` }
atomic.LoadUint64(objects.foo.Addr())

First option guarantees that value is accessed atomically. It also involves less typing.

Second option is more consistent. Get, Set and Addr types are consistent. Memory is accessed the same whether it is a standalone value or a compound type:

type compound { bar uint64 }
var objects struct { foo *VariableT[compound] `ebpf:"foo"` }
atomic.LoadUint64(objects.foo.Addr().bar)

Would you consider such a wrapper also for VariableSpecs (and in case it can be applied also to MapSpecs) as well?

Should be easier since we have just one flavour of VariableSpec.

I've updated #1572 to emit a Variable to the Collection for every VariableSpec in the CollectionSpec. Get(), Set() and VariablePointer() will return ErrNotSupported on kernels without BPF_F_MMAPABLE. This ensures LoadAndAssign will continue to work as usual after upgrades, but users opting in to interactions with Variable can only expect that to work on newer kernels. I think that's more user-friendly than failing LoadAndAssign.

@smagnani96 FYI #1572 has been merged, and #1607 was opened to keep track of the off-heap Go pointers, though keep in mind this might take a while to be resolved upstream. Then it's at least another 6 months of Go releases until ebpf-go can start depending on the behaviour.

I suggest we shelve the atomics use case in bpf2go for now, or we special-case atomic.(U)int* in Variable.Set() and .Get(). Only loads and stores would be supported, but those may cover the majority of use cases. If really needed, we can consider implementing Variable.Add(), but that gets weird when used on structs. Let's start with the basics, we can expand as we go.