Repository: tetratelabs/wazero
Pull request #1475 (2023-05-16 07:01:24)

linux: support using huge pages
I would like to add support for using huge pages on linux to back memory mappings of code segments. Huge pages are useful to reduce the number of TLB misses when managing large number of memory pages.

Because testing this feature is complex and the benefits may largely depend on workloads, but also because linux distributions may have huge pages disabled by default, I would like to put it behind a feature flag so it can be enabled and disabled at runtime without requiring to recompile wazero. For this, I added a new package in `internal/features` which implements the loading of a comma-separated list feature flags from `WAZEROFEATURES` environment variable. I chose this model because a similar mechanism is employed throughout the Go standard library with the `GODEBUG`. However, I am open to feedback and happy to change it if it doesn't fit.

I considered allowing wazero to more cleverly detect whether it was able to use huge pages but decided against it and in favor of letting the user decide because it seems like there are too many risks of making the wrong choice. Since huge pages are an optimization, I would prefer to have the default behavior favor portability, and give users the ability to extensively test the feature prior to enabling it on production workloads.

For the full documentation on the reasons why using huge pages may be useful, see https://www.kernel.org/doc/Documentation/admin-guide/mm/hugetlbpage.rst

achille-roussel: My intention is to keep this an option in the future. While it is an experiment today, we won't be able to work around the fact that using huge pages may not be desirable in some cases and users should have a way to opt-in. 

I'll send an alternative suggestion for how to achieve this without interpreting an environnement variable. 
achille-roussel: I revisited the implementation of the feature flagging mechanism in the latest update, notably:

- the environment variable is now only used in the wazero CLI and tests, it does not get inherited by programs using wazero as a library
- I opted to keep the feature list as an environment variable in the wazero CLI because it is an interface with a more flexible dependency model than a command line argument; we can drop support of the variable in the future without breaking usage of the CLI nor having to retain a `-features` option which does nothing; but please let me know if you feel strongly otherwise
- I added an `experimental.EnableFeatures` function so programs using wazero can enable features while remaining in charge of defining the user interface for this


If you don't mind, please take another look and let me know if you have more comments!
codefromthecrypt: > My intention is to keep this an option in the future. While it is an experiment today, we won't be able to work around the fact that using huge pages may not be desirable in some cases and users should have a way to opt-in.

I think something like this can be done, but should be an option. For example, someone can compile once to have wazero read settings from ENV variables, where by default there's no weird global config store.

I don't like starting down this path as it quickly becomes spring, but anyway, as long as env is opt-in, it can be contained. Meanwhile, we're still experimental so don't have to debate here.
achille-roussel: Thanks everyone for the review and feedback!

@codefromthecrypt let's try to get this into a shape where we all feel good about the long-term maintenance of the feature! I definitely value your experience and opinion a lot, so if you think this is the wrong move it's a good signal that we should try something different.

The way I see it is there is a fact that we can't work around, using huge pages has an effect on contention of the TLB, which cannot be isolated to a single `wazero.Runtime` instance since it is going to be managed by the kernel, the process is the only unit of isolation that we have. Allowing the feature to be configured per runtime would be misleading. The reason why we make it configurable is not because users can benefit from having it enabled on some runtime instances but not others, but because it may not be desirable to default to using huge pages. Now finding a way to express this form of per-process configuration can be tricky since wazero has so far successfully implemented isolation at the runtime or module scope.

Here are a few suggestions I can make:

- We could remove the ability to enable huge pages on the wazero CLI, making it possible to enable it only for programs using wazero as a library via `experimental.EnableFeatures`. While this would address the use case I have that originally motivated this change, it feels like the wrong move not to have a way to leverage the feature in vanilla wazero.

- We could enable the feature by default; the current implementation falls back to `mmap(2)` without huge pages on error, so it is somewhat flexible and should not break applications running on systems where huge pages are not enabled. The worst risk is seeing degraded performance.

- We could configure the feature via a command line argument, calling `experimental.EnableFeatures` from the wazero CLI. This would effectively remove the configuration via an environment variable. Since the intent is to use this in conjecture with `-cpuprofile`, enabling the command only on `dev` version could be a way to not leak into the user intreface.

