From 360981ae1d323e7d15605da34f425e56289f61f0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Sep 2020 15:15:02 -0700 Subject: [PATCH] libct/cgroups: rewrite getHugePageSizeFromFilenames This is a function to convert huge page sizes (obtained by reading /sys/kernel/mm/hugepages directory entries) to strings user for hugetlb cgroup controller resource files. Those strings are when used to get the hugetlb resource statistics. This function used external library, floating point numbers, and can (theoretically) produce invalid values, since the kernel only uses KB, MB, and GB suffixes. Rewrite it to produce the same strings as used in the kernel (see [1]). As a result, it's also faster, more future-proof (entries that do not start with "hugepages-" and/or incorrect suffix are skipped), and does more input sanity checks. As a side effect, libcontainer no longer depends on docker/go-units. While at it, add more test cases. Before: BenchmarkGetHugePageSize-8 187452 6265 ns/op BenchmarkGetHugePageSizeImpl-8 396769 2998 ns/op After: BenchmarkGetHugePageSize-8 222898 4554 ns/op BenchmarkGetHugePageSizeImpl-8 4738924 241 ns/op NOTE on removing HugePageSizeUnitList -- this was added by commit 6f77e35da and was used by kubernetes code in [2], which was later superceded by [3], so there are (hopefully) no external users. If there are any, they should not be doing that. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/hugetlb_cgroup.c?id=eff48ddeab782e35e58ccc8853f7386bbae9dec4#n574 [2] https://github.com/kubernetes/kubernetes/pull/78495 [3] https://github.com/kubernetes/kubernetes/pull/84154 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils.go | 45 ++++++++++------- libcontainer/cgroups/utils_test.go | 77 ++++++++++++++++++++++-------- 2 files changed, 86 insertions(+), 36 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 10baebc6b..bc98375ea 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -15,7 +15,6 @@ import ( "sync" "time" - units "github.com/docker/go-units" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -30,13 +29,6 @@ var ( isUnified bool ) -// HugePageSizeUnitList is a list of the units used by the linux kernel when -// naming the HugePage control files. -// https://www.kernel.org/doc/Documentation/cgroup-v1/hugetlb.txt -// TODO Since the kernel only use KB, MB and GB; TB and PB should be removed, -// depends on https://github.com/docker/go-units/commit/a09cd47f892041a4fac473133d181f5aea6fa393 -var HugePageSizeUnitList = []string{"B", "KB", "MB", "GB", "TB", "PB"} - // IsCgroup2UnifiedMode returns whether we are running in cgroup v2 unified mode. func IsCgroup2UnifiedMode() bool { isUnifiedOnce.Do(func() { @@ -299,15 +291,36 @@ func GetHugePageSize() ([]string, error) { } func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) { - var pageSizes []string - for _, fileName := range fileNames { - nameArray := strings.Split(fileName, "-") - pageSize, err := units.RAMInBytes(nameArray[1]) - if err != nil { - return []string{}, err + pageSizes := make([]string, 0, len(fileNames)) + + for _, file := range fileNames { + // example: hugepages-1048576kB + val := strings.TrimPrefix(file, "hugepages-") + if len(val) == len(file) { + // unexpected file name: no prefix found + continue } - sizeString := units.CustomSize("%g%s", float64(pageSize), 1024.0, HugePageSizeUnitList) - pageSizes = append(pageSizes, sizeString) + // The suffix is always "kB" (as of Linux 5.9) + eLen := len(val) - 2 + val = strings.TrimSuffix(val, "kB") + if len(val) != eLen { + logrus.Warnf("GetHugePageSize: %s: invalid filename suffix (expected \"kB\")", file) + continue + } + size, err := strconv.Atoi(val) + if err != nil { + return nil, err + } + // 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. + if size >= (1 << 20) { + val = strconv.Itoa(size>>20) + "GB" + } else if size >= (1 << 10) { + val = strconv.Itoa(size>>10) + "MB" + } else { + val += "KB" + } + pageSizes = append(pageSizes, val) } return pageSizes, nil diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index dc41074dd..4a5d95eb6 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -4,11 +4,12 @@ package cgroups import ( "bytes" - "errors" "fmt" "reflect" "strings" "testing" + + "github.com/sirupsen/logrus" ) const fedoraMountinfo = `15 35 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw @@ -429,36 +430,72 @@ func BenchmarkGetHugePageSizeImpl(b *testing.B) { } func TestGetHugePageSizeImpl(t *testing.T) { - testCases := []struct { - inputFiles []string - outputPageSizes []string - err error + input []string + output []string + isErr bool + isWarn bool }{ { - inputFiles: []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"}, - outputPageSizes: []string{"1GB", "2MB", "32MB", "64KB"}, - err: nil, + input: []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"}, + output: []string{"1GB", "2MB", "32MB", "64KB"}, }, { - inputFiles: []string{}, - outputPageSizes: []string{}, - err: nil, + input: []string{}, + output: []string{}, }, - { - inputFiles: []string{"hugepages-a"}, - outputPageSizes: []string{}, - err: errors.New("invalid size: 'a'"), + { // not a number + input: []string{"hugepages-akB"}, + isErr: true, + }, + { // no prefix (silently skipped) + input: []string{"1024kB"}, + }, + { // invalid prefix (silently skipped) + input: []string{"whatever-1024kB"}, + }, + { // invalid suffix (skipped with a warning) + input: []string{"hugepages-1024gB"}, + isWarn: true, + }, + { // no suffix (skipped with a warning) + input: []string{"hugepages-1024"}, + isWarn: true, }, } + // Need to catch warnings. + savedOut := logrus.StandardLogger().Out + defer logrus.SetOutput(savedOut) + warns := new(bytes.Buffer) + logrus.SetOutput(warns) + for _, c := range testCases { - pageSizes, err := getHugePageSizeFromFilenames(c.inputFiles) - if len(pageSizes) != 0 && len(c.outputPageSizes) != 0 && !reflect.DeepEqual(pageSizes, c.outputPageSizes) { - t.Errorf("expected %s, got %s", c.outputPageSizes, pageSizes) + warns.Reset() + output, err := getHugePageSizeFromFilenames(c.input) + if err != nil { + t.Logf("(input %v, error %v)", c.input, err) + if !c.isErr { + t.Error("unexpected error ^^^^") + } + // no more checks + continue } - if err != nil && err.Error() != c.err.Error() { - t.Errorf("expected error %s, got %s", c.err, err) + 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) } } }