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

Add perf event grouping. #2578

Merged
merged 1 commit into from
Sep 11, 2020
Merged

Add perf event grouping. #2578

merged 1 commit into from
Sep 11, 2020

Conversation

Creatone
Copy link
Collaborator

@Creatone Creatone commented Jun 9, 2020 •
edited
Loading

It provides support for grouping core perf events.

Part of #2483

@k8s-ci-robot
Copy link
Collaborator

Hi @Creatone. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dashpole
Copy link
Collaborator

dashpole commented Jun 9, 2020

/ok-to-test

@Creatone Creatone force-pushed the creatone/perf-grouping branch 2 times, most recently from dfd25a8 to c6eef8a Compare July 6, 2020 13:00
@Creatone Creatone changed the title Add perf event grouping. [WIP] Add perf event grouping. Jul 6, 2020
@dashpole
Copy link
Collaborator

dashpole commented Jul 6, 2020

let me know when this is ready for review, and remove the WIP tag.

@Creatone
Copy link
Collaborator Author

Creatone commented Jul 7, 2020

@dashpole I need to add grouping uncore events. In a couple of days, it'll be finished.

@Creatone
Copy link
Collaborator Author

Creatone commented Jul 14, 2020 •
edited
Loading

@dashpole There is a small change in the configuration file. Now you can pass a single event name or an array with event names if you want to group.

Single elements can be read in a group format so I did also some refactoring.

Grouping doesn't work for multiple PMUs.

@Creatone Creatone changed the title [WIP] Add perf event grouping. Add perf event grouping. Jul 14, 2020
@dashpole
Copy link
Collaborator

This looks reasonable to me, but i'd like @katarzyna-z or @iwankgb to take a look as well

iwankgb
iwankgb requested changes Jul 26, 2020
View reviewed changes
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/uncore_libpfm.go Outdated Show resolved Hide resolved
perf/uncore_libpfm.go Show resolved Hide resolved
perf/uncore_libpfm.go Show resolved Hide resolved
perf/manager_libpfm.go Outdated Show resolved Hide resolved
perf/manager_libpfm.go Outdated Show resolved Hide resolved
@katarzyna-z
Copy link
Collaborator

I think that it is worth to change this line and replace fmt.Printf with klog error in this pull request.

katarzyna-z
katarzyna-z reviewed Jul 27, 2020
View reviewed changes
perf/uncore_libpfm.go Outdated Show resolved Hide resolved
katarzyna-z
katarzyna-z reviewed Jul 27, 2020
View reviewed changes
perf/uncore_libpfm.go Outdated Show resolved Hide resolved
@Creatone Creatone force-pushed the creatone/perf-grouping branch 5 times, most recently from 0b3e42d to 520dd7c Compare August 5, 2020 13:40
katarzyna-z
katarzyna-z reviewed Aug 10, 2020
View reviewed changes
perf/manager_libpfm.go Outdated Show resolved Hide resolved
katarzyna-z
katarzyna-z approved these changes Aug 13, 2020
View reviewed changes
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, but please squash commits

@Creatone
Copy link
Collaborator Author

@katarzyna-z after lgtm from @iwankgb

@iwankgb
Copy link
Collaborator

iwankgb commented Aug 13, 2020

I will review during the weekend.

@dashpole dashpole self-assigned this Aug 13, 2020
@Creatone Creatone mentioned this pull request Aug 24, 2020
Merged
iwankgb
iwankgb requested changes Aug 29, 2020
View reviewed changes
Copy link
Collaborator

@iwankgb iwankgb left a comment

Choose a reason for hiding this comment

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

I know it took me more than it should have. I'm sorry.

perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/collector_libpfm.go Outdated Show resolved Hide resolved
perf/uncore_libpfm.go
if ok {
if customEvent.Type != 0 {
pmuPrefix = uncorePMUPrefix
// CPUs file descriptors of group leader needed for perf_event_open.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are uncore metrics. Why they are CPU dependent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because of perf_event_open arguments. There is an abstraction that one CPU corresponds to one socket.

For example CPU range in /sys/device/uncore_imc_0/cpumask of 2 socket machine:

$ cat /sys/devices/uncore_imc_0/cpumask 
0-1

@Creatone
Copy link
Collaborator Author

Creatone commented Sep 3, 2020

@iwankgb

@iwankgb
Copy link
Collaborator

iwankgb commented Sep 4, 2020

@Creatone @dashpole I officially give my blessing to this PR.

Add perf event grouping.
54ac157 
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
dashpole
dashpole approved these changes Sep 11, 2020
View reviewed changes
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.

Thanks @iwankgb and @katarzyna-z for the reviews. This looks really good.

@dashpole dashpole merged commit 7676a45 into google:master Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwankgb iwankgb iwankgb approved these changes

@dashpole dashpole dashpole approved these changes

@katarzyna-z katarzyna-z katarzyna-z approved these changes

Assignees

@dashpole dashpole

Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Creatone @k8s-ci-robot @dashpole @katarzyna-z @iwankgb