From 13674f43d3ff08a6896ecefd1dd201f948cc7e0c Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 26 Jul 2022 15:36:47 -0400 Subject: [PATCH 1/5] libct/intelrdt: delete IsMBAScEnabled() This function is unused, and removing it simplifies the changes which follow this commit. Signed-off-by: Cory Snider --- libcontainer/intelrdt/intelrdt.go | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 3953f930d25..6ea5b2851c9 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -182,8 +182,6 @@ var ( catEnabled bool // The flag to indicate if Intel RDT/MBA is enabled mbaEnabled bool - // The flag to indicate if Intel RDT/MBA Software Controller is enabled - mbaScEnabled bool // For Intel RDT initialization initOnce sync.Once @@ -216,12 +214,7 @@ func featuresInit() { catEnabled = true } } - if mbaScEnabled { - // We confirm MBA Software Controller is enabled in step 2, - // MBA should be enabled because MBA Software Controller - // depends on MBA - mbaEnabled = true - } else if flagsSet.MBA { + if flagsSet.MBA { if _, err := os.Stat(filepath.Join(root, "info", "MB")); err == nil { mbaEnabled = true } @@ -260,11 +253,6 @@ func findIntelRdtMountpointDir() (string, error) { return "", errNotFound } - // Check if MBA Software Controller is enabled through mount option "-o mba_MBps" - if strings.Contains(","+mi[0].VFSOptions+",", ",mba_MBps,") { - mbaScEnabled = true - } - return mi[0].Mountpoint, nil } @@ -493,12 +481,6 @@ func IsMBAEnabled() bool { return mbaEnabled } -// Check if Intel RDT/MBA Software Controller is enabled -func IsMBAScEnabled() bool { - featuresInit() - return mbaScEnabled -} - // Get the path of the clos group in "resource control" filesystem that the container belongs to func (m *Manager) getIntelRdtPath() (string, error) { rootPath, err := Root() From 9f107489b0742825cd32f6d9328b30174a00aef1 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 26 Jul 2022 15:36:47 -0400 Subject: [PATCH 2/5] libct/intelrdt: skip reading /proc/cpuinfo Reading /proc/cpuinfo is a surprisingly expensive operation. Since kernel version 4.12 [1], opening /proc/cpuinfo on an x86 system can block for around 20 milliseconds while the kernel samples the current CPU frequency. There is a very recent patch [2] which gets rid of the delay, but has yet to make it into the mainline kenel. Regardless, kernels for which opening /proc/cpuinfo takes 20ms will continue to be run in production for years to come. libcontainer only opens /proc/cpuinfo to read the processor feature flags so all the delays to get an accurate snapshot of the CPU frequency are just wasted time. If we wanted to, we could interrogate the CPU features directly from userspace using the `CPUID` instruction. However, Intel and AMD CPUs have flags in different positions for their analogous sub-features and there are CPU quirks [3] which would need to be accounted for. Some Haswell server CPUs support RDT/CAT but are missing the `CPUID` flags advertising their support; the kernel checks for support on that processor family by probing the the hardware using privileged RDMSR/WRMSR instructions [4]. This sort of probing could not be implemented in userspace so it would not be possible to check for RDT feature support in userspace without false negatives on some hardware configurations. It looks like libcontainer reads the CPU feature flags as a kind of optimization so that it can skip checking whether the kernel supports an RDT sub-feature if the hardware support is missing. As the kernel only exposes subtrees in the `resctrl` filesystem for RDT sub-features with hardware and kernel support, checking the CPU feature flags is redundant from a correctness point of view. Remove the /proc/cpuinfo check as it is an optimization which actually hurts performance. [1]: https://unix.stackexchange.com/a/526679 [2]: https://lore.kernel.org/all/20220415161206.875029458@linutronix.de/ [3]: https://github.com/torvalds/linux/blob/7cf6a8a17f5b134b7e783c2d45c53298faef82a7/arch/x86/kernel/cpu/resctrl/core.c#L834-L851 [4]: https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/x86/kernel/cpu/resctrl/core.c#L111-L153 Signed-off-by: Cory Snider --- libcontainer/intelrdt/intelrdt.go | 101 ++++++------------------------ 1 file changed, 18 insertions(+), 83 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 6ea5b2851c9..82bfd32859d 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -1,7 +1,6 @@ package intelrdt import ( - "bufio" "bytes" "errors" "fmt" @@ -199,40 +198,28 @@ func featuresInit() { return } - // 2. Check if hardware and kernel support Intel RDT sub-features. - flagsSet, err := parseCpuInfoFile("/proc/cpuinfo") - if err != nil { - return - } - - // 3. Double check if Intel RDT sub-features are available in - // "resource control" filesystem. Intel RDT sub-features can be + // 2. Check if Intel RDT sub-features are available in "resource + // control" filesystem. Intel RDT sub-features can be // selectively disabled or enabled by kernel command line // (e.g., rdt=!l3cat,mba) in 4.14 and newer kernel - if flagsSet.CAT { - if _, err := os.Stat(filepath.Join(root, "info", "L3")); err == nil { - catEnabled = true - } + if _, err := os.Stat(filepath.Join(root, "info", "L3")); err == nil { + catEnabled = true } - if flagsSet.MBA { - if _, err := os.Stat(filepath.Join(root, "info", "MB")); err == nil { - mbaEnabled = true - } + if _, err := os.Stat(filepath.Join(root, "info", "MB")); err == nil { + mbaEnabled = true } - if flagsSet.MBMTotal || flagsSet.MBMLocal || flagsSet.CMT { - if _, err := os.Stat(filepath.Join(root, "info", "L3_MON")); err != nil { - return - } - enabledMonFeatures, err = getMonFeatures(root) - if err != nil { - return - } - if enabledMonFeatures.mbmTotalBytes || enabledMonFeatures.mbmLocalBytes { - mbmEnabled = true - } - if enabledMonFeatures.llcOccupancy { - cmtEnabled = true - } + if _, err := os.Stat(filepath.Join(root, "info", "L3_MON")); err != nil { + return + } + enabledMonFeatures, err = getMonFeatures(root) + if err != nil { + return + } + if enabledMonFeatures.mbmTotalBytes || enabledMonFeatures.mbmLocalBytes { + mbmEnabled = true + } + if enabledMonFeatures.llcOccupancy { + cmtEnabled = true } }) } @@ -286,58 +273,6 @@ func Root() (string, error) { return intelRdtRoot, intelRdtRootErr } -type cpuInfoFlags struct { - CAT bool // Cache Allocation Technology - MBA bool // Memory Bandwidth Allocation - - // Memory Bandwidth Monitoring related. - MBMTotal bool - MBMLocal bool - - CMT bool // Cache Monitoring Technology -} - -func parseCpuInfoFile(path string) (cpuInfoFlags, error) { - infoFlags := cpuInfoFlags{} - - f, err := os.Open(path) - if err != nil { - return infoFlags, err - } - defer f.Close() - - s := bufio.NewScanner(f) - for s.Scan() { - line := s.Text() - - // Search "cat_l3" and "mba" flags in first "flags" line - if strings.HasPrefix(line, "flags") { - flags := strings.Split(line, " ") - // "cat_l3" flag for CAT and "mba" flag for MBA - for _, flag := range flags { - switch flag { - case "cat_l3": - infoFlags.CAT = true - case "mba": - infoFlags.MBA = true - case "cqm_mbm_total": - infoFlags.MBMTotal = true - case "cqm_mbm_local": - infoFlags.MBMLocal = true - case "cqm_occup_llc": - infoFlags.CMT = true - } - } - return infoFlags, nil - } - } - if err := s.Err(); err != nil { - return infoFlags, err - } - - return infoFlags, nil -} - // Gets a single uint64 value from the specified file. func getIntelRdtParamUint(path, file string) (uint64, error) { fileName := filepath.Join(path, file) From c156bde7cc9bb721f7110b987992d7f9c8b951a0 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 26 Jul 2022 15:36:47 -0400 Subject: [PATCH 3/5] libct/intelrdt: elide parsing mountinfo The intelrdt package only needs to parse mountinfo to find the mount point of the resctrl filesystem. Users are generally going to mount the resctrl filesystem to the pre-created /sys/fs/resctrl directory, so there is a common case where mountinfo parsing is not required. Optimize for the common case with a fast path which checks both for the existence of the /sys/fs/resctrl directory and whether the resctrl filesystem was mounted to that path using a single statfs syscall. Signed-off-by: Cory Snider --- libcontainer/intelrdt/intelrdt.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 82bfd32859d..4cd9ffc108f 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -224,7 +224,7 @@ func featuresInit() { }) } -// Return the mount point path of Intel RDT "resource control" filesystem. +// findIntelRdtMountpointDir returns the mount point of the 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 @@ -250,24 +250,32 @@ var ( rootOnce sync.Once ) +// The kernel creates this (empty) directory if resctrl is supported by the +// hardware and kernel. The user is responsible for mounting the resctrl +// filesystem, and they could mount it somewhere else if they wanted to. +const defaultResctrlMountpoint = "/sys/fs/resctrl" + // Root returns the Intel RDT "resource control" filesystem mount point. func Root() (string, error) { rootOnce.Do(func() { - // If resctrl is available, kernel creates this directory. - if unix.Access("/sys/fs/resctrl", unix.F_OK) != nil { - intelRdtRootErr = errNotFound + // Does this system support resctrl? + var statfs unix.Statfs_t + if err := unix.Statfs(defaultResctrlMountpoint, &statfs); err != nil { + if errors.Is(err, unix.ENOENT) { + err = errNotFound + } + intelRdtRootErr = err return } - // NB: ideally, we could just do statfs and RDTGROUP_SUPER_MAGIC check, but - // we have to parse mountinfo since we're also interested in mount options. - root, err := findIntelRdtMountpointDir() - if err != nil { - intelRdtRootErr = err + // Has the resctrl fs been mounted to the default mount point? + if statfs.Type == unix.RDTGROUP_SUPER_MAGIC { + intelRdtRoot = defaultResctrlMountpoint return } - intelRdtRoot = root + // The resctrl fs could have been mounted somewhere nonstandard. + intelRdtRoot, intelRdtRootErr = findIntelRdtMountpointDir() }) return intelRdtRoot, intelRdtRootErr From 56daf36be2c5c944f3ee7a8d37cf809c514ffcf7 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 26 Jul 2022 15:36:47 -0400 Subject: [PATCH 4/5] libct/intelrdt: skip remove unless configured The OCI runtime spec mandates "[i]f intelRdt is not set, the runtime MUST NOT manipulate any resctrl pseudo-filesystems." Attempting to delete files counts as manipulating, so stop doing that when the container's RDT configuration is nil. Signed-off-by: Cory Snider --- libcontainer/intelrdt/intelrdt.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 4cd9ffc108f..8eb87a3f09e 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -479,7 +479,7 @@ func (m *Manager) Destroy() error { // Don't remove resctrl group if closid has been explicitly specified. The // group is likely externally managed, i.e. by some other entity than us. // There are probably other containers/tasks sharing the same group. - if m.config.IntelRdt == nil || m.config.IntelRdt.ClosID == "" { + if m.config.IntelRdt != nil && m.config.IntelRdt.ClosID == "" { m.mu.Lock() defer m.mu.Unlock() if err := os.RemoveAll(m.GetPath()); err != nil { From ea0bd7826846da19d16d805cf1f8582f01448300 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 26 Jul 2022 15:36:47 -0400 Subject: [PATCH 5/5] libct/intelrdt: check if available iff configured Unless the container's runtime config has intelRdt configuration set, any checks for whether Intel RDT is supported or the resctrl filesystem is mounted are a waste of time as, per the OCI Runtime Spec, "the runtime MUST NOT manipulate any resctrl pseudo-filesystems." And in the likely case where Intel RDT is supported by both the hardware and kernel but the resctrl filesystem is not mounted, these checks can get expensive as the intelrdt package needs to parse mountinfo to check whether the filesystem has been mounted to a non-standard path. Optimize for the common case of containers with no intelRdt configuration by only performing the checks when the container has opted in. Signed-off-by: Cory Snider --- libcontainer/intelrdt/intelrdt.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 8eb87a3f09e..0f5c457b099 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -152,9 +152,13 @@ type Manager struct { path string } -// NewManager returns a new instance of Manager, or nil, if the Intel RDT -// functionality is not available from hardware or not enabled in the kernel. +// NewManager returns a new instance of Manager, or nil if the Intel RDT +// functionality is not specified in the config, available from hardware or +// enabled in the kernel. func NewManager(config *configs.Config, id string, path string) *Manager { + if config.IntelRdt == nil { + return nil + } if _, err := Root(); err != nil { // Intel RDT is not available. return nil