Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] New architecture: wasm #1552

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[RFC] New architecture: wasm #1552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions btf/btf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,8 +632,16 @@ func (s *Spec) TypeByName(name string, typ interface{}) error {
wanted = typPtr.Elem().Type()
}

if !wanted.AssignableTo(typeInterface) {
return fmt.Errorf("%T does not satisfy Type interface", typ)
switch wanted {
case reflect.TypeOf((**Datasec)(nil)).Elem():
// Those types are already assignable to Type. No need to call
// AssignableTo. Avoid it when possible to workaround
// limitation in tinygo:
// https://github.com/tinygo-org/tinygo/issues/4277
default:
if !wanted.AssignableTo(typeInterface) {
return fmt.Errorf("%T does not satisfy Type interface", typ)
}
}

types, err := s.AnyTypesByName(name)
Expand Down
2 changes: 1 addition & 1 deletion internal/endian_le.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//go:build 386 || amd64 || amd64p32 || arm || arm64 || loong64 || mipsle || mips64le || mips64p32le || ppc64le || riscv64
//go:build 386 || amd64 || amd64p32 || arm || arm64 || loong64 || mipsle || mips64le || mips64p32le || ppc64le || riscv64 || wasm

package internal

Expand Down
2 changes: 1 addition & 1 deletion internal/io.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func NewBufferedSectionReader(ra io.ReaderAt, off, n int64) *bufio.Reader {
// of 4096. Allow arches with larger pages to allocate more, but don't
// allocate a fixed 4k buffer if we only need to read a small segment.
buf := n
if ps := int64(os.Getpagesize()); n > ps {
if ps := int64(Getpagesize()); n > ps {
buf = ps
}

Expand Down
9 changes: 9 additions & 0 deletions internal/page_others.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//go:build !wasm

package internal

import "os"

func Getpagesize() int {
return os.Getpagesize()
}
8 changes: 8 additions & 0 deletions internal/page_wasm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//go:build wasm

package internal

func Getpagesize() int {
// A WebAssembly page has a constant size of 65,536 bytes, i.e., 64KiB
return 64 * 1024
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about tinygo, but some Googling reveals that it implements a subset of the Go "os" package, and it looks like it implements os.Getpagesize() as well for the arches it targets. Why is it hardcoded here? This is the crux of the PR but isn't mentioned anywhere.

In any case, I'd like this to go into the respective stdlib, either in Google's implementation or tinygo if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the following error if I try to use os.Getpagesize:

$ tinygo build -o wasm.wasm main.go
# github.com/cilium/ebpf/internal
../../cilium/ebpf/internal/page_wasm.go:8:12: undefined: os.Getpagesize

According to https://pkg.go.dev/github.com/tinygo-org/tinygo/src/os#Getpagesize, it was added in tinygo 0.24.0 and I use 0.30.0, but it still does not work.

Issue: tinygo-org/tinygo#1285

I agree it should ideally be fixed in tinygo or Go... I could try to make a PR there. But I would like to have a workaround in cilium/ebpf meanwhile. Btw, syscall.Getpagesize() works, presumably thanks to tinygo-org/tinygo#2856; could that be an option to use that in cilium/ebpf as a workaround?

In tinygo-org/tinygo#1285, they suggest to return -1... That would not work for cilium/ebpf when used in unix.Mmap but we wouldn't call those methods from wasm context. And for internal.NewBufferedSectionReader(n=-1), it seems the implementation would use a buffer of 16 bytes when given a negative number, so that would work (but might be slow).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could try to make a PR there. But I would like to have a workaround in cilium/ebpf meanwhile.

If we merge a workaround, there's no guarantee you will get around to making that upstream contribution. 😉 I really don't want to hardcode non-bpf-related platform constants, that's definitely the runtime/stdlib's responsibility.

syscall.Getpagesize() works

Underlyingly, os.Getpagesize() simply calls syscall.Getpagesize() (https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/os/types.go;l=13), so sounds like a trivial fix in tinygo? Might be a good exercise to understand the underlying tech a bit better.

I prefer to make only the absolute minimum changes to the lib to force us to address issues in the underlying layers. This is good for the health of the platform and potentially enables more software to run correctly out-of-the-box.

}
5 changes: 2 additions & 3 deletions map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"io"
"math/rand"
"os"
"path/filepath"
"reflect"
"slices"
Expand Down Expand Up @@ -507,7 +506,7 @@ func handleMapCreateError(attr sys.MapCreateAttr, spec *MapSpec, err error) erro
// BPF_MAP_TYPE_RINGBUF's max_entries must be a power-of-2 multiple of kernel's page size.
if errors.Is(err, unix.EINVAL) &&
(attr.MapType == sys.BPF_MAP_TYPE_RINGBUF || attr.MapType == sys.BPF_MAP_TYPE_USER_RINGBUF) {
pageSize := uint32(os.Getpagesize())
pageSize := uint32(internal.Getpagesize())
maxEntries := attr.MaxEntries
if maxEntries%pageSize != 0 || !internal.IsPow(maxEntries) {
return fmt.Errorf("map create: %w (ring map size %d not a multiple of page size %d)", err, maxEntries, pageSize)
Expand Down Expand Up @@ -950,7 +949,7 @@ func (m *Map) nextKey(key interface{}, nextKeyOut sys.Pointer) error {
}

var mmapProtectedPage = sync.OnceValues(func() ([]byte, error) {
return unix.Mmap(-1, 0, os.Getpagesize(), unix.PROT_NONE, unix.MAP_ANON|unix.MAP_SHARED)
return unix.Mmap(-1, 0, internal.Getpagesize(), unix.PROT_NONE, unix.MAP_ANON|unix.MAP_SHARED)
})

// guessNonExistentKey attempts to perform a map lookup that returns ENOENT.
Expand Down
9 changes: 8 additions & 1 deletion syscalls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,14 @@ var (
// invalidBPFObjNameChar returns true if char may not appear in
// a BPF object name.
func invalidBPFObjNameChar(char rune) bool {
dotAllowed := objNameAllowsDot() == nil
var dotAllowed bool
if runtime.GOOS == "js" {
// In Javascript environments, BPF objects are not going to be
// loaded to a kernel, so we can use dots without testing.
dotAllowed = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this logic in objNameAllowsDot instead? Also, instead of hardcoding a check on runtime.GOOS, we should let the syscall fail and conclude that it means a dot can be used.

Copy link
Collaborator

@ti-mo ti-mo Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some reading on this last week, apparently ENOSYS is returned by much of the tinygo syscall package for wasm. This would be a good sentinel to conclude that a syscall can't be made and we need to fall back to a sane default. We can make things on the Windows side behave similarly, so high-level logic only needs to check for ENOSYS.

cc @lmb

} else {
dotAllowed = objNameAllowsDot() == nil
}

switch {
case char >= 'A' && char <= 'Z':
Expand Down
Loading