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.

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

Handling zeros in readPerfStat #2632

Merged
merged 7 commits into from
Aug 3, 2020
Merged

Conversation

iwankgb
Copy link
Collaborator

@iwankgb iwankgb commented Jul 26, 2020

Adds more robust handling for zeros in readPerfStat and fixes #2629 (hopefully).

@iwankgb
Copy link
Collaborator Author

iwankgb commented Jul 26, 2020

While working on the PR I noticed following linter issues:

ubuntu@malyna4:~/go/src/github.com/google/cadvisor$ golangci-lint run --build-tags libpfm,cgo
WARN [linters context/goanalysis] Failed to get persisted facts: failed to get data from low-level cache by key fact_purity/facts for package yaml: failed to stat file /home/ubuntu/.cache/golangci-lint/4c/4c05768a80d95b176a37aae80ce01dbba0a393ee328bae71015e9bd10d999a71-d: stat /home/ubuntu/.cache/golangci-lint/4c/4c05768a80d95b176a37aae80ce01dbba0a393ee328bae71015e9bd10d999a71-d: no such file or directory
WARN [linters context/goanalysis] Failed to get persisted facts: failed to get data from low-level cache by key ctrlflow/facts for package yaml: failed to stat file /home/ubuntu/.cache/golangci-lint/68/684eaab543416e6ecf05612d3269cb5401637f7c9ab58a9e6d4ff3b84ad99df5-d: stat /home/ubuntu/.cache/golangci-lint/68/684eaab543416e6ecf05612d3269cb5401637f7c9ab58a9e6d4ff3b84ad99df5-d: no such file or directory
integration/tests/api/perf_test.go:50:28: Error return value of `framework.RetryForDuration` is not checked (errcheck)
	framework.RetryForDuration(func() error {
	                          ^
perf/uncore_libpfm.go:302:9: `if` block ends with a `return` statement, so drop this `else` and outdent its block (golint)
	} else {
	       ^
perf/uncore_libpfm.go:101:27: S1019: should use make(uncorePMUs) instead (gosimple)
	pmus := make(uncorePMUs, 0)
	                         ^
perf/uncore_libpfm_test.go:43:80: SA9002: file mode '777' evaluates to 01411; did you mean '0777'? (staticcheck)
	err = ioutil.WriteFile(filepath.Join(firstPMUPath, "cpumask"), []byte("0-1"), 777)
	                                                                              ^
perf/uncore_libpfm_test.go:47:76: SA9002: file mode '777' evaluates to 01411; did you mean '0777'? (staticcheck)
	err = ioutil.WriteFile(filepath.Join(firstPMUPath, "type"), []byte("18"), 777)
	                                                                          ^
perf/uncore_libpfm_test.go:58:81: SA9002: file mode '777' evaluates to 01411; did you mean '0777'? (staticcheck)
	err = ioutil.WriteFile(filepath.Join(secondPMUPath, "cpumask"), []byte("0,1"), 777)
	                                                                               ^
perf/uncore_libpfm_test.go:62:77: SA9002: file mode '777' evaluates to 01411; did you mean '0777'? (staticcheck)
	err = ioutil.WriteFile(filepath.Join(secondPMUPath, "type"), []byte("19"), 777)

I will fix them in separate commit.

iwankgb added 2 commits July 26, 2020 22:21
Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@critical.today>
Copy link
Collaborator

@katarzyna-z katarzyna-z left a comment

Choose a reason for hiding this comment

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

lgtm

@katarzyna-z
Copy link
Collaborator

@wacuuu Could you check this fix?

@@ -112,12 +112,17 @@ func readPerfStat(file readerCloser, name string, cpu int) (*info.PerfStat, erro
}

scalingRatio := 1.0
if perfData.TimeEnabled != 0 {
if perfData.TimeRunning != 0 && perfData.TimeEnabled != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the same fix should be provided for uncore perf events, see this line

ScalingRatio: 1.0,
Value: 0,
Name: "some metric",
Cpu: 4,
Copy link

Choose a reason for hiding this comment

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

For complicity:
missing the testcase that started whole of the issue:
{ Value: 0, TimeEnabled: 3, TimeRunning: 0, }

Copy link
Collaborator

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm

@dashpole
Copy link
Collaborator

I'll leave this open for a day to let you fix the remaining comments above

@iwankgb
Copy link
Collaborator Author

iwankgb commented Aug 3, 2020

@dashpole I think it can be merged :)

@dashpole dashpole merged commit a6e4fcb into google:master Aug 3, 2020
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.

Wrong values for perf core events when starting the measurement
4 participants