- We could decide that the trade-off of introducing global configuration is not worth it, and come up with a way to enable huge pages per wazero runtime. We should likely document the limitations we are aware of in this case, so users don't assume that they will be able to fine-tune each runtime instance.

Each approach makes different trade-offs, I don't think I can come up with the right model on my own, and happy to consider other suggestions as well.

I hope this context is useful, I'll sleep on it to reflect but please chime in if you have questions or comments!
codefromthecrypt: If I understand correctly, you feel the only valid way to enable this setting is by ENV variable, because of its side effects. I don't think we should change the way we configure wazero based on the side effects of each switch. While true certain things have side effects (such as mounting root writeable!), my suggestion is to separate policy and advice from mechanics. We expose configuration and it is up to the user to decide to use env, flags, whatever means they decide to enable or disable things, no sneaky backdoors they can't undo or prevent.

For example, someone using a CLI tool like cobra etc, they should be able to document with those tools how ENV variables configure things. I wouldn't say.. well mounting root writeable is dangerous, so I think you should do that with ENV ;) I hope the analogy helps explain why I'm troubled about starting to dictate how properties are configured at the process level.
codefromthecrypt: on the choices btw, here's my 2p.

I don't think these choices are exclusive and that they talk about the CLI makes me think they are really about the CLI and how it is configured, not the wazero library in general!

> We could enable the feature by default

I prefer this until an end user says we shouldn't. I'm not an end user.

More settings are more problems, as this discussion has proven ;)

> We could decide that the trade-off of introducing global configuration is not worth it, and come up with a way to enable huge pages per wazero runtime

We have RuntimeConfig exactly for this purpose, it is what it was made for, and how I would suggest enabling the below.


> We could remove the ability to enable huge pages on the wazero CLI
> We could configure the feature via a command line argument

These two are very specific, about the CLI. What troubles me is making a general abstraction about env (e.g. in another package) instead of addressing things like experiments or otherwise in the CLI. I would react very differently, it this env logic was just a function in the CLI's package.

What we do scoped and only scoped to the CLI is extremely flexible. What is triggering to me is making a generic system to subvert RuntimeConfig via ENV as opposed to allow people (including our CLI) to set properties.

It is maybe this point that I've created a misunderstanding. I think of our CLI as basically a consumer of the library. We can and have hacked it to use experimental stuff. However, its configuration approach should live in its package, and not bleed into the main codebase. the CLI is only in our repo for convenience, and anybody should be able to configure anything it can however they like.

hope this ramble helps!
codefromthecrypt: ps here's how I would have expected this to be configured, if saying out loud helps.

Right now (experimental)

```go
ctx = experimental.WithHugePages(ctx)
// or
ctx = experimental.WithRuntimeFeatures(ctx, experimental.HUGE_PAGES)
r := wazero.NewRuntimeWithConfig(ctx, config)
defer r.Close(ctx)

// blah blah
```

Later (no longer experimental)

```go
config := wazero.NewRuntimeConfig().
                            WithHugePages(true)
// or
config := wazero.NewRuntimeConfig().
                            WithFeatures(RuntimeFeatures.HUGE_PAGES)
r := wazero.NewRuntimeWithConfig(ctx, config)
defer r.Close(ctx)

// blah blah
```

And the CLI would decide whether to use flags or ad-hoc env depending on if something is experimental or not, just like we enable other stuff such as experimental logging.
achille-roussel: I learned a bunch in the discussions and implementation of this change, thanks a lot for the discussions and reviews!

