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

Peter Zijlstra
2024-08-10T21:55:49Z

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.

> };
>
> 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,

> +};
>
> - 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
>