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

libct/intelrdt: skip reading /proc/cpuinfo #3485

Merged
merged 5 commits into from
Jul 28, 2022
Merged

libct/intelrdt: skip reading /proc/cpuinfo #3485

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
13674f4
libct/intelrdt: delete IsMBAScEnabled()
corhere Jul 26, 2022
9f10748
libct/intelrdt: skip reading /proc/cpuinfo
corhere Jul 26, 2022
c156bde
libct/intelrdt: elide parsing mountinfo
corhere Jul 26, 2022
56daf36
libct/intelrdt: skip remove unless configured
corhere Jul 26, 2022
ea0bd78
libct/intelrdt: check if available iff configured
corhere Jul 26, 2022
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
155 changes: 42 additions & 113 deletions libcontainer/intelrdt/intelrdt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package intelrdt

import (
"bufio"
"bytes"
"errors"
"fmt"
Expand Down Expand Up @@ -153,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
}
kolyshkin marked this conversation as resolved.
Show resolved Hide resolved
if _, err := Root(); err != nil {
// Intel RDT is not available.
return nil
Expand All @@ -182,8 +185,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
Expand All @@ -201,50 +202,33 @@ 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, there's one consideration that I don't know if we should care about here. If resctrlfs is mounted with -o cdp we will have L3CODE/ and L3DATA/ (instead of L3/) under the info/ dir.

I don't remember if CDP is officially supported in the runtime spec but I guess specifying something like L3DATA:0=ff\nL3CODE:0=ff in intelRdt.l3CacheSchema would work in practice.

Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That certainly sounds like we could get false negatives on IsCATEnabled() if resctrlfs is mounted with -o cdp and is worth investigating further. However, as the code in main would have the exact same deficiency, any improvements to feature detection or how a container's L3 allocation config is applied when RDT/CDP is enabled in the kernel probably should be made in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it seems this can be addressed in a follow-up. @marquiz feel free to create it :)

Copy link
Contributor

@marquiz marquiz Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

K, sounds good. Especially as the old code has the same limitation as you pointed out

Copy link
Member

Choose a reason for hiding this comment

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

Opened #3537 so that we don't forget 👍

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
if _, err := os.Stat(filepath.Join(root, "info", "MB")); err == nil {
mbaEnabled = true
} else if flagsSet.MBA {
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
}
})
}

// 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
Expand All @@ -260,11 +244,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
}

Expand All @@ -275,81 +254,37 @@ 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
Comment on lines +275 to +277
Copy link
Member

Choose a reason for hiding this comment

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

heh (very!) random comment 😅; I recall some "lunch time" discussions I had with @kolyshkin about "where" comments should go for conditional statements.

My take was that putting the comment before the condition is an easy pitfal to describe the code, not the intent (and you may end up with describing the "if" statement, only in words instead of code), whereas putting it inside the branch "gently" forces one to write the branch itself (why we're doing it).

// check if foo is enabled
if foo {
    do stuff
}
if foo {
  // foo is enabled; start the flux capacitor to go back to the future.
  do stuff
}

(Of course, it highly depends on the situation; in some cases you'd want to describe a more complex conditional block as a whole to describe the whole intent.)

return
}

intelRdtRoot = root
// The resctrl fs could have been mounted somewhere nonstandard.
intelRdtRoot, intelRdtRootErr = findIntelRdtMountpointDir()
})

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)
Expand Down Expand Up @@ -493,12 +428,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()
Expand Down Expand Up @@ -554,7 +483,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 == "" {
kolyshkin marked this conversation as resolved.
Show resolved Hide resolved
m.mu.Lock()
defer m.mu.Unlock()
if err := os.RemoveAll(m.GetPath()); err != nil {
Expand Down