diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 7fde663d9..61be6d7e9 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -17,16 +17,8 @@ import ( ) const ( - numaNodeSymbol = "N" - numaStatColumnSeparator = " " - numaStatKeyValueSeparator = "=" - numaStatMaxColumns = math.MaxUint8 + 1 - numaStatValueIndex = 1 - numaStatTypeIndex = 0 - numaStatColumnSliceLength = 2 - cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes" - cgroupMemoryLimit = "memory.limit_in_bytes" - cgroupMemoryPagesByNuma = "memory.numa_stat" + cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes" + cgroupMemoryLimit = "memory.limit_in_bytes" ) type MemoryGroup struct { @@ -277,47 +269,81 @@ func getMemoryData(path, name string) (cgroups.MemoryData, error) { } func getPageUsageByNUMA(cgroupPath string) (cgroups.PageUsageByNUMA, error) { + const ( + maxColumns = math.MaxUint8 + 1 + filename = "memory.numa_stat" + ) stats := cgroups.PageUsageByNUMA{} - file, err := fscommon.OpenFile(cgroupPath, cgroupMemoryPagesByNuma, os.O_RDONLY) + file, err := fscommon.OpenFile(cgroupPath, filename, os.O_RDONLY) if os.IsNotExist(err) { return stats, nil } else if err != nil { return stats, err } + // File format is documented in linux/Documentation/cgroup-v1/memory.txt + // and it looks like this: + // + // total= N0= N1= ... + // file= N0= N1= ... + // anon= N0= N1= ... + // unevictable= N0= N1= ... + // hierarchical_= N0= N1= ... + scanner := bufio.NewScanner(file) for scanner.Scan() { - var statsType string - statsByType := cgroups.PageStats{Nodes: map[uint8]uint64{}} - columns := strings.SplitN(scanner.Text(), numaStatColumnSeparator, numaStatMaxColumns) + var field *cgroups.PageStats - for _, column := range columns { - pagesByNode := strings.SplitN(column, numaStatKeyValueSeparator, numaStatColumnSliceLength) + line := scanner.Text() + columns := strings.SplitN(line, " ", maxColumns) + for i, column := range columns { + byNode := strings.SplitN(column, "=", 2) + // Some custom kernels have non-standard fields, like + // numa_locality 0 0 0 0 0 0 0 0 0 0 + // numa_exectime 0 + if len(byNode) < 2 { + if i == 0 { + // Ignore/skip those. + break + } else { + // The first column was already validated, + // so be strict to the rest. + return stats, fmt.Errorf("malformed line %q in %s", + line, filename) + } + } + key, val := byNode[0], byNode[1] + if i == 0 { // First column: key is name, val is total. + field = getNUMAField(&stats, key) + if field == nil { // unknown field (new kernel?) + break + } + field.Total, err = strconv.ParseUint(val, 0, 64) + if err != nil { + return stats, err + } + field.Nodes = map[uint8]uint64{} + } else { // Subsequent columns: key is N, val is usage. + if len(key) < 2 || key[0] != 'N' { + // This is definitely an error. + return stats, fmt.Errorf("malformed line %q in %s", + line, filename) + } - if strings.HasPrefix(pagesByNode[numaStatTypeIndex], numaNodeSymbol) { - nodeID, err := strconv.ParseUint(pagesByNode[numaStatTypeIndex][1:], 10, 8) + n, err := strconv.ParseUint(key[1:], 10, 8) if err != nil { return cgroups.PageUsageByNUMA{}, err } - statsByType.Nodes[uint8(nodeID)], err = strconv.ParseUint(pagesByNode[numaStatValueIndex], 0, 64) - if err != nil { - return cgroups.PageUsageByNUMA{}, err - } - } else { - statsByType.Total, err = strconv.ParseUint(pagesByNode[numaStatValueIndex], 0, 64) + usage, err := strconv.ParseUint(val, 10, 64) if err != nil { return cgroups.PageUsageByNUMA{}, err } - statsType = pagesByNode[numaStatTypeIndex] + field.Nodes[uint8(n)] = usage } - err := addNUMAStatsByType(&stats, statsByType, statsType) - if err != nil { - return cgroups.PageUsageByNUMA{}, err - } } } err = scanner.Err() @@ -328,26 +354,24 @@ func getPageUsageByNUMA(cgroupPath string) (cgroups.PageUsageByNUMA, error) { return stats, nil } -func addNUMAStatsByType(stats *cgroups.PageUsageByNUMA, byTypeStats cgroups.PageStats, statsType string) error { - switch statsType { +func getNUMAField(stats *cgroups.PageUsageByNUMA, name string) *cgroups.PageStats { + switch name { case "total": - stats.Total = byTypeStats + return &stats.Total case "file": - stats.File = byTypeStats + return &stats.File case "anon": - stats.Anon = byTypeStats + return &stats.Anon case "unevictable": - stats.Unevictable = byTypeStats + return &stats.Unevictable case "hierarchical_total": - stats.Hierarchical.Total = byTypeStats + return &stats.Hierarchical.Total case "hierarchical_file": - stats.Hierarchical.File = byTypeStats + return &stats.Hierarchical.File case "hierarchical_anon": - stats.Hierarchical.Anon = byTypeStats + return &stats.Hierarchical.Anon case "hierarchical_unevictable": - stats.Hierarchical.Unevictable = byTypeStats - default: - return fmt.Errorf("unsupported NUMA page type found: %s", statsType) + return &stats.Hierarchical.Unevictable } return nil } diff --git a/libcontainer/cgroups/fs/memory_test.go b/libcontainer/cgroups/fs/memory_test.go index 52244fc7b..00fb338b7 100644 --- a/libcontainer/cgroups/fs/memory_test.go +++ b/libcontainer/cgroups/fs/memory_test.go @@ -25,11 +25,18 @@ unevictable=0 N0=0 N1=0 N2=0 N3=0 hierarchical_total=768133 N0=509113 N1=138887 N2=20464 N3=99669 hierarchical_file=722017 N0=496516 N1=119997 N2=20181 N3=85323 hierarchical_anon=46096 N0=12597 N1=18890 N2=283 N3=14326 -hierarchical_unevictable=20 N0=0 N1=0 N2=0 N3=20` +hierarchical_unevictable=20 N0=0 N1=0 N2=0 N3=20 +` memoryNUMAStatNoHierarchyContents = `total=44611 N0=32631 N1=7501 N2=1982 N3=2497 file=44428 N0=32614 N1=7335 N2=1982 N3=2497 anon=183 N0=17 N1=166 N2=0 N3=0 -unevictable=0 N0=0 N1=0 N2=0 N3=0` +unevictable=0 N0=0 N1=0 N2=0 N3=0 +` + // Some custom kernels has extra fields that should be ignored + memoryNUMAStatExtraContents = `numa_locality 0 0 0 0 0 0 0 0 0 0 +numa_exectime 0 +whatever=100 N0=0 +` ) func TestMemorySetMemory(t *testing.T) { @@ -288,7 +295,7 @@ func TestMemoryStats(t *testing.T) { "memory.kmem.failcnt": memoryFailcnt, "memory.kmem.limit_in_bytes": memoryLimitContents, "memory.use_hierarchy": memoryUseHierarchyContents, - "memory.numa_stat": memoryNUMAStatContents, + "memory.numa_stat": memoryNUMAStatContents + memoryNUMAStatExtraContents, }) memory := &MemoryGroup{} @@ -486,7 +493,7 @@ func TestNoHierarchicalNumaStat(t *testing.T) { helper := NewCgroupTestUtil("memory", t) defer helper.cleanup() helper.writeFileContents(map[string]string{ - "memory.numa_stat": memoryNUMAStatNoHierarchyContents, + "memory.numa_stat": memoryNUMAStatNoHierarchyContents + memoryNUMAStatExtraContents, }) actualStats, err := getPageUsageByNUMA(helper.CgroupPath) @@ -505,6 +512,49 @@ func TestNoHierarchicalNumaStat(t *testing.T) { expectPageUsageByNUMAEquals(t, pageUsageByNUMA, actualStats) } +func TestBadNumaStat(t *testing.T) { + memoryNUMAStatBadContents := []struct { + desc, contents string + }{ + { + desc: "Nx where x is not a number", + contents: `total=44611 N0=44611, +file=44428 Nx=0 +`, + }, { + desc: "Nx where x > 255", + contents: `total=44611 N333=444`, + }, { + desc: "Nx argument missing", + contents: `total=44611 N0=123 N1=`, + }, { + desc: "Nx argument is not a number", + contents: `total=44611 N0=123 N1=a`, + }, { + desc: "Missing = after Nx", + contents: `total=44611 N0=123 N1`, + }, { + desc: "No Nx at non-first position", + contents: `total=44611 N0=32631 +file=44428 N0=32614 +anon=183 N0=12 badone +`, + }, + } + helper := NewCgroupTestUtil("memory", t) + defer helper.cleanup() + for _, c := range memoryNUMAStatBadContents { + helper.writeFileContents(map[string]string{ + "memory.numa_stat": c.contents, + }) + + _, err := getPageUsageByNUMA(helper.CgroupPath) + if err == nil { + t.Errorf("case %q: expected error, got nil", c.desc) + } + } +} + func TestWithoutNumaStat(t *testing.T) { helper := NewCgroupTestUtil("memory", t) defer helper.cleanup() diff --git a/libcontainer/cgroups/fs/name.go b/libcontainer/cgroups/fs/name.go index 9a5af709d..7cada9149 100644 --- a/libcontainer/cgroups/fs/name.go +++ b/libcontainer/cgroups/fs/name.go @@ -19,7 +19,7 @@ func (s *NameGroup) Name() string { func (s *NameGroup) Apply(path string, d *cgroupData) error { if s.Join { // ignore errors if the named cgroup does not exist - join(path, d.pid) + _ = join(path, d.pid) } return nil }