[PATCH v2 0/9] perf/x86: Some cleanups

[PATCH v2 0/9] perf/x86: Some cleanups

[Peter Zijlstra]

Hi,

I found I still had these patches laying about. I dropped the removal of the
minimal period or 2 and there's a few extra patches since the first posting.

  https://lkml.kernel.org/r/20220511142037.353492804@infradead.org

[PATCH v2 1/9] perf/x86: Add two more x86_pmu methods

[Peter Zijlstra]

In order to clean up x86_perf_event_{set_period,update)() start by
adding them as x86_pmu methods.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/core.c       |   22 +++++++++++++++++-----
 arch/x86/events/perf_event.h |    5 +++++
 2 files changed, 22 insertions(+), 5 deletions(-)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -72,6 +72,9 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_add,  *x
 DEFINE_STATIC_CALL_NULL(x86_pmu_del,  *x86_pmu.del);
 DEFINE_STATIC_CALL_NULL(x86_pmu_read, *x86_pmu.read);
 
+DEFINE_STATIC_CALL_NULL(x86_pmu_set_period, *x86_pmu.set_period);
+DEFINE_STATIC_CALL_NULL(x86_pmu_update,     *x86_pmu.update);
+
 DEFINE_STATIC_CALL_NULL(x86_pmu_schedule_events,       *x86_pmu.schedule_events);
 DEFINE_STATIC_CALL_NULL(x86_pmu_get_event_constraints, *x86_pmu.get_event_constraints);
 DEFINE_STATIC_CALL_NULL(x86_pmu_put_event_constraints, *x86_pmu.put_event_constraints);
@@ -1518,7 +1521,7 @@ static void x86_pmu_start(struct perf_ev
 
 	if (flags & PERF_EF_RELOAD) {
 		WARN_ON_ONCE(!(event->hw.state & PERF_HES_UPTODATE));
-		x86_perf_event_set_period(event);
+		static_call(x86_pmu_set_period)(event);
 	}
 
 	event->hw.state = 0;
@@ -1610,7 +1613,7 @@ void x86_pmu_stop(struct perf_event *eve
 		 * Drain the remaining delta count out of a event
 		 * that we are disabling:
 		 */
-		x86_perf_event_update(event);
+		static_call(x86_pmu_update)(event);
 		hwc->state |= PERF_HES_UPTODATE;
 	}
 }
