Merge pull request opencontainers#3234 from kolyshkin/hugepage-v2

libct/cg: refactor/improve/rename GetHugePageSize -> HugePageSizes

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")
 

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 {

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,

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

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.

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)
+			}
+		})
 	}
 }