Patch:
diff --git a/internal/platform/mmap_linux.go b/internal/platform/mmap_linux.go
new file mode 100644
index 00000000..55906e82
--- /dev/null
+++ b/internal/platform/mmap_linux.go
@@ -0,0 +1,76 @@
+package platform
+
+import (
+	"math/bits"
+	"os"
+	"sort"
+	"strconv"
+	"strings"
+	"syscall"
+)
+
+const (
+	// https://man7.org/linux/man-pages/man2/mmap.2.html
+	__MAP_HUGE_SHIFT = 26
+	__MAP_HUGETLB    = 0x40000
+)
+
+var hugePagesConfigs []hugePagesConfig
+
+type hugePagesConfig struct {
+	size int
+	flag int
+}
+
+func (hpc *hugePagesConfig) match(size int) bool {
+	return (size & (hpc.size - 1)) == 0
+}
+
+func init() {
+	dirents, err := os.ReadDir("/sys/kernel/mm/hugepages/")
+	if err != nil {
+		return
+	}
+
+	for _, dirent := range dirents {
+		name := dirent.Name()
+		if !strings.HasPrefix(name, "hugepages-") {
+			continue
+		}
+		if !strings.HasSuffix(name, "kB") {
+			continue
+		}
+		n, err := strconv.ParseUint(name[10:len(name)-2], 10, 64)
+		if err != nil {
+			continue
+		}
+		if bits.OnesCount64(n) != 1 {
+			continue
+		}
+		n *= 1024
+		hugePagesConfigs = append(hugePagesConfigs, hugePagesConfig{
+			size: int(n),
+			flag: int(bits.TrailingZeros64(n)<<__MAP_HUGE_SHIFT) | __MAP_HUGETLB,
+		})
+	}
+
+	sort.Slice(hugePagesConfigs, func(i, j int) bool {
+		return hugePagesConfigs[i].size > hugePagesConfigs[j].size
+	})
+}
+
+func mmapCodeSegment(size, prot int) ([]byte, error) {
+	flags := syscall.MAP_ANON | syscall.MAP_PRIVATE
+
+	for _, hugePagesConfig := range hugePagesConfigs {
+		if hugePagesConfig.match(size) {
+			b, err := syscall.Mmap(-1, 0, size, prot, flags|hugePagesConfig.flag)
+			if err != nil {
+				continue
+			}
+			return b, nil
+		}
+	}
+
+	return syscall.Mmap(-1, 0, size, prot, flags)
+}
diff --git a/internal/platform/mmap_other.go b/internal/platform/mmap_other.go
new file mode 100644
index 00000000..620cdd3c
--- /dev/null
+++ b/internal/platform/mmap_other.go
@@ -0,0 +1,18 @@
+// This uses syscall.Mprotect. Go's SDK only supports this on darwin and linux.
+//go:build darwin || freebsd
+
+package platform
+
+import "syscall"
+
+func mmapCodeSegment(size, prot int) ([]byte, error) {
+	return syscall.Mmap(
+		-1,
+		0,
+		size,
+		prot,
+		// Anonymous as this is not an actual file, but a memory,
+		// Private as this is in-process memory region.
+		syscall.MAP_ANON|syscall.MAP_PRIVATE,
+	)
+}
diff --git a/internal/platform/mmap.go b/internal/platform/mmap_unix.go
similarity index 58%
rename from internal/platform/mmap.go
rename to internal/platform/mmap_unix.go
index 870528db..cd6f5112 100644
--- a/internal/platform/mmap.go
+++ b/internal/platform/mmap_unix.go
@@ -1,4 +1,3 @@
-// This uses syscall.Mprotect. Go's SDK only supports this on darwin and linux.
 //go:build darwin || linux || freebsd
 
 package platform
@@ -8,6 +7,11 @@ import (
 	"unsafe"
 )
 
+const (
+	mmapProtAMD64 = syscall.PROT_READ | syscall.PROT_WRITE | syscall.PROT_EXEC
+	mmapProtARM64 = syscall.PROT_READ | syscall.PROT_WRITE
+)
+
 func munmapCodeSegment(code []byte) error {
 	return syscall.Munmap(code)
 }