@@ -1700,7 +1703,7 @@ int x86_pmu_handle_irq(struct pt_regs *r
 
 		event = cpuc->events[idx];
 
-		val = x86_perf_event_update(event);
+		val = static_call(x86_pmu_update)(event);
 		if (val & (1ULL << (x86_pmu.cntval_bits - 1)))
 			continue;
 
@@ -1709,7 +1712,7 @@ int x86_pmu_handle_irq(struct pt_regs *r
 		 */
 		handled++;
 
-		if (!x86_perf_event_set_period(event))
+		if (!static_call(x86_pmu_set_period)(event))
 			continue;
 
 		perf_sample_data_init(&data, 0, event->hw.last_period);
@@ -2023,6 +2026,9 @@ static void x86_pmu_static_call_update(v
 	static_call_update(x86_pmu_del, x86_pmu.del);
 	static_call_update(x86_pmu_read, x86_pmu.read);
 
+	static_call_update(x86_pmu_set_period, x86_pmu.set_period);
+	static_call_update(x86_pmu_update, x86_pmu.update);
+
 	static_call_update(x86_pmu_schedule_events, x86_pmu.schedule_events);
 	static_call_update(x86_pmu_get_event_constraints, x86_pmu.get_event_constraints);
 	static_call_update(x86_pmu_put_event_constraints, x86_pmu.put_event_constraints);
@@ -2042,7 +2048,7 @@ static void x86_pmu_static_call_update(v
 
 static void _x86_pmu_read(struct perf_event *event)
 {
-	x86_perf_event_update(event);
+	static_call(x86_pmu_update)(event);
 }
 
 void x86_pmu_show_pmu_cap(int num_counters, int num_counters_fixed,
@@ -2148,6 +2154,12 @@ static int __init init_hw_perf_events(vo
 	if (!x86_pmu.guest_get_msrs)
 		x86_pmu.guest_get_msrs = (void *)&__static_call_return0;
 
+	if (!x86_pmu.set_period)
+		x86_pmu.set_period = x86_perf_event_set_period;
+
+	if (!x86_pmu.update)
+		x86_pmu.update = x86_perf_event_update;
+
 	x86_pmu_static_call_update();
 
 	/*
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -735,6 +735,8 @@ struct x86_pmu {
 	void		(*add)(struct perf_event *);
 	void		(*del)(struct perf_event *);
 	void		(*read)(struct perf_event *event);
+	int		(*set_period)(struct perf_event *event);
+	u64		(*update)(struct perf_event *event);
 	int		(*hw_config)(struct perf_event *event);
 	int		(*schedule_events)(struct cpu_hw_events *cpuc, int n, int *assign);
 	unsigned	eventsel;
@@ -1031,6 +1033,9 @@ static struct perf_pmu_format_hybrid_att
 struct pmu *x86_get_pmu(unsigned int cpu);
 extern struct x86_pmu x86_pmu __read_mostly;
 
+DECLARE_STATIC_CALL(x86_pmu_set_period, *x86_pmu.set_period);
+DECLARE_STATIC_CALL(x86_pmu_update,     *x86_pmu.update);
+
 static __always_inline struct x86_perf_task_context_opt *task_context_opt(void *ctx)
 {
 	if (static_cpu_has(X86_FEATURE_ARCH_LBR))

[PATCH v2 2/9] perf/x86/intel: Move the topdown stuff into the intel driver

[Peter Zijlstra]

Use the new x86_pmu::{set_period,update}() methods to push the topdown
stuff into the Intel driver, where it belongs.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/core.c       |    7 -------
 arch/x86/events/intel/core.c |   28 +++++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 10 deletions(-)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -119,9 +119,6 @@ u64 x86_perf_event_update(struct perf_ev
 	if (unlikely(!hwc->event_base))
 		return 0;
 
-	if (unlikely(is_topdown_count(event)) && x86_pmu.update_topdown_event)
-		return x86_pmu.update_topdown_event(event);
-
 	/*
 	 * Careful: an NMI might modify the previous event value.
 	 *
@@ -1373,10 +1370,6 @@ int x86_perf_event_set_period(struct per
 	if (unlikely(!hwc->event_base))
 		return 0;
 
-	if (unlikely(is_topdown_count(event)) &&
-	    x86_pmu.set_topdown_event_period)
-		return x86_pmu.set_topdown_event_period(event);
-
 	/*
 	 * If we are way outside a reasonable range then just skip forward:
 	 */
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2301,7 +2301,7 @@ static void intel_pmu_nhm_workaround(voi
 	for (i = 0; i < 4; i++) {
 		event = cpuc->events[i];
 		if (event)
-			x86_perf_event_update(event);
+			static_call(x86_pmu_update)(event);
 	}
 
 	for (i = 0; i < 4; i++) {
@@ -2316,7 +2316,7 @@ static void intel_pmu_nhm_workaround(voi
 		event = cpuc->events[i];
 
 		if (event) {
-			x86_perf_event_set_period(event);
+			static_call(x86_pmu_set_period)(event);
 			__x86_pmu_enable_event(&event->hw,
 					ARCH_PERFMON_EVENTSEL_ENABLE);
 		} else
@@ -2793,7 +2793,7 @@ static void intel_pmu_add_event(struct p
  */
 int intel_pmu_save_and_restart(struct perf_event *event)
 {
-	x86_perf_event_update(event);
+	static_call(x86_pmu_update)(event);
 	/*
 	 * For a checkpointed counter always reset back to 0.  This
 	 * avoids a situation where the counter overflows, aborts the
@@ -2805,9 +2805,27 @@ int intel_pmu_save_and_restart(struct pe
 		wrmsrl(event->hw.event_base, 0);
 		local64_set(&event->hw.prev_count, 0);
 	}
+	return static_call(x86_pmu_set_period)(event);
+}
+
+static int intel_pmu_set_period(struct perf_event *event)
+{
+	if (unlikely(is_topdown_count(event)) &&
+	    x86_pmu.set_topdown_event_period)
+		return x86_pmu.set_topdown_event_period(event);
+
 	return x86_perf_event_set_period(event);
 }
 
+static u64 intel_pmu_update(struct perf_event *event)
+{
+	if (unlikely(is_topdown_count(event)) &&
+	    x86_pmu.update_topdown_event)
+		return x86_pmu.update_topdown_event(event);
+
+	return x86_perf_event_update(event);
+}
+
 static void intel_pmu_reset(void)
 {
 	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
@@ -4635,6 +4653,10 @@ static __initconst const struct x86_pmu
 	.enable_all		= core_pmu_enable_all,
 	.enable			= core_pmu_enable_event,
 	.disable		= x86_pmu_disable_event,
+
+	.set_period		= intel_pmu_set_period,
+	.update			= intel_pmu_update,
+
 	.hw_config		= core_pmu_hw_config,
 	.schedule_events	= x86_schedule_events,
 	.eventsel		= MSR_ARCH_PERFMON_EVENTSEL0,

[PATCH v2 3/9] perf/x86: Change x86_pmu::limit_period signature

[Peter Zijlstra]

In preparation for making it a static_call, change the signature.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/amd/core.c   |    8 +++-----
 arch/x86/events/core.c       |   13 ++++++++-----
 arch/x86/events/intel/core.c |   19 ++++++++-----------
 arch/x86/events/perf_event.h |    2 +-
 4 files changed, 20 insertions(+), 22 deletions(-)

--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -1222,16 +1222,14 @@ static ssize_t amd_event_sysfs_show(char
 	return x86_event_sysfs_show(page, config, event);
 }
 
-static u64 amd_pmu_limit_period(struct perf_event *event, u64 left)
+static void amd_pmu_limit_period(struct perf_event *event, s64 *left)
 {
 	/*
 	 * Decrease period by the depth of the BRS feature to get the last N
 	 * taken branches and approximate the desired period
 	 */
-	if (has_branch_stack(event) && left > x86_pmu.lbr_nr)
-		left -= x86_pmu.lbr_nr;
-
-	return left;
+	if (has_branch_stack(event) && *left > x86_pmu.lbr_nr)
+		*left -= x86_pmu.lbr_nr;
 }
 
 static __initconst const struct x86_pmu amd_pmu = {
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -621,8 +621,9 @@ int x86_pmu_hw_config(struct perf_event
 		event->hw.config |= event->attr.config & X86_RAW_EVENT_MASK;
 
 	if (event->attr.sample_period && x86_pmu.limit_period) {
-		if (x86_pmu.limit_period(event, event->attr.sample_period) >
-				event->attr.sample_period)
+		s64 left = event->attr.sample_period;
+		x86_pmu.limit_period(event, &left);
+		if (left > event->attr.sample_period)
 			return -EINVAL;
 	}
 
@@ -1396,9 +1397,9 @@ int x86_perf_event_set_period(struct per
 		left = x86_pmu.max_period;
 
 	if (x86_pmu.limit_period)
-		left = x86_pmu.limit_period(event, left);
+		x86_pmu.limit_period(event, &left);
 
-	per_cpu(pmc_prev_left[idx], smp_processor_id()) = left;
+	this_cpu_write(pmc_prev_left[idx], left);
 
 	/*
 	 * The hw event starts counting from this event offset,
@@ -2675,7 +2676,9 @@ static int x86_pmu_check_period(struct p
 		return -EINVAL;
 
 	if (value && x86_pmu.limit_period) {
-		if (x86_pmu.limit_period(event, value) > value)
+		s64 left = value;
+		x86_pmu.limit_period(event, &left);
+		if (left > value)
 			return -EINVAL;
 	}
 
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4342,28 +4342,25 @@ static u8 adl_get_hybrid_cpu_type(void)
  * Therefore the effective (average) period matches the requested period,
  * despite coarser hardware granularity.
  */
-static u64 bdw_limit_period(struct perf_event *event, u64 left)
+static void bdw_limit_period(struct perf_event *event, s64 *left)
 {
 	if ((event->hw.config & INTEL_ARCH_EVENT_MASK) ==
 			X86_CONFIG(.event=0xc0, .umask=0x01)) {
-		if (left < 128)
-			left = 128;
-		left &= ~0x3fULL;
+		if (*left < 128)
+			*left = 128;
+		*left &= ~0x3fULL;
 	}
-	return left;
 }
 
-static u64 nhm_limit_period(struct perf_event *event, u64 left)
+static void nhm_limit_period(struct perf_event *event, s64 *left)
 {
-	return max(left, 32ULL);
+	*left = max(*left, 32LL);
 }
 
-static u64 spr_limit_period(struct perf_event *event, u64 left)
+static void spr_limit_period(struct perf_event *event, s64 *left)
 {
 	if (event->attr.precise_ip == 3)
-		return max(left, 128ULL);
-
-	return left;
+		*left = max(*left, 128LL);
 }
 
 PMU_FORMAT_ATTR(event,	"config:0-7"	);
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -783,7 +783,7 @@ struct x86_pmu {
 	struct event_constraint *event_constraints;
 	struct x86_pmu_quirk *quirks;
 	int		perfctr_second_write;
-	u64		(*limit_period)(struct perf_event *event, u64 l);
+	void		(*limit_period)(struct perf_event *event, s64 *l);
 
 	/* PMI handler bits */
 	unsigned int	late_ack		:1,

[PATCH v2 4/9] perf/x86: Add a x86_pmu::limit_period static_call

[Peter Zijlstra]

Avoid a branch and indirect call.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/core.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -72,8 +72,9 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_add,  *x
 DEFINE_STATIC_CALL_NULL(x86_pmu_del,  *x86_pmu.del);
 DEFINE_STATIC_CALL_NULL(x86_pmu_read, *x86_pmu.read);
 
-DEFINE_STATIC_CALL_NULL(x86_pmu_set_period, *x86_pmu.set_period);
-DEFINE_STATIC_CALL_NULL(x86_pmu_update,     *x86_pmu.update);
+DEFINE_STATIC_CALL_NULL(x86_pmu_set_period,   *x86_pmu.set_period);
+DEFINE_STATIC_CALL_NULL(x86_pmu_update,       *x86_pmu.update);
+DEFINE_STATIC_CALL_NULL(x86_pmu_limit_period, *x86_pmu.limit_period);
 
 DEFINE_STATIC_CALL_NULL(x86_pmu_schedule_events,       *x86_pmu.schedule_events);
 DEFINE_STATIC_CALL_NULL(x86_pmu_get_event_constraints, *x86_pmu.get_event_constraints);
@@ -1391,8 +1392,7 @@ int x86_perf_event_set_period(struct per
 	if (left > x86_pmu.max_period)
 		left = x86_pmu.max_period;
 
-	if (x86_pmu.limit_period)
-		x86_pmu.limit_period(event, &left);
+	static_call_cond(x86_pmu_limit_period)(event, &left);
 
 	this_cpu_write(pmc_prev_left[idx], left);
 
@@ -2017,6 +2017,7 @@ static void x86_pmu_static_call_update(v
 
 	static_call_update(x86_pmu_set_period, x86_pmu.set_period);
 	static_call_update(x86_pmu_update, x86_pmu.update);
+	static_call_update(x86_pmu_limit_period, x86_pmu.limit_period);
 
 	static_call_update(x86_pmu_schedule_events, x86_pmu.schedule_events);
 	static_call_update(x86_pmu_get_event_constraints, x86_pmu.get_event_constraints);

[PATCH v2 5/9] perf/x86/intel: Remove x86_pmu::set_topdown_event_period

[Peter Zijlstra]

Now that it is all internal to the intel driver, remove
x86_pmu::set_topdown_event_period.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/intel/core.c |   16 ++++++++++------
 arch/x86/events/perf_event.h |    1 -
 2 files changed, 10 insertions(+), 7 deletions(-)

--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2520,6 +2520,8 @@ static int adl_set_topdown_event_period(
 	return icl_set_topdown_event_period(event);
 }
 
+DEFINE_STATIC_CALL(intel_pmu_set_topdown_event_period, x86_perf_event_set_period);
+
 static inline u64 icl_get_metrics_event_value(u64 metric, u64 slots, int idx)
 {
 	u32 val;
@@ -2810,9 +2812,8 @@ int intel_pmu_save_and_restart(struct pe
 
 static int intel_pmu_set_period(struct perf_event *event)
 {
-	if (unlikely(is_topdown_count(event)) &&
-	    x86_pmu.set_topdown_event_period)
-		return x86_pmu.set_topdown_event_period(event);
+	if (unlikely(is_topdown_count(event)))
+		return static_call(intel_pmu_set_topdown_event_period)(event);
 
 	return x86_perf_event_set_period(event);
 }
@@ -6191,7 +6192,8 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 4;
 		x86_pmu.update_topdown_event = icl_update_topdown_event;
-		x86_pmu.set_topdown_event_period = icl_set_topdown_event_period;
+		static_call_update(intel_pmu_set_topdown_event_period,
+				   &icl_set_topdown_event_period);
 		pr_cont("Icelake events, ");
 		name = "icelake";
 		break;
@@ -6228,7 +6230,8 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 8;
 		x86_pmu.update_topdown_event = icl_update_topdown_event;
-		x86_pmu.set_topdown_event_period = icl_set_topdown_event_period;
+		static_call_update(intel_pmu_set_topdown_event_period,
+				   &icl_set_topdown_event_period);
 		pr_cont("Sapphire Rapids events, ");
 		name = "sapphire_rapids";
 		break;
@@ -6264,7 +6267,8 @@ __init int intel_pmu_init(void)
 		x86_pmu.pebs_latency_data = adl_latency_data_small;
 		x86_pmu.num_topdown_events = 8;
 		x86_pmu.update_topdown_event = adl_update_topdown_event;
-		x86_pmu.set_topdown_event_period = adl_set_topdown_event_period;
+		static_call_update(intel_pmu_set_topdown_event_period,
+				   &adl_set_topdown_event_period);
 
 		x86_pmu.filter_match = intel_pmu_filter_match;
 		x86_pmu.get_event_constraints = adl_get_event_constraints;
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -890,7 +890,6 @@ struct x86_pmu {
 	 */
 	int		num_topdown_events;
 	u64		(*update_topdown_event)(struct perf_event *event);
-	int		(*set_topdown_event_period)(struct perf_event *event);
 
 	/*
 	 * perf task context (i.e. struct perf_event_context::task_ctx_data)

[PATCH v2 6/9] perf/x86/intel: Remove x86_pmu::update_topdown_event

[Peter Zijlstra]

Now that it is all internal to the intel driver, remove
x86_pmu::update_topdown_event.

Assumes that is_topdown_count(event) can only be true when the
hardware has topdown stuff and the function is set.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/intel/core.c |   22 ++++++++++++----------
 arch/x86/events/perf_event.h |    1 -
 2 files changed, 12 insertions(+), 11 deletions(-)

--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2672,6 +2672,7 @@ static u64 adl_update_topdown_event(stru
 	return icl_update_topdown_event(event);
 }
 
+DEFINE_STATIC_CALL(intel_pmu_update_topdown_event, x86_perf_event_update);
 
 static void intel_pmu_read_topdown_event(struct perf_event *event)
 {
@@ -2683,7 +2684,7 @@ static void intel_pmu_read_topdown_event
 		return;
 
 	perf_pmu_disable(event->pmu);
-	x86_pmu.update_topdown_event(event);
+	static_call(intel_pmu_update_topdown_event)(event);
 	perf_pmu_enable(event->pmu);
 }
 
@@ -2691,7 +2692,7 @@ static void intel_pmu_read_event(struct
 {
 	if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
 		intel_pmu_auto_reload_read(event);
-	else if (is_topdown_count(event) && x86_pmu.update_topdown_event)
+	else if (is_topdown_count(event))
 		intel_pmu_read_topdown_event(event);
 	else
 		x86_perf_event_update(event);
@@ -2820,9 +2821,8 @@ static int intel_pmu_set_period(struct p
 
 static u64 intel_pmu_update(struct perf_event *event)
 {
-	if (unlikely(is_topdown_count(event)) &&
-	    x86_pmu.update_topdown_event)
-		return x86_pmu.update_topdown_event(event);
+	if (unlikely(is_topdown_count(event)))
+		return static_call(intel_pmu_update_topdown_event)(event);
 
 	return x86_perf_event_update(event);
 }
@@ -2950,8 +2950,7 @@ static int handle_pmi_common(struct pt_r
 	 */
 	if (__test_and_clear_bit(GLOBAL_STATUS_PERF_METRICS_OVF_BIT, (unsigned long *)&status)) {
 		handled++;
-		if (x86_pmu.update_topdown_event)
-			x86_pmu.update_topdown_event(NULL);
+		static_call(intel_pmu_update_topdown_event)(NULL);
 	}
 
 	/*
@@ -6191,7 +6190,8 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 4;
-		x86_pmu.update_topdown_event = icl_update_topdown_event;
+		static_call_update(intel_pmu_update_topdown_event,
+				   &icl_update_topdown_event);
 		static_call_update(intel_pmu_set_topdown_event_period,
 				   &icl_set_topdown_event_period);
 		pr_cont("Icelake events, ");
@@ -6229,7 +6229,8 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 8;
-		x86_pmu.update_topdown_event = icl_update_topdown_event;
+		static_call_update(intel_pmu_update_topdown_event,
+				   &icl_update_topdown_event);
 		static_call_update(intel_pmu_set_topdown_event_period,
 				   &icl_set_topdown_event_period);
 		pr_cont("Sapphire Rapids events, ");
@@ -6266,7 +6267,8 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_adl();
 		x86_pmu.pebs_latency_data = adl_latency_data_small;
 		x86_pmu.num_topdown_events = 8;
-		x86_pmu.update_topdown_event = adl_update_topdown_event;
+		static_call_update(intel_pmu_update_topdown_event,
+				   &adl_update_topdown_event);
 		static_call_update(intel_pmu_set_topdown_event_period,
 				   &adl_set_topdown_event_period);
 
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -889,7 +889,6 @@ struct x86_pmu {
 	 * Intel perf metrics
 	 */
 	int		num_topdown_events;
-	u64		(*update_topdown_event)(struct perf_event *event);
 
 	/*
 	 * perf task context (i.e. struct perf_event_context::task_ctx_data)

[PATCH v2 7/9] perf/x86/p4: Remove perfctr_second_write quirk

[Peter Zijlstra]

Now that we have a x86_pmu::set_period() method, use it to remove the
perfctr_second_write quirk from the generic code.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/core.c       |   12 +-----------
 arch/x86/events/intel/p4.c   |   37 +++++++++++++++++++++++++++----------
 arch/x86/events/perf_event.h |    2 +-
 3 files changed, 29 insertions(+), 22 deletions(-)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1356,7 +1356,7 @@ static void x86_pmu_enable(struct pmu *p
 	static_call(x86_pmu_enable_all)(added);
 }
 
-static DEFINE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
+DEFINE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
 
 /*
  * Set the next IRQ period, based on the hwc->period_left value.
@@ -1416,16 +1416,6 @@ int x86_perf_event_set_period(struct per
 	if (is_counter_pair(hwc))
 		wrmsrl(x86_pmu_event_addr(idx + 1), 0xffff);
 
-	/*
-	 * Due to erratum on certan cpu we need
-	 * a second write to be sure the register
-	 * is updated properly
-	 */
-	if (x86_pmu.perfctr_second_write) {
-		wrmsrl(hwc->event_base,
-			(u64)(-left) & x86_pmu.cntval_mask);
-	}
-
 	perf_event_update_userpage(event);
 
 	return ret;
--- a/arch/x86/events/intel/p4.c
+++ b/arch/x86/events/intel/p4.c
@@ -1006,6 +1006,29 @@ static void p4_pmu_enable_all(int added)
 	}
 }
 
+static int p4_pmu_set_period(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	s64 left = this_cpu_read(pmc_prev_left[hwc->idx]);
+	int ret;
+
+	ret = x86_perf_event_set_period(event);
+
+	if (hwc->event_base) {
+		/*
+		 * This handles erratum N15 in intel doc 249199-029,
+		 * the counter may not be updated correctly on write
+		 * so we need a second write operation to do the trick
+		 * (the official workaround didn't work)
+		 *
+		 * the former idea is taken from OProfile code
+		 */
+		wrmsrl(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
+	}
+
+	return ret;
+}
+
 static int p4_pmu_handle_irq(struct pt_regs *regs)
 {
 	struct perf_sample_data data;
@@ -1044,7 +1067,7 @@ static int p4_pmu_handle_irq(struct pt_r
 		/* event overflow for sure */
 		perf_sample_data_init(&data, 0, hwc->last_period);
 
-		if (!x86_perf_event_set_period(event))
+		if (!static_call(x86_pmu_set_period)(event))
 			continue;
 
 
@@ -1316,6 +1339,9 @@ static __initconst const struct x86_pmu
 	.enable_all		= p4_pmu_enable_all,
 	.enable			= p4_pmu_enable_event,
 	.disable		= p4_pmu_disable_event,
+
+	.set_period		= p4_pmu_set_period,
+
 	.eventsel		= MSR_P4_BPU_CCCR0,
 	.perfctr		= MSR_P4_BPU_PERFCTR0,
 	.event_map		= p4_pmu_event_map,
@@ -1334,15 +1360,6 @@ static __initconst const struct x86_pmu
 	.max_period		= (1ULL << (ARCH_P4_CNTRVAL_BITS - 1)) - 1,
 	.hw_config		= p4_hw_config,
 	.schedule_events	= p4_pmu_schedule_events,
-	/*
-	 * This handles erratum N15 in intel doc 249199-029,
-	 * the counter may not be updated correctly on write
-	 * so we need a second write operation to do the trick
-	 * (the official workaround didn't work)
-	 *
-	 * the former idea is taken from OProfile code
-	 */
-	.perfctr_second_write	= 1,
 
 	.format_attrs		= intel_p4_formats_attr,
 };
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -772,7 +772,6 @@ struct x86_pmu {
 
 	struct event_constraint *event_constraints;
 	struct x86_pmu_quirk *quirks;
-	int		perfctr_second_write;
 	void		(*limit_period)(struct perf_event *event, s64 *l);
 
 	/* PMI handler bits */
@@ -1049,6 +1048,7 @@ static inline bool x86_pmu_has_lbr_calls
 }
 
 DECLARE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
+DECLARE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
 
 int x86_perf_event_set_period(struct perf_event *event);
 

[PATCH v2 8/9] perf/x86/intel: Shadow MSR_ARCH_PERFMON_FIXED_CTR_CTRL

[Peter Zijlstra]

Less RDMSR is more better.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/intel/core.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2405,6 +2405,8 @@ static inline void intel_clear_masks(str
 	__clear_bit(idx, (unsigned long *)&cpuc->intel_cp_status);
 }
 
+static DEFINE_PER_CPU(u64, intel_fixed_ctrl);
+
 static void intel_pmu_disable_fixed(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
@@ -2426,8 +2428,9 @@ static void intel_pmu_disable_fixed(stru
 	intel_clear_masks(event, idx);
 
 	mask = 0xfULL << ((idx - INTEL_PMC_IDX_FIXED) * 4);
-	rdmsrl(hwc->config_base, ctrl_val);
+	ctrl_val = this_cpu_read(intel_fixed_ctrl);
 	ctrl_val &= ~mask;
+	this_cpu_write(intel_fixed_ctrl, ctrl_val);
 	wrmsrl(hwc->config_base, ctrl_val);
 }
 
@@ -2746,9 +2749,10 @@ static void intel_pmu_enable_fixed(struc
 		mask |= ICL_FIXED_0_ADAPTIVE << (idx * 4);
 	}
 
-	rdmsrl(hwc->config_base, ctrl_val);
+	ctrl_val = this_cpu_read(intel_fixed_ctrl);
 	ctrl_val &= ~mask;
 	ctrl_val |= bits;
+	this_cpu_write(intel_fixed_ctrl, ctrl_val);
 	wrmsrl(hwc->config_base, ctrl_val);
 }
 

[PATCH v2 9/9] perf/x86/intel: Optimize short PEBS counters

[Peter Zijlstra]

XXX: crazy idea; really not sure this is worth the extra complexity

It is possible to have the counter programmed to a value smaller than
the sampling period. In that case, the code suppresses the sample,
recalculates the remaining events and reprograms the counter.

This should also work for PEBS counters (and it does); however
triggering a full PEBS assist and parsing the event from the DS is
more overhead than is required.

As such, detect this case and temporarily suppress PEBS. This will
then trigger a regular PMI for the counter which will reprogram the
event and re-enable PEBS once the target period is in reach.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/intel/core.c |   80 ++++++++++++++++++++++++++++++++++++++-----
 arch/x86/events/perf_event.h |    9 ++++
 2 files changed, 81 insertions(+), 8 deletions(-)

--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2722,12 +2722,7 @@ static void intel_pmu_enable_fixed(struc
 
 	intel_set_masks(event, idx);
 
-	/*
-	 * Enable IRQ generation (0x8), if not PEBS,
-	 * and enable ring-3 counting (0x2) and ring-0 counting (0x1)
-	 * if requested:
-	 */
-	if (!event->attr.precise_ip)
+	if (hwc->config & ARCH_PERFMON_EVENTSEL_INT)
 		bits |= 0x8;
 	if (hwc->config & ARCH_PERFMON_EVENTSEL_USR)
 		bits |= 0x2;
@@ -2816,12 +2811,75 @@ int intel_pmu_save_and_restart(struct pe
 	return static_call(x86_pmu_set_period)(event);
 }
 
+static void intel_pmu_update_config(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	u64 config = hwc->config;
+
+	if (hwc->idx >= INTEL_PMC_IDX_FIXED) { /* PEBS is limited to real PMCs */
+		u64 mask = 0xf, bits = 0;
+
+		if (config & ARCH_PERFMON_EVENTSEL_INT)
+			bits |= 0x8;
+		if (config & ARCH_PERFMON_EVENTSEL_USR)
+			bits |= 0x2;
+		if (config & ARCH_PERFMON_EVENTSEL_OS)
+			bits |= 0x1;
+
+		bits <<= (hwc->idx * 4);
+		mask <<= (hwc->idx * 4);
+
+		config = this_cpu_read(intel_fixed_ctrl);
+		config &= ~mask;
+		config |= bits;
+		this_cpu_write(intel_fixed_ctrl, config);
+	}
+
+	wrmsrl(hwc->config_base, config);
+}
+
+static void intel_pmu_handle_short_pebs(struct perf_event *event)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+	struct hw_perf_event *hwc = &event->hw;
+
+	/* if the event is not enabled; intel_pmu_pebs_enable() DTRT */
+	if (!test_bit(hwc->idx, cpuc->active_mask))
+		return;
+
+	WARN_ON_ONCE(cpuc->enabled);
+
+	if (intel_pmu_is_short_pebs(event)) {
+
+		/* stripped down intel_pmu_pebs_disable() */
+		cpuc->pebs_enabled &= ~(1ULL << hwc->idx);
+		hwc->config |= ARCH_PERFMON_EVENTSEL_INT;
+
+		intel_pmu_update_config(event);
+
+	} else if (!(cpuc->pebs_enabled & (1ULL << hwc->idx))) {
+
+		/* stripped down intel_pmu_pebs_enable() */
+		hwc->config &= ~ARCH_PERFMON_EVENTSEL_INT;
+		cpuc->pebs_enabled |= (1ULL << hwc->idx);
+
+		intel_pmu_update_config(event);
+	}
+}
+
 static int intel_pmu_set_period(struct perf_event *event)
 {
+	int ret;
+
 	if (unlikely(is_topdown_count(event)))
 		return static_call(intel_pmu_set_topdown_event_period)(event);
 
-	return x86_perf_event_set_period(event);
+	ret = x86_perf_event_set_period(event);
+
+	if (event->attr.precise_ip)
+		intel_pmu_handle_short_pebs(event);
+
+	return ret;
 }
 
 static u64 intel_pmu_update(struct perf_event *event)
@@ -2975,6 +3033,9 @@ static int handle_pmi_common(struct pt_r
 		 * MSR_IA32_PEBS_ENABLE is not updated. Because the
 		 * cpuc->enabled has been forced to 0 in PMI.
 		 * Update the MSR if pebs_enabled is changed.
+		 *
+		 * Also; short counters temporarily disable PEBS, see
+		 * intel_pmu_set_period().
 		 */
 		if (pebs_enabled != cpuc->pebs_enabled)
 			wrmsrl(MSR_IA32_PEBS_ENABLE, cpuc->pebs_enabled);
@@ -3856,7 +3917,10 @@ static int intel_pmu_hw_config(struct pe
 		if ((event->attr.config & INTEL_ARCH_EVENT_MASK) == INTEL_FIXED_VLBR_EVENT)
 			return -EINVAL;
 
-		if (!(event->attr.freq || (event->attr.wakeup_events && !event->attr.watermark))) {
+		if (!(event->attr.freq ||
+		      (event->attr.wakeup_events && !event->attr.watermark) ||
+		      event->attr.sample_period > x86_pmu.max_period)) {
+
 			event->hw.flags |= PERF_X86_EVENT_AUTO_RELOAD;
 			if (!(event->attr.sample_type &
 			      ~intel_pmu_large_pebs_flags(event))) {
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1063,6 +1063,15 @@ static inline bool x86_pmu_has_lbr_calls
 DECLARE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
 DECLARE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
 
+static inline bool intel_pmu_is_short_pebs(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	s64 counter = this_cpu_read(pmc_prev_left[hwc->idx]);
+	s64 left = local64_read(&hwc->period_left);
+
+	return counter < left;
+}
+
 int x86_perf_event_set_period(struct perf_event *event);
 
 /*

Re: [PATCH v2 9/9] perf/x86/intel: Optimize short PEBS counters

[Liang, Kan]

On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
> XXX: crazy idea; really not sure this is worth the extra complexity
> 
> It is possible to have the counter programmed to a value smaller than
> the sampling period.

I'm not quite sure how the above case can be triggered.

For the most of the cases, the pmc_prev_left[idx] should be the same as
the hwc->period_left.

For the left < 2 or the limit_period case, I think perf usually program
a larger value, so the pmc_prev_left[idx] > hwc->period_left.

It looks like the only case, which triggers the pmc_prev_left[idx] <
hwc->period_left, is the left > max_period. I don't think it's common
for a user to set a period which is larger than the HW counter limit.
Even if they set a huge period, the PEBS overhead should not be an
issue, since it may causes days to trigger a sample.

If so, it may not be a good idea to introduce such complexity to only
handle such rare cases.

Thanks,
Kan

> In that case, the code suppresses the sample,
> recalculates the remaining events and reprograms the counter.
> 
> This should also work for PEBS counters (and it does); however
> triggering a full PEBS assist and parsing the event from the DS is
> more overhead than is required.
> 
> As such, detect this case and temporarily suppress PEBS. This will
> then trigger a regular PMI for the counter which will reprogram the
> event and re-enable PEBS once the target period is in reach.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/events/intel/core.c |   80 ++++++++++++++++++++++++++++++++++++++-----
>  arch/x86/events/perf_event.h |    9 ++++
>  2 files changed, 81 insertions(+), 8 deletions(-)
> 
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2722,12 +2722,7 @@ static void intel_pmu_enable_fixed(struc
>  
>  	intel_set_masks(event, idx);
>  
> -	/*
> -	 * Enable IRQ generation (0x8), if not PEBS,
> -	 * and enable ring-3 counting (0x2) and ring-0 counting (0x1)
> -	 * if requested:
> -	 */
> -	if (!event->attr.precise_ip)
> +	if (hwc->config & ARCH_PERFMON_EVENTSEL_INT)
>  		bits |= 0x8;
>  	if (hwc->config & ARCH_PERFMON_EVENTSEL_USR)
>  		bits |= 0x2;
> @@ -2816,12 +2811,75 @@ int intel_pmu_save_and_restart(struct pe
>  	return static_call(x86_pmu_set_period)(event);
>  }
>  
> +static void intel_pmu_update_config(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	u64 config = hwc->config;
> +
> +	if (hwc->idx >= INTEL_PMC_IDX_FIXED) { /* PEBS is limited to real PMCs */
> +		u64 mask = 0xf, bits = 0;
> +
> +		if (config & ARCH_PERFMON_EVENTSEL_INT)
> +			bits |= 0x8;
> +		if (config & ARCH_PERFMON_EVENTSEL_USR)
> +			bits |= 0x2;
> +		if (config & ARCH_PERFMON_EVENTSEL_OS)
> +			bits |= 0x1;
> +
> +		bits <<= (hwc->idx * 4);
> +		mask <<= (hwc->idx * 4);
> +
> +		config = this_cpu_read(intel_fixed_ctrl);
> +		config &= ~mask;
> +		config |= bits;
> +		this_cpu_write(intel_fixed_ctrl, config);
> +	}
> +
> +	wrmsrl(hwc->config_base, config);
> +}
> +
> +static void intel_pmu_handle_short_pebs(struct perf_event *event)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	/* if the event is not enabled; intel_pmu_pebs_enable() DTRT */
> +	if (!test_bit(hwc->idx, cpuc->active_mask))
> +		return;
> +
> +	WARN_ON_ONCE(cpuc->enabled);
> +
> +	if (intel_pmu_is_short_pebs(event)) {
> +
> +		/* stripped down intel_pmu_pebs_disable() */
> +		cpuc->pebs_enabled &= ~(1ULL << hwc->idx);
> +		hwc->config |= ARCH_PERFMON_EVENTSEL_INT;
> +
> +		intel_pmu_update_config(event);
> +
> +	} else if (!(cpuc->pebs_enabled & (1ULL << hwc->idx))) {
> +
> +		/* stripped down intel_pmu_pebs_enable() */
> +		hwc->config &= ~ARCH_PERFMON_EVENTSEL_INT;
> +		cpuc->pebs_enabled |= (1ULL << hwc->idx);
> +
> +		intel_pmu_update_config(event);
> +	}
> +}
> +
>  static int intel_pmu_set_period(struct perf_event *event)
>  {
> +	int ret;
> +
>  	if (unlikely(is_topdown_count(event)))
>  		return static_call(intel_pmu_set_topdown_event_period)(event);
>  
> -	return x86_perf_event_set_period(event);
> +	ret = x86_perf_event_set_period(event);
> +
> +	if (event->attr.precise_ip)
> +		intel_pmu_handle_short_pebs(event);
> +
> +	return ret;
>  }
>  
>  static u64 intel_pmu_update(struct perf_event *event)
> @@ -2975,6 +3033,9 @@ static int handle_pmi_common(struct pt_r
>  		 * MSR_IA32_PEBS_ENABLE is not updated. Because the
>  		 * cpuc->enabled has been forced to 0 in PMI.
>  		 * Update the MSR if pebs_enabled is changed.
> +		 *
> +		 * Also; short counters temporarily disable PEBS, see
> +		 * intel_pmu_set_period().
>  		 */
>  		if (pebs_enabled != cpuc->pebs_enabled)
>  			wrmsrl(MSR_IA32_PEBS_ENABLE, cpuc->pebs_enabled);
> @@ -3856,7 +3917,10 @@ static int intel_pmu_hw_config(struct pe
>  		if ((event->attr.config & INTEL_ARCH_EVENT_MASK) == INTEL_FIXED_VLBR_EVENT)
>  			return -EINVAL;
>  
> -		if (!(event->attr.freq || (event->attr.wakeup_events && !event->attr.watermark))) {
> +		if (!(event->attr.freq ||
> +		      (event->attr.wakeup_events && !event->attr.watermark) ||
> +		      event->attr.sample_period > x86_pmu.max_period)) {
> +
>  			event->hw.flags |= PERF_X86_EVENT_AUTO_RELOAD;
>  			if (!(event->attr.sample_type &
>  			      ~intel_pmu_large_pebs_flags(event))) {
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -1063,6 +1063,15 @@ static inline bool x86_pmu_has_lbr_calls
>  DECLARE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
>  DECLARE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
>  
> +static inline bool intel_pmu_is_short_pebs(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	s64 counter = this_cpu_read(pmc_prev_left[hwc->idx]);
> +	s64 left = local64_read(&hwc->period_left);
> +
> +	return counter < left;
> +}
> +
>  int x86_perf_event_set_period(struct perf_event *event);
>  
>  /*
> 
> 

Re: [PATCH v2 9/9] perf/x86/intel: Optimize short PEBS counters

[Peter Zijlstra]

On Mon, Aug 29, 2022 at 11:55:12AM -0400, Liang, Kan wrote:
> 
> 
> On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
> > XXX: crazy idea; really not sure this is worth the extra complexity
> > 
> > It is possible to have the counter programmed to a value smaller than
> > the sampling period.
> 
> I'm not quite sure how the above case can be triggered.
> 
> For the most of the cases, the pmc_prev_left[idx] should be the same as
> the hwc->period_left.
> 
> For the left < 2 or the limit_period case, I think perf usually program
> a larger value, so the pmc_prev_left[idx] > hwc->period_left.
> 
> It looks like the only case, which triggers the pmc_prev_left[idx] <
> hwc->period_left, is the left > max_period. I don't think it's common
> for a user to set a period which is larger than the HW counter limit.
> Even if they set a huge period, the PEBS overhead should not be an
> issue, since it may causes days to trigger a sample.
> 
> If so, it may not be a good idea to introduce such complexity to only
> handle such rare cases.

Yeah, happy to forget this patch exists.. I wrote this things months ago
and I'm not entirely sure why :-)

Re: [PATCH v2 2/9] perf/x86/intel: Move the topdown stuff into the intel driver

[Liang, Kan]

On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
> Use the new x86_pmu::{set_period,update}() methods to push the topdown
> stuff into the Intel driver, where it belongs.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/events/core.c       |    7 -------
>  arch/x86/events/intel/core.c |   28 +++++++++++++++++++++++++---
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -119,9 +119,6 @@ u64 x86_perf_event_update(struct perf_ev
>  	if (unlikely(!hwc->event_base))
>  		return 0;
>  
> -	if (unlikely(is_topdown_count(event)) && x86_pmu.update_topdown_event)
> -		return x86_pmu.update_topdown_event(event);
> -
>  	/*
>  	 * Careful: an NMI might modify the previous event value.
>  	 *
> @@ -1373,10 +1370,6 @@ int x86_perf_event_set_period(struct per
>  	if (unlikely(!hwc->event_base))
>  		return 0;
>  
> -	if (unlikely(is_topdown_count(event)) &&
> -	    x86_pmu.set_topdown_event_period)
> -		return x86_pmu.set_topdown_event_period(event);
> -
>  	/*
>  	 * If we are way outside a reasonable range then just skip forward:
>  	 */
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2301,7 +2301,7 @@ static void intel_pmu_nhm_workaround(voi
>  	for (i = 0; i < 4; i++) {
>  		event = cpuc->events[i];
>  		if (event)
> -			x86_perf_event_update(event);
> +			static_call(x86_pmu_update)(event);
>  	}
>  
>  	for (i = 0; i < 4; i++) {
> @@ -2316,7 +2316,7 @@ static void intel_pmu_nhm_workaround(voi
>  		event = cpuc->events[i];
>  
>  		if (event) {
> -			x86_perf_event_set_period(event);
> +			static_call(x86_pmu_set_period)(event);
>  			__x86_pmu_enable_event(&event->hw,
>  					ARCH_PERFMON_EVENTSEL_ENABLE);
>  		} else
> @@ -2793,7 +2793,7 @@ static void intel_pmu_add_event(struct p
>   */
>  int intel_pmu_save_and_restart(struct perf_event *event)
>  {
> -	x86_perf_event_update(event);
> +	static_call(x86_pmu_update)(event);
>  	/*
>  	 * For a checkpointed counter always reset back to 0.  This
>  	 * avoids a situation where the counter overflows, aborts the
> @@ -2805,9 +2805,27 @@ int intel_pmu_save_and_restart(struct pe
>  		wrmsrl(event->hw.event_base, 0);
>  		local64_set(&event->hw.prev_count, 0);
>  	}
> +	return static_call(x86_pmu_set_period)(event);
> +}
> +
> +static int intel_pmu_set_period(struct perf_event *event)
> +{
> +	if (unlikely(is_topdown_count(event)) &&
> +	    x86_pmu.set_topdown_event_period)
> +		return x86_pmu.set_topdown_event_period(event);
> +
>  	return x86_perf_event_set_period(event);
>  }
>  
> +static u64 intel_pmu_update(struct perf_event *event)
> +{
> +	if (unlikely(is_topdown_count(event)) &&
> +	    x86_pmu.update_topdown_event)
> +		return x86_pmu.update_topdown_event(event);
> +
> +	return x86_perf_event_update(event);
> +}
> +
>  static void intel_pmu_reset(void)
>  {
>  	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
> @@ -4635,6 +4653,10 @@ static __initconst const struct x86_pmu
>  	.enable_all		= core_pmu_enable_all,
>  	.enable			= core_pmu_enable_event,
>  	.disable		= x86_pmu_disable_event,
> +
> +	.set_period		= intel_pmu_set_period,
> +	.update			= intel_pmu_update,

I tried the patch, but it impacts the topdown.
The root cause is that these should be added for intel_pmu rather than
core_pmu.

Thanks,
Kan
>  	.hw_config		= core_pmu_hw_config,
>  	.schedule_events	= x86_schedule_events,
>  	.eventsel		= MSR_ARCH_PERFMON_EVENTSEL0,
> 
> 

Re: [PATCH v2 8/9] perf/x86/intel: Shadow MSR_ARCH_PERFMON_FIXED_CTR_CTRL

[Liang, Kan]

On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
> Less RDMSR is more better.

I had an RFC patch which does a further step to move the fixed
control register write to right before the entire PMU re-enabling, which
could also save some writes if there are several fixed counters enabled.
https://lore.kernel.org/lkml/20220804140729.2951259-1-kan.liang@linux.intel.com/

Do you have any comments for the RFC patch?

If the method is OK, I will rebase the RFC patch on top of this patch.

Thanks,
Kan
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/events/intel/core.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2405,6 +2405,8 @@ static inline void intel_clear_masks(str
>  	__clear_bit(idx, (unsigned long *)&cpuc->intel_cp_status);
>  }
>  
> +static DEFINE_PER_CPU(u64, intel_fixed_ctrl);
> +
>  static void intel_pmu_disable_fixed(struct perf_event *event)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
> @@ -2426,8 +2428,9 @@ static void intel_pmu_disable_fixed(stru
>  	intel_clear_masks(event, idx);
>  
>  	mask = 0xfULL << ((idx - INTEL_PMC_IDX_FIXED) * 4);
> -	rdmsrl(hwc->config_base, ctrl_val);
> +	ctrl_val = this_cpu_read(intel_fixed_ctrl);
>  	ctrl_val &= ~mask;
> +	this_cpu_write(intel_fixed_ctrl, ctrl_val);>  	wrmsrl(hwc->config_base, ctrl_val);
>  }
>  
> @@ -2746,9 +2749,10 @@ static void intel_pmu_enable_fixed(struc
>  		mask |= ICL_FIXED_0_ADAPTIVE << (idx * 4);
>  	}
>  
> -	rdmsrl(hwc->config_base, ctrl_val);
> +	ctrl_val = this_cpu_read(intel_fixed_ctrl);
>  	ctrl_val &= ~mask;
>  	ctrl_val |= bits;
> +	this_cpu_write(intel_fixed_ctrl, ctrl_val);
>  	wrmsrl(hwc->config_base, ctrl_val);
>  }
>  
> 
> 

Re: [PATCH v2 2/9] perf/x86/intel: Move the topdown stuff into the intel driver

[Peter Zijlstra]

On Wed, Aug 31, 2022 at 09:41:06AM -0400, Liang, Kan wrote:
> >  static void intel_pmu_reset(void)
> >  {
> >  	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
> > @@ -4635,6 +4653,10 @@ static __initconst const struct x86_pmu
> >  	.enable_all		= core_pmu_enable_all,
> >  	.enable			= core_pmu_enable_event,
> >  	.disable		= x86_pmu_disable_event,
> > +
> > +	.set_period		= intel_pmu_set_period,
> > +	.update			= intel_pmu_update,
> 
> I tried the patch, but it impacts the topdown.
> The root cause is that these should be added for intel_pmu rather than
> core_pmu.
> 

Dang; must be a rebase fail on my end, sorry about that. Let me go fix
that.

Re: [PATCH v2 8/9] perf/x86/intel: Shadow MSR_ARCH_PERFMON_FIXED_CTR_CTRL

[Peter Zijlstra]

On Wed, Aug 31, 2022 at 09:52:19AM -0400, Liang, Kan wrote:
> 
> 
> On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
> > Less RDMSR is more better.
> 
> I had an RFC patch which does a further step to move the fixed
> control register write to right before the entire PMU re-enabling, which
> could also save some writes if there are several fixed counters enabled.
> https://lore.kernel.org/lkml/20220804140729.2951259-1-kan.liang@linux.intel.com/
> 
> Do you have any comments for the RFC patch?
> 

Oh, I like that better, let me just replace my patch with that.

Re: [PATCH v2 8/9] perf/x86/intel: Shadow MSR_ARCH_PERFMON_FIXED_CTR_CTRL

[Peter Zijlstra]

On Thu, Sep 01, 2022 at 11:10:49AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 31, 2022 at 09:52:19AM -0400, Liang, Kan wrote:
> > 
> > 
> > On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
> > > Less RDMSR is more better.
> > 
> > I had an RFC patch which does a further step to move the fixed
> > control register write to right before the entire PMU re-enabling, which
> > could also save some writes if there are several fixed counters enabled.
> > https://lore.kernel.org/lkml/20220804140729.2951259-1-kan.liang@linux.intel.com/
> > 
> > Do you have any comments for the RFC patch?
> > 
> 
> Oh, I like that better, let me just replace my patch with that.

git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/wip.cleanup

Should have your patch instead of mine for the FIXED_CTR_CTRL and have
the pmu methods in the right place.

Re: [PATCH v2 8/9] perf/x86/intel: Shadow MSR_ARCH_PERFMON_FIXED_CTR_CTRL

[Liang, Kan]

On 2022-09-01 6:04 a.m., Peter Zijlstra wrote:
> On Thu, Sep 01, 2022 at 11:10:49AM +0200, Peter Zijlstra wrote:
>> On Wed, Aug 31, 2022 at 09:52:19AM -0400, Liang, Kan wrote:
>>>
>>>
>>> On 2022-08-29 6:10 a.m., Peter Zijlstra wrote:
>>>> Less RDMSR is more better.
>>>
>>> I had an RFC patch which does a further step to move the fixed
>>> control register write to right before the entire PMU re-enabling, which
>>> could also save some writes if there are several fixed counters enabled.
>>> https://lore.kernel.org/lkml/20220804140729.2951259-1-kan.liang@linux.intel.com/
>>>
>>> Do you have any comments for the RFC patch?
>>>
>>
>> Oh, I like that better, let me just replace my patch with that.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/wip.cleanup
> 
> Should have your patch instead of mine for the FIXED_CTR_CTRL and have
> the pmu methods in the right place.

Thanks. I will do more test today with the updated branch.

Thanks,
Kan

[tip: perf/core] perf/x86/p4: Remove perfctr_second_write quirk

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     dbf4e792beadafc684ef455453c613ff182c7723
Gitweb:        https://git.kernel.org/tip/dbf4e792beadafc684ef455453c613ff182c7723
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Fri, 20 May 2022 15:38:43 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:04 +02:00

perf/x86/p4: Remove perfctr_second_write quirk

Now that we have a x86_pmu::set_period() method, use it to remove the
perfctr_second_write quirk from the generic code.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.839502514@infradead.org
---
 arch/x86/events/core.c       | 12 +-----------
 arch/x86/events/intel/p4.c   | 37 +++++++++++++++++++++++++----------
 arch/x86/events/perf_event.h |  2 +-
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 05830bb..b30b8bb 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1356,7 +1356,7 @@ static void x86_pmu_enable(struct pmu *pmu)
 	static_call(x86_pmu_enable_all)(added);
 }
 
-static DEFINE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
+DEFINE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
 
 /*
  * Set the next IRQ period, based on the hwc->period_left value.
@@ -1416,16 +1416,6 @@ int x86_perf_event_set_period(struct perf_event *event)
 	if (is_counter_pair(hwc))
 		wrmsrl(x86_pmu_event_addr(idx + 1), 0xffff);
 
-	/*
-	 * Due to erratum on certan cpu we need
-	 * a second write to be sure the register
-	 * is updated properly
-	 */
-	if (x86_pmu.perfctr_second_write) {
-		wrmsrl(hwc->event_base,
-			(u64)(-left) & x86_pmu.cntval_mask);
-	}
-
 	perf_event_update_userpage(event);
 
 	return ret;
diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
index 7951a5d..03bbcc2 100644
--- a/arch/x86/events/intel/p4.c
+++ b/arch/x86/events/intel/p4.c
@@ -1006,6 +1006,29 @@ static void p4_pmu_enable_all(int added)
 	}
 }
 
+static int p4_pmu_set_period(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	s64 left = this_cpu_read(pmc_prev_left[hwc->idx]);
+	int ret;
+
+	ret = x86_perf_event_set_period(event);
+
+	if (hwc->event_base) {
+		/*
+		 * This handles erratum N15 in intel doc 249199-029,
+		 * the counter may not be updated correctly on write
+		 * so we need a second write operation to do the trick
+		 * (the official workaround didn't work)
+		 *
+		 * the former idea is taken from OProfile code
+		 */
+		wrmsrl(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
+	}
+
+	return ret;
+}
+
 static int p4_pmu_handle_irq(struct pt_regs *regs)
 {
 	struct perf_sample_data data;
@@ -1044,7 +1067,7 @@ static int p4_pmu_handle_irq(struct pt_regs *regs)
 		/* event overflow for sure */
 		perf_sample_data_init(&data, 0, hwc->last_period);
 
-		if (!x86_perf_event_set_period(event))
+		if (!static_call(x86_pmu_set_period)(event))
 			continue;
 
 
@@ -1316,6 +1339,9 @@ static __initconst const struct x86_pmu p4_pmu = {
 	.enable_all		= p4_pmu_enable_all,
 	.enable			= p4_pmu_enable_event,
 	.disable		= p4_pmu_disable_event,
+
+	.set_period		= p4_pmu_set_period,
+
 	.eventsel		= MSR_P4_BPU_CCCR0,
 	.perfctr		= MSR_P4_BPU_PERFCTR0,
 	.event_map		= p4_pmu_event_map,
@@ -1334,15 +1360,6 @@ static __initconst const struct x86_pmu p4_pmu = {
 	.max_period		= (1ULL << (ARCH_P4_CNTRVAL_BITS - 1)) - 1,
 	.hw_config		= p4_hw_config,
 	.schedule_events	= p4_pmu_schedule_events,
-	/*
-	 * This handles erratum N15 in intel doc 249199-029,
-	 * the counter may not be updated correctly on write
-	 * so we need a second write operation to do the trick
-	 * (the official workaround didn't work)
-	 *
-	 * the former idea is taken from OProfile code
-	 */
-	.perfctr_second_write	= 1,
 
 	.format_attrs		= intel_p4_formats_attr,
 };
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 386ebfa..20c2ee2 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -780,7 +780,6 @@ struct x86_pmu {
 
 	struct event_constraint *event_constraints;
 	struct x86_pmu_quirk *quirks;
-	int		perfctr_second_write;
 	void		(*limit_period)(struct perf_event *event, s64 *l);
 
 	/* PMI handler bits */
@@ -1060,6 +1059,7 @@ static inline bool x86_pmu_has_lbr_callstack(void)
 }
 
 DECLARE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
+DECLARE_PER_CPU(u64 [X86_PMC_IDX_MAX], pmc_prev_left);
 
 int x86_perf_event_set_period(struct perf_event *event);
 

[tip: perf/core] perf/x86/intel: Remove x86_pmu::update_topdown_event

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     1acab2e01c9c5df00f2fddf3473014dea89dcb5f
Gitweb:        https://git.kernel.org/tip/1acab2e01c9c5df00f2fddf3473014dea89dcb5f
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 11 May 2022 17:02:05 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:03 +02:00

perf/x86/intel: Remove x86_pmu::update_topdown_event

Now that it is all internal to the intel driver, remove
x86_pmu::update_topdown_event.

Assumes that is_topdown_count(event) can only be true when the
hardware has topdown stuff and the function is set.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.771635301@infradead.org
---
 arch/x86/events/intel/core.c | 22 ++++++++++++----------
 arch/x86/events/perf_event.h |  1 -
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 75400ed..1e429e8 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2673,6 +2673,7 @@ static u64 adl_update_topdown_event(struct perf_event *event)
 	return icl_update_topdown_event(event);
 }
 
+DEFINE_STATIC_CALL(intel_pmu_update_topdown_event, x86_perf_event_update);
 
 static void intel_pmu_read_topdown_event(struct perf_event *event)
 {
@@ -2684,7 +2685,7 @@ static void intel_pmu_read_topdown_event(struct perf_event *event)
 		return;
 
 	perf_pmu_disable(event->pmu);
-	x86_pmu.update_topdown_event(event);
+	static_call(intel_pmu_update_topdown_event)(event);
 	perf_pmu_enable(event->pmu);
 }
 
@@ -2692,7 +2693,7 @@ static void intel_pmu_read_event(struct perf_event *event)
 {
 	if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
 		intel_pmu_auto_reload_read(event);
-	else if (is_topdown_count(event) && x86_pmu.update_topdown_event)
+	else if (is_topdown_count(event))
 		intel_pmu_read_topdown_event(event);
 	else
 		x86_perf_event_update(event);
@@ -2821,9 +2822,8 @@ static int intel_pmu_set_period(struct perf_event *event)
 
 static u64 intel_pmu_update(struct perf_event *event)
 {
-	if (unlikely(is_topdown_count(event)) &&
-	    x86_pmu.update_topdown_event)
-		return x86_pmu.update_topdown_event(event);
+	if (unlikely(is_topdown_count(event)))
+		return static_call(intel_pmu_update_topdown_event)(event);
 
 	return x86_perf_event_update(event);
 }
@@ -2990,8 +2990,7 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
 	 */
 	if (__test_and_clear_bit(GLOBAL_STATUS_PERF_METRICS_OVF_BIT, (unsigned long *)&status)) {
 		handled++;
-		if (x86_pmu.update_topdown_event)
-			x86_pmu.update_topdown_event(NULL);
+		static_call(intel_pmu_update_topdown_event)(NULL);
 	}
 
 	/*
@@ -6292,7 +6291,8 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 4;
-		x86_pmu.update_topdown_event = icl_update_topdown_event;
+		static_call_update(intel_pmu_update_topdown_event,
+				   &icl_update_topdown_event);
 		static_call_update(intel_pmu_set_topdown_event_period,
 				   &icl_set_topdown_event_period);
 		pr_cont("Icelake events, ");
@@ -6331,7 +6331,8 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 8;
-		x86_pmu.update_topdown_event = icl_update_topdown_event;
+		static_call_update(intel_pmu_update_topdown_event,
+				   &icl_update_topdown_event);
 		static_call_update(intel_pmu_set_topdown_event_period,
 				   &icl_set_topdown_event_period);
 		pr_cont("Sapphire Rapids events, ");
@@ -6369,7 +6370,8 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_adl();
 		x86_pmu.pebs_latency_data = adl_latency_data_small;
 		x86_pmu.num_topdown_events = 8;
-		x86_pmu.update_topdown_event = adl_update_topdown_event;
+		static_call_update(intel_pmu_update_topdown_event,
+				   &adl_update_topdown_event);
 		static_call_update(intel_pmu_set_topdown_event_period,
 				   &adl_set_topdown_event_period);
 
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index be27ead..386ebfa 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -889,7 +889,6 @@ struct x86_pmu {
 	 * Intel perf metrics
 	 */
 	int		num_topdown_events;
-	u64		(*update_topdown_event)(struct perf_event *event);
 
 	/*
 	 * perf task context (i.e. struct perf_event_context::task_ctx_data)

[tip: perf/core] perf/x86/intel: Remove x86_pmu::set_topdown_event_period

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     2368516731901c391826f7cf23516173193652fa
Gitweb:        https://git.kernel.org/tip/2368516731901c391826f7cf23516173193652fa
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 11 May 2022 16:41:25 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:03 +02:00

perf/x86/intel: Remove x86_pmu::set_topdown_event_period

Now that it is all internal to the intel driver, remove
x86_pmu::set_topdown_event_period.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.706354189@infradead.org
---
 arch/x86/events/intel/core.c | 16 ++++++++++------
 arch/x86/events/perf_event.h |  1 -
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 92cc390..75400ed 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2521,6 +2521,8 @@ static int adl_set_topdown_event_period(struct perf_event *event)
 	return icl_set_topdown_event_period(event);
 }
 
+DEFINE_STATIC_CALL(intel_pmu_set_topdown_event_period, x86_perf_event_set_period);
+
 static inline u64 icl_get_metrics_event_value(u64 metric, u64 slots, int idx)
 {
 	u32 val;
@@ -2811,9 +2813,8 @@ int intel_pmu_save_and_restart(struct perf_event *event)
 
 static int intel_pmu_set_period(struct perf_event *event)
 {
-	if (unlikely(is_topdown_count(event)) &&
-	    x86_pmu.set_topdown_event_period)
-		return x86_pmu.set_topdown_event_period(event);
+	if (unlikely(is_topdown_count(event)))
+		return static_call(intel_pmu_set_topdown_event_period)(event);
 
 	return x86_perf_event_set_period(event);
 }
@@ -6292,7 +6293,8 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 4;
 		x86_pmu.update_topdown_event = icl_update_topdown_event;
-		x86_pmu.set_topdown_event_period = icl_set_topdown_event_period;
+		static_call_update(intel_pmu_set_topdown_event_period,
+				   &icl_set_topdown_event_period);
 		pr_cont("Icelake events, ");
 		name = "icelake";
 		break;
@@ -6330,7 +6332,8 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_skl(pmem);
 		x86_pmu.num_topdown_events = 8;
 		x86_pmu.update_topdown_event = icl_update_topdown_event;
-		x86_pmu.set_topdown_event_period = icl_set_topdown_event_period;
+		static_call_update(intel_pmu_set_topdown_event_period,
+				   &icl_set_topdown_event_period);
 		pr_cont("Sapphire Rapids events, ");
 		name = "sapphire_rapids";
 		break;
@@ -6367,7 +6370,8 @@ __init int intel_pmu_init(void)
 		x86_pmu.pebs_latency_data = adl_latency_data_small;
 		x86_pmu.num_topdown_events = 8;
 		x86_pmu.update_topdown_event = adl_update_topdown_event;
-		x86_pmu.set_topdown_event_period = adl_set_topdown_event_period;
+		static_call_update(intel_pmu_set_topdown_event_period,
+				   &adl_set_topdown_event_period);
 
 		x86_pmu.filter_match = intel_pmu_filter_match;
 		x86_pmu.get_event_constraints = adl_get_event_constraints;
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index e82d2d2..be27ead 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -890,7 +890,6 @@ struct x86_pmu {
 	 */
 	int		num_topdown_events;
 	u64		(*update_topdown_event)(struct perf_event *event);
-	int		(*set_topdown_event_period)(struct perf_event *event);
 
 	/*
 	 * perf task context (i.e. struct perf_event_context::task_ctx_data)

[tip: perf/core] perf/x86: Add a x86_pmu::limit_period static_call

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     08b3068fab207e3c7d79799d434e1d648524cac6
Gitweb:        https://git.kernel.org/tip/08b3068fab207e3c7d79799d434e1d648524cac6
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 10 May 2022 21:28:18 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:03 +02:00

perf/x86: Add a x86_pmu::limit_period static_call

Avoid a branch and indirect call.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.640658334@infradead.org
---
 arch/x86/events/core.c |  9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 1e90bc7..05830bb 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -72,8 +72,9 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_add,  *x86_pmu.add);
 DEFINE_STATIC_CALL_NULL(x86_pmu_del,  *x86_pmu.del);
 DEFINE_STATIC_CALL_NULL(x86_pmu_read, *x86_pmu.read);
 
-DEFINE_STATIC_CALL_NULL(x86_pmu_set_period, *x86_pmu.set_period);
-DEFINE_STATIC_CALL_NULL(x86_pmu_update,     *x86_pmu.update);
+DEFINE_STATIC_CALL_NULL(x86_pmu_set_period,   *x86_pmu.set_period);
+DEFINE_STATIC_CALL_NULL(x86_pmu_update,       *x86_pmu.update);
+DEFINE_STATIC_CALL_NULL(x86_pmu_limit_period, *x86_pmu.limit_period);
 
 DEFINE_STATIC_CALL_NULL(x86_pmu_schedule_events,       *x86_pmu.schedule_events);
 DEFINE_STATIC_CALL_NULL(x86_pmu_get_event_constraints, *x86_pmu.get_event_constraints);
@@ -1396,8 +1397,7 @@ int x86_perf_event_set_period(struct perf_event *event)
 	if (left > x86_pmu.max_period)
 		left = x86_pmu.max_period;
 
-	if (x86_pmu.limit_period)
-		x86_pmu.limit_period(event, &left);
+	static_call_cond(x86_pmu_limit_period)(event, &left);
 
 	this_cpu_write(pmc_prev_left[idx], left);
 
@@ -2024,6 +2024,7 @@ static void x86_pmu_static_call_update(void)
 
 	static_call_update(x86_pmu_set_period, x86_pmu.set_period);
 	static_call_update(x86_pmu_update, x86_pmu.update);
+	static_call_update(x86_pmu_limit_period, x86_pmu.limit_period);
 
 	static_call_update(x86_pmu_schedule_events, x86_pmu.schedule_events);
 	static_call_update(x86_pmu_get_event_constraints, x86_pmu.get_event_constraints);

[tip: perf/core] perf/x86: Change x86_pmu::limit_period signature

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     28f0f3c44b5c35be657a4f922dcdfb48285f4373
Gitweb:        https://git.kernel.org/tip/28f0f3c44b5c35be657a4f922dcdfb48285f4373
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 10 May 2022 21:28:25 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:02 +02:00

perf/x86: Change x86_pmu::limit_period signature

In preparation for making it a static_call, change the signature.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.573713839@infradead.org
---
 arch/x86/events/amd/core.c   |  8 +++-----
 arch/x86/events/core.c       | 13 ++++++++-----
 arch/x86/events/intel/core.c | 19 ++++++++-----------
 arch/x86/events/perf_event.h |  2 +-
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index bd99d2a..8b70237 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -1224,16 +1224,14 @@ static ssize_t amd_event_sysfs_show(char *page, u64 config)
 	return x86_event_sysfs_show(page, config, event);
 }
 
-static u64 amd_pmu_limit_period(struct perf_event *event, u64 left)
+static void amd_pmu_limit_period(struct perf_event *event, s64 *left)
 {
 	/*
 	 * Decrease period by the depth of the BRS feature to get the last N
 	 * taken branches and approximate the desired period
 	 */
-	if (has_branch_stack(event) && left > x86_pmu.lbr_nr)
-		left -= x86_pmu.lbr_nr;
-
-	return left;
+	if (has_branch_stack(event) && *left > x86_pmu.lbr_nr)
+		*left -= x86_pmu.lbr_nr;
 }
 
 static __initconst const struct x86_pmu amd_pmu = {
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index b074e71..1e90bc7 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -621,8 +621,9 @@ int x86_pmu_hw_config(struct perf_event *event)
 		event->hw.config |= event->attr.config & X86_RAW_EVENT_MASK;
 
 	if (event->attr.sample_period && x86_pmu.limit_period) {
-		if (x86_pmu.limit_period(event, event->attr.sample_period) >
-				event->attr.sample_period)
+		s64 left = event->attr.sample_period;
+		x86_pmu.limit_period(event, &left);
+		if (left > event->attr.sample_period)
 			return -EINVAL;
 	}
 
@@ -1396,9 +1397,9 @@ int x86_perf_event_set_period(struct perf_event *event)
 		left = x86_pmu.max_period;
 
 	if (x86_pmu.limit_period)
-		left = x86_pmu.limit_period(event, left);
+		x86_pmu.limit_period(event, &left);
 
-	per_cpu(pmc_prev_left[idx], smp_processor_id()) = left;
+	this_cpu_write(pmc_prev_left[idx], left);
 
 	/*
 	 * The hw event starts counting from this event offset,
@@ -2677,7 +2678,9 @@ static int x86_pmu_check_period(struct perf_event *event, u64 value)
 		return -EINVAL;
 
 	if (value && x86_pmu.limit_period) {
-		if (x86_pmu.limit_period(event, value) > value)
+		s64 left = value;
+		x86_pmu.limit_period(event, &left);
+		if (left > value)
 			return -EINVAL;
 	}
 
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index feed732..92cc390 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4344,28 +4344,25 @@ static u8 adl_get_hybrid_cpu_type(void)
  * Therefore the effective (average) period matches the requested period,
  * despite coarser hardware granularity.
  */
-static u64 bdw_limit_period(struct perf_event *event, u64 left)
+static void bdw_limit_period(struct perf_event *event, s64 *left)
 {
 	if ((event->hw.config & INTEL_ARCH_EVENT_MASK) ==
 			X86_CONFIG(.event=0xc0, .umask=0x01)) {
-		if (left < 128)
-			left = 128;
-		left &= ~0x3fULL;
+		if (*left < 128)
+			*left = 128;
+		*left &= ~0x3fULL;
 	}
-	return left;
 }
 
-static u64 nhm_limit_period(struct perf_event *event, u64 left)
+static void nhm_limit_period(struct perf_event *event, s64 *left)
 {
-	return max(left, 32ULL);
+	*left = max(*left, 32LL);
 }
 
-static u64 spr_limit_period(struct perf_event *event, u64 left)
+static void spr_limit_period(struct perf_event *event, s64 *left)
 {
 	if (event->attr.precise_ip == 3)
-		return max(left, 128ULL);
-
-	return left;
+		*left = max(*left, 128LL);
 }
 
 PMU_FORMAT_ATTR(event,	"config:0-7"	);
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 7ae1a6c..e82d2d2 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -781,7 +781,7 @@ struct x86_pmu {
 	struct event_constraint *event_constraints;
 	struct x86_pmu_quirk *quirks;
 	int		perfctr_second_write;
-	u64		(*limit_period)(struct perf_event *event, u64 l);
+	void		(*limit_period)(struct perf_event *event, s64 *l);
 
 	/* PMI handler bits */
 	unsigned int	late_ack		:1,

[tip: perf/core] perf/x86/intel: Move the topdown stuff into the intel driver

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     e577bb17a1eaa35b86ee873a786e603be768d668
Gitweb:        https://git.kernel.org/tip/e577bb17a1eaa35b86ee873a786e603be768d668
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 10 May 2022 21:28:06 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:02 +02:00

perf/x86/intel: Move the topdown stuff into the intel driver

Use the new x86_pmu::{set_period,update}() methods to push the topdown
stuff into the Intel driver, where it belongs.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.505933457@infradead.org
---
 arch/x86/events/core.c       |  7 -------
 arch/x86/events/intel/core.c | 26 +++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index bb559b7..b074e71 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -119,9 +119,6 @@ u64 x86_perf_event_update(struct perf_event *event)
 	if (unlikely(!hwc->event_base))
 		return 0;
 
-	if (unlikely(is_topdown_count(event)) && x86_pmu.update_topdown_event)
-		return x86_pmu.update_topdown_event(event);
-
 	/*
 	 * Careful: an NMI might modify the previous event value.
 	 *
@@ -1373,10 +1370,6 @@ int x86_perf_event_set_period(struct perf_event *event)
 	if (unlikely(!hwc->event_base))
 		return 0;
 
-	if (unlikely(is_topdown_count(event)) &&
-	    x86_pmu.set_topdown_event_period)
-		return x86_pmu.set_topdown_event_period(event);
-
 	/*
 	 * If we are way outside a reasonable range then just skip forward:
 	 */
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index ba101c2..feed732 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2302,7 +2302,7 @@ static void intel_pmu_nhm_workaround(void)
 	for (i = 0; i < 4; i++) {
 		event = cpuc->events[i];
 		if (event)
-			x86_perf_event_update(event);
+			static_call(x86_pmu_update)(event);
 	}
 
 	for (i = 0; i < 4; i++) {
@@ -2317,7 +2317,7 @@ static void intel_pmu_nhm_workaround(void)
 		event = cpuc->events[i];
 
 		if (event) {
-			x86_perf_event_set_period(event);
+			static_call(x86_pmu_set_period)(event);
 			__x86_pmu_enable_event(&event->hw,
 					ARCH_PERFMON_EVENTSEL_ENABLE);
 		} else
@@ -2794,7 +2794,7 @@ static void intel_pmu_add_event(struct perf_event *event)
  */
 int intel_pmu_save_and_restart(struct perf_event *event)
 {
-	x86_perf_event_update(event);
+	static_call(x86_pmu_update)(event);
 	/*
 	 * For a checkpointed counter always reset back to 0.  This
 	 * avoids a situation where the counter overflows, aborts the
@@ -2806,9 +2806,27 @@ int intel_pmu_save_and_restart(struct perf_event *event)
 		wrmsrl(event->hw.event_base, 0);
 		local64_set(&event->hw.prev_count, 0);
 	}
+	return static_call(x86_pmu_set_period)(event);
+}
+
+static int intel_pmu_set_period(struct perf_event *event)
+{
+	if (unlikely(is_topdown_count(event)) &&
+	    x86_pmu.set_topdown_event_period)
+		return x86_pmu.set_topdown_event_period(event);
+
 	return x86_perf_event_set_period(event);
 }
 
+static u64 intel_pmu_update(struct perf_event *event)
+{
+	if (unlikely(is_topdown_count(event)) &&
+	    x86_pmu.update_topdown_event)
+		return x86_pmu.update_topdown_event(event);
+
+	return x86_perf_event_update(event);
+}
+
 static void intel_pmu_reset(void)
 {
 	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
@@ -4786,6 +4804,8 @@ static __initconst const struct x86_pmu intel_pmu = {
 	.add			= intel_pmu_add_event,
 	.del			= intel_pmu_del_event,
 	.read			= intel_pmu_read_event,
+	.set_period		= intel_pmu_set_period,
+	.update			= intel_pmu_update,
 	.hw_config		= intel_pmu_hw_config,
 	.schedule_events	= x86_schedule_events,
 	.eventsel		= MSR_ARCH_PERFMON_EVENTSEL0,

[tip: perf/core] perf/x86: Add two more x86_pmu methods

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     73759c346341d39dfde39701476c0376dea0a98b
Gitweb:        https://git.kernel.org/tip/73759c346341d39dfde39701476c0376dea0a98b
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 10 May 2022 21:27:22 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 07 Sep 2022 21:54:02 +02:00

perf/x86: Add two more x86_pmu methods

In order to clean up x86_perf_event_{set_period,update)() start by
adding them as x86_pmu methods.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20220829101321.440196408@infradead.org
---
 arch/x86/events/core.c       | 22 +++++++++++++++++-----
 arch/x86/events/perf_event.h |  5 +++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index bb34a28..bb559b7 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -72,6 +72,9 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_add,  *x86_pmu.add);
 DEFINE_STATIC_CALL_NULL(x86_pmu_del,  *x86_pmu.del);
 DEFINE_STATIC_CALL_NULL(x86_pmu_read, *x86_pmu.read);
 
+DEFINE_STATIC_CALL_NULL(x86_pmu_set_period, *x86_pmu.set_period);
+DEFINE_STATIC_CALL_NULL(x86_pmu_update,     *x86_pmu.update);
+
 DEFINE_STATIC_CALL_NULL(x86_pmu_schedule_events,       *x86_pmu.schedule_events);
 DEFINE_STATIC_CALL_NULL(x86_pmu_get_event_constraints, *x86_pmu.get_event_constraints);
 DEFINE_STATIC_CALL_NULL(x86_pmu_put_event_constraints, *x86_pmu.put_event_constraints);
@@ -1518,7 +1521,7 @@ static void x86_pmu_start(struct perf_event *event, int flags)
 
 	if (flags & PERF_EF_RELOAD) {
 		WARN_ON_ONCE(!(event->hw.state & PERF_HES_UPTODATE));
-		x86_perf_event_set_period(event);
+		static_call(x86_pmu_set_period)(event);
 	}
 
 	event->hw.state = 0;
@@ -1610,7 +1613,7 @@ void x86_pmu_stop(struct perf_event *event, int flags)
 		 * Drain the remaining delta count out of a event
 		 * that we are disabling:
 		 */
-		x86_perf_event_update(event);
+		static_call(x86_pmu_update)(event);
 		hwc->state |= PERF_HES_UPTODATE;
 	}
 }
@@ -1700,7 +1703,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
 
 		event = cpuc->events[idx];
 
-		val = x86_perf_event_update(event);
+		val = static_call(x86_pmu_update)(event);
 		if (val & (1ULL << (x86_pmu.cntval_bits - 1)))
 			continue;
 
@@ -1709,7 +1712,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
 		 */
 		handled++;
 
-		if (!x86_perf_event_set_period(event))
+		if (!static_call(x86_pmu_set_period)(event))
 			continue;
 
 		perf_sample_data_init(&data, 0, event->hw.last_period);
@@ -2025,6 +2028,9 @@ static void x86_pmu_static_call_update(void)
 	static_call_update(x86_pmu_del, x86_pmu.del);
 	static_call_update(x86_pmu_read, x86_pmu.read);
 
+	static_call_update(x86_pmu_set_period, x86_pmu.set_period);
+	static_call_update(x86_pmu_update, x86_pmu.update);
+
 	static_call_update(x86_pmu_schedule_events, x86_pmu.schedule_events);
 	static_call_update(x86_pmu_get_event_constraints, x86_pmu.get_event_constraints);
 	static_call_update(x86_pmu_put_event_constraints, x86_pmu.put_event_constraints);
@@ -2044,7 +2050,7 @@ static void x86_pmu_static_call_update(void)
 
 static void _x86_pmu_read(struct perf_event *event)
 {
-	x86_perf_event_update(event);
+	static_call(x86_pmu_update)(event);
 }
 
 void x86_pmu_show_pmu_cap(int num_counters, int num_counters_fixed,
@@ -2151,6 +2157,12 @@ static int __init init_hw_perf_events(void)
 	if (!x86_pmu.guest_get_msrs)
 		x86_pmu.guest_get_msrs = (void *)&__static_call_return0;
 
+	if (!x86_pmu.set_period)
+		x86_pmu.set_period = x86_perf_event_set_period;
+
+	if (!x86_pmu.update)
+		x86_pmu.update = x86_perf_event_update;
+
 	x86_pmu_static_call_update();
 
 	/*
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 4a3dde2..7ae1a6c 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -743,6 +743,8 @@ struct x86_pmu {
 	void		(*add)(struct perf_event *);
 	void		(*del)(struct perf_event *);
 	void		(*read)(struct perf_event *event);
+	int		(*set_period)(struct perf_event *event);
+	u64		(*update)(struct perf_event *event);
 	int		(*hw_config)(struct perf_event *event);
 	int		(*schedule_events)(struct cpu_hw_events *cpuc, int n, int *assign);
 	unsigned	eventsel;
@@ -1042,6 +1044,9 @@ static struct perf_pmu_format_hybrid_attr format_attr_hybrid_##_name = {\
 struct pmu *x86_get_pmu(unsigned int cpu);
 extern struct x86_pmu x86_pmu __read_mostly;
 
+DECLARE_STATIC_CALL(x86_pmu_set_period, *x86_pmu.set_period);
+DECLARE_STATIC_CALL(x86_pmu_update,     *x86_pmu.update);
+
 static __always_inline struct x86_perf_task_context_opt *task_context_opt(void *ctx)
 {
 	if (static_cpu_has(X86_FEATURE_ARCH_LBR))