Re: [PATCH 3/4] perf/x86/intel: Support hybrid PMU with multiple atom uarchs

Mi, Dapeng
2024-08-16T03:54:52Z

On 8/12/2024 11:27 AM, Zhenyu Wang wrote:
> On 2024.08.12 11:18:34 +0800, Mi, Dapeng wrote:
>> On 8/11/2024 5:55 AM, Peter Zijlstra wrote:
>>> On Thu, Aug 08, 2024 at 02:02:09PM +0000, Dapeng Mi wrote:
>>>> arch/x86/events/intel/core.c | 24 +++++++++++++++++-------
>>>> arch/x86/events/perf_event.h | 18 +++++++++++++++++-
>>>> 2 files changed, 34 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>>>> index 0c9c2706d4ec..b6429bc009c0 100644
>>>> --- a/arch/x86/events/intel/core.c
>>>> +++ b/arch/x86/events/intel/core.c
>>>> @@ -6218,6 +6227,7 @@ static inline int intel_pmu_v6_addr_offset(int index, bool eventsel)
>>>> static const struct { enum hybrid_pmu_type id; char *name; } intel_hybrid_pmu_type_map[] __initconst = {
>>>> { hybrid_small, "cpu_atom" },
>>>> { hybrid_big, "cpu_core" },
>>>> + { hybrid_small2, "cpu_atom2" },
>>> This is awfully uninspired and quite terrible. How is one supposed to
>>> know which is which? A possibly better naming might be: hybrid_tiny,
>>> "cpu_lowpower" or whatever.
>> We have lots of discussion internally about the naming, but unfortunately
>> we can't come to a conclusion. The reason that we select "cpu_atom2" is
>> that it's generic enough and won't expose too much model specific
>> information, we can reuse it if there are similar platforms in the future.
>> But of course I admit the name is indeed uninspired and easy to cause
>> confusion.
>>
>> The other names which I ever discussed are "cpu_lp_soc", "cpu_soc" and
>> "cpu_atom_soc", but this name would expose some model specific architecture
>> information more or less, not sure which one is better. How is your opinion
>> on this?
>>
> Now I don't like to put 'soc' in name as it's specific for platform design
> e.g ARL-H, but pmu actually only cares about cpu type. Maybe "cpu_atom_lp"
> is good enough.

Synced with Kan, Andi and Zhenyu, all prefer to use the name
"cpu_lowpower". If no one objects it, it would be used as official name.

>
>>>> };
>>>>
>>>> static __always_inline int intel_pmu_init_hybrid(enum hybrid_pmu_type pmus)
>>>> @@ -6250,7 +6260,7 @@ static __always_inline int intel_pmu_init_hybrid(enum hybrid_pmu_type pmus)
>>>> 0, x86_pmu_num_counters(&pmu->pmu), 0, 0);
>>>>
>>>> pmu->intel_cap.capabilities = x86_pmu.intel_cap.capabilities;
>>>> - if (pmu->pmu_type & hybrid_small) {
>>>> + if (pmu->pmu_type & hybrid_small_all) {
>>>> pmu->intel_cap.perf_metrics = 0;
>>>> pmu->intel_cap.pebs_output_pt_available = 1;
>>>> pmu->mid_ack = true;
>>>> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
>>>> index 5d1677844e04..f7b55c909eff 100644
>>>> --- a/arch/x86/events/perf_event.h
>>>> +++ b/arch/x86/events/perf_event.h
>>>> @@ -668,6 +668,13 @@ enum {
>>>> #define PERF_PEBS_DATA_SOURCE_GRT_MAX 0x10
>>>> #define PERF_PEBS_DATA_SOURCE_GRT_MASK (PERF_PEBS_DATA_SOURCE_GRT_MAX - 1)
>>>>
>>>> +
>>>> +/*
>>>> + * CPUID.1AH.EAX[31:0] uniquely identifies the microarchitecture
>>>> + * of the core. Bits 31-24 indicates its core type (Core or Atom)
>>>> + * and Bits [23:0] indicates the native model ID of the core.
>>>> + * Core type and native model ID are defined in below enumerations.
>>>> + */
>>>> enum hybrid_cpu_type {
>>>> HYBRID_INTEL_NONE,
>>>> HYBRID_INTEL_ATOM = 0x20,
>>>> @@ -676,12 +683,21 @@ enum hybrid_cpu_type {
>>>>
>>>> #define X86_HYBRID_PMU_ATOM_IDX 0
>>>> #define X86_HYBRID_PMU_CORE_IDX 1
>>>> +#define X86_HYBRID_PMU_ATOM2_IDX 2
>>>> enum hybrid_pmu_type {
>>>> not_hybrid,
>>>> hybrid_small = BIT(X86_HYBRID_PMU_ATOM_IDX),
>>>> hybrid_big = BIT(X86_HYBRID_PMU_CORE_IDX),
>>>> + hybrid_small2 = BIT(X86_HYBRID_PMU_ATOM2_IDX),
>>>> + /* The belows are only used for matching */
>>>> + hybrid_big_small = hybrid_big | hybrid_small,
>>>> + hybrid_small_all = hybrid_small | hybrid_small2,
>>>> + hybrid_big_small_arl_h = hybrid_big | hybrid_small_all,
>>> Same complaint, how about:
>>>
>>> + hybrid_tiny = BIT(X86_HYBRID_PMU_TINY_IDX),
>>> hybrid_big_small = hybrid_big | hybrid_small,
>>> + hybrid_small_tiny = hybrid_small | hybrid_tiny,
>>> + hybrid_big_small_tiny = hybrid_big_small | hybrid_tiny,
>> Sure. I would adjust the macro name base on the above discussed final name.
>> Thanks.
>>
>>
>>>
>>>> +};
>>>>
>>>> - hybrid_big_small = hybrid_big | hybrid_small, /* only used for matching */
>>>> +enum atom_native_id {
>>>> + cmt_native_id = 0x2, /* Crestmont */
>>>> + skt_native_id = 0x3, /* Skymont */
>>>> };
>>>>
>>>> struct x86_hybrid_pmu {
>>>> --
>>>> 2.40.1
>>>>