| Message ID | 20240617-add-cpu-type-v1-3-b88998c01e76@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add CPU-type to topology | expand |
On 6/17/24 02:11, Pawan Gupta wrote: > find_hybrid_pmu_for_cpu() uses get_this_hybrid_cpu_type() to get the CPU > type, but it returns an invalid cpu-type when X86_FEATURE_HYBRID_CPU is not > set. Some hybrid variants do enumerate cpu-type regardless of > X86_FEATURE_HYBRID_CPU. I'm not fully sure what point this is trying to make. Is this trying to make the case that get_this_hybrid_cpu_type() and topology_cpu_type() are equivalent or pointing out a difference?
On Mon, Jun 17, 2024 at 07:50:53AM -0700, Dave Hansen wrote: > On 6/17/24 02:11, Pawan Gupta wrote: > > find_hybrid_pmu_for_cpu() uses get_this_hybrid_cpu_type() to get the CPU > > type, but it returns an invalid cpu-type when X86_FEATURE_HYBRID_CPU is not > > set. Some hybrid variants do enumerate cpu-type regardless of > > X86_FEATURE_HYBRID_CPU. > > I'm not fully sure what point this is trying to make. Sorry it was not clear. I will rephrase it. > Is this trying to make the case that get_this_hybrid_cpu_type() and > topology_cpu_type() are equivalent or pointing out a difference? Pointing out a difference. get_this_hybrid_cpu_type() misses a case when cpu-type is enumerated regardless of X86_FEATURE_HYBRID_CPU. I don't think checking for the hybrid feature is necessary here, because there is an existing fixup for this case: static struct x86_hybrid_pmu *find_hybrid_pmu_for_cpu(void) { u8 cpu_type = topology_cpu_type(smp_processor_id()); int i; /* * This is running on a CPU model that is known to have hybrid * configurations. But the CPU told us it is not hybrid, shame * on it. There should be a fixup function provided for these * troublesome CPUs (->get_hybrid_cpu_type). */ if (cpu_type == HYBRID_INTEL_NONE) { if (x86_pmu.get_hybrid_cpu_type) cpu_type = x86_pmu.get_hybrid_cpu_type(); else return NULL; }
On 6/17/24 11:09, Pawan Gupta wrote: >> Is this trying to make the case that get_this_hybrid_cpu_type() and >> topology_cpu_type() are equivalent or pointing out a difference? > Pointing out a difference. get_this_hybrid_cpu_type() misses a case when > cpu-type is enumerated regardless of X86_FEATURE_HYBRID_CPU. I don't think > checking for the hybrid feature is necessary here, because there is an > existing fixup for this case: OK, that makes sense. Could you include that in the changelog, please?
On Mon, Jun 17, 2024 at 11:17:35AM -0700, Dave Hansen wrote: > On 6/17/24 11:09, Pawan Gupta wrote: > >> Is this trying to make the case that get_this_hybrid_cpu_type() and > >> topology_cpu_type() are equivalent or pointing out a difference? > > Pointing out a difference. get_this_hybrid_cpu_type() misses a case when > > cpu-type is enumerated regardless of X86_FEATURE_HYBRID_CPU. I don't think > > checking for the hybrid feature is necessary here, because there is an > > existing fixup for this case: > > OK, that makes sense. Could you include that in the changelog, please? Sure.
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c index 38c1b1f1deaa..8067a735705a 100644 --- a/arch/x86/events/intel/core.c +++ b/arch/x86/events/intel/core.c @@ -4753,7 +4753,7 @@ static void intel_pmu_check_hybrid_pmus(struct x86_hybrid_pmu *pmu) static struct x86_hybrid_pmu *find_hybrid_pmu_for_cpu(void) { - u8 cpu_type = get_this_hybrid_cpu_type(); + u8 cpu_type = topology_cpu_type(smp_processor_id()); int i; /*
find_hybrid_pmu_for_cpu() uses get_this_hybrid_cpu_type() to get the CPU type, but it returns an invalid cpu-type when X86_FEATURE_HYBRID_CPU is not set. Some hybrid variants do enumerate cpu-type regardless of X86_FEATURE_HYBRID_CPU. Replace get_this_hybrid_cpu_type() with topology_cpu_type() to get cpu-type irrespective of hybrid status. Moreover, get_this_hybrid_cpu_type() executes the CPUID instruction to get the cpu-type, which is slower than using the per-cpu value. Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> --- arch/x86/events/intel/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)