-
Notifications
You must be signed in to change notification settings - Fork 13
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
Record: Allow to configure Perf Events based on Model #112
Conversation
@@ -39,7 +39,7 @@ static L1_INSTRUCTIONS: NamedTypeCtr = NamedTypeCtr { | |||
static BACKEND_STALLS: NamedTypeCtr = NamedTypeCtr { | |||
perf_type: PerfType::RAW, | |||
name: "Backend-Stalls", | |||
config: 0x10a2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correcting a bug too? I didn't see it mentioned in commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it to the commit.
src/data.rs
Outdated
@@ -4,8 +4,10 @@ pub mod diskstats; | |||
pub mod flamegraphs; | |||
#[cfg(target_arch = "aarch64")] | |||
pub mod grv_perf_events; | |||
pub mod intel_icelake_perf_events; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go after line 8 below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Changing this as it should only be compiled on x86. rustfmt moved these :( .
src/data.rs
Outdated
@@ -4,8 +4,10 @@ pub mod diskstats; | |||
pub mod flamegraphs; | |||
#[cfg(target_arch = "aarch64")] | |||
pub mod grv_perf_events; | |||
pub mod intel_icelake_perf_events; | |||
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] | |||
pub mod intel_perf_events; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment as to which Intel architecture does this file apply to?
@@ -0,0 +1,40 @@ | |||
use crate::data::perf_stat::{NamedCtr, NamedTypeCtr, PerfType}; | |||
|
|||
static CYCLES: NamedTypeCtr = NamedTypeCtr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are same as the default in intel_perf_events.rs. So, does this architecture specific config file provide delta counters or all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We make sure the nr and dr are in the same file. We don't want to refer to other files for a PMU event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments, 1 change request.
Also, fix a bug in the default Intel PMU config for Backend Stalls.
Icelake and Sapphire Rapids have different configurations for the PMU events. Change them based on the Model during run-time.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.