Re: [RESEND Patch 1/2] perf/x86: Remove perf_events_lapic_init() calling from x86_pmu_enable()

Re: [RESEND Patch 1/2] perf/x86: Remove perf_events_lapic_init() calling from x86_pmu_enable()

[Peter Zijlstra]

On Wed, Jul 03, 2024 at 06:57:02AM +0800, Dapeng Mi wrote:
> perf_events_lapic_init() helper is called to configure PMI to NMI vector
> and clear MASK bit simultaneously by writing APIC_LVTPC MSR. It's called
> firstly to initialize APIC_LVTPC MSR by init_hw_perf_events(), and the
> PMI handler would always to clear the MASK bit in APIC_LVTPC MSR by
> writing APIC_LVTPC MSR directly.
> 
> So it becomes unnecessary to call perf_events_lapic_init() again in
> x86_pmu_enable(), and worse x86_pmu_enable() could be called very
> frequently in some scenarios with very high context-switches. This would
> cause performance overhead which can't be ignored especially in KVM guest
> environment since frequent APIC_LVTPC writing would cause huge number
> of VM-Exits.
> 
> For example, in guest environment Geekbench score (running multiplxing
> perf-stat command in background) increases 1% and perf-sched benchmark
> increases 7% after removing perf_events_lapic_init() calling from
> x86_pmu_enable().
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  arch/x86/events/core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 5b0dd07b1ef1..580923443813 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1347,7 +1347,6 @@ static void x86_pmu_enable(struct pmu *pmu)
>  			x86_pmu_start(event, PERF_EF_RELOAD);
>  		}
>  		cpuc->n_added = 0;
> -		perf_events_lapic_init();
>  	}

Stephane, I don't suppose you remember why you put that there? Afaict
the above reasoning is correct and this one is entirely superfluous, but
this hardware is 'funny' at the best of times and maybe I'm overlooking
something.

[RESEND Patch 1/2] perf/x86: Remove perf_events_lapic_init() calling from x86_pmu_enable()

[Dapeng Mi]

perf_events_lapic_init() helper is called to configure PMI to NMI vector
and clear MASK bit simultaneously by writing APIC_LVTPC MSR. It's called
firstly to initialize APIC_LVTPC MSR by init_hw_perf_events(), and the
PMI handler would always to clear the MASK bit in APIC_LVTPC MSR by
writing APIC_LVTPC MSR directly.

So it becomes unnecessary to call perf_events_lapic_init() again in
x86_pmu_enable(), and worse x86_pmu_enable() could be called very
frequently in some scenarios with very high context-switches. This would
cause performance overhead which can't be ignored especially in KVM guest
environment since frequent APIC_LVTPC writing would cause huge number
of VM-Exits.

For example, in guest environment Geekbench score (running multiplxing
perf-stat command in background) increases 1% and perf-sched benchmark
increases 7% after removing perf_events_lapic_init() calling from
x86_pmu_enable().

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/events/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 5b0dd07b1ef1..580923443813 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1347,7 +1347,6 @@ static void x86_pmu_enable(struct pmu *pmu)
 			x86_pmu_start(event, PERF_EF_RELOAD);
 		}
 		cpuc->n_added = 0;
-		perf_events_lapic_init();
 	}
 
 	cpuc->enabled = 1;

base-commit: 73e931504f8e0d42978bfcda37b323dbbd1afc08
-- 
2.40.1

[RESEND Patch 2/2] perf/x86: Typos and invalid indents fix

[Dapeng Mi]

Fix several typos and invalid indents.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/events/intel/core.c      | 2 +-
 arch/x86/include/asm/perf_event.h | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 38c1b1f1deaa..9600af53d1bc 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3697,7 +3697,7 @@ static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
 	intel_put_shared_regs_event_constraints(cpuc, event);
 
 	/*
-	 * is PMU has exclusive counter restrictions, then
+	 * If PMU has exclusive counter restrictions, then
 	 * all events are subject to and must call the
 	 * put_excl_constraints() routine
 	 */
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 7f1e17250546..d54aa3eb65f5 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -399,15 +399,15 @@ static inline bool is_topdown_idx(int idx)
  *
  * With this fake counter assigned, the guest LBR event user (such as KVM),
  * can program the LBR registers on its own, and we don't actually do anything
- * with then in the host context.
+ * with them in the host context.
  */
-#define INTEL_PMC_IDX_FIXED_VLBR	(GLOBAL_STATUS_LBRS_FROZEN_BIT)
+#define INTEL_PMC_IDX_FIXED_VLBR		(GLOBAL_STATUS_LBRS_FROZEN_BIT)
 
 /*
  * Pseudo-encoding the guest LBR event as event=0x00,umask=0x1b,
  * since it would claim bit 58 which is effectively Fixed26.
  */
-#define INTEL_FIXED_VLBR_EVENT	0x1b00
+#define INTEL_FIXED_VLBR_EVENT			0x1b00
 
 /*
  * Adaptive PEBS v4
-- 
2.40.1