Repository: opencontainers/runc
Pull request #3306 (2021-12-03 04:55:22)

Faster Intel RDT init if the feature is unavailable
In a (quite common) case RDT is not supported by the kernel/hardware,
it does not make sense to parse /proc/cpuinfo and /proc/self/mountinfo,
and yet the current code does it (on every runc exec, for example).
    
Fortunately, there is a quick way to check whether RDT is available --
if so, kernel creates /sys/fs/resctrl directory. Check its existence,
and skip all the other initialization if it's not present.

(plus some cleanups)
    


kolyshkin: @marquiz @xiaochenshen @Creatone PTAL (I don't even have hardware to test this)
xiaochenshen: @kolyshkin Thank you for working on this improvement.
Intel RDT kernel code checks hardware capabilities via CPUID, sets corresponding CPU flags in /proc/cpuinfo if features are supported by hardware, and then creates directory `/sys/fs/resctrl/` as the *default* mount point for resctrl filesystem. So checking existence of `/sys/fs/resctrl/` could fast return on platforms which don't support Intel RDT.

FYI - Actually on Intel RDT enabled platforms, we don't have to mount resctrl filesystem into directory `/sys/fs/resctrl/` every time. Here is a rare case to mount to another mount point (e.g., `/mnt/resctrl/`):
```
mount -t resctrl resctrl /mnt/resctrl/
cat /proc/self/mountinfo | grep resctrl
63 45 0:48 / /mnt/resctrl rw,relatime shared:40 - resctrl /mnt/resctrl rw
```
In this case, we have to call `findIntelRdtMountpointDir()` in `Root()` to get the real mount point `/mnt/resctrl/` instead of `/sys/fs/resctrl/`

So in my opinion, checking existence of `/sys/fs/resctrl/` is more like a capability check rather than a root directory check. We don't have to check it inside `Root()`. How about moving it out of `Root()` to the beginning of `featuresInit()`, prior to `parseCpuInfoFile()`? We could return even earlier on platforms which don't support Intel RDT.
kolyshkin: > How about moving it out of `Root()` to the beginning of `featuresInit()`, prior to `parseCpuInfoFile()`?

Sorry, this was my intention indeed I switched to other work and then back, and published a WIP patch as a result.

Will fix and let you know.

Patch:
diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go
index 8b6bf3ef..27cc18f6 100644
--- a/libcontainer/intelrdt/intelrdt.go
+++ b/libcontainer/intelrdt/intelrdt.go
@@ -5,7 +5,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -13,6 +12,8 @@ import (
 	"sync"
 
 	"github.com/moby/sys/mountinfo"
+	"golang.org/x/sys/unix"
+
 	"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
@@ -194,21 +195,21 @@ var (
 	// For Intel RDT initialization
 	initOnce sync.Once
 
-	errNotFound = errors.New("Intel RDT resctrl mount point not found")
+	errNotFound = errors.New("Intel RDT not available")
 )
 
 // Check if Intel RDT sub-features are enabled in featuresInit()
 func featuresInit() {
 	initOnce.Do(func() {
-		// 1. Check if hardware and kernel support Intel RDT sub-features
-		flagsSet, err := parseCpuInfoFile("/proc/cpuinfo")
+		// 1. Check if Intel RDT "resource control" filesystem is available.
+		// The user guarantees to mount the filesystem.
+		root, err := Root()
 		if err != nil {
 			return
 		}
 
-		// 2. Check if Intel RDT "resource control" filesystem is available.
-		// The user guarantees to mount the filesystem.
-		root, err := Root()
+		// 2. Check if hardware and kernel support Intel RDT sub-features.
+		flagsSet, err := parseCpuInfoFile("/proc/cpuinfo")
 		if err != nil {
 			return
 		}
@@ -250,9 +251,9 @@ func featuresInit() {
 	})
 }
 
-// Return the mount point path of Intel RDT "resource control" filesysem
-func findIntelRdtMountpointDir(f io.Reader) (string, error) {
-	mi, err := mountinfo.GetMountsFromReader(f, func(m *mountinfo.Info) (bool, bool) {
+// Return the mount point path of Intel RDT "resource control" filesystem.
+func findIntelRdtMountpointDir() (string, error) {
+	mi, err := mountinfo.GetMounts(func(m *mountinfo.Info) (bool, bool) {
 		// similar to mountinfo.FSTypeFilter but stops after the first match
 		if m.FSType == "resctrl" {
 			return false, true // don't skip, stop
@@ -276,35 +277,30 @@ func findIntelRdtMountpointDir(f io.Reader) (string, error) {
 
 // For Root() use only.
 var (
-	intelRdtRoot string
-	rootMu       sync.Mutex
+	intelRdtRoot    string
+	intelRdtRootErr error
+	rootOnce        sync.Once
 )
 
 // Root returns the Intel RDT "resource control" filesystem mount point.
 func Root() (string, error) {
-	rootMu.Lock()
-	defer rootMu.Unlock()
-
-	if intelRdtRoot != "" {
-		return intelRdtRoot, nil
-	}
+	rootOnce.Do(func() {
+		// If resctrl is available, kernel creates this directory.
+		if unix.Access("/sys/fs/resctrl", unix.F_OK) != nil {
+			intelRdtRootErr = errNotFound
+			return
+		}
 
-	f, err := os.Open("/proc/self/mountinfo")
-	if err != nil {
-		return "", err
-	}
-	root, err := findIntelRdtMountpointDir(f)
-	f.Close()
-	if err != nil {
-		return "", err
-	}
+		root, err := findIntelRdtMountpointDir()
+		if err != nil {
+			intelRdtRootErr = err
+			return
+		}
 
-	if _, err := os.Stat(root); err != nil {
-		return "", err
-	}
+		intelRdtRoot = root
+	})
 
-	intelRdtRoot = root
-	return intelRdtRoot, nil
+	return intelRdtRoot, intelRdtRootErr
 }
 
 type cpuInfoFlags struct {


Test patch:
diff --git a/libcontainer/intelrdt/intelrdt_test.go b/libcontainer/intelrdt/intelrdt_test.go
index 3534b438..2184a146 100644
--- a/libcontainer/intelrdt/intelrdt_test.go
+++ b/libcontainer/intelrdt/intelrdt_test.go
@@ -1,8 +1,6 @@
 package intelrdt
 
 import (
-	"errors"
-	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -127,141 +125,3 @@ func TestApply(t *testing.T) {
 		t.Fatalf("unexpected tasks file, expected '1235', got %q", pids)
 	}
 }
-
-const (
-	mountinfoValid = `18 40 0:18 / /sys rw,nosuid,nodev,noexec,relatime shared:6 - sysfs sysfs rw
-￼19 40 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
-￼20 40 0:5 / /dev rw,nosuid shared:2 - devtmpfs devtmpfs rw,size=131927256k,nr_inodes=32981814,mode=755
-￼21 18 0:17 / /sys/kernel/security rw,nosuid,nodev,noexec,relatime shared:7 - securityfs securityfs rw
-￼22 20 0:19 / /dev/shm rw,nosuid,nodev shared:3 - tmpfs tmpfs rw
-￼23 20 0:12 / /dev/pts rw,nosuid,noexec,relatime shared:4 - devpts devpts rw,gid=5,mode=620,ptmxmode=000
-￼24 40 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,mode=755
-￼25 18 0:21 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs ro,mode=755
-￼26 25 0:22 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 - cgroup cgroup rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
-￼27 18 0:23 / /sys/fs/pstore rw,nosuid,nodev,noexec,relatime shared:20 - pstore pstore rw
-￼28 25 0:24 / /sys/fs/cgroup/perf_event rw,nosuid,nodev,noexec,relatime shared:10 - cgroup cgroup rw,perf_event
-￼29 25 0:25 / /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:11 - cgroup cgroup rw,cpuacct,cpu
-￼30 25 0:26 / /sys/fs/cgroup/memory rw,nosuid,nodev,noexec,relatime shared:12 - cgroup cgroup rw,memory
-￼31 25 0:27 / /sys/fs/cgroup/devices rw,nosuid,nodev,noexec,relatime shared:13 - cgroup cgroup rw,devices
-￼32 25 0:28 / /sys/fs/cgroup/hugetlb rw,nosuid,nodev,noexec,relatime shared:14 - cgroup cgroup rw,hugetlb
-￼33 25 0:29 / /sys/fs/cgroup/blkio rw,nosuid,nodev,noexec,relatime shared:15 - cgroup cgroup rw,blkio
-￼34 25 0:30 / /sys/fs/cgroup/pids rw,nosuid,nodev,noexec,relatime shared:16 - cgroup cgroup rw,pids
-￼35 25 0:31 / /sys/fs/cgroup/cpuset rw,nosuid,nodev,noexec,relatime shared:17 - cgroup cgroup rw,cpuset
-￼36 25 0:32 / /sys/fs/cgroup/freezer rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,freezer
-￼37 25 0:33 / /sys/fs/cgroup/net_cls,net_prio rw,nosuid,nodev,noexec,relatime shared:19 - cgroup cgroup rw,net_prio,net_cls
-￼38 18 0:34 / /sys/kernel/config rw,relatime shared:21 - configfs configfs rw
-￼40 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
-￼16 18 0:6 / /sys/kernel/debug rw,relatime shared:23 - debugfs debugfs rw
-￼41 18 0:16 / /sys/fs/resctrl rw,relatime shared:24 - resctrl resctrl rw
-￼42 20 0:36 / /dev/hugepages rw,relatime shared:25 - hugetlbfs hugetlbfs rw
-￼43 19 0:37 / /proc/sys/fs/binfmt_misc rw,relatime shared:26 - autofs systemd-1 rw,fd=32,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=35492
-￼44 20 0:15 / /dev/mqueue rw,relatime shared:27 - mqueue mqueue rw
-￼45 40 8:1 / /boot rw,relatime shared:28 - ext4 /dev/sda1 rw,stripe=4,data=ordered
-￼46 40 253:1 / /home rw,relatime shared:29 - ext4 /dev/mapper/vvhg-vvhg rw,data=ordered
-￼47 40 0:38 / /var/lib/nfs/rpc_pipefs rw,relatime shared:30 - rpc_pipefs sunrpc rw
-￼125 24 0:20 /mesos/containers /run/mesos/containers rw,nosuid shared:22 - tmpfs tmpfs rw,mode=755
-￼123 40 253:0 /var/lib/docker/containers /var/lib/docker/containers rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
-￼129 40 253:0 /var/lib/docker/overlay2 /var/lib/docker/overlay2 rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
-￼119 24 0:39 / /run/user/1009 rw,nosuid,nodev,relatime shared:100 - tmpfs tmpfs rw,size=26387788k,mode=700,uid=1009,gid=1009`
-
-	mountinfoMbaSc = `18 40 0:18 / /sys rw,nosuid,nodev,noexec,relatime shared:6 - sysfs sysfs rw
-19 40 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
-20 40 0:5 / /dev rw,nosuid shared:2 - devtmpfs devtmpfs rw,size=131927256k,nr_inodes=32981814,mode=755
-21 18 0:17 / /sys/kernel/security rw,nosuid,nodev,noexec,relatime shared:7 - securityfs securityfs rw
-22 20 0:19 / /dev/shm rw,nosuid,nodev shared:3 - tmpfs tmpfs rw
-23 20 0:12 / /dev/pts rw,nosuid,noexec,relatime shared:4 - devpts devpts rw,gid=5,mode=620,ptmxmode=000
-24 40 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,mode=755
-25 18 0:21 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs ro,mode=755
-26 25 0:22 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 - cgroup cgroup rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
-27 18 0:23 / /sys/fs/pstore rw,nosuid,nodev,noexec,relatime shared:20 - pstore pstore rw
-28 25 0:24 / /sys/fs/cgroup/perf_event rw,nosuid,nodev,noexec,relatime shared:10 - cgroup cgroup rw,perf_event
-29 25 0:25 / /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:11 - cgroup cgroup rw,cpuacct,cpu
-30 25 0:26 / /sys/fs/cgroup/memory rw,nosuid,nodev,noexec,relatime shared:12 - cgroup cgroup rw,memory
-31 25 0:27 / /sys/fs/cgroup/devices rw,nosuid,nodev,noexec,relatime shared:13 - cgroup cgroup rw,devices
-32 25 0:28 / /sys/fs/cgroup/hugetlb rw,nosuid,nodev,noexec,relatime shared:14 - cgroup cgroup rw,hugetlb
-33 25 0:29 / /sys/fs/cgroup/blkio rw,nosuid,nodev,noexec,relatime shared:15 - cgroup cgroup rw,blkio
-34 25 0:30 / /sys/fs/cgroup/pids rw,nosuid,nodev,noexec,relatime shared:16 - cgroup cgroup rw,pids
-35 25 0:31 / /sys/fs/cgroup/cpuset rw,nosuid,nodev,noexec,relatime shared:17 - cgroup cgroup rw,cpuset
-36 25 0:32 / /sys/fs/cgroup/freezer rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,freezer
-37 25 0:33 / /sys/fs/cgroup/net_cls,net_prio rw,nosuid,nodev,noexec,relatime shared:19 - cgroup cgroup rw,net_prio,net_cls
-38 18 0:34 / /sys/kernel/config rw,relatime shared:21 - configfs configfs rw
-40 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
-16 18 0:6 / /sys/kernel/debug rw,relatime shared:23 - debugfs debugfs rw
-41 18 0:16 / /sys/fs/resctrl rw,relatime shared:24 - resctrl resctrl rw,mba_MBps
-42 20 0:36 / /dev/hugepages rw,relatime shared:25 - hugetlbfs hugetlbfs rw
-43 19 0:37 / /proc/sys/fs/binfmt_misc rw,relatime shared:26 - autofs systemd-1 rw,fd=32,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=35492
-44 20 0:15 / /dev/mqueue rw,relatime shared:27 - mqueue mqueue rw
-45 40 8:1 / /boot rw,relatime shared:28 - ext4 /dev/sda1 rw,stripe=4,data=ordered
-46 40 253:1 / /home rw,relatime shared:29 - ext4 /dev/mapper/vvhg-vvhg rw,data=ordered
-47 40 0:38 / /var/lib/nfs/rpc_pipefs rw,relatime shared:30 - rpc_pipefs sunrpc rw
-125 24 0:20 /mesos/containers /run/mesos/containers rw,nosuid shared:22 - tmpfs tmpfs rw,mode=755
-123 40 253:0 /var/lib/docker/containers /var/lib/docker/containers rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
-129 40 253:0 /var/lib/docker/overlay2 /var/lib/docker/overlay2 rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
-119 24 0:39 / /run/user/1009 rw,nosuid,nodev,relatime shared:100 - tmpfs tmpfs rw,size=26387788k,mode=700,uid=1009,gid=1009`
-)
-
-func TestFindIntelRdtMountpointDir(t *testing.T) {
-	testCases := []struct {
-		name            string
-		input           io.Reader
-		isNotFoundError bool
-		isError         bool
-		mbaScEnabled    bool
-		mountpoint      string
-	}{
-		{
-			name:       "Valid mountinfo with MBA Software Controller disabled",
-			input:      strings.NewReader(mountinfoValid),
-			mountpoint: "/sys/fs/resctrl",
-		},
-		{
-			name:         "Valid mountinfo with MBA Software Controller enabled",
-			input:        strings.NewReader(mountinfoMbaSc),
-			mbaScEnabled: true,
-			mountpoint:   "/sys/fs/resctrl",
-		},
-		{
-			name:            "Empty mountinfo",
-			input:           strings.NewReader(""),
-			isNotFoundError: true,
-		},
-		{
-			name:    "Broken mountinfo",
-			input:   strings.NewReader("baa"),
-			isError: true,
-		},
-	}
-
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			mbaScEnabled = false
-			mp, err := findIntelRdtMountpointDir(tc.input)
-			if tc.isNotFoundError {
-				if !errors.Is(err, errNotFound) {
-					t.Errorf("expected errNotFound error, got %+v", err)
-				}
-				return
-			}
-			if tc.isError {
-				if err == nil {
-					t.Error("expected error, got nil")
-				}
-				return
-			}
-			if err != nil {
-				t.Errorf("expected nil, got %+v", err)
-				return
-			}
-			// no errors, check the results
-			if tc.mbaScEnabled != mbaScEnabled {
-				t.Errorf("expected mbaScEnabled=%v, got %v",
-					tc.mbaScEnabled, mbaScEnabled)
-			}
-			if tc.mountpoint != mp {
-				t.Errorf("expected mountpoint=%q, got %q",
-					tc.mountpoint, mp)
-			}
-		})
-	}
-}
diff --git a/libcontainer/intelrdt/util_test.go b/libcontainer/intelrdt/util_test.go
index f1b4244e..b29d685e 100644
--- a/libcontainer/intelrdt/util_test.go
+++ b/libcontainer/intelrdt/util_test.go
@@ -26,7 +26,12 @@ func NewIntelRdtTestUtil(t *testing.T) *intelRdtTestUtil {
 	config := &configs.Config{
 		IntelRdt: &configs.IntelRdt{},
 	}
+
+	// Assign fake intelRtdRoot value, returned by Root().
 	intelRdtRoot = t.TempDir()
+	// Make sure Root() won't even try to parse mountinfo.
+	rootOnce.Do(func() {})
+
 	testIntelRdtPath := filepath.Join(intelRdtRoot, "resctrl")
 
 	// Ensure the full mock Intel RDT "resource control" filesystem path exists