Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cpuinfo: fixes and enhancements #297

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

cpuinfo: fixes and enhancements #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0174b24
cpuinfo_test: fix test data
kolyshkin May 18, 2020
928d8ea
parseCPUInfoARM: fix for kernel 3.8+
kolyshkin May 18, 2020
611c664
parseCPUInfoARM: only parse Features once
kolyshkin May 18, 2020
c2b7b68
parseCPUInfo: optimize line scan
kolyshkin May 18, 2020
a175916
cpuinfo: read data in place
kolyshkin May 18, 2020
a6effd3
parseCPUInfo*: add error checking
kolyshkin May 18, 2020
e1d29d8
cpuinfo: rm GetFirstNonEmptyLine, simplify scan
kolyshkin May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
136 changes: 52 additions & 84 deletions cpuinfo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,12 @@ package procfs

import (
"bufio"
"bytes"
"errors"
"io"
"os"
"regexp"
"strconv"
"strings"

"github.com/prometheus/procfs/internal/util"
)

// CPUInfo contains general information about a system CPU found in /proc/cpuinfo
Expand Down Expand Up @@ -64,36 +63,26 @@ var (
// CPUInfo returns information about current system CPUs.
// See https://www.kernel.org/doc/Documentation/filesystems/proc.txt
func (fs FS) CPUInfo() ([]CPUInfo, error) {
data, err := util.ReadFileNoStat(fs.proc.Path("cpuinfo"))
file, err := os.Open(fs.proc.Path("cpuinfo"))
if err != nil {
return nil, err
}
return parseCPUInfo(data)
defer file.Close()
return parseCPUInfo(file)
}

func parseCPUInfoX86(info []byte) ([]CPUInfo, error) {
scanner := bufio.NewScanner(bytes.NewReader(info))
func parseCPUInfoX86(info io.Reader) ([]CPUInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided to prefer reading everything in a buffer first. While this is unlikely to change mid reading, I think it's what we decided to to in general. But not sure, @SuperQ wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been doing that previously, although I think this pattern is fine also. The OS is probably buffering the file content anyway. And I think io.Reader is better to use as a parameter for most parseXXX functions because it's a little more generic than a string or byte slice.

If we do want to buffer the entire file first, then it would probably be better to create a reader from the byte slice and then pass the reader to the parse function instead of passing the byte slice directly. The byte slice parameter here might just be left over from me thinking I would need random access instead of line by line access to parse certain file formats.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds reasonable, then let's keep is a reader.

scanner := bufio.NewScanner(info)

// find the first "processor" line
firstLine := firstNonEmptyLine(scanner)
if !strings.HasPrefix(firstLine, "processor") || !strings.Contains(firstLine, ":") {
return nil, errors.New("invalid cpuinfo file: " + firstLine)
}
field := strings.SplitN(firstLine, ": ", 2)
v, err := strconv.ParseUint(field[1], 0, 32)
if err != nil {
return nil, err
}
firstcpu := CPUInfo{Processor: uint(v)}
cpuinfo := []CPUInfo{firstcpu}
i := 0
cpuinfo := []CPUInfo{}
i := -1

for scanner.Scan() {
line := scanner.Text()
if !strings.Contains(line, ":") {
field := strings.SplitN(line, ": ", 2)
if len(field) != 2 {
continue
}
field := strings.SplitN(line, ": ", 2)
switch strings.TrimSpace(field[0]) {
case "processor":
cpuinfo = append(cpuinfo, CPUInfo{}) // start of the next processor
Expand Down Expand Up @@ -183,22 +172,16 @@ func parseCPUInfoX86(info []byte) ([]CPUInfo, error) {
cpuinfo[i].PowerManagement = field[1]
}
}
return cpuinfo, nil
return cpuinfo, scanner.Err()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If scanner.Err() is not nil here, I'm wondering if we should return a nil instead of cpuinfo. At least that's the pattern we've usually followed for other parts of this library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if scanner.Err() is not nil here, I'm wondering if we should return a nil instead of cpuinfo. At least that's the pattern we've usually followed for other parts of this library.

It seems like an unnecessary complication. A user should always check for the error first. If the error is non-nil, there are no guarantees wrt what the result would be.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok makes sense, I'm fine with this as-is.

}

func parseCPUInfoARM(info []byte) ([]CPUInfo, error) {
scanner := bufio.NewScanner(bytes.NewReader(info))
func parseCPUInfoARM(info io.Reader) ([]CPUInfo, error) {
scanner := bufio.NewScanner(info)

firstLine := firstNonEmptyLine(scanner)
if !strings.HasPrefix(firstLine, "Processor") || !strings.Contains(firstLine, ":") {
return nil, errors.New("invalid cpuinfo file: " + firstLine)
}
field := strings.SplitN(firstLine, ": ", 2)
commonCPUInfo := CPUInfo{VendorID: field[1]}

cpuinfo := []CPUInfo{}
i := -1
featuresLine := ""
cpuinfo := []CPUInfo{{}}
i := 0
var features []string
modelName := ""

for scanner.Scan() {
line := scanner.Text()
Expand All @@ -207,49 +190,56 @@ func parseCPUInfoARM(info []byte) ([]CPUInfo, error) {
}
field := strings.SplitN(line, ": ", 2)
switch strings.TrimSpace(field[0]) {
case "Processor":
// this appears at the first line in kernels < 3.8
modelName = field[1]
cpuinfo[i].VendorID = field[1]
case "processor":
cpuinfo = append(cpuinfo, commonCPUInfo) // start of the next processor
i++
v, err := strconv.ParseUint(field[1], 0, 32)
if err != nil {
return nil, err
}
cpuinfo[i].Processor = uint(v)
if v != 0 {
cpuinfo = append(cpuinfo, CPUInfo{VendorID: modelName}) // start of the next processor
i++
cpuinfo[i].Processor = uint(v)
}
case "model name":
// this is what kernel 3.8+ gives instead of "Processor:"
cpuinfo[i].VendorID = field[1]
case "BogoMIPS":
v, err := strconv.ParseFloat(field[1], 64)
if err != nil {
return nil, err
}
cpuinfo[i].BogoMips = v
case "Features":
featuresLine = line
if len(features) == 0 {
// only parse once
features = strings.Fields(field[1])
}
cpuinfo[i].Flags = features
}
}
fields := strings.SplitN(featuresLine, ": ", 2)
for i := range cpuinfo {
cpuinfo[i].Flags = strings.Fields(fields[1])
}
return cpuinfo, nil

return cpuinfo, scanner.Err()
}

func parseCPUInfoS390X(info []byte) ([]CPUInfo, error) {
scanner := bufio.NewScanner(bytes.NewReader(info))
func parseCPUInfoS390X(info io.Reader) ([]CPUInfo, error) {
scanner := bufio.NewScanner(info)

firstLine := firstNonEmptyLine(scanner)
if !strings.HasPrefix(firstLine, "vendor_id") || !strings.Contains(firstLine, ":") {
return nil, errors.New("invalid cpuinfo file: " + firstLine)
}
field := strings.SplitN(firstLine, ": ", 2)
cpuinfo := []CPUInfo{}
commonCPUInfo := CPUInfo{VendorID: field[1]}
commonCPUInfo := CPUInfo{}

for scanner.Scan() {
line := scanner.Text()
if !strings.Contains(line, ":") {
field := strings.SplitN(line, ": ", 2)
if len(field) != 2 {
continue
}
field := strings.SplitN(line, ": ", 2)
switch strings.TrimSpace(field[0]) {
case "vendor_id":
commonCPUInfo.VendorID = field[1]
case "bogomips per cpu":
v, err := strconv.ParseFloat(field[1], 64)
if err != nil {
Expand Down Expand Up @@ -297,31 +287,21 @@ func parseCPUInfoS390X(info []byte) ([]CPUInfo, error) {
}
}

return cpuinfo, nil
return cpuinfo, scanner.Err()
}

func parseCPUInfoPPC(info []byte) ([]CPUInfo, error) {
scanner := bufio.NewScanner(bytes.NewReader(info))
func parseCPUInfoPPC(info io.Reader) ([]CPUInfo, error) {
scanner := bufio.NewScanner(info)

firstLine := firstNonEmptyLine(scanner)
if !strings.HasPrefix(firstLine, "processor") || !strings.Contains(firstLine, ":") {
return nil, errors.New("invalid cpuinfo file: " + firstLine)
}
field := strings.SplitN(firstLine, ": ", 2)
v, err := strconv.ParseUint(field[1], 0, 32)
if err != nil {
return nil, err
}
firstcpu := CPUInfo{Processor: uint(v)}
cpuinfo := []CPUInfo{firstcpu}
i := 0
cpuinfo := []CPUInfo{}
i := -1

for scanner.Scan() {
line := scanner.Text()
if !strings.Contains(line, ":") {
field := strings.SplitN(line, ": ", 2)
if len(field) != 2 {
continue
}
field := strings.SplitN(line, ": ", 2)
switch strings.TrimSpace(field[0]) {
case "processor":
cpuinfo = append(cpuinfo, CPUInfo{}) // start of the next processor
Expand All @@ -342,17 +322,5 @@ func parseCPUInfoPPC(info []byte) ([]CPUInfo, error) {
cpuinfo[i].CPUMHz = v
}
}
return cpuinfo, nil
}

// firstNonEmptyLine advances the scanner to the first non-empty line
// and returns the contents of that line
func firstNonEmptyLine(scanner *bufio.Scanner) string {
for scanner.Scan() {
line := scanner.Text()
if strings.TrimSpace(line) != "" {
return line
}
}
return ""
return cpuinfo, scanner.Err()
}
82 changes: 69 additions & 13 deletions cpuinfo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,29 @@

package procfs

import "testing"
import (
"strings"
"testing"
)

const (
cpuinfoArm7 = `
Processor : ARMv7 Processor rev 5 (v7l)
// non-SMP system before kernel v3.8, commit
// https://github.com/torvalds/linux/commit/b4b8f770eb
cpuinfoArm6 = `Processor : ARMv6-compatible processor rev 5 (v6l)
BogoMIPS : 791.34
Features : swp half thumb fastmult vfp edsp java
CPU implementer : 0x41
CPU architecture: 6TEJ
CPU variant : 0x1
CPU part : 0xb36
CPU revision : 5

Hardware : IMAPX200
Revision : 0000
Serial : 0000000000000000`

// SMP system before kernel 3.8
cpuinfoArm7old = `Processor : ARMv7 Processor rev 5 (v7l)
processor : 0
BogoMIPS : 2400.00

Expand All @@ -37,8 +55,18 @@ Hardware : sun8i
Revision : 0000
Serial : 5400503583203c3c040e`

cpuinfoS390x = `
vendor_id : IBM/S390
// kernel v3.8+
cpuinfoArm7 = `processor : 0
model name : ARMv7 Processor rev 2 (v7l)
BogoMIPS : 50.00
Features : half thumb fastmult vfp edsp thumbee vfpv3 tls idiva idivt vfpd32 lpae
CPU implementer : 0x56
CPU architecture: 7
CPU variant : 0x2
CPU part : 0x584
CPU revision : 2`

cpuinfoS390x = `vendor_id : IBM/S390
# processors : 4
bogomips per cpu: 3033.00
max thread id : 0
Expand Down Expand Up @@ -69,11 +97,9 @@ cpu MHz static : 5000

cpu number : 3
cpu MHz dynamic : 5000
cpu MHz static : 5000
`
cpu MHz static : 5000`

cpuinfoPpc64 = `
processor : 0
cpuinfoPpc64 = `processor : 0
cpu : POWER7 (architected), altivec supported
clock : 3000.000000MHz
revision : 2.1 (pvr 003f 0201)
Expand Down Expand Up @@ -153,8 +179,23 @@ func TestCPUInfoX86(t *testing.T) {
}
}

func TestCPUInfoParseARM(t *testing.T) {
cpuinfo, err := parseCPUInfoARM([]byte(cpuinfoArm7))
func TestCPUInfoParseARM6(t *testing.T) {
cpuinfo, err := parseCPUInfoARM(strings.NewReader(cpuinfoArm6))
if err != nil || cpuinfo == nil {
t.Fatalf("unable to parse arm cpu info: %v", err)
}
if want, have := 1, len(cpuinfo); want != have {
t.Errorf("want number of processors %v, have %v", want, have)
}
if want, have := "ARMv6-compatible processor rev 5 (v6l)", cpuinfo[0].VendorID; want != have {
t.Errorf("want vendor %v, have %v", want, have)
}
if want, have := "thumb", cpuinfo[0].Flags[2]; want != have {
t.Errorf("want flag %v, have %v", want, have)
}
}
func TestCPUInfoParseARM7old(t *testing.T) {
cpuinfo, err := parseCPUInfoARM(strings.NewReader(cpuinfoArm7old))
if err != nil || cpuinfo == nil {
t.Fatalf("unable to parse arm cpu info: %v", err)
}
Expand All @@ -169,8 +210,23 @@ func TestCPUInfoParseARM(t *testing.T) {
}
}

func TestCPUInfoParseARM(t *testing.T) {
cpuinfo, err := parseCPUInfoARM(strings.NewReader(cpuinfoArm7))
if err != nil || cpuinfo == nil {
t.Fatalf("unable to parse arm cpu info: %v", err)
}
if want, have := 1, len(cpuinfo); want != have {
t.Errorf("want number of processors %v, have %v", want, have)
}
if want, have := "ARMv7 Processor rev 2 (v7l)", cpuinfo[0].VendorID; want != have {
t.Errorf("want vendor %v, have %v", want, have)
}
if want, have := "fastmult", cpuinfo[0].Flags[2]; want != have {
t.Errorf("want flag %v, have %v", want, have)
}
}
func TestCPUInfoParseS390X(t *testing.T) {
cpuinfo, err := parseCPUInfoS390X([]byte(cpuinfoS390x))
cpuinfo, err := parseCPUInfoS390X(strings.NewReader(cpuinfoS390x))
if err != nil || cpuinfo == nil {
t.Fatalf("unable to parse s390x cpu info: %v", err)
}
Expand All @@ -189,7 +245,7 @@ func TestCPUInfoParseS390X(t *testing.T) {
}

func TestCPUInfoParsePPC(t *testing.T) {
cpuinfo, err := parseCPUInfoPPC([]byte(cpuinfoPpc64))
cpuinfo, err := parseCPUInfoPPC(strings.NewReader(cpuinfoPpc64))
if err != nil || cpuinfo == nil {
t.Fatalf("unable to parse ppc cpu info: %v", err)
}
Expand Down