Re: [PATCH 2/3] perf/x86/intel: Add a distinct name for Granite Rapids

[Liang, Kan]

On 2024-07-09 5:56 a.m., Peter Zijlstra wrote:
> On Mon, Jul 08, 2024 at 12:33:35PM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> Currently, the Sapphire Rapids and Granite Rapids share the same PMU
>> name, sapphire_rapids. Because from the kernel’s perspective, GNR is
>> similar to SPR. The only key difference is that they support different
>> extra MSRs. The code path and the PMU name are shared.
>>
>> However, from end users' perspective, they are quite different. Besides
>> the extra MSRs, GNR has a newer PEBS format, supports Retire Latency,
>> supports new CPUID enumeration architecture, doesn't required the
>> load-latency AUX event, has additional TMA Level 1 Architectural Events,
>> etc. The differences can be enumerated by CPUID or the PERF_CAPABILITIES
>> MSR. They weren't reflected in the model-specific kernel setup.
>> But it is worth to have a distinct PMU name for GNR.
>>
>> Fixes: a6742cb90b56 ("perf/x86/intel: Fix the FRONTEND encoding on GNR and MTL")
>> Suggested-by: Ahmad Yasin <ahmad.yasin@xxxxxxxxx>
>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>>  arch/x86/events/intel/core.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index b61367991a16..7a9f931a1f48 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -6943,12 +6943,17 @@ __init int intel_pmu_init(void)
>>  	case INTEL_EMERALDRAPIDS_X:
>>  		x86_pmu.flags |= PMU_FL_MEM_LOADS_AUX;
>>  		x86_pmu.extra_regs = intel_glc_extra_regs;
>> +		pr_cont("Sapphire Rapids events, ");
>> +		name = "sapphire_rapids";
>>  		fallthrough;
>>  	case INTEL_GRANITERAPIDS_X:
>>  	case INTEL_GRANITERAPIDS_D:
>>  		intel_pmu_init_glc(NULL);
>> -		if (!x86_pmu.extra_regs)
>> +		if (!x86_pmu.extra_regs) {
>>  			x86_pmu.extra_regs = intel_rwc_extra_regs;
>> +			pr_cont("Granite Rapids events, ");
>> +			name = "granite_rapids";
>> +		}
>>  		x86_pmu.pebs_ept = 1;
>>  		x86_pmu.hw_config = hsw_hw_config;
>>  		x86_pmu.get_event_constraints = glc_get_event_constraints;
>> @@ -6959,8 +6964,6 @@ __init int intel_pmu_init(void)
>>  		td_attr = glc_td_events_attrs;
>>  		tsx_attr = glc_tsx_events_attrs;
>>  		intel_pmu_pebs_data_source_skl(true);
>> -		pr_cont("Sapphire Rapids events, ");
>> -		name = "sapphire_rapids";
>>  		break;
> 
> For some reason this didn't want to apply cleanly (something trivial),
> but since I had to edit it, my fingers slipped and I ended up with the
> below. That ok?

Yes, the patch looks good. Thanks!

Thanks,
Kan
> 
> ---
> Subject: perf/x86/intel: Add a distinct name for Granite Rapids
> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> Date: Mon, 8 Jul 2024 12:33:35 -0700
> 
> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> 
> Currently, the Sapphire Rapids and Granite Rapids share the same PMU
> name, sapphire_rapids. Because from the kernel’s perspective, GNR is
> similar to SPR. The only key difference is that they support different
> extra MSRs. The code path and the PMU name are shared.
> 
> However, from end users' perspective, they are quite different. Besides
> the extra MSRs, GNR has a newer PEBS format, supports Retire Latency,
> supports new CPUID enumeration architecture, doesn't required the
> load-latency AUX event, has additional TMA Level 1 Architectural Events,
> etc. The differences can be enumerated by CPUID or the PERF_CAPABILITIES
> MSR. They weren't reflected in the model-specific kernel setup.
> But it is worth to have a distinct PMU name for GNR.
> 
> Fixes: a6742cb90b56 ("perf/x86/intel: Fix the FRONTEND encoding on GNR and MTL")
> Suggested-by: Ahmad Yasin <ahmad.yasin@xxxxxxxxx>
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://lkml.kernel.org/r/20240708193336.1192217-3-kan.liang@xxxxxxxxxxxxxxx
> ---
>  arch/x86/events/intel/core.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -6788,12 +6788,18 @@ __init int intel_pmu_init(void)
>  	case INTEL_FAM6_EMERALDRAPIDS_X:
>  		x86_pmu.flags |= PMU_FL_MEM_LOADS_AUX;
>  		x86_pmu.extra_regs = intel_glc_extra_regs;
> -		fallthrough;
> +		pr_cont("Sapphire Rapids events, ");
> +		name = "sapphire_rapids";
> +		goto glc_common;
> +
>  	case INTEL_FAM6_GRANITERAPIDS_X:
>  	case INTEL_FAM6_GRANITERAPIDS_D:
> +		x86_pmu.extra_regs = intel_rwc_extra_regs;
> +		pr_cont("Granite Rapids events, ");
> +		name = "granite_rapids";
> +
> +	glc_common:
>  		intel_pmu_init_glc(NULL);
> -		if (!x86_pmu.extra_regs)
> -			x86_pmu.extra_regs = intel_rwc_extra_regs;
>  		x86_pmu.pebs_ept = 1;
>  		x86_pmu.hw_config = hsw_hw_config;
>  		x86_pmu.get_event_constraints = glc_get_event_constraints;
> @@ -6804,8 +6810,6 @@ __init int intel_pmu_init(void)
>  		td_attr = glc_td_events_attrs;
>  		tsx_attr = glc_tsx_events_attrs;
>  		intel_pmu_pebs_data_source_skl(true);
> -		pr_cont("Sapphire Rapids events, ");
> -		name = "sapphire_rapids";
>  		break;
>  
>  	case INTEL_FAM6_ALDERLAKE:
>