perf/x86/intel: Move branch tracing setup to intel object

PATCH New Jiri Olsa 2 participants 6 messages Nov 21, 2018 – Nov 22, 2018

Subsystems (3)

Stable branch x86 perf events

lore

Share & tools

lore

Jiri Olsa

author reviewer

2018-11-21 10:16 UTC #0 lore raw

PATCH 1/3 perf/x86/intel: Move branch tracing setup to intel object

Moving branch tracing setup to Intel core object into separate intel_pmu_bts_config function, because it's Intel specific.

Signed-off-by Jiri Olsa <[email protected]>

---
 arch/x86/events/core.c       | 20 ------------------
 arch/x86/events/intel/core.c | 41 +++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 21 deletions(-)

arch/x86/events/core.c +0 ~~-20~~

index 106911b603bd..374a19712e20 100644
@@ -438,26 +438,6 @@ int x86_setup_perfctr(struct perf_event *event)
 	if (config == -1LL)
 		return -EINVAL;
 
-	/*
-	 * Branch tracing:
-	 */
-	if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
-	    !attr->freq && hwc->sample_period == 1) {
-		/* BTS is not supported by this architecture. */
-		if (!x86_pmu.bts_active)
-			return -EOPNOTSUPP;
-
-		/* BTS is currently only allowed for user-mode. */
-		if (!attr->exclude_kernel)
-			return -EOPNOTSUPP;
-
-		/* disallow bts if conflicting events are present */
-		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
-			return -EBUSY;
-
-		event->destroy = hw_perf_lbr_event_destroy;
-	}
-
 	hwc->config |= config;
 
 	return 0;

arch/x86/events/intel/core.c +40 -1

57 lines not on this page · raw message

Signature · 1 line

2.17.2

Jiri Olsa

author reviewer

2018-11-21 10:16 UTC #1 lore raw

PATCH 2/3 perf/x86/intel: Add generic branch tracing check to intel_pmu_has_bts

Currently we check the branch tracing only by checking for the PERF_COUNT_HW_BRANCH_INSTRUCTIONS event of PERF_TYPE_HARDWARE type. But we can define the same event with the PERF_TYPE_RAW type.

Changing the intel_pmu_has_bts code to check on event's final hw config value, so both HW types are covered.

Adding unlikely to intel_pmu_has_bts condition calls, because it was used in the original code in intel_bts_constraints.

Signed-off-by Jiri Olsa <[email protected]>

---
 arch/x86/events/intel/core.c | 17 +++--------------
 arch/x86/events/perf_event.h | 13 +++++++++----
 2 files changed, 12 insertions(+), 18 deletions(-)

arch/x86/events/intel/core.c +3 ~~-14~~

