Re: [PATCH] perf/x86/intel: Define bit macros for FixCntrCtl MSR

[Dapeng Mi]

On Thu, Mar 30, 2023 at 10:46:01AM -0400, Liang, Kan wrote:
> Date: Thu, 30 Mar 2023 10:46:01 -0400
> From: "Liang, Kan" <kan.liang@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] perf/x86/intel: Define bit macros for FixCntrCtl MSR
> 
> 
> 
> On 2023-03-30 9:07 a.m., Peter Zijlstra wrote:
> > On Thu, Mar 30, 2023 at 09:28:46AM +0800, Dapeng Mi wrote:
> >> Define bit macros for FixCntrCtl MSR and replace the bit hardcoding
> >> with these bit macros. This would make code be more human-readable.
> >>
> >> Perf commands 'perf stat -e "instructions,cycles,ref-cycles"' and
> >> 'perf record -e "instructions,cycles,ref-cycles"' pass.
> >>
> >> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> >> ---
> >>  arch/x86/events/intel/core.c      | 18 +++++++++---------
> >>  arch/x86/include/asm/perf_event.h | 10 ++++++++++
> >>  2 files changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> >> index 070cc4ef2672..b7c0bb78ed59 100644
> >> --- a/arch/x86/events/intel/core.c
> >> +++ b/arch/x86/events/intel/core.c
> >> @@ -2451,7 +2451,7 @@ static void intel_pmu_disable_fixed(struct perf_event *event)
> >>  
> >>  	intel_clear_masks(event, idx);
> >>  
> >> -	mask = 0xfULL << ((idx - INTEL_PMC_IDX_FIXED) * 4);
> >> +	mask = intel_fixed_bits(idx - INTEL_PMC_IDX_FIXED, INTEL_FIXED_BITS_MASK);
> >>  	cpuc->fixed_ctrl_val &= ~mask;
> > 
> > So maybe it's me, but I find the original far easier to read :/ That new
> > things I need to look up every single identifier before I can tell wth
> > it does.
> 
> The intel_fixed_bits() tries to replace the duplicate "bit << (idx *
> 4);". I think it should be a good improvement. Maybe we should rename it
> to intel_shift_fixed_bits_by_idx(). Is it better?
> 
> If not, I think at least we should use some macros to replace the magic
> number.
> 
> Thanks,
> Kan

Comparing previous magic numbers, the following macros can help developers
to know the meaning of the piece of code rapidly and don't need to
check the hardware specs and get its meaning, and developers could more
easily confirm his code logic is correct and don't confirm with spec
again.

+#define INTEL_FIXED_0_KERNEL				(1ULL << 0)
+#define INTEL_FIXED_0_USER				(1ULL << 1)
+#define INTEL_FIXED_0_ANYTHREAD			(1ULL << 2)
+#define INTEL_FIXED_0_ENABLE_PMI			(1ULL << 3)

As for the macro intel_fixed_bits, it indeed hides some details, but it
make the code looks cleaner and developer can use it more easily and don't
worry about the details. Like what Kan said, maybe we can get a new name to
make it be more understandably. 
-- 
Thanks,
Dapeng Mi