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

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

[Dapeng Mi]

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@linux.intel.com>
---
 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;
 }
 
@@ -2750,25 +2750,25 @@ static void intel_pmu_enable_fixed(struct perf_event *event)
 	 * if requested:
 	 */
 	if (!event->attr.precise_ip)
-		bits |= 0x8;
+		bits |= INTEL_FIXED_0_ENABLE_PMI;
 	if (hwc->config & ARCH_PERFMON_EVENTSEL_USR)
-		bits |= 0x2;
+		bits |= INTEL_FIXED_0_USER;
 	if (hwc->config & ARCH_PERFMON_EVENTSEL_OS)
-		bits |= 0x1;
+		bits |= INTEL_FIXED_0_KERNEL;
 
 	/*
 	 * ANY bit is supported in v3 and up
 	 */
 	if (x86_pmu.version > 2 && hwc->config & ARCH_PERFMON_EVENTSEL_ANY)
-		bits |= 0x4;
+		bits |= INTEL_FIXED_0_ANYTHREAD;
 
 	idx -= INTEL_PMC_IDX_FIXED;
-	bits <<= (idx * 4);
-	mask = 0xfULL << (idx * 4);
+	bits = intel_fixed_bits(idx, bits);
+	mask = intel_fixed_bits(idx, INTEL_FIXED_BITS_MASK);
 
 	if (x86_pmu.intel_cap.pebs_baseline && event->attr.precise_ip) {
-		bits |= ICL_FIXED_0_ADAPTIVE << (idx * 4);
-		mask |= ICL_FIXED_0_ADAPTIVE << (idx * 4);
+		bits |= intel_fixed_bits(idx, ICL_FIXED_0_ADAPTIVE);
+		mask |= intel_fixed_bits(idx, ICL_FIXED_0_ADAPTIVE);
 	}
 
 	cpuc->fixed_ctrl_val &= ~mask;
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 8fc15ed5e60b..bdaf015a620e 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -32,11 +32,21 @@
 #define ARCH_PERFMON_EVENTSEL_INV			(1ULL << 23)
 #define ARCH_PERFMON_EVENTSEL_CMASK			0xFF000000ULL
 
+#define INTEL_FIXED_BITS_MASK				0xFULL
+#define INTEL_FIXED_BITS_STRIDE			4
+#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)
+
 #define HSW_IN_TX					(1ULL << 32)
 #define HSW_IN_TX_CHECKPOINTED				(1ULL << 33)
 #define ICL_EVENTSEL_ADAPTIVE				(1ULL << 34)
 #define ICL_FIXED_0_ADAPTIVE				(1ULL << 32)
 
+#define intel_fixed_bits(_idx, _bits)			\
+	((_bits) << ((_idx) * INTEL_FIXED_BITS_STRIDE))
+
 #define AMD64_EVENTSEL_INT_CORE_ENABLE			(1ULL << 36)
 #define AMD64_EVENTSEL_GUESTONLY			(1ULL << 40)
 #define AMD64_EVENTSEL_HOSTONLY				(1ULL << 41)
-- 
2.34.1

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

[Peter Zijlstra]

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@linux.intel.com>
> ---
>  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.

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

[Liang, Kan]

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@linux.intel.com>
>> ---
>>  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

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@linux.intel.com>
> 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@linux.intel.com>
> >> ---
> >>  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