From b99ca25ad0f7595408a9d3aed192d749b947f493 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 25 Mar 2021 15:50:23 -0700 Subject: [PATCH 1/7] libct/cg/fs2/memory: fix swap reporting cgroup v1 reports combined mem+swap in stats.MemoryStats.SwapUsage. In cgroup v2, swap is separate. For the sake of compatibility, make v2 report mem+swap as well. This also includes Limit. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/memory.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go index cc8eaeb2e..53b5523d5 100644 --- a/libcontainer/cgroups/fs2/memory.go +++ b/libcontainer/cgroups/fs2/memory.go @@ -4,6 +4,7 @@ package fs2 import ( "bufio" + "math" "os" "strconv" @@ -89,6 +90,9 @@ func statMemory(dirPath string, stats *cgroups.Stats) error { stats.MemoryStats.Stats[t] = v } stats.MemoryStats.Cache = stats.MemoryStats.Stats["file"] + // Unlike cgroup v1 which has memory.use_hierarchy binary knob, + // cgroup v2 is always hierarchical. + stats.MemoryStats.UseHierarchy = true memoryUsage, err := getMemoryDataV2(dirPath, "") if err != nil { @@ -99,9 +103,15 @@ func statMemory(dirPath string, stats *cgroups.Stats) error { if err != nil { return err } + // As cgroup v1 reports SwapUsage values as mem+swap combined, + // while in cgroup v2 swap values do not include memory, + // report combined mem+swap for v1 compatibility. + swapUsage.Usage += memoryUsage.Usage + if swapUsage.Limit != math.MaxUint64 { + swapUsage.Limit += memoryUsage.Limit + } stats.MemoryStats.SwapUsage = swapUsage - stats.MemoryStats.UseHierarchy = true return nil } From a9c47fe70f4fc1c235b859d352e4df78a4ce8964 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 26 Mar 2021 12:41:15 -0700 Subject: [PATCH 2/7] libct/cg/fs[2]/getMemoryData[V2]: optimize Existing code ignores ENOENT error in case we're reading data from controls that might not be enabled. While this is correct, the code can be improved: 1. Check name != "" instead of moduleName != "memory", as these checks are equivalent but the new one is faster. 2. It does not make sense to ignore subsequent errors -- if the control is not available, we won't hit this codepath. 3. Add a comment explaining why we ignore the error. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/memory.go | 13 +++---------- libcontainer/cgroups/fs2/memory.go | 8 ++++---- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 5d52d3257..beae12be9 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -260,7 +260,9 @@ func getMemoryData(path, name string) (cgroups.MemoryData, error) { value, err := fscommon.GetCgroupParamUint(path, usage) if err != nil { - if moduleName != "memory" && os.IsNotExist(err) { + if name != "" && os.IsNotExist(err) { + // Ignore ENOENT as swap and kmem controllers + // are optional in the kernel. return cgroups.MemoryData{}, nil } return cgroups.MemoryData{}, fmt.Errorf("failed to parse %s - %v", usage, err) @@ -268,25 +270,16 @@ func getMemoryData(path, name string) (cgroups.MemoryData, error) { memoryData.Usage = value value, err = fscommon.GetCgroupParamUint(path, maxUsage) if err != nil { - if moduleName != "memory" && os.IsNotExist(err) { - return cgroups.MemoryData{}, nil - } return cgroups.MemoryData{}, fmt.Errorf("failed to parse %s - %v", maxUsage, err) } memoryData.MaxUsage = value value, err = fscommon.GetCgroupParamUint(path, failcnt) if err != nil { - if moduleName != "memory" && os.IsNotExist(err) { - return cgroups.MemoryData{}, nil - } return cgroups.MemoryData{}, fmt.Errorf("failed to parse %s - %v", failcnt, err) } memoryData.Failcnt = value value, err = fscommon.GetCgroupParamUint(path, limit) if err != nil { - if moduleName != "memory" && os.IsNotExist(err) { - return cgroups.MemoryData{}, nil - } return cgroups.MemoryData{}, fmt.Errorf("failed to parse %s - %v", limit, err) } memoryData.Limit = value diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go index 53b5523d5..b1d9ef6b8 100644 --- a/libcontainer/cgroups/fs2/memory.go +++ b/libcontainer/cgroups/fs2/memory.go @@ -127,7 +127,10 @@ func getMemoryDataV2(path, name string) (cgroups.MemoryData, error) { value, err := fscommon.GetCgroupParamUint(path, usage) if err != nil { - if moduleName != "memory" && os.IsNotExist(err) { + if name != "" && os.IsNotExist(err) { + // Ignore EEXIST as there's no swap accounting + // if kernel CONFIG_MEMCG_SWAP is not set or + // swapaccount=0 kernel boot parameter is given. return cgroups.MemoryData{}, nil } return cgroups.MemoryData{}, errors.Wrapf(err, "failed to parse %s", usage) @@ -136,9 +139,6 @@ func getMemoryDataV2(path, name string) (cgroups.MemoryData, error) { value, err = fscommon.GetCgroupParamUint(path, limit) if err != nil { - if moduleName != "memory" && os.IsNotExist(err) { - return cgroups.MemoryData{}, nil - } return cgroups.MemoryData{}, errors.Wrapf(err, "failed to parse %s", limit) } memoryData.Limit = value From 9455395b067fbd243ff3c1d047cbdd0782c8087d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 25 Mar 2021 16:44:54 -0700 Subject: [PATCH 3/7] libct/cg/fs2/memory.Stat: add usage for root cgroup There is no memory.{current,max} for the root node in cgroup v2, and thus stats for "/sys/fs/cgroup" return an error. The same thing works with cgroup v1 (as there are memory.{usage,limit}_in_bytes files in the root node). Emulate stats for /sys/fs/cgroup by getting numbers from /proc/self/meminfo. NOTE that both memory.current (in cgroup v2) and memory.usage_in_bytes (in cgroup v1) include page cache etc into the number, so we do the same when calculating memory usage (as opposed to number reported by "free", which excludes page cache and buffers). [v2: check for cgroup files first, as future kernels might add it] [v3: don't subtract cache from mem_used, simplifying the logic] [Initially, I wanted to avoid parsing yet another /proc file and instead mock some numbers using data from memory.stat but was unable to come up with formulae that make sense.] Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/memory.go | 67 ++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go index b1d9ef6b8..3d1284a7c 100644 --- a/libcontainer/cgroups/fs2/memory.go +++ b/libcontainer/cgroups/fs2/memory.go @@ -7,11 +7,13 @@ import ( "math" "os" "strconv" + "strings" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" "github.com/pkg/errors" + "golang.org/x/sys/unix" ) // numToStr converts an int64 value to a string for writing to a @@ -96,6 +98,11 @@ func statMemory(dirPath string, stats *cgroups.Stats) error { memoryUsage, err := getMemoryDataV2(dirPath, "") if err != nil { + if errors.Is(err, unix.ENOENT) && dirPath == UnifiedMountpoint { + // The root cgroup does not have memory.{current,max} + // so emulate those using data from /proc/meminfo. + return statsFromMeminfo(stats) + } return err } stats.MemoryStats.Usage = memoryUsage @@ -145,3 +152,63 @@ func getMemoryDataV2(path, name string) (cgroups.MemoryData, error) { return memoryData, nil } + +func statsFromMeminfo(stats *cgroups.Stats) error { + f, err := os.Open("/proc/meminfo") + if err != nil { + return err + } + defer f.Close() + + // Fields we are interested in. + var ( + swap_free uint64 + swap_total uint64 + main_total uint64 + main_free uint64 + ) + mem := map[string]*uint64{ + "SwapFree": &swap_free, + "SwapTotal": &swap_total, + "MemTotal": &main_total, + "MemFree": &main_free, + } + + found := 0 + sc := bufio.NewScanner(f) + for sc.Scan() { + parts := strings.SplitN(sc.Text(), ":", 3) + if len(parts) != 2 { + // Should not happen. + continue + } + k := parts[0] + p, ok := mem[k] + if !ok { + // Unknown field -- not interested. + continue + } + vStr := strings.TrimSpace(strings.TrimSuffix(parts[1], " kB")) + *p, err = strconv.ParseUint(vStr, 10, 64) + if err != nil { + return errors.Wrap(err, "parsing /proc/meminfo "+k) + } + + found++ + if found == len(mem) { + // Got everything we need -- skip the rest. + break + } + } + if sc.Err() != nil { + return sc.Err() + } + + stats.MemoryStats.SwapUsage.Usage = (swap_total - swap_free) * 1024 + stats.MemoryStats.SwapUsage.Limit = math.MaxUint64 + + stats.MemoryStats.Usage.Usage = (main_total - main_free) * 1024 + stats.MemoryStats.Usage.Limit = math.MaxUint64 + + return nil +} From 6121f8b69f0914e796470f549e7e67f4e7d6879e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 31 Mar 2021 11:38:32 -0700 Subject: [PATCH 4/7] libct/cg/fs2.Stat: always call statCpu Giuseppe found out that cpu.stat for a cgroup is available even if the cpu controller is not enabled for it. So, let's call statCpu regradress, and ignore ENOENT. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/fs2.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 733f9dcfd..c211f7596 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -130,10 +130,9 @@ func (m *manager) GetStats() (*cgroups.Stats, error) { } } // cpu (since kernel 4.15) - if _, ok := m.controllers["cpu"]; ok { - if err := statCpu(m.dirPath, st); err != nil { - errs = append(errs, err) - } + // Note cpu.stat is available even if the controller is not enabled. + if err := statCpu(m.dirPath, st); err != nil && !os.IsNotExist(err) { + errs = append(errs, err) } // hugetlb (since kernel 5.6) if _, ok := m.controllers["hugetlb"]; ok { From 10f9a982ac1beff3740b9f148b3112b0c5ae8307 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 31 Mar 2021 11:55:07 -0700 Subject: [PATCH 5/7] libct/cg/fs2/getPidsWithoutController: optimize It is inefficient to create an associative map for the whole purpose of counting the number of elements in it, especially if the elements are all unique. It uses more CPU than necessary and creates some work for the garbage collector. The file we read contains PIDs and newlines, and the easiest/fastest way to get the number of PIDs is just to count the newlines. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/pids.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/libcontainer/cgroups/fs2/pids.go b/libcontainer/cgroups/fs2/pids.go index 16e1c2195..72654b094 100644 --- a/libcontainer/cgroups/fs2/pids.go +++ b/libcontainer/cgroups/fs2/pids.go @@ -40,13 +40,8 @@ func statPidsWithoutController(dirPath string, stats *cgroups.Stats) error { if err != nil { return err } - pids := make(map[string]string) - for _, i := range strings.Split(contents, "\n") { - if i != "" { - pids[i] = i - } - } - stats.PidsStats.Current = uint64(len(pids)) + pids := strings.Count(contents, "\n") + stats.PidsStats.Current = uint64(pids) stats.PidsStats.Limit = 0 return nil } From 85416b87428b0a2a227e2c1602557e52dcb83335 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 31 Mar 2021 12:01:39 -0700 Subject: [PATCH 6/7] libct/cg/fs2.statPids: fall back directly When getting pids stats, instead of checking whether the pids controller is available, let's use a fall back function in case pids.current does not exist. This simplifies the logic in fs2.GetStats. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/fs2.go | 10 ++-------- libcontainer/cgroups/fs2/pids.go | 6 +++++- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index c211f7596..030527acd 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -108,14 +108,8 @@ func (m *manager) GetStats() (*cgroups.Stats, error) { } // pids (since kernel 4.5) - if _, ok := m.controllers["pids"]; ok { - if err := statPids(m.dirPath, st); err != nil { - errs = append(errs, err) - } - } else { - if err := statPidsWithoutController(m.dirPath, st); err != nil { - errs = append(errs, err) - } + if err := statPids(m.dirPath, st); err != nil { + errs = append(errs, err) } // memory (since kernel 4.5) if _, ok := m.controllers["memory"]; ok { diff --git a/libcontainer/cgroups/fs2/pids.go b/libcontainer/cgroups/fs2/pids.go index 72654b094..366212347 100644 --- a/libcontainer/cgroups/fs2/pids.go +++ b/libcontainer/cgroups/fs2/pids.go @@ -3,6 +3,7 @@ package fs2 import ( + "os" "path/filepath" "strings" @@ -30,7 +31,7 @@ func setPids(dirPath string, cgroup *configs.Cgroup) error { return nil } -func statPidsWithoutController(dirPath string, stats *cgroups.Stats) error { +func statPidsFromCgroupProcs(dirPath string, stats *cgroups.Stats) error { // if the controller is not enabled, let's read PIDS from cgroups.procs // (or threads if cgroup.threads is enabled) contents, err := fscommon.ReadFile(dirPath, "cgroup.procs") @@ -49,6 +50,9 @@ func statPidsWithoutController(dirPath string, stats *cgroups.Stats) error { func statPids(dirPath string, stats *cgroups.Stats) error { current, err := fscommon.GetCgroupParamUint(dirPath, "pids.current") if err != nil { + if os.IsNotExist(err) { + return statPidsFromCgroupProcs(dirPath, stats) + } return errors.Wrap(err, "failed to parse pids.current") } From 0f8d2b6bed46dff4a5ab772be85529af4a03a8f1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 31 Mar 2021 12:13:48 -0700 Subject: [PATCH 7/7] libct/cg/fs2.Stat: don't look for available controllers Some controllers might still have stats available even if they are disabled (this is definitely so for cpu.stat -- see earlier commit). Some stat methods might implement sensible fallbacks (see previous commit for statPids. In the view of all that, it makes sense to not check if a particular controller is available, but rather ignore ENOENT from it. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/fs2.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 030527acd..0014371b4 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -103,25 +103,18 @@ func (m *manager) GetStats() (*cgroups.Stats, error) { ) st := cgroups.NewStats() - if err := m.getControllers(); err != nil { - return st, err - } // pids (since kernel 4.5) if err := statPids(m.dirPath, st); err != nil { errs = append(errs, err) } // memory (since kernel 4.5) - if _, ok := m.controllers["memory"]; ok { - if err := statMemory(m.dirPath, st); err != nil { - errs = append(errs, err) - } + if err := statMemory(m.dirPath, st); err != nil && !os.IsNotExist(err) { + errs = append(errs, err) } // io (since kernel 4.5) - if _, ok := m.controllers["io"]; ok { - if err := statIo(m.dirPath, st); err != nil { - errs = append(errs, err) - } + if err := statIo(m.dirPath, st); err != nil && !os.IsNotExist(err) { + errs = append(errs, err) } // cpu (since kernel 4.15) // Note cpu.stat is available even if the controller is not enabled. @@ -129,10 +122,8 @@ func (m *manager) GetStats() (*cgroups.Stats, error) { errs = append(errs, err) } // hugetlb (since kernel 5.6) - if _, ok := m.controllers["hugetlb"]; ok { - if err := statHugeTlb(m.dirPath, st); err != nil { - errs = append(errs, err) - } + if err := statHugeTlb(m.dirPath, st); err != nil && !os.IsNotExist(err) { + errs = append(errs, err) } if len(errs) > 0 && !m.rootless { return st, errors.Errorf("error while statting cgroup v2: %+v", errs)