Skip to content

Add parser for sysfs devices/system/cpu/... #94

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.

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

Merged
merged 15 commits into from
Oct 4, 2018
Merged

Add parser for sysfs devices/system/cpu/... #94

merged 15 commits into from
Oct 4, 2018

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Jun 12, 2018

  • Add parser for devices/system/cpu/cpu*/cpufreq/....
  • Add helper util.ReadUintFromFile().
  • Move sysReadFile() helper from sysfs to internal/util.

Signed-off-by: Ben Kochie superq@gmail.com

@SuperQ SuperQ force-pushed the superq/cpu branch 2 times, most recently from a685b80 to dac3ead Compare June 12, 2018 15:45
@SuperQ SuperQ changed the title WIP: Add parser for sysfs devices/system/cpu/... Add parser for sysfs devices/system/cpu/... Jun 12, 2018
@SuperQ SuperQ requested a review from mdlayher June 12, 2018 18:22
@SuperQ SuperQ force-pushed the superq/cpu branch 2 times, most recently from 52671f0 to bebb299 Compare June 12, 2018 23:14
@SuperQ SuperQ requested a review from grobie June 27, 2018 14:54
@SuperQ
Copy link
Member Author

SuperQ commented Jun 27, 2018

Ping @mdlayher :-)

@mdlayher
Copy link
Contributor

Ack, sorry, out of town. Will try to get to this on Monday.

@SuperQ
Copy link
Member Author

SuperQ commented Jul 24, 2018

More poke @mdlayher 🍏


systemCpufreq := SystemCpufreq{}
for _, cpu := range cpus {
_, cpuName := filepath.Split(cpu)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not use filepath.Base() instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, no reason, just overlooked.

@SuperQ SuperQ force-pushed the superq/cpu branch 2 times, most recently from 50617e9 to 7c1ae43 Compare July 25, 2018 11:23
@SuperQ
Copy link
Member Author

SuperQ commented Jul 25, 2018

Hrm, tests are failing due to something else:

# honnef.co/go/tools/lint/lintutil
../../../honnef.co/go/tools/lint/lintutil/util.go:273: undefined: types.SizesFor
make: *** [/home/travis/gopath/bin/staticcheck] Error 2

@SuperQ
Copy link
Member Author

SuperQ commented Jul 26, 2018

@grobie Poke? Please? 😀

Copy link
Contributor

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

golint output (not all relevant to this PR):

✔ ~/src/github.com/prometheus/procfs [superq/cpu|✔] 
12:12 $ golint ./...
bcache/bcache.go:32:6: type name will be used as bcache.BcacheStats by other packages, and that stutters; consider calling this Stats
internal/util/parse.go:54:1: exported function ReadUintFromFile should have comment or be unexported
internal/util/parse.go:66:1: comment on exported function SysReadFile should be of the form "SysReadFile ..."
sysfs/net_class_test.go:47:28: should drop = 0 from declaration of var netDevGroup; it is the zero value
sysfs/system_cpu.go:26:6: type SystemCpuCpufreq should be SystemCPUCpufreq

}

// SystemCpufreq is a collection of SystemCpuCpufreq for every CPU.
type SystemCpufreq map[string]SystemCpuCpufreq
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are no methods on this type, I'd remove the type.

)

// SystemCpuCpufreq contains stats from devices/system/cpu/cpu[0-9]*/cpufreq/...
type SystemCpuCpufreq struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

*CPU throughout, as golint will mention too.

}

