[PATCH 0/3] perf/x86/intel/pt: Fix stop/start with no update

[PATCH 0/3] perf/x86/intel/pt: Fix stop/start with no update

[Adrian Hunter]

Hi

Here is a fix for Intel PT.

There are 2 preparatory patches with no functional change intended.

The fix is to prevent nesting of perf_aux_output_begin() which
triggers the WARNING in perf_aux_output_begin():

	nest = READ_ONCE(rb->aux_nest);
	/*
	 * Nesting is not supported for AUX area, make sure nested
	 * writers are caught early
	 */
	if (WARN_ON_ONCE(nest))
		goto err_put;

The nesting happens when Intel PT gets throttled.  See the patch for more
details.


Adrian Hunter (3):
      perf/x86/intel/pt: Factor out pt_config_enable()
      perf/x86/intel/pt: Use bitwise access for PERF_HES_STOPPED
      perf/x86/intel/pt: Fix stop/start with no update

 arch/x86/events/intel/pt.c | 96 +++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 31 deletions(-)


Regards
Adrian

[PATCH 1/3] perf/x86/intel/pt: Factor out pt_config_enable()

[Adrian Hunter]

pt_config() enables tracing by allowing NMIs and pause/resume, issuing
the necessary barriers, and calling pt_config_start().  A later change
needs to re-enable tracing on a (re-)start path without repeating the
full pt_config() setup (filters, RTIT_CTL, buffer configuration).

Factor that enabling sequence out into a new helper, pt_config_enable(),
so it can be called on its own.

No functional change intended.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 arch/x86/events/intel/pt.c | 41 ++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index b5726b50e77d..dc1be7f6e04b 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -502,6 +502,29 @@ static u64 pt_config_filters(struct perf_event *event)
 	return rtit_ctl;
 }
 
+static void pt_config_enable(struct perf_event *event)
+{
+	struct pt *pt = this_cpu_ptr(&pt_ctx);
+
+	/*
+	 * Allow resume before starting so as not to overwrite a value set by a
+	 * PMI.
+	 */
+	barrier();
+	WRITE_ONCE(pt->resume_allowed, 1);
+	/* Configuration is complete, it is now OK to handle an NMI */
+	barrier();
+	WRITE_ONCE(pt->handle_nmi, 1);
+	barrier();
+	pt_config_start(event);
+	barrier();
+	/*
+	 * Allow pause after starting so its pt_config_stop() doesn't race with
+	 * pt_config_start().
+	 */
+	WRITE_ONCE(pt->pause_allowed, 1);
+}
+
 static void pt_config(struct perf_event *event)
 {
 	struct pt *pt = this_cpu_ptr(&pt_ctx);
@@ -541,23 +564,7 @@ static void pt_config(struct perf_event *event)
 
 	event->hw.aux_config = reg;
 
-	/*
-	 * Allow resume before starting so as not to overwrite a value set by a
-	 * PMI.
-	 */
-	barrier();
-	WRITE_ONCE(pt->resume_allowed, 1);
-	/* Configuration is complete, it is now OK to handle an NMI */
-	barrier();
-	WRITE_ONCE(pt->handle_nmi, 1);
-	barrier();
-	pt_config_start(event);
-	barrier();
-	/*
-	 * Allow pause after starting so its pt_config_stop() doesn't race with
-	 * pt_config_start().
-	 */
-	WRITE_ONCE(pt->pause_allowed, 1);
+	pt_config_enable(event);
 }
 
 static void pt_config_stop(struct perf_event *event)
-- 
2.53.0

[PATCH 2/3] perf/x86/intel/pt: Use bitwise access for PERF_HES_STOPPED

[Adrian Hunter]

The Intel PT driver reads and writes event->hw.state as a whole value,
assuming it is either 0 or PERF_HES_STOPPED.  That is true today, but a
subsequent fix needs to also track an open AUX output buffer using the
PERF_HES_UPTODATE bit of the same field.

When more than one bit can be set, whole-value assignments would
overwrite the other bits and whole-value comparisons would fail to match.

Convert all accesses to set, clear and test the PERF_HES_STOPPED bit
individually, in preparation for that change.

No functional change intended: event->hw.state currently only ever
holds 0 or PERF_HES_STOPPED, so the bitwise forms are equivalent.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 arch/x86/events/intel/pt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index dc1be7f6e04b..2163e5ccdc04 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1540,12 +1540,12 @@ void intel_pt_interrupt(void)
 
 	perf_aux_output_end(&pt->handle, local_xchg(&buf->data_size, 0));
 
-	if (!event->hw.state) {
+	if (!(event->hw.state & PERF_HES_STOPPED)) {
 		int ret;
 
 		buf = perf_aux_output_begin(&pt->handle, event);
 		if (!buf) {
-			event->hw.state = PERF_HES_STOPPED;
+			event->hw.state |= PERF_HES_STOPPED;
 			WRITE_ONCE(pt->resume_allowed, 0);
 			return;
 		}
@@ -1639,7 +1639,7 @@ static void pt_event_start(struct perf_event *event, int mode)
 			goto fail_end_stop;
 	}
 
-	hwc->state = 0;
+	hwc->state &= ~PERF_HES_STOPPED;
 
 	pt_config_buffer(buf);
 	pt_config(event);
@@ -1649,7 +1649,7 @@ static void pt_event_start(struct perf_event *event, int mode)
 fail_end_stop:
 	perf_aux_output_end(&pt->handle, 0);
 fail_stop:
-	hwc->state = PERF_HES_STOPPED;
+	hwc->state |= PERF_HES_STOPPED;
 }
 
 static void pt_event_stop(struct perf_event *event, int mode)
@@ -1680,10 +1680,10 @@ static void pt_event_stop(struct perf_event *event, int mode)
 
 	pt_config_stop(event);
 
-	if (event->hw.state == PERF_HES_STOPPED)
+	if (event->hw.state & PERF_HES_STOPPED)
 		return;
 
-	event->hw.state = PERF_HES_STOPPED;
+	event->hw.state |= PERF_HES_STOPPED;
 
 	if (mode & PERF_EF_UPDATE) {
 		struct pt_buffer *buf = perf_get_aux(&pt->handle);
@@ -1778,10 +1778,10 @@ static int pt_event_add(struct perf_event *event, int mode)
 	if (mode & PERF_EF_START) {
 		pt_event_start(event, 0);
 		ret = -EINVAL;
-		if (hwc->state == PERF_HES_STOPPED)
+		if (hwc->state & PERF_HES_STOPPED)
 			goto fail;
 	} else {
-		hwc->state = PERF_HES_STOPPED;
+		hwc->state |= PERF_HES_STOPPED;
 	}
 
 	ret = 0;
-- 
2.53.0

[PATCH 3/3] perf/x86/intel/pt: Fix stop/start with no update

[Adrian Hunter]

If pt_event_stop() is called without PERF_EF_UPDATE flag, then
perf_aux_output_end() is not called.  A subsequent call to pt_event_start()
will call perf_aux_output_begin() again which violates the rule against
nesting and triggers a WARNING in perf_aux_output_begin().

Originally, pt_event_stop() was never called without PERF_EF_UPDATE,
because the only code paths to do so are from event overflow, and Intel PT
does not do that.

However the introduction of group throttling by commit 9734e25fbf5ae
("perf: Fix the throttle logic for a group") meant that an Intel PT event
could be throttled if it was part of a group.  Throttling calls PMU
 ->stop() / ->start() callbacks without flags.

An example is when AUX area sampling is used. The following commands
hit the issue:

  echo 10000 > /proc/sys/kernel/perf_event_max_sample_rate

  perf record -F32000 --aux-sample -e '{intel_pt//u,cycles:u}' \
  -- bash -c 'for i in `seq 1 100000` ; do true ; done'

Use PERF_HES_UPTODATE to track whether perf_aux_output_begin() and
perf_aux_output_end() are balanced.  A cleared PERF_HES_UPTODATE bit
indicates that an AUX output context is still open.

Amend pt_event_start() / pt_event_stop() accordingly so that begin/end
stay balanced:

 - In non-snapshot mode, stop() always closes the buffer (the buffer may
   have run out of space, and that accounting is done by the update), so
   a following start() opens a fresh one as before.

 - In snapshot/overwrite mode, stop() without PERF_EF_UPDATE leaves the
   buffer open so that pt_event_snapshot_aux() can still copy from it,
   and start() then only re-enables tracing instead of calling
   perf_aux_output_begin() again.

Note that pt_event_del() calls pt_event_stop() with PERF_EF_UPDATE flag set
(as is required by the documentation), so a final call to
perf_aux_output_end() is assured.

Fixes: 52ca9ced3f707 ("perf/x86/intel/pt: Add Intel PT PMU driver")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 arch/x86/events/intel/pt.c | 45 ++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 2163e5ccdc04..5754cd405562 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1540,6 +1540,8 @@ void intel_pt_interrupt(void)
 
 	perf_aux_output_end(&pt->handle, local_xchg(&buf->data_size, 0));
 
+	event->hw.state |= PERF_HES_UPTODATE;
+
 	if (!(event->hw.state & PERF_HES_STOPPED)) {
 		int ret;
 
@@ -1561,6 +1563,8 @@ void intel_pt_interrupt(void)
 
 		pt_config_buffer(buf);
 		pt_config_start(event);
+
+		event->hw.state &= ~PERF_HES_UPTODATE;
 	}
 }
 