@@ -15,20 +19,8 @@ func munmapCodeSegment(code []byte) error {
 // mmapCodeSegmentAMD64 gives all read-write-exec permission to the mmap region
 // to enter the function. Otherwise, segmentation fault exception is raised.
 func mmapCodeSegmentAMD64(size int) ([]byte, error) {
-	buf, err := syscall.Mmap(
-		-1,
-		0,
-		size,
-		// The region must be RWX: RW for writing native codes, X for executing the region.
-		syscall.PROT_READ|syscall.PROT_WRITE|syscall.PROT_EXEC,
-		// Anonymous as this is not an actual file, but a memory,
-		// Private as this is in-process memory region.
-		syscall.MAP_ANON|syscall.MAP_PRIVATE,
-	)
-	if err != nil {
-		return nil, err
-	}
-	return buf, err
+	// The region must be RWX: RW for writing native codes, X for executing the region.
+	return mmapCodeSegment(size, mmapProtAMD64)
 }
 
 // mmapCodeSegmentARM64 cannot give all read-write-exec permission to the mmap region.
@@ -36,20 +28,8 @@ func mmapCodeSegmentAMD64(size int) ([]byte, error) {
 // to the region so that we can write contents at call-sites. Callers are responsible to
 // execute MprotectRX on the returned buffer.
 func mmapCodeSegmentARM64(size int) ([]byte, error) {
-	buf, err := syscall.Mmap(
-		-1,
-		0,
-		size,
-		// The region must be RW: RW for writing native codes.
-		syscall.PROT_READ|syscall.PROT_WRITE,
-		// Anonymous as this is not an actual file, but a memory,
-		// Private as this is in-process memory region.
-		syscall.MAP_ANON|syscall.MAP_PRIVATE,
-	)
-	if err != nil {
-		return nil, err
-	}
-	return buf, err
+	// The region must be RW: RW for writing native codes.
+	return mmapCodeSegment(size, mmapProtARM64)
 }
 
 // MprotectRX is like syscall.Mprotect, defined locally so that freebsd compiles.
diff --git a/internal/platform/mremap_linux.go b/internal/platform/mremap_linux.go
deleted file mode 100644
index 77bfec41..00000000
--- a/internal/platform/mremap_linux.go
+++ /dev/null
@@ -1,43 +0,0 @@
-package platform
-
-import (
-	"syscall"
-	"unsafe"
-)
-
-const (
-	__MREMAP_MAYMOVE = 1
-)
-
-func remapCodeSegmentAMD64(code []byte, size int) ([]byte, error) {
-	return remapCodeSegment(code, size)
-}
-
-func remapCodeSegmentARM64(code []byte, size int) ([]byte, error) {
-	return remapCodeSegment(code, size)
-}
-
-func remapCodeSegment(code []byte, size int) ([]byte, error) {
-	p, err := mremap(*(*unsafe.Pointer)(unsafe.Pointer(&code)), len(code), size, __MREMAP_MAYMOVE)
-	if err != nil {
-		return nil, err
-	}
-	return unsafe.Slice((*byte)(p), size), nil
-}
-
-//go:nosplit
-func mremap(oldAddr unsafe.Pointer, oldSize, newSize, flags int) (unsafe.Pointer, error) {
-	p, _, err := syscall.Syscall6(
-		syscall.SYS_MREMAP,
-		uintptr(oldAddr),
-		uintptr(oldSize),
-		uintptr(newSize),
-		uintptr(flags),
-		uintptr(0),
-		uintptr(0),
-	)
-	if err != 0 {
-		return nil, err
-	}
-	return unsafe.Pointer(p), nil
-}
diff --git a/internal/platform/mremap_other.go b/internal/platform/mremap_other.go
index ea3dbdfc..5be501c1 100644
--- a/internal/platform/mremap_other.go
+++ b/internal/platform/mremap_other.go
@@ -1,4 +1,4 @@
-//go:build !linux
+//go:build !(darwin || linux || freebsd)
 
 package platform
 
@@ -12,21 +12,6 @@ func remapCodeSegmentAMD64(code []byte, size int) ([]byte, error) {
 	return b, nil
 }
 
-// mustMunmapCodeSegment panics instead of returning an error to the
-// application.
-//
-// # Why panic?
-//
-// It is less disruptive to the application to leak the previous block if it
-// could be unmapped than to leak the new block and return an error.
-// Realistically, either scenarios are pretty hard to debug, so we panic. There
-// is less as the dominant runtime.GOOS (linux) doesn't use this anyway.
-func mustMunmapCodeSegment(code []byte) {
-	if err := munmapCodeSegment(code); err != nil {
-		panic(err)
-	}
-}
-
 func remapCodeSegmentARM64(code []byte, size int) ([]byte, error) {
 	b, err := mmapCodeSegmentARM64(size)
 	if err != nil {
diff --git a/internal/platform/mremap_unix.go b/internal/platform/mremap_unix.go
new file mode 100644
index 00000000..5017aaf7
--- /dev/null
+++ b/internal/platform/mremap_unix.go
@@ -0,0 +1,21 @@
+//go:build darwin || linux || freebsd
+
+package platform
+
+func remapCodeSegmentAMD64(code []byte, size int) ([]byte, error) {
+	return remapCodeSegment(code, size, mmapProtAMD64)
+}
+
+func remapCodeSegmentARM64(code []byte, size int) ([]byte, error) {
+	return remapCodeSegment(code, size, mmapProtARM64)
+}
+
+func remapCodeSegment(code []byte, size, prot int) ([]byte, error) {
+	b, err := mmapCodeSegment(size, prot)
+	if err != nil {
+		return nil, err
+	}
+	copy(b, code)
+	mustMunmapCodeSegment(code)
+	return b, nil
+}
diff --git a/internal/platform/platform.go b/internal/platform/platform.go
index 46f69991..2a0b78de 100644
--- a/internal/platform/platform.go
+++ b/internal/platform/platform.go
@@ -71,3 +71,17 @@ func MunmapCodeSegment(code []byte) error {
 	}
 	return munmapCodeSegment(code)
 }
+
+// mustMunmapCodeSegment panics instead of returning an error to the
+// application.
+//
+// # Why panic?
+//
+// It is less disruptive to the application to leak the previous block if it
+// could be unmapped than to leak the new block and return an error.
+// Realistically, either scenarios are pretty hard to debug, so we panic.
+func mustMunmapCodeSegment(code []byte) {
+	if err := munmapCodeSegment(code); err != nil {
+		panic(err)
+	}
+}
diff --git a/internal/sysfs/datasync_unsupported.go b/internal/sysfs/datasync_unsupported.go
index 91a2acf2..4261f565 100644
--- a/internal/sysfs/datasync_unsupported.go
+++ b/internal/sysfs/datasync_unsupported.go
@@ -9,5 +9,5 @@ import (
 
 func datasync(f *os.File) syscall.Errno {
 	// Attempt to sync everything, even if we only need to sync the data.
-	return sync(f)
+	return fsync(f)
 }
diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go
index df39d16b..ab1cd2e1 100644
--- a/internal/sysfs/osfile.go
+++ b/internal/sysfs/osfile.go
@@ -209,7 +209,7 @@ func (f *osFile) Truncate(size int64) (errno syscall.Errno) {
 
 // Sync implements the same method as documented on api.File
 func (f *osFile) Sync() syscall.Errno {
-	return sync(f.file)
+	return fsync(f.file)
 }
 
 // Datasync implements the same method as documented on api.File
diff --git a/internal/sysfs/sync.go b/internal/sysfs/sync.go
index edb673c1..ba336c8d 100644
--- a/internal/sysfs/sync.go
+++ b/internal/sysfs/sync.go
@@ -9,6 +9,6 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func sync(f *os.File) syscall.Errno {
+func fsync(f *os.File) syscall.Errno {
 	return platform.UnwrapOSError(f.Sync())
 }
diff --git a/internal/sysfs/sync_windows.go b/internal/sysfs/sync_windows.go
index 6a30957a..e83dc304 100644
--- a/internal/sysfs/sync_windows.go
+++ b/internal/sysfs/sync_windows.go
@@ -7,7 +7,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func sync(f *os.File) syscall.Errno {
+func fsync(f *os.File) syscall.Errno {
 	errno := platform.UnwrapOSError(f.Sync())
 	// Coerce error performing stat on a directory to 0, as it won't work
 	// on Windows.


Test patch:
diff --git a/internal/platform/mmap_linux_test.go b/internal/platform/mmap_linux_test.go
new file mode 100644
index 00000000..5f4a6b79
--- /dev/null
+++ b/internal/platform/mmap_linux_test.go
@@ -0,0 +1,23 @@
+package platform
+
+import (
+	"os"
+	"testing"
+
+	"github.com/tetratelabs/wazero/internal/testing/require"
+)
+
+func TestHugePageConfigs(t *testing.T) {
+	dirents, err := os.ReadDir("/sys/kernel/mm/hugepages/")
+	require.NoError(t, err)
+	require.Equal(t, len(dirents), len(hugePagesConfigs))
+
+	for _, hugePagesConfig := range hugePagesConfigs {
+		require.NotEqual(t, 0, hugePagesConfig.size)
+		require.NotEqual(t, 0, hugePagesConfig.flag)
+	}
+
+	for i := 1; i < len(hugePagesConfigs); i++ {
+		require.True(t, hugePagesConfigs[i-1].size > hugePagesConfigs[i].size)
+	}
+}