[POC] bpf2go/struct_ops: implement StructOps support#1973
[POC] bpf2go/struct_ops: implement StructOps support#1973shun159 wants to merge 4 commits intocilium:mainfrom
Conversation
Implement code generation and library support for declarative initialization of struct_ops maps. This allows users to initialize struct_ops member, using standard Go structs during the LoadAndAssign process. bpf2go changes: - Scans for ebpf.StructOpsMap in the CollectionSpec and generates corresponding Go struct types. - Automatically maps kernel function pointers to *ebpf.Program and scalar members to appropriate Go types. Library changes: - Modify assignValues and LoadAndAssign to pass the reflect.Value of target fields, enabling the library to read pre-instantiated data from the user-provided objects struct. - Introduce loadStructOps to synchronize Go struct fields with the MapSpec before the map is created in the kernel. - Implement a "partial override" strategy in structOpsPatchValue: only non-zero fields in the Go struct are patched into the ELF section data, preserving ELF-defined defaults for uninitialized fields. - Support early relocation of struct_ops function pointers by injecting ebpf.Program FDs into the section data buffer during the patch process. Ref: cilium#1957 Signed-off-by: shun159 <dreamdiagnosis@gmail.com>
Signed-off-by: shun159 <dreamdiagnosis@gmail.com>
dae125c to
067c0da
Compare
Signed-off-by: shun159 <dreamdiagnosis@gmail.com>
7a81514 to
60b3842
Compare
Signed-off-by: shun159 <dreamdiagnosis@gmail.com>
dylandreimerink
left a comment
There was a problem hiding this comment.
Thanks again for working on this. I flagged a few things for you to look at.
| ms := cs.Maps[name] | ||
| if ms != nil && ms.Type == StructOpsMap { | ||
| vType := typ | ||
| if vType.Kind() == reflect.Ptr { | ||
| vType = vType.Elem() | ||
| } | ||
|
|
||
| if vType.Kind() == reflect.Struct && typ != reflect.TypeOf((*Map)(nil)) { | ||
| return loader.loadStructOps(name, val) | ||
| } | ||
| } |
There was a problem hiding this comment.
This code should go into loadMap, as its a special case of loading a map.
| dstOff := int(km.Offset.Bytes()) | ||
|
|
||
| if srcOff < 0 || srcOff+mSize > len(data) { | ||
| fmt.Println(srcOff, srcOff+mSize, len(data)) |
There was a problem hiding this comment.
I think you forgot to remove this after debugging.
| // the operation is skipped to preserve the original default value defined in the ELF. | ||
| // Currently, nested structs or unions are not supported for non-zero updates to | ||
| // avoid complex recursive patching. | ||
| func structOpsCopyMemberFromStructValue(fieldVal reflect.Value, m btf.Member, dstOff int, secData []byte) error { |
There was a problem hiding this comment.
That is a very verbose function name. Perhaps just structOpsCopyMember? That is about as long as I would go.
| srcPtr := unsafe.Pointer(fieldVal.UnsafeAddr()) | ||
| srcData := unsafe.Slice((*byte)(srcPtr), kSize) |
There was a problem hiding this comment.
We shouldn't be unsafe casting like this. Use the sysenc.Marshal utility instead. It internally has optimizations to do an unsafe cast as well, but only when it determines it is safe to do so, and will fallback to slower but safe logic otherwise.
| return nil | ||
| } | ||
|
|
||
| func structOpsFindMemberByName(st *btf.Struct, name string) (*btf.Member, error) { |
There was a problem hiding this comment.
Also very verbose name for what it does.
I would just inline this. This function is not reused outside of its parent. You are creating an error just to then continue in its parent function.
It can be something like:
var m btf.Member
if idx := slices.IndexFunc(st.Members, func(m btf.Member) bool {
return m.Name == name
} {
m = st.Members[idx]
} else {
continue
}
| // {{ .Name.StructOps }} contains all struct_ops types. | ||
| type {{ .Name.StructOps }} struct { | ||
| {{- range $name, $id := .StructOps }} | ||
| {{ $id }} {{ $.Name }}{{ $id }} `ebpf:"{{ $name }}"` | ||
| {{- end }} |
There was a problem hiding this comment.
I would consider surrounding this with an {{ if .StructOps }}. Programs, maps, and variables are expected to be in every generated file. But is odd to have empty struct ops related types, even when no struct ops are in use, which will be the majority of cases.
| var fieldType string | ||
| if ptr, ok := btf.As[*btf.Pointer](btf.UnderlyingType(m.Type)); ok { | ||
| if _, ok := btf.As[*btf.FuncProto](btf.UnderlyingType(ptr.Target)); ok { | ||
| fieldType = "*ebpf.Program" | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic of emitting a *ebpf.Program for pointers to functions seems to be the only reason for having this logic here. I think it would be better to adjust the GoFormatter in btf/format.go to do this instead, so you can reuse the rest of the existing logic for writing go types.
cmd/bpf2go/gen/output.go
Outdated
|
|
||
| sb.WriteString(fmt.Sprintf("// %s is a struct type for the struct_ops map.\n", goTypeName)) | ||
| sb.WriteString(fmt.Sprintf("type %s struct {\n", goTypeName)) | ||
| sb.WriteString(fmt.Sprintf("\t_ structs.HostLayout\n")) |
There was a problem hiding this comment.
This commit can be squashed into the previous one.
| testStructOps | ||
| } | ||
|
|
||
| // testStructOps contains all struct_ops types. | ||
| type testStructOps struct { |
There was a problem hiding this comment.
All of this churn in these files can be avoided if we only emit these when there is at least one struct_ops type.
| if _, ok := btf.As[*btf.Struct](underlying); ok { | ||
| size, _ := btf.Sizeof(m.Type) | ||
| fieldType = fmt.Sprintf("[%d]byte", size) | ||
| } else if _, ok := btf.As[*btf.Union](underlying); ok { | ||
| size, _ := btf.Sizeof(m.Type) | ||
| fieldType = fmt.Sprintf("[%d]byte", size) | ||
| } |
There was a problem hiding this comment.
This also deviates from the GoFormatter behavior, which would make nested structs here.
So question, did we do this because recursive emitting is difficult or is there a technical reason? If its just difficult, usage of the GoFormatter should solve it. Otherwise, I would suggest modifying the BTF before its passed to the formatter to get a similar end result.
Implement code generation and library support for declarative initialization of struct_ops maps. This allows users to initialize struct_ops member, using standard Go structs during the LoadAndAssign process.
bpf2go changes:
Library changes:
Ref: #1957