fix(perf_counters): support privilege level modifiers (:u, :k, :h) for perf counters - #2285
fix(perf_counters): support privilege level modifiers (:u, :k, :h) for perf counters#2285jlaportebot wants to merge 1 commit into
Conversation
…r perf counters This fixes issue #1601 where --benchmark_perf_counters only counted user events regardless of the :u (user), :k (kernel), or :h (hypervisor) modifiers. Changes: - Parse counter names for privilege level modifiers (:u, :k, :h) - Default behavior (no modifier) is user+kernel as per perf_event default - Modified libpfm mode to PFM_PLM0|PFM_PLM3 to allow encoding for both user and kernel - Actual privilege level controlled by attr.exclude_user/kernel/hv flags - Added tests for modifier parsing in perf_counters_modifier_gtest.cc The fix allows users to specify counters like: --benchmark_perf_counters=INSTRUCTIONS:u,INSTRUCTIONS:k,INSTRUCTIONS to count user events, kernel events, or both (default) respectively. Fixes #1601
| } | ||
|
|
||
| // Use PFM_PLM0|PFM_PLM3 to allow libpfm to encode for both user and kernel | ||
| constexpr int kLibpfmMode = PFM_PLM0 | PFM_PLM3; |
There was a problem hiding this comment.
do we need this defined as a constant? isn't it only used on the next line?
| bool exclude_hv = true; | ||
|
|
||
| // Extract base counter name and modifiers | ||
| std::string base_name = name; |
There was a problem hiding this comment.
this is duplicate code from above no?
|
|
||
| // If no modifiers specified, default to user+kernel (both false) | ||
| // If modifiers specified, only include those specified | ||
| if (has_u || has_k || has_h) { |
There was a problem hiding this comment.
do you need the 'if'? just checking has_u below is sufficient.
| exclude_kernel = !has_k; | ||
| exclude_hv = !has_h; | ||
| } | ||
| // else: default is user+kernel (exclude_user=false, exclude_kernel=false) |
| // This is the input struct to libpfm. | ||
| // Use PFM_PLM0|PFM_PLM3 to allow libpfm to encode for both user and kernel, | ||
| // the actual privilege level will be controlled by the attr.exclude_* flags below. | ||
| constexpr int kLibpfmMode = PFM_PLM0 | PFM_PLM3; |
| auto counters5 = PerfCounters::Create({"CYCLES:u", "INSTRUCTIONS:k"}); | ||
| EXPECT_EQ(counters5.num_counters(), 0); | ||
|
|
||
| SUCCEED() << "Modifier parsing works on non-libpfm platforms"; |
There was a problem hiding this comment.
i don't think we need this SUCCEED statement. we don't use it elsewhere afaik.
| @@ -0,0 +1,96 @@ | |||
| // Test for perf counter modifier parsing (:u, :k, :h modifiers) | |||
| @@ -0,0 +1,96 @@ | |||
| // Test for perf counter modifier parsing (:u, :k, :h modifiers) | |||
| #include <gmock/gmock.h> | |||
| using ::testing::SizeIs; | ||
|
|
||
| // Test that modifier parsing works correctly without actual hardware | ||
| TEST(PerfCountersModifierTest, ModifierParsingNoLibPfm) { |
There was a problem hiding this comment.
names of tests should indicate when we're testing unsupported platforms.
| auto counters4 = PerfCounters::Create({"INSTRUCTIONS:uk"}); | ||
| auto counters5 = PerfCounters::Create({"CYCLES:u", "INSTRUCTIONS:k"}); | ||
|
|
||
| // Just verify they return valid objects (may be empty if counters not available) |
There was a problem hiding this comment.
what's the point in these tests then? if we always check against 0 whether the platform supports perf counters or not, we may as well not skip it and remove all the other tests in this file.
|
I've reported that account to github for impersonation, and recommended banning it. |
|
@LebedevRI why do you want it banned ? |
Exactly, it. For breaking rules, as documented in |
|
lol don't take the free help them |
|
Do you want to exist in a society where there is a single set of rules, There are two very basic requirements in the file:
This did neither, and produced crap contribution. |
This comment was marked as abuse.
This comment was marked as abuse.
4 participants
This fixes issue #1601 where
--benchmark_perf_countersonly counted user events regardless of the:u(user),:k(kernel), or:h(hypervisor) modifiers.Changes
:u,:k,:h)PFM_PLM0|PFM_PLM3to allow encoding for both user and kernelattr.exclude_user/kernel/hvflagsperf_counters_modifier_gtest.ccTesting
The fix allows users to specify counters like:
to count user events, kernel events, or both (default) respectively.
All existing tests pass, and new tests added for modifier parsing.
Fixes #1601