[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <61c1018e-fcdc-461b-8de4-845b33f619c0@intel.com>
Date: Mon, 27 Jan 2025 10:19:22 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: <kan.liang@...ux.intel.com>, <peterz@...radead.org>, <mingo@...hat.com>,
<acme@...nel.org>, <namhyung@...nel.org>, <irogers@...gle.com>,
<alexander.shishkin@...ux.intel.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf/x86/intel: Clean up PEBS-via-PT on hybrid
On 24/01/25 20:34, kan.liang@...ux.intel.com wrote:
> From: Kan Liang <kan.liang@...ux.intel.com>
>
> The PEBS-via-PT feature is exposed for the e-core of some hybrid
> platforms, e.g., ADL and MTL. But it never works.
>
> $ dmesg | grep PEBS
> [ 1.793888] core: cpu_atom PMU driver: PEBS-via-PT
>
> $ perf record -c 1000 -e '{intel_pt/branch=0/,
> cpu_atom/cpu-cycles,aux-output/pp}' -C8
> Error:
> The sys_perf_event_open() syscall returned with 22 (Invalid argument)
> for event (cpu_atom/cpu-cycles,aux-output/pp).
> "dmesg | grep -i perf" may provide additional information.
>
> The "PEBS-via-PT" is printed if the corresponding bit of per-PMU
> capabilities is set. Since the feature is supported by the e-core HW,
> perf sets the bit for e-core. However, for Intel PT, if a feature is not
> supported on all CPUs, it is not supported at all. The PEBS-via-PT event
> cannot be created successfully.
>
> The PEBS-via-PT is no longer enumerated on the latest hybrid platform. It
> will be deprecated on future platforms with Arch PEBS. Let's remove it
> from the existing hybrid platforms.
>
> Fixes: d9977c43bff8 ("perf/x86: Register hybrid PMUs")
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> ---
> arch/x86/events/intel/core.c | 10 ----------
> arch/x86/events/intel/ds.c | 2 +-
> 2 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 7f1b6b90a5fb..0a1030eb6db8 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -4981,11 +4981,6 @@ static void intel_pmu_check_hybrid_pmus(struct x86_hybrid_pmu *pmu)
> else
> pmu->intel_ctrl &= ~(1ULL << GLOBAL_CTRL_EN_PERF_METRICS);
>
> - if (pmu->intel_cap.pebs_output_pt_available)
> - pmu->pmu.capabilities |= PERF_PMU_CAP_AUX_OUTPUT;
> - else
> - pmu->pmu.capabilities &= ~PERF_PMU_CAP_AUX_OUTPUT;
> -
> intel_pmu_check_event_constraints(pmu->event_constraints,
> pmu->cntr_mask64,
> pmu->fixed_cntr_mask64,
> @@ -5063,9 +5058,6 @@ static bool init_hybrid_pmu(int cpu)
>
> pr_info("%s PMU driver: ", pmu->name);
>
> - if (pmu->intel_cap.pebs_output_pt_available)
> - pr_cont("PEBS-via-PT ");
> -
> pr_cont("\n");
>
> x86_pmu_show_pmu_cap(&pmu->pmu);
> @@ -6420,11 +6412,9 @@ static __always_inline int intel_pmu_init_hybrid(enum hybrid_pmu_type pmus)
> pmu->intel_cap.capabilities = x86_pmu.intel_cap.capabilities;
> if (pmu->pmu_type & hybrid_small_tiny) {
> pmu->intel_cap.perf_metrics = 0;
> - pmu->intel_cap.pebs_output_pt_available = 1;
> pmu->mid_ack = true;
> } else if (pmu->pmu_type & hybrid_big) {
> pmu->intel_cap.perf_metrics = 1;
> - pmu->intel_cap.pebs_output_pt_available = 0;
> pmu->late_ack = true;
> }
> }
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 13a78a8a2780..123b0e4392b4 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -2742,7 +2742,7 @@ void __init intel_ds_init(void)
> }
> pr_cont("PEBS fmt%d%c%s, ", format, pebs_type, pebs_qual);
>
> - if (!is_hybrid() && x86_pmu.intel_cap.pebs_output_pt_available) {
> + if (x86_pmu.intel_cap.pebs_output_pt_available) {
I'd suggest keeping the !is_hybrid() check here
since it documents and enforces the effective policy.
> pr_cont("PEBS-via-PT, ");
> x86_get_pmu(smp_processor_id())->capabilities |= PERF_PMU_CAP_AUX_OUTPUT;
> }
Powered by blists - more mailing lists