diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index b574d3a05f7..fb4fcc7f75b 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -13,25 +13,22 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) -var ( - subsystems = []subsystem{ - &CpusetGroup{}, - &DevicesGroup{}, - &MemoryGroup{}, - &CpuGroup{}, - &CpuacctGroup{}, - &PidsGroup{}, - &BlkioGroup{}, - &HugetlbGroup{}, - &NetClsGroup{}, - &NetPrioGroup{}, - &PerfEventGroup{}, - &FreezerGroup{}, - &RdmaGroup{}, - &NameGroup{GroupName: "name=systemd", Join: true}, - } - HugePageSizes, _ = cgroups.GetHugePageSize() -) +var subsystems = []subsystem{ + &CpusetGroup{}, + &DevicesGroup{}, + &MemoryGroup{}, + &CpuGroup{}, + &CpuacctGroup{}, + &PidsGroup{}, + &BlkioGroup{}, + &HugetlbGroup{}, + &NetClsGroup{}, + &NetPrioGroup{}, + &PerfEventGroup{}, + &FreezerGroup{}, + &RdmaGroup{}, + &NameGroup{GroupName: "name=systemd", Join: true}, +} var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist") diff --git a/libcontainer/cgroups/fs/hugetlb.go b/libcontainer/cgroups/fs/hugetlb.go index 86650128c8d..8ddd6fdd837 100644 --- a/libcontainer/cgroups/fs/hugetlb.go +++ b/libcontainer/cgroups/fs/hugetlb.go @@ -29,11 +29,11 @@ func (s *HugetlbGroup) Set(path string, r *configs.Resources) error { } func (s *HugetlbGroup) GetStats(path string, stats *cgroups.Stats) error { - hugetlbStats := cgroups.HugetlbStats{} if !cgroups.PathExists(path) { return nil } - for _, pageSize := range HugePageSizes { + hugetlbStats := cgroups.HugetlbStats{} + for _, pageSize := range cgroups.HugePageSizes() { usage := "hugetlb." + pageSize + ".usage_in_bytes" value, err := fscommon.GetCgroupParamUint(path, usage) if err != nil { diff --git a/libcontainer/cgroups/fs/hugetlb_test.go b/libcontainer/cgroups/fs/hugetlb_test.go index ba54f35a16c..f4aea7eb552 100644 --- a/libcontainer/cgroups/fs/hugetlb_test.go +++ b/libcontainer/cgroups/fs/hugetlb_test.go @@ -31,14 +31,14 @@ func TestHugetlbSetHugetlb(t *testing.T) { hugetlbAfter = 512 ) - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { writeFileContents(t, path, map[string]string{ fmt.Sprintf(limit, pageSize): strconv.Itoa(hugetlbBefore), }) } r := &configs.Resources{} - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { r.HugetlbLimit = []*configs.HugepageLimit{ { Pagesize: pageSize, @@ -51,7 +51,7 @@ func TestHugetlbSetHugetlb(t *testing.T) { } } - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { limit := fmt.Sprintf(limit, pageSize) value, err := fscommon.GetCgroupParamUint(path, limit) if err != nil { @@ -65,7 +65,7 @@ func TestHugetlbSetHugetlb(t *testing.T) { func TestHugetlbStats(t *testing.T) { path := tempDir(t, "hugetlb") - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { writeFileContents(t, path, map[string]string{ fmt.Sprintf(usage, pageSize): hugetlbUsageContents, fmt.Sprintf(maxUsage, pageSize): hugetlbMaxUsageContents, @@ -80,7 +80,7 @@ func TestHugetlbStats(t *testing.T) { t.Fatal(err) } expectedStats := cgroups.HugetlbStats{Usage: 128, MaxUsage: 256, Failcnt: 100} - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { expectHugetlbStatEquals(t, expectedStats, actualStats.HugetlbStats[pageSize]) } } @@ -101,7 +101,7 @@ func TestHugetlbStatsNoUsageFile(t *testing.T) { func TestHugetlbStatsNoMaxUsageFile(t *testing.T) { path := tempDir(t, "hugetlb") - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { writeFileContents(t, path, map[string]string{ fmt.Sprintf(usage, pageSize): hugetlbUsageContents, }) @@ -117,7 +117,7 @@ func TestHugetlbStatsNoMaxUsageFile(t *testing.T) { func TestHugetlbStatsBadUsageFile(t *testing.T) { path := tempDir(t, "hugetlb") - for _, pageSize := range HugePageSizes { + for _, pageSize := range cgroups.HugePageSizes() { writeFileContents(t, path, map[string]string{ fmt.Sprintf(usage, pageSize): "bad", maxUsage: hugetlbMaxUsageContents, diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go index a228d3f2599..c92a7e64af0 100644 --- a/libcontainer/cgroups/fs2/hugetlb.go +++ b/libcontainer/cgroups/fs2/hugetlb.go @@ -26,10 +26,8 @@ func setHugeTlb(dirPath string, r *configs.Resources) error { } func statHugeTlb(dirPath string, stats *cgroups.Stats) error { - hugePageSizes, _ := cgroups.GetHugePageSize() hugetlbStats := cgroups.HugetlbStats{} - - for _, pagesize := range hugePageSizes { + for _, pagesize := range cgroups.HugePageSizes() { value, err := fscommon.GetCgroupParamUint(dirPath, "hugetlb."+pagesize+".current") if err != nil { return err diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 6c96882c680..13ebf52abe4 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -302,40 +302,61 @@ func RemovePaths(paths map[string]string) (err error) { return fmt.Errorf("Failed to remove paths: %v", paths) } -func GetHugePageSize() ([]string, error) { - dir, err := os.OpenFile("/sys/kernel/mm/hugepages", unix.O_DIRECTORY|unix.O_RDONLY, 0) - if err != nil { - return nil, err - } - files, err := dir.Readdirnames(0) - dir.Close() - if err != nil { - return nil, err - } +var ( + hugePageSizes []string + initHPSOnce sync.Once +) - return getHugePageSizeFromFilenames(files) +func HugePageSizes() []string { + initHPSOnce.Do(func() { + dir, err := os.OpenFile("/sys/kernel/mm/hugepages", unix.O_DIRECTORY|unix.O_RDONLY, 0) + if err != nil { + return + } + files, err := dir.Readdirnames(0) + dir.Close() + if err != nil { + return + } + + hugePageSizes, err = getHugePageSizeFromFilenames(files) + if err != nil { + logrus.Warn("HugePageSizes: ", err) + } + }) + + return hugePageSizes } func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) { pageSizes := make([]string, 0, len(fileNames)) + var warn error for _, file := range fileNames { // example: hugepages-1048576kB val := strings.TrimPrefix(file, "hugepages-") if len(val) == len(file) { - // unexpected file name: no prefix found + // Unexpected file name: no prefix found, ignore it. continue } - // The suffix is always "kB" (as of Linux 5.9) + // The suffix is always "kB" (as of Linux 5.13). If we find + // something else, produce an error but keep going. eLen := len(val) - 2 val = strings.TrimSuffix(val, "kB") if len(val) != eLen { - logrus.Warnf("GetHugePageSize: %s: invalid filename suffix (expected \"kB\")", file) + // Highly unlikely. + if warn == nil { + warn = errors.New(file + `: invalid suffix (expected "kB")`) + } continue } size, err := strconv.Atoi(val) if err != nil { - return nil, err + // Highly unlikely. + if warn == nil { + warn = fmt.Errorf("%s: %w", file, err) + } + continue } // Model after https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/hugetlb_cgroup.c?id=eff48ddeab782e35e58ccc8853f7386bbae9dec4#n574 // but in our case the size is in KB already. @@ -349,7 +370,7 @@ func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) { pageSizes = append(pageSizes, val) } - return pageSizes, nil + return pageSizes, warn } // GetPids returns all pids, that were added to cgroup at path. diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index 929cf511a83..c9feb84484c 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -8,7 +8,6 @@ import ( "testing" "github.com/moby/sys/mountinfo" - "github.com/sirupsen/logrus" ) const fedoraMountinfo = `15 35 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw @@ -448,19 +447,6 @@ func TestFindCgroupMountpointAndRoot(t *testing.T) { } } -func BenchmarkGetHugePageSize(b *testing.B) { - var ( - output []string - err error - ) - for i := 0; i < b.N; i++ { - output, err = GetHugePageSize() - } - if err != nil || len(output) == 0 { - b.Fatal("unexpected results") - } -} - func BenchmarkGetHugePageSizeImpl(b *testing.B) { var ( input = []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"} @@ -477,71 +463,78 @@ func BenchmarkGetHugePageSizeImpl(b *testing.B) { func TestGetHugePageSizeImpl(t *testing.T) { testCases := []struct { + doc string input []string output []string isErr bool - isWarn bool }{ { + doc: "normal input", input: []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"}, output: []string{"1GB", "2MB", "32MB", "64KB"}, }, { + doc: "empty input", input: []string{}, output: []string{}, }, - { // not a number + { + doc: "not a number", input: []string{"hugepages-akB"}, isErr: true, }, - { // no prefix (silently skipped) + { + doc: "no prefix (silently skipped)", input: []string{"1024kB"}, }, - { // invalid prefix (silently skipped) + { + doc: "invalid prefix (silently skipped)", input: []string{"whatever-1024kB"}, }, - { // invalid suffix (skipped with a warning) - input: []string{"hugepages-1024gB"}, - isWarn: true, + { + doc: "invalid suffix", + input: []string{"hugepages-1024gB"}, + isErr: true, }, - { // no suffix (skipped with a warning) - input: []string{"hugepages-1024"}, - isWarn: true, + { + doc: "no suffix", + input: []string{"hugepages-1024"}, + isErr: true, + }, + { + doc: "mixed valid and invalid entries", + input: []string{"hugepages-4194304kB", "hugepages-2048kB", "hugepages-akB", "hugepages-64kB"}, + output: []string{"4GB", "2MB", "64KB"}, + isErr: true, + }, + { + doc: "more mixed valid and invalid entries", + input: []string{"hugepages-2048kB", "hugepages-kB", "hugepages-64kB"}, + output: []string{"2MB", "64KB"}, + isErr: true, }, } - // Need to catch warnings. - savedOut := logrus.StandardLogger().Out - defer logrus.SetOutput(savedOut) - warns := new(bytes.Buffer) - logrus.SetOutput(warns) - for _, c := range testCases { - warns.Reset() - output, err := getHugePageSizeFromFilenames(c.input) - if err != nil { - if !c.isErr { - t.Errorf("input %v, expected nil, got error: %v", c.input, err) + c := c + t.Run(c.doc, func(t *testing.T) { + output, err := getHugePageSizeFromFilenames(c.input) + t.Log("input:", c.input, "; output:", output, "; err:", err) + if err != nil { + if !c.isErr { + t.Errorf("input %v, expected nil, got error: %v", c.input, err) + } + // no more checks + return } - // no more checks - continue - } - if c.isErr { - t.Errorf("input %v, expected error, got error: nil, output: %v", c.input, output) - // no more checks - continue - } - // check for warnings - if c.isWarn && warns.Len() == 0 { - t.Errorf("input %v, expected a warning, got none", c.input) - } - if !c.isWarn && warns.Len() > 0 { - t.Errorf("input %v, unexpected warning: %s", c.input, warns.String()) - } - // check output - if len(output) != len(c.output) || (len(output) > 0 && !reflect.DeepEqual(output, c.output)) { - t.Errorf("input %v, expected %v, got %v", c.input, c.output, output) - } + if c.isErr { + t.Errorf("input %v, expected error, got error: nil, output: %v", c.input, output) + } + // check output + if len(output) != len(c.output) || (len(output) > 0 && !reflect.DeepEqual(output, c.output)) { + t.Errorf("input %v, expected %v, got %v", c.input, c.output, output) + } + }) } }