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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZrmBMMSdvLPGlgEI@debian-scheme>
Date: Mon, 12 Aug 2024 11:27:44 +0800
From: Zhenyu Wang <zhenyuw@...ux.intel.com>
To: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Kan Liang <kan.liang@...ux.intel.com>, linux-kernel@...r.kernel.org,
	Andi Kleen <ak@...ux.intel.com>,
	Zhenyu Wang <zhenyuw@...ux.intel.com>,
	Yongwei Ma <yongwei.ma@...el.com>,
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
	Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [PATCH 3/4] perf/x86/intel: Support hybrid PMU with multiple
 atom uarchs

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.

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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)