// NewSystemCpufreq returns CPU frequency metrics for all CPUs.
func (fs FS) NewSystemCpufreq() (SystemCpufreq, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I think my preference would be to return a slice with the CPU's name as a member of the structure. Maps are usually a little gross in APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following the pattern in net_class.go, which returns a type NetClass map[string]NetClassIface.

But, it also includes a Name in the NetClassIface, so that it can be identified stand-alone. I can do the same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference would be a slice with field Name in both cases. Maps can be clunky to use and the undefined map iteration order behavior bites a lot of people on a regular basis.

if _, err := os.Stat(cpuCpufreqPath); os.IsNotExist(err) {
continue
} else {
if _, err = os.Stat(filepath.Join(cpuCpufreqPath, "scaling_cur_freq")); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd personally go with a loop with the file names as the slice elements over if/else if.

}

func parseCpufreqCpuinfo(prefix string, cpuPath string) (SystemCpuCpufreq, error) {
current, err := util.ReadUintFromFile(filepath.Join(cpuPath, prefix+"_cur_freq"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can probably shorten this up some by using loops for all of the types that need "ReadUintFromFile" and then "SysReadFile".

@SuperQ SuperQ force-pushed the superq/cpu branch 2 times, most recently from d5d1caa to 633577a Compare July 27, 2018 13:52
if err != nil {
return 0, err
}
value, err := strconv.ParseUint(strings.TrimSpace(string(data)), 10, 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you could just return strconv.ParseUint(/* ... */) at this point.

}

// SystemCpufreq is a collection of SystemCPUCpufreq for every CPU.
type SystemCpufreq map[string]SystemCPUCpufreq
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you see my previous thought?

Since there are no methods on this type, I'd remove the type.

return nil, err
}

return b[:n], nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it seems like a common need, we could bytes.TrimSpace on the return value here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also returning it as a string makes sense.

}

// SystemCpufreq is a collection of SystemCPUCpufreq for every CPU.
type SystemCpufreq []SystemCPUCpufreq
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this type has no methods, I would probably remove it and just return the slice directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does now. func (s SystemCpufreq) parseCpufreqCpuinfo(...)

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 @mdlayher means getting rid of SystemCpufreq and returning []SystemCPUCpufreq instead and I agree on that.

return systemCpufreq, nil
}

func (s SystemCpufreq) parseCpufreqCpuinfo(prefix string, cpuPath string) (*SystemCPUCpufreq, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an idea to tighten up this function: https://play.golang.org/p/T35yJhQlsmZ

Same logic could apply to the SysReadFile parts with a second loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, good idea.

)

// SystemCPUCpufreq contains stats from devices/system/cpu/cpu[0-9]*/cpufreq/...
type SystemCPUCpufreq struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would go with something like SystemCPUFrequency as that's more human friendly than the Linux directory name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer the literal Linux names most of the time.

Copy link
Member

Choose a reason for hiding this comment

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

tbh, I don't like either of these names. The name should convey what this is about. So maybe (System)CPUStats?

Copy link
Member Author

Choose a reason for hiding this comment

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

I strongly prefer to use the names we get from Linux, this is the least surprising mapping between the library and the kernel.

Copy link
Member

Choose a reason for hiding this comment

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

I get where you're coming from but if you just look at the code without knowing the linux names it's just hard to read. Let's append at least Stats or something because this struct isn't a "frequency" then?

@SuperQ
Copy link
Member Author

SuperQ commented Jul 28, 2018

@mdlayher Any other issues to fix up?

@mdlayher
Copy link
Contributor

Sorry, not much spare time this week. @grobie and @discordianfish ?

systemCPUCpufreqClass.MinimumFrequency = uintOut[2]
systemCPUCpufreqClass.TransitionLatency = uintOut[3]

stringFiles := [5]string{
Copy link
Member

Choose a reason for hiding this comment

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

Use []string instead. No need to pre-allocate this. Pretty sure the compiler does the right thing anyway.

"related_cpus",
"scaling_setspeed",
}
var stringOut [5]string
Copy link
Member

Choose a reason for hiding this comment

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

I'd perfer stringOut := make([]string, len(stringFiles)). No need to optimize on cost of readability.

* Add parser for `devices/system/cpu/cpu*/cpufreq/...`.
* Add helper `util.ReadUintFromFile()`.

Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
* Add `Name` to information struct.
* Use parser as method.

Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
SuperQ added 6 commits October 2, 2018 11:11
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ
Copy link
Member Author

SuperQ commented Oct 2, 2018

Ok, I've cleaned up a bunch of things. I think I've resolved all the comments, PTAL.

@SuperQ
Copy link
Member Author

SuperQ commented Oct 2, 2018

/cc @metalmatze

@metalmatze
Copy link
Member

Looking at this I can't find any issues regarding the Go code itself (except some nits that don't change anything, probably personal preference). As I said earlier today, I'm not an expert when it comes to these internal APIs.
So LGTM.

@SuperQ
Copy link
Member Author

SuperQ commented Oct 4, 2018

@discordianfish Any last issues?

Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM

@SuperQ SuperQ merged commit 6bfc2c7 into master Oct 4, 2018
@SuperQ SuperQ deleted the superq/cpu branch October 4, 2018 13:16
remijouannet pushed a commit to remijouannet/procfs that referenced this pull request Oct 20, 2022
* Add parser for sysfs devices/system/cpu/...

Signed-off-by: Ben Kochie <superq@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants