From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932947AbcHWI52 (ORCPT ); Tue, 23 Aug 2016 04:57:28 -0400 Received: from mga04.intel.com ([192.55.52.120]:46306 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757482AbcHWI5C (ORCPT ); Tue, 23 Aug 2016 04:57:02 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,565,1464678000"; d="scan'208";a="752371676" From: Alexander Shishkin To: Peter Zijlstra Cc: Ingo Molnar , linux-kernel@vger.kernel.org, vince@deater.net, eranian@google.com, Arnaldo Carvalho de Melo , Alexander Shishkin Subject: [PATCH 1/3] perf/x86/intel/bts: Fix confused ordering of PMU callbacks Date: Tue, 23 Aug 2016 11:54:32 +0300 Message-Id: <1471942474-1854-2-git-send-email-alexander.shishkin@linux.intel.com> X-Mailer: git-send-email 2.8.0.rc3 In-Reply-To: <1471942474-1854-1-git-send-email-alexander.shishkin@linux.intel.com> References: <1471942474-1854-1-git-send-email-alexander.shishkin@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Signed-off-by: Alexander Shishkin --- arch/x86/events/intel/bts.c | 85 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 21 deletions(-) diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c index 0a6e393a2e..0b8ccff564 100644 --- a/arch/x86/events/intel/bts.c +++ b/arch/x86/events/intel/bts.c @@ -204,6 +204,16 @@ static void bts_update(struct bts_ctx *bts) static int 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::started, 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 local barrier to + * enforce CPU ordering. + */ + static void __bts_event_start(struct perf_event *event) { struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); @@ -221,10 +231,13 @@ static void __bts_event_start(struct perf_event *event) /* * local barrier to make sure that ds configuration made it - * before we enable BTS + * before we enable BTS and bts::started is set */ wmb(); + /* PMI handler: this counter is running and likely generating PMIs */ + WRITE_ONCE(bts->started, 1); + intel_pmu_enable_bts(config); } @@ -251,9 +264,6 @@ static void bts_event_start(struct perf_event *event, int flags) __bts_event_start(event); - /* PMI handler: this counter is running and likely generating PMIs */ - ACCESS_ONCE(bts->started) = 1; - return; fail_end_stop: @@ -265,28 +275,34 @@ fail_stop: static void __bts_event_stop(struct perf_event *event) { + struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); + + /* PMI handler: don't restart this counter */ + WRITE_ONCE(bts->started, 0); + /* * No extra synchronization is mandated by the documentation to have * BTS data stores globally visible. */ 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) { struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); - struct bts_buffer *buf = perf_get_aux(&bts->handle); + struct bts_buffer *buf; - /* PMI handler: don't restart this counter */ - ACCESS_ONCE(bts->started) = 0; + if (READ_ONCE(bts->started)) { + __bts_event_stop(event); + + /* order buf (handle::event load) against bts::started store */ + mb(); + } + + buf = perf_get_aux(&bts->handle); - __bts_event_stop(event); + event->hw.state |= PERF_HES_STOPPED; if (flags & PERF_EF_UPDATE) { bts_update(bts); @@ -311,7 +327,13 @@ void intel_bts_enable_local(void) { struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); - if (bts->handle.event && bts->started) + if (!READ_ONCE(bts->started)) + return; + + /* matches wmb() ordering bts::started store against handle::event */ + rmb(); + + if (bts->handle.event) __bts_event_start(bts->handle.event); } @@ -319,6 +341,12 @@ void intel_bts_disable_local(void) { struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); + if (!READ_ONCE(bts->started)) + return; + + /* matches wmb() ordering bts::started store against handle::event */ + rmb(); + if (bts->handle.event) __bts_event_stop(bts->handle.event); } @@ -407,9 +435,15 @@ int intel_bts_interrupt(void) struct perf_event *event = bts->handle.event; struct bts_buffer *buf; s64 old_head; - int err; + int err = -ENOSPC; - if (!event || !bts->started) + if (!READ_ONCE(bts->started)) + return 0; + + /* matches wmb() ordering bts::started store against handle::event */ + rmb(); + + if (!event) return 0; buf = perf_get_aux(&bts->handle); @@ -432,12 +466,21 @@ int intel_bts_interrupt(void) !!local_xchg(&buf->lost, 0)); buf = perf_aux_output_begin(&bts->handle, event); - if (!buf) - return 1; + if (buf) + err = bts_buffer_reset(buf, &bts->handle); - err = bts_buffer_reset(buf, &bts->handle); - if (err) - perf_aux_output_end(&bts->handle, 0, false); + if (err) { + WRITE_ONCE(bts->started, 0); + + if (buf) { + /* + * cleared bts::started should be visible before + * cleared handle::event + */ + wmb(); + perf_aux_output_end(&bts->handle, 0, false); + } + } return 1; } -- 2.8.1