index 8c1efb4d8f47..3fd3842571e8 100644
@@ -2470,16 +2470,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 static struct event_constraint *
 intel_bts_constraints(struct perf_event *event)
 {
-	struct hw_perf_event *hwc = &event->hw;
-	unsigned int hw_event, bts_event;
-
-	if (event->attr.freq)
-		return NULL;
-
-	hw_event = hwc->config & INTEL_ARCH_EVENT_MASK;
-	bts_event = x86_pmu.event_map(PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
-
-	if (unlikely(hw_event == bts_event && hwc->sample_period == 1))
+	if (unlikely(intel_pmu_has_bts(event)))
 		return &bts_constraint;
 
 	return NULL;

@@ -3101,10 +3092,8 @@ static unsigned long intel_pmu_large_pebs_flags(struct perf_event *event)
 static int intel_pmu_bts_config(struct perf_event *event)
 {
 	struct perf_event_attr *attr = &event->attr;
-	struct hw_perf_event *hwc = &event->hw;
 
-	if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
-	    !attr->freq && hwc->sample_period == 1) {
+	if (unlikely(intel_pmu_has_bts(event))) {
 		/* BTS is not supported by this architecture. */
 		if (!x86_pmu.bts_active)
 			return -EOPNOTSUPP;

@@ -3166,7 +3155,7 @@ static int intel_pmu_hw_config(struct perf_event *event)
 		/*
 		 * BTS is set up earlier in this path, so don't account twice
 		 */
-		if (!intel_pmu_has_bts(event)) {
+		if (!unlikely(intel_pmu_has_bts(event))) {
 			/* disallow lbr if conflicting events are present */
 			if (x86_add_exclusive(x86_lbr_exclusive_lbr))
 				return -EBUSY;

arch/x86/events/perf_event.h +9 -4

index adae087cecdd..78d7b7031bfc 100644
@@ -859,11 +859,16 @@ static inline int amd_pmu_init(void)
 
 static inline bool intel_pmu_has_bts(struct perf_event *event)
 {
-	if (event->attr.config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
-	    !event->attr.freq && event->hw.sample_period == 1)
-		return true;
+	struct hw_perf_event *hwc = &event->hw;
+	unsigned int hw_event, bts_event;
+
+	if (event->attr.freq)
+		return false;
+
+	hw_event = hwc->config & INTEL_ARCH_EVENT_MASK;
+	bts_event = x86_pmu.event_map(PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
 
-	return false;
+	return hw_event == bts_event && hwc->sample_period == 1;
 }
 
 int intel_pmu_save_and_restart(struct perf_event *event);

Signature · 1 line

2.17.2

tip-bot for Jiri Olsa

merge bot

2018-11-22 10:14 UTC #2 lore raw

[tip:perf/urgent] perf/x86/intel: Add generic branch tracing check to intel_pmu_has_bts()

perf/x86/intel: Add generic branch tracing check to intel_pmu_has_bts()

Commit-ID:  67266c1080ad56c31af72b9c18355fde8ccc124a
Gitweb:     https://git.kernel.org/tip/67266c1080ad56c31af72b9c18355fde8ccc124a
Author:     Jiri Olsa <[email protected]>
AuthorDate: Wed, 21 Nov 2018 11:16:11 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 22 Nov 2018 09:45:09 +0100

perf/x86/intel: Add generic branch tracing check to intel_pmu_has_bts()

Currently we check the branch tracing only by checking for the PERF_COUNT_HW_BRANCH_INSTRUCTIONS event of PERF_TYPE_HARDWARE type. But we can define the same event with the PERF_TYPE_RAW type.

Changing the intel_pmu_has_bts() code to check on event's final hw config value, so both HW types are covered.

Adding unlikely to intel_pmu_has_bts() condition calls, because it was used in the original code in intel_bts_constraints.

Signed-off-by Jiri Olsa <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Cc <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/events/intel/core.c | 17 +++--------------
 arch/x86/events/perf_event.h | 13 +++++++++----
 2 files changed, 12 insertions(+), 18 deletions(-)

arch/x86/events/intel/core.c +3 ~~-14~~

index a95079abeb03..a15a73788693 100644
@@ -2474,16 +2474,7 @@ done:
 static struct event_constraint *
 intel_bts_constraints(struct perf_event *event)
 {
-	struct hw_perf_event *hwc = &event->hw;
-	unsigned int hw_event, bts_event;
-
-	if (event->attr.freq)
-		return NULL;
-
-	hw_event = hwc->config & INTEL_ARCH_EVENT_MASK;
-	bts_event = x86_pmu.event_map(PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
-
-	if (unlikely(hw_event == bts_event && hwc->sample_period == 1))
+	if (unlikely(intel_pmu_has_bts(event)))
 		return &bts_constraint;
 
 	return NULL;

@@ -3105,10 +3096,8 @@ static unsigned long intel_pmu_large_pebs_flags(struct perf_event *event)
 static int intel_pmu_bts_config(struct perf_event *event)
 {
 	struct perf_event_attr *attr = &event->attr;
-	struct hw_perf_event *hwc = &event->hw;
 
-	if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
-	    !attr->freq && hwc->sample_period == 1) {
+	if (unlikely(intel_pmu_has_bts(event))) {
 		/* BTS is not supported by this architecture. */
 		if (!x86_pmu.bts_active)
 			return -EOPNOTSUPP;

@@ -3170,7 +3159,7 @@ static int intel_pmu_hw_config(struct perf_event *event)
 		/*
 		 * BTS is set up earlier in this path, so don't account twice
 		 */
-		if (!intel_pmu_has_bts(event)) {
+		if (!unlikely(intel_pmu_has_bts(event))) {
 			/* disallow lbr if conflicting events are present */
 			if (x86_add_exclusive(x86_lbr_exclusive_lbr))
 				return -EBUSY;

arch/x86/events/perf_event.h +9 -4

index adae087cecdd..78d7b7031bfc 100644
@@ -859,11 +859,16 @@ static inline int amd_pmu_init(void)
 
 static inline bool intel_pmu_has_bts(struct perf_event *event)
 {
-	if (event->attr.config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
-	    !event->attr.freq && event->hw.sample_period == 1)
-		return true;
+	struct hw_perf_event *hwc = &event->hw;
+	unsigned int hw_event, bts_event;
+
+	if (event->attr.freq)
+		return false;
+
+	hw_event = hwc->config & INTEL_ARCH_EVENT_MASK;
+	bts_event = x86_pmu.event_map(PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
 
-	return false;
+	return hw_event == bts_event && hwc->sample_period == 1;
 }
 
 int intel_pmu_save_and_restart(struct perf_event *event);

Jiri Olsa

author reviewer

2018-11-21 10:16 UTC #3 lore raw

PATCH 3/3 perf/x86/intel: Disallow precise_ip on BTS event

In reply to Jiri Olsa

Vince reported crash in bts flush code when touching the callchain data, which was supposed to be initialized as an 'early' callchain, but intel_pmu_drain_bts_buffer does not do that.

  BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
  ...
  Call Trace:
   <IRQ>
   intel_pmu_drain_bts_buffer+0x151/0x220
   ? intel_get_event_constraints+0x219/0x360
   ? perf_assign_events+0xe2/0x2a0
   ? select_idle_sibling+0x22/0x3a0
   ? __update_load_avg_se+0x1ec/0x270
   ? enqueue_task_fair+0x377/0xdd0
   ? cpumask_next_and+0x19/0x20
   ? load_balance+0x134/0x950
   ? check_preempt_curr+0x7a/0x90
   ? ttwu_do_wakeup+0x19/0x140
   x86_pmu_stop+0x3b/0x90
   x86_pmu_del+0x57/0x160
   event_sched_out.isra.106+0x81/0x170
   group_sched_out.part.108+0x51/0xc0
   __perf_event_disable+0x7f/0x160
   event_function+0x8c/0xd0
   remote_function+0x3c/0x50
   flush_smp_call_function_queue+0x35/0xe0
   smp_call_function_single_interrupt+0x3a/0xd0
   call_function_single_interrupt+0xf/0x20
   </IRQ>

It was triggered by fuzzer but can be easily reproduced by:

  # perf record -e cpu/branch-instructions/pu -g -c 1

Peter suggested not to allow branch tracing for precise events:

Quoted 5 lines

Now arguably, this is really stupid behaviour. Who in his right mind wants callchain output on BTS entries. And even if they do, BTS + precise_ip is nonsensical.
So in my mind disallowing precise_ip on BTS would be the simplest fix.

Fixes 6cbc304f2f36 ("perf/x86/intel: Fix unwind errors from PEBS entries (mk-II)")

Reported-by Vince Weaver <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 arch/x86/events/intel/core.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/x86/events/intel/core.c +4 -0

index 3fd3842571e8..284893373d69 100644
@@ -3102,6 +3102,10 @@ static int intel_pmu_bts_config(struct perf_event *event)
 		if (!attr->exclude_kernel)
 			return -EOPNOTSUPP;
 
+		/* BTS is not allowed for precise events. */
+		if (attr->precise_ip)
+			return -EOPNOTSUPP;
+
 		/* disallow bts if conflicting events are present */
 		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
 			return -EBUSY;

Signature · 1 line

2.17.2

tip-bot for Jiri Olsa

merge bot

2018-11-22 10:15 UTC #4 lore raw

[tip:perf/urgent] perf/x86/intel: Disallow precise_ip on BTS events

perf/x86/intel: Disallow precise_ip on BTS events

Commit-ID:  472de49fdc53365c880ab81ae2b5cfdd83db0b06
Gitweb:     https://git.kernel.org/tip/472de49fdc53365c880ab81ae2b5cfdd83db0b06
Author:     Jiri Olsa <[email protected]>
AuthorDate: Wed, 21 Nov 2018 11:16:12 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 22 Nov 2018 09:45:10 +0100

perf/x86/intel: Disallow precise_ip on BTS events

Vince reported a crash in the BTS flush code when touching the callchain data, which was supposed to be initialized as an 'early' callchain, but intel_pmu_drain_bts_buffer() does not do that:

  BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
  ...
  Call Trace:
   <IRQ>
   intel_pmu_drain_bts_buffer+0x151/0x220
   ? intel_get_event_constraints+0x219/0x360
   ? perf_assign_events+0xe2/0x2a0
   ? select_idle_sibling+0x22/0x3a0
   ? __update_load_avg_se+0x1ec/0x270
   ? enqueue_task_fair+0x377/0xdd0
   ? cpumask_next_and+0x19/0x20
   ? load_balance+0x134/0x950
   ? check_preempt_curr+0x7a/0x90
   ? ttwu_do_wakeup+0x19/0x140
   x86_pmu_stop+0x3b/0x90
   x86_pmu_del+0x57/0x160
   event_sched_out.isra.106+0x81/0x170
   group_sched_out.part.108+0x51/0xc0
   __perf_event_disable+0x7f/0x160
   event_function+0x8c/0xd0
   remote_function+0x3c/0x50
   flush_smp_call_function_queue+0x35/0xe0
   smp_call_function_single_interrupt+0x3a/0xd0
   call_function_single_interrupt+0xf/0x20
   </IRQ>

It was triggered by fuzzer but can be easily reproduced by:

  # perf record -e cpu/branch-instructions/pu -g -c 1

Peter suggested not to allow branch tracing for precise events:

Quoted 5 lines

Now arguably, this is really stupid behaviour. Who in his right mind wants callchain output on BTS entries. And even if they do, BTS + precise_ip is nonsensical.
So in my mind disallowing precise_ip on BTS would be the simplest fix.

Reported-by Vince Weaver <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Cc <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Fixes 6cbc304f2f36 ("perf/x86/intel: Fix unwind errors from PEBS entries (mk-II)")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/events/intel/core.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/x86/events/intel/core.c +4 -0

index a15a73788693..ecc3e34ca955 100644
@@ -3106,6 +3106,10 @@ static int intel_pmu_bts_config(struct perf_event *event)
 		if (!attr->exclude_kernel)
 			return -EOPNOTSUPP;
 
+		/* BTS is not allowed for precise events. */
+		if (attr->precise_ip)
+			return -EOPNOTSUPP;
+
 		/* disallow bts if conflicting events are present */
 		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
 			return -EBUSY;

tip-bot for Jiri Olsa

merge bot

2018-11-22 10:14 UTC #5 lore raw

[tip:perf/urgent] perf/x86/intel: Move branch tracing setup to the Intel-specific source file

In reply to Jiri Olsa

perf/x86/intel: Move branch tracing setup to the Intel-specific source file

Commit-ID:  ed6101bbf6266ee83e620b19faa7c6ad56bb41ab
Gitweb:     https://git.kernel.org/tip/ed6101bbf6266ee83e620b19faa7c6ad56bb41ab
Author:     Jiri Olsa <[email protected]>
AuthorDate: Wed, 21 Nov 2018 11:16:10 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 22 Nov 2018 09:45:09 +0100

perf/x86/intel: Move branch tracing setup to the Intel-specific source file

Moving branch tracing setup to Intel core object into separate intel_pmu_bts_config function, because it's Intel specific.

Signed-off-by Jiri Olsa <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Cc <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/events/core.c       | 20 --------------------
 arch/x86/events/intel/core.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 21 deletions(-)

arch/x86/events/core.c +0 ~~-20~~

index 106911b603bd..374a19712e20 100644
@@ -438,26 +438,6 @@ int x86_setup_perfctr(struct perf_event *event)
 	if (config == -1LL)
 		return -EINVAL;
 
-	/*
-	 * Branch tracing:
-	 */
-	if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
-	    !attr->freq && hwc->sample_period == 1) {
-		/* BTS is not supported by this architecture. */
-		if (!x86_pmu.bts_active)
-			return -EOPNOTSUPP;
-
-		/* BTS is currently only allowed for user-mode. */
-		if (!attr->exclude_kernel)
-			return -EOPNOTSUPP;
-
-		/* disallow bts if conflicting events are present */
-		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
-			return -EBUSY;
-
-		event->destroy = hw_perf_lbr_event_destroy;
-	}
-
 	hwc->config |= config;
 
 	return 0;

arch/x86/events/intel/core.c +40 -1

57 lines not on this page · raw message