Commit a9a94401 authored by Alexander Shishkin's avatar Alexander Shishkin Committed by Ingo Molnar

perf/x86/intel/bts: Fix confused ordering of PMU callbacks

The intel_bts driver is using a CPU-local 'started' variable to order
callbacks and PMIs and make sure that AUX transactions don't get messed
up. However, the ordering rules in regard to this variable is a complete
mess, which recently resulted in perf_fuzzer-triggered warnings and
panics.

The general ordering rule that is patch is enforcing is that this
cpu-local variable be set only when the cpu-local AUX transaction is
active; consequently, this variable is to be checked before the AUX
related bits can be touched.
Reported-by: default avatarVince Weaver <vincent.weaver@maine.edu>
Signed-off-by: default avatarAlexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: default avatarPeter Zijlstra (Intel) <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160906132353.19887-4-alexander.shishkin@linux.intel.comSigned-off-by: default avatarIngo Molnar <mingo@kernel.org>
parent b79ccadd
...@@ -31,7 +31,17 @@ ...@@ -31,7 +31,17 @@
struct bts_ctx { struct bts_ctx {
struct perf_output_handle handle; struct perf_output_handle handle;
struct debug_store ds_back; struct debug_store ds_back;
int started; int state;
};
/* BTS context states: */
enum {
/* no ongoing AUX transactions */
BTS_STATE_STOPPED = 0,
/* AUX transaction is on, BTS tracing is disabled */
BTS_STATE_INACTIVE,
/* AUX transaction is on, BTS tracing is running */
BTS_STATE_ACTIVE,
}; };
static DEFINE_PER_CPU(struct bts_ctx, bts_ctx); static DEFINE_PER_CPU(struct bts_ctx, bts_ctx);
...@@ -204,6 +214,15 @@ static void bts_update(struct bts_ctx *bts) ...@@ -204,6 +214,15 @@ static void bts_update(struct bts_ctx *bts)
static int static int
bts_buffer_reset(struct bts_buffer *buf, struct perf_output_handle *handle); bts_buffer_reset(struct bts_buffer *buf, struct perf_output_handle *handle);
/*
* Ordering PMU callbacks wrt themselves and the PMI is done by means
* of bts::state, which:
* - is set when bts::handle::event is valid, that is, between
* perf_aux_output_begin() and perf_aux_output_end();
* - is zero otherwise;
* - is ordered against bts::handle::event with a compiler barrier.
*/
static void __bts_event_start(struct perf_event *event) static void __bts_event_start(struct perf_event *event)
{ {
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
...@@ -221,10 +240,13 @@ static void __bts_event_start(struct perf_event *event) ...@@ -221,10 +240,13 @@ static void __bts_event_start(struct perf_event *event)
/* /*
* local barrier to make sure that ds configuration made it * local barrier to make sure that ds configuration made it
* before we enable BTS * before we enable BTS and bts::state goes ACTIVE
*/ */
wmb(); wmb();
/* INACTIVE/STOPPED -> ACTIVE */
WRITE_ONCE(bts->state, BTS_STATE_ACTIVE);
intel_pmu_enable_bts(config); intel_pmu_enable_bts(config);
} }
...@@ -251,9 +273,6 @@ static void bts_event_start(struct perf_event *event, int flags) ...@@ -251,9 +273,6 @@ static void bts_event_start(struct perf_event *event, int flags)
__bts_event_start(event); __bts_event_start(event);
/* PMI handler: this counter is running and likely generating PMIs */
ACCESS_ONCE(bts->started) = 1;
return; return;
fail_end_stop: fail_end_stop:
...@@ -263,30 +282,34 @@ static void bts_event_start(struct perf_event *event, int flags) ...@@ -263,30 +282,34 @@ static void bts_event_start(struct perf_event *event, int flags)
event->hw.state = PERF_HES_STOPPED; event->hw.state = PERF_HES_STOPPED;
} }
static void __bts_event_stop(struct perf_event *event) static void __bts_event_stop(struct perf_event *event, int state)
{ {
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
/* ACTIVE -> INACTIVE(PMI)/STOPPED(->stop()) */
WRITE_ONCE(bts->state, state);
/* /*
* No extra synchronization is mandated by the documentation to have * No extra synchronization is mandated by the documentation to have
* BTS data stores globally visible. * BTS data stores globally visible.
*/ */
intel_pmu_disable_bts(); intel_pmu_disable_bts();
if (event->hw.state & PERF_HES_STOPPED)
return;
ACCESS_ONCE(event->hw.state) |= PERF_HES_STOPPED;
} }
static void bts_event_stop(struct perf_event *event, int flags) static void bts_event_stop(struct perf_event *event, int flags)
{ {
struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
struct bts_buffer *buf = perf_get_aux(&bts->handle); struct bts_buffer *buf = NULL;
int state = READ_ONCE(bts->state);
if (state == BTS_STATE_ACTIVE)
__bts_event_stop(event, BTS_STATE_STOPPED);
/* PMI handler: don't restart this counter */ if (state != BTS_STATE_STOPPED)
ACCESS_ONCE(bts->started) = 0; buf = perf_get_aux(&bts->handle);
__bts_event_stop(event); event->hw.state |= PERF_HES_STOPPED;
if (flags & PERF_EF_UPDATE) { if (flags & PERF_EF_UPDATE) {
bts_update(bts); bts_update(bts);
...@@ -296,6 +319,7 @@ static void bts_event_stop(struct perf_event *event, int flags) ...@@ -296,6 +319,7 @@ static void bts_event_stop(struct perf_event *event, int flags)
bts->handle.head = bts->handle.head =
local_xchg(&buf->data_size, local_xchg(&buf->data_size,
buf->nr_pages << PAGE_SHIFT); buf->nr_pages << PAGE_SHIFT);
perf_aux_output_end(&bts->handle, local_xchg(&buf->data_size, 0), perf_aux_output_end(&bts->handle, local_xchg(&buf->data_size, 0),
!!local_xchg(&buf->lost, 0)); !!local_xchg(&buf->lost, 0));
} }
...@@ -310,8 +334,20 @@ static void bts_event_stop(struct perf_event *event, int flags) ...@@ -310,8 +334,20 @@ static void bts_event_stop(struct perf_event *event, int flags)
void intel_bts_enable_local(void) void intel_bts_enable_local(void)
{ {
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
int state = READ_ONCE(bts->state);
/*
* Here we transition from INACTIVE to ACTIVE;
* if we instead are STOPPED from the interrupt handler,
* stay that way. Can't be ACTIVE here though.
*/
if (WARN_ON_ONCE(state == BTS_STATE_ACTIVE))
return;
if (state == BTS_STATE_STOPPED)
return;
if (bts->handle.event && bts->started) if (bts->handle.event)
__bts_event_start(bts->handle.event); __bts_event_start(bts->handle.event);
} }
...@@ -319,8 +355,15 @@ void intel_bts_disable_local(void) ...@@ -319,8 +355,15 @@ void intel_bts_disable_local(void)
{ {
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
/*
* Here we transition from ACTIVE to INACTIVE;
* do nothing for STOPPED or INACTIVE.
*/
if (READ_ONCE(bts->state) != BTS_STATE_ACTIVE)
return;
if (bts->handle.event) if (bts->handle.event)
__bts_event_stop(bts->handle.event); __bts_event_stop(bts->handle.event, BTS_STATE_INACTIVE);
} }
static int static int
...@@ -407,9 +450,13 @@ int intel_bts_interrupt(void) ...@@ -407,9 +450,13 @@ int intel_bts_interrupt(void)
struct perf_event *event = bts->handle.event; struct perf_event *event = bts->handle.event;
struct bts_buffer *buf; struct bts_buffer *buf;
s64 old_head; s64 old_head;
int err; int err = -ENOSPC;
if (!event || !bts->started) /*
* this is wrapped in intel_bts_enable_local/intel_bts_disable_local,
* so we can only be INACTIVE or STOPPED
*/
if (READ_ONCE(bts->state) == BTS_STATE_STOPPED)
return 0; return 0;
buf = perf_get_aux(&bts->handle); buf = perf_get_aux(&bts->handle);
...@@ -432,12 +479,21 @@ int intel_bts_interrupt(void) ...@@ -432,12 +479,21 @@ int intel_bts_interrupt(void)
!!local_xchg(&buf->lost, 0)); !!local_xchg(&buf->lost, 0));
buf = perf_aux_output_begin(&bts->handle, event); buf = perf_aux_output_begin(&bts->handle, event);
if (!buf) if (buf)
return 1; err = bts_buffer_reset(buf, &bts->handle);
err = bts_buffer_reset(buf, &bts->handle); if (err) {
if (err) WRITE_ONCE(bts->state, BTS_STATE_STOPPED);
perf_aux_output_end(&bts->handle, 0, false);
if (buf) {
/*
* BTS_STATE_STOPPED should be visible before
* cleared handle::event
*/
barrier();
perf_aux_output_end(&bts->handle, 0, false);
}
}
return 1; return 1;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment