Merge pull request #2755 from kolyshkin/numa-stat

libct/cg/fs: getPageUsageByNUMA: rewrite/optimize, fix panic, add more tests

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=<total pages> N0=<node 0 pages> N1=<node 1 pages> ...
+	// file=<total file pages> N0=<node 0 pages> N1=<node 1 pages> ...
+	// anon=<total anon pages> N0=<node 0 pages> N1=<node 1 pages> ...
+	// unevictable=<total anon pages> N0=<node 0 pages> N1=<node 1 pages> ...
+	// hierarchical_<counter>=<counter pages> N0=<node 0 pages> N1=<node 1 pages> ...
+
 	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<id>, 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
 }

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

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
 }