@@ -1629,6 +1633,18 @@ static void pt_event_start(struct perf_event *event, int mode)
 		return;
 	}
 
+	/*
+	 * Re-start subsequent to a call to pt_event_stop() without the
+	 * PERF_EF_UPDATE flag. Absence of PERF_HES_UPTODATE indicates that
+	 * perf_aux_output_begin() has already been called. This path can
+	 * come about only in snapshot/overwrite mode - see pt_event_stop().
+	 */
+	if (!(hwc->state & PERF_HES_UPTODATE)) {
+		hwc->state &= ~PERF_HES_STOPPED;
+		pt_config_enable(event);
+		return;
+	}
+
 	buf = perf_aux_output_begin(&pt->handle, event);
 	if (!buf)
 		goto fail_stop;
@@ -1639,7 +1655,7 @@ static void pt_event_start(struct perf_event *event, int mode)
 			goto fail_end_stop;
 	}
 
-	hwc->state &= ~PERF_HES_STOPPED;
+	hwc->state &= ~(PERF_HES_STOPPED | PERF_HES_UPTODATE);
 
 	pt_config_buffer(buf);
 	pt_config(event);
@@ -1649,12 +1665,13 @@ static void pt_event_start(struct perf_event *event, int mode)
 fail_end_stop:
 	perf_aux_output_end(&pt->handle, 0);
 fail_stop:
-	hwc->state |= PERF_HES_STOPPED;
+	hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
 }
 
 static void pt_event_stop(struct perf_event *event, int mode)
 {
 	struct pt *pt = this_cpu_ptr(&pt_ctx);
+	struct pt_buffer *buf;
 
 	if (mode & PERF_EF_PAUSE) {
 		if (READ_ONCE(pt->pause_allowed))
@@ -1680,17 +1697,24 @@ static void pt_event_stop(struct perf_event *event, int mode)
 
 	pt_config_stop(event);
 
-	if (event->hw.state & PERF_HES_STOPPED)
-		return;
-
 	event->hw.state |= PERF_HES_STOPPED;
 
-	if (mode & PERF_EF_UPDATE) {
-		struct pt_buffer *buf = perf_get_aux(&pt->handle);
+	if (event->hw.state & PERF_HES_UPTODATE)
+		return;
 
-		if (!buf)
-			return;
+	buf = perf_get_aux(&pt->handle);
+	if (!buf)
+		return;
 
+	/*
+	 * When not in snapshot/overwrite mode, there is a possibility that the
+	 * buffer has run out of space. The accounting for that is handled by
+	 * the update, so always update in that case. Snapshot/overwrite mode is
+	 * treated differently to allow for pt_event_snapshot_aux() which can
+	 * still get called if the AUX-sampling event is not stopped until after
+	 * PT is stopped.
+	 */
+	if ((mode & PERF_EF_UPDATE) || !buf->snapshot) {
 		if (WARN_ON_ONCE(pt->handle.event != event))
 			return;
 
@@ -1705,6 +1729,7 @@ static void pt_event_stop(struct perf_event *event, int mode)
 				local_xchg(&buf->data_size,
 					   buf->nr_pages << PAGE_SHIFT);
 		perf_aux_output_end(&pt->handle, local_xchg(&buf->data_size, 0));
+		event->hw.state |= PERF_HES_UPTODATE;
 	}
 }
 
@@ -1775,6 +1800,8 @@ static int pt_event_add(struct perf_event *event, int mode)
 	if (pt->handle.event)
 		goto fail;
 
+	event->hw.state |= PERF_HES_UPTODATE;
+
 	if (mode & PERF_EF_START) {
 		pt_event_start(event, 0);
 		ret = -EINVAL;
-- 
2.53.0