From a4d4c4dd9f1b91088f547dad5fb948f831a855c0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 5 Oct 2021 17:14:16 -0700 Subject: [PATCH 1/4] libct/cg: GetHugePageSize -> HugePageSizes 1. Since GetHugePageSize do not have any external users (checked by sourcegraph), and no internal user ever uses its second return value (the error), let's drop it. 2. Rename GetHugePageSize -> HugePageSizes (drop the Get prefix as per Go guidelines, add suffix since we return many sizes). Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs.go | 2 +- libcontainer/cgroups/fs2/hugetlb.go | 2 +- libcontainer/cgroups/utils.go | 11 ++++++----- libcontainer/cgroups/utils_test.go | 11 ++++------- 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index b574d3a05f7..90a0b49b0e5 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -30,7 +30,7 @@ var ( &RdmaGroup{}, &NameGroup{GroupName: "name=systemd", Join: true}, } - HugePageSizes, _ = cgroups.GetHugePageSize() + HugePageSizes = cgroups.HugePageSizes() ) var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist") diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go index a228d3f2599..3955f0781a8 100644 --- a/libcontainer/cgroups/fs2/hugetlb.go +++ b/libcontainer/cgroups/fs2/hugetlb.go @@ -26,7 +26,7 @@ func setHugeTlb(dirPath string, r *configs.Resources) error { } func statHugeTlb(dirPath string, stats *cgroups.Stats) error { - hugePageSizes, _ := cgroups.GetHugePageSize() + hugePageSizes := cgroups.HugePageSizes() hugetlbStats := cgroups.HugetlbStats{} for _, pagesize := range hugePageSizes { diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 6c96882c680..f96e4dedf1a 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -302,18 +302,19 @@ func RemovePaths(paths map[string]string) (err error) { return fmt.Errorf("Failed to remove paths: %v", paths) } -func GetHugePageSize() ([]string, error) { +func HugePageSizes() []string { dir, err := os.OpenFile("/sys/kernel/mm/hugepages", unix.O_DIRECTORY|unix.O_RDONLY, 0) if err != nil { - return nil, err + return nil } files, err := dir.Readdirnames(0) dir.Close() if err != nil { - return nil, err + return nil } - return getHugePageSizeFromFilenames(files) + hugePageSizes, _ := getHugePageSizeFromFilenames(files) + return hugePageSizes } func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) { @@ -330,7 +331,7 @@ func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) { eLen := len(val) - 2 val = strings.TrimSuffix(val, "kB") if len(val) != eLen { - logrus.Warnf("GetHugePageSize: %s: invalid filename suffix (expected \"kB\")", file) + logrus.Warnf("HugePageSizes: %s: invalid filename suffix (expected \"kB\")", file) continue } size, err := strconv.Atoi(val) diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index 929cf511a83..a0a261dee1a 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -448,15 +448,12 @@ func TestFindCgroupMountpointAndRoot(t *testing.T) { } } -func BenchmarkGetHugePageSize(b *testing.B) { - var ( - output []string - err error - ) +func BenchmarkHugePageSizes(b *testing.B) { + var output []string for i := 0; i < b.N; i++ { - output, err = GetHugePageSize() + output = HugePageSizes() } - if err != nil || len(output) == 0 { + if len(output) == 0 { b.Fatal("unexpected results") } } From 39d4c8d5f969eb0623d72c744c2a83ef89d1ea2d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 5 Oct 2021 17:18:03 -0700 Subject: [PATCH 2/4] libct/cg: lazy init for HugePageSizes I have noticed that libct/cg/fs allocates 8K during init on every runc execution: > init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.028 ms clock, 8512 bytes, 13 allocs Apparently this is caused by global HugePageSizes variable init, which is only used from GetStats (i.e. it is never used by runc itself). Remove it, and use HugePageSizes() directly instead. Make it init-once, so that GetStats (which, I guess, is periodically called by kubernetes) does not re-read huge page sizes over and over. This also removes 12 allocs and 8K from libct/cg/fs init section: > $ time GODEBUG=inittrace=1 ./runc --help 2>&1 | grep cgroups/fs > init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.003 ms clock, 16 bytes, 1 allocs Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs.go | 35 +++++++++++-------------- libcontainer/cgroups/fs/hugetlb.go | 4 +-- libcontainer/cgroups/fs/hugetlb_test.go | 14 +++++----- libcontainer/cgroups/fs2/hugetlb.go | 4 +-- libcontainer/cgroups/utils.go | 28 +++++++++++++------- libcontainer/cgroups/utils_test.go | 10 ------- 6 files changed, 44 insertions(+), 51 deletions(-) diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index 90a0b49b0e5..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.HugePageSizes() -) +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 3955f0781a8..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.HugePageSizes() 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 f96e4dedf1a..1577c41efc8 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -302,18 +302,26 @@ func RemovePaths(paths map[string]string) (err error) { return fmt.Errorf("Failed to remove paths: %v", paths) } +var ( + hugePageSizes []string + initHPSOnce sync.Once +) + func HugePageSizes() []string { - dir, err := os.OpenFile("/sys/kernel/mm/hugepages", unix.O_DIRECTORY|unix.O_RDONLY, 0) - if err != nil { - return nil - } - files, err := dir.Readdirnames(0) - dir.Close() - if err != nil { - return nil - } + 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, _ = getHugePageSizeFromFilenames(files) + }) - hugePageSizes, _ := getHugePageSizeFromFilenames(files) return hugePageSizes } diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index a0a261dee1a..708015d4447 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -448,16 +448,6 @@ func TestFindCgroupMountpointAndRoot(t *testing.T) { } } -func BenchmarkHugePageSizes(b *testing.B) { - var output []string - for i := 0; i < b.N; i++ { - output = HugePageSizes() - } - if len(output) == 0 { - b.Fatal("unexpected results") - } -} - func BenchmarkGetHugePageSizeImpl(b *testing.B) { var ( input = []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"} From f13a93257078dccd0c9471d167775fcbcc17ed62 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 6 Oct 2021 13:50:12 -0700 Subject: [PATCH 3/4] libct/cg: HugePageSizes: simplify code and test 1. Instead of distinguishing between errors and warnings, let's treat all errors as warnings, thus simplifying the code. This changes the function behaviour for input like hugepages-BadNumberKb -- previously, the error from Atoi("BadNumber") was considered fatal, now it's just another warnings. 2. Move the warning logging to HugePageSizes, thus simplifying the test case, which no longer needs to read what logrus writes. Note that we do not want to log all the warnings (as chances are very low we'll get any, and if we do this means the code need to be updated), only the first one. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils.go | 24 ++++++++++++----- libcontainer/cgroups/utils_test.go | 41 +++++++++++++----------------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 1577c41efc8..13ebf52abe4 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -319,7 +319,10 @@ func HugePageSizes() []string { return } - hugePageSizes, _ = getHugePageSizeFromFilenames(files) + hugePageSizes, err = getHugePageSizeFromFilenames(files) + if err != nil { + logrus.Warn("HugePageSizes: ", err) + } }) return hugePageSizes @@ -327,24 +330,33 @@ func HugePageSizes() []string { 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("HugePageSizes: %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. @@ -358,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 708015d4447..259313ba545 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 @@ -467,7 +466,6 @@ func TestGetHugePageSizeImpl(t *testing.T) { input []string output []string isErr bool - isWarn bool }{ { input: []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"}, @@ -487,25 +485,29 @@ func TestGetHugePageSizeImpl(t *testing.T) { { // invalid prefix (silently skipped) input: []string{"whatever-1024kB"}, }, - { // invalid suffix (skipped with a warning) - input: []string{"hugepages-1024gB"}, - isWarn: true, + { // invalid suffix + input: []string{"hugepages-1024gB"}, + isErr: true, + }, + { // no suffix + input: []string{"hugepages-1024"}, + isErr: true, }, - { // no suffix (skipped with a warning) - input: []string{"hugepages-1024"}, - isWarn: true, + { // mixed valid and invalid entries + input: []string{"hugepages-4194304kB", "hugepages-2048kB", "hugepages-akB", "hugepages-64kB"}, + output: []string{"4GB", "2MB", "64KB"}, + isErr: true, + }, + { // 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) + 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) @@ -515,15 +517,6 @@ func TestGetHugePageSizeImpl(t *testing.T) { } 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)) { From 1da84d1afff11f24c7084eb3e1b706b21ba8d3af Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 19 Nov 2021 11:59:56 -0800 Subject: [PATCH 4/4] libct/cg: TestGetHugePageSizeImpl: use t.Run Move test case comments to doc strings, and use t.Run. Suggested-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils_test.go | 57 ++++++++++++++++++------------ 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index 259313ba545..c9feb84484c 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -463,42 +463,52 @@ func BenchmarkGetHugePageSizeImpl(b *testing.B) { func TestGetHugePageSizeImpl(t *testing.T) { testCases := []struct { + doc string input []string output []string isErr 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 + { + doc: "invalid suffix", input: []string{"hugepages-1024gB"}, isErr: true, }, - { // no suffix + { + doc: "no suffix", input: []string{"hugepages-1024"}, isErr: true, }, - { // mixed valid and invalid entries + { + doc: "mixed valid and invalid entries", input: []string{"hugepages-4194304kB", "hugepages-2048kB", "hugepages-akB", "hugepages-64kB"}, output: []string{"4GB", "2MB", "64KB"}, isErr: true, }, - { // mixed valid and invalid entries + { + doc: "more mixed valid and invalid entries", input: []string{"hugepages-2048kB", "hugepages-kB", "hugepages-64kB"}, output: []string{"2MB", "64KB"}, isErr: true, @@ -506,22 +516,25 @@ func TestGetHugePageSizeImpl(t *testing.T) { } for _, c := range testCases { - 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) + 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) - } - // 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) + } + }) } }