[PATCH] perf/x86/intel/pt: Clean up error handling in pt_event_add()

[PATCH] perf/x86/intel/pt: Clean up error handling in pt_event_add()

[Ingo Molnar]

* Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > That's not the style that the rest of this file uses.  Every function
> > uses direct returns where possible except pt_event_add() and that
> > function seems buggy.
> 
> Indeed.
> 
> > arch/x86/kernel/cpu/perf_event_intel_pt.c
> >   1000  
> >   1001          if (mode & PERF_EF_START) {
> >   1002                  pt_event_start(event, 0);
> >   1003                  if (hwc->state = PERF_HES_STOPPED) {
> >   1004                          pt_event_del(event, 0);
> >   1005                          ret = -EBUSY;
> >                                 ^^^^^^^^^^^^
> > We set "ret" here but then return zero.
> >
> >   1006                  }
> >   1007          } else {
> >   1008                  hwc->state = PERF_HES_STOPPED;
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > Shouldn't we set "ret" here?
> 
> No, or we'll end up returning -EBUSY where we should return zero for
> snapshot counters. It can be done above the quoted if statement.
> 
> How does the following look to you?
> 
> From 726515f8bbef2ca02c495695b9451533d1bc6207 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Wed, 15 Apr 2015 12:56:52 +0300
> Subject: [PATCH] perf/x86/intel/pt: cleanup error paths in pt_event_add()
> 
> pt_event_add() ends up returning 0 instead of -EBUSY in case of failure
> to start the newly added event. This is a result of complex handling of
> its return code.
> 
> This patch makes the return code handling of pt_event_add() more obvious
> and fixes the mentioned bug.

So it's still not obvious enough IMO - I wrote the patch below. 
Untested.

NOTE: I materially changed the existing clean up logic in the 
pt_event_start() failure case to use the direct perf_aux_output_end() 
path, not pt_event_del(). I could not convince myself that 
pt_event_del() is really needed there - but I might be wrong.

In any case, these functions are a mess and they are barely 
documented! Please add proper comments about what the interaction and 
expected rules of perf_aux_output_begin(), pt_buffer_reset_offsets(), 
pt_buffer_reset_markers(), pt_event_start(), perf_aux_output_end() et 
al is, right now it's a guessing game mostly. (in a separate patch 
please)

Btw., pt_event_start() has weird error handling as well: it should 
probably return an error code, instead of open coding event->hw.state 
= PERF_HES_STOPPED. This would have to be changed in all PMU drivers, 
with core perf setting hw.state to PERF_HES_STOPPED or so?

Thanks,

	Ingo

---

 arch/x86/kernel/cpu/perf_event_intel_pt.c | 32 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
index f2770641c0fd..1b298caf09c1 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
@@ -988,38 +988,36 @@ static int pt_event_add(struct perf_event *event, int mode)
 	int ret = -EBUSY;
 
 	if (pt->handle.event)
-		goto out;
+		goto fail;
 
 	buf = perf_aux_output_begin(&pt->handle, event);
-	if (!buf) {
-		ret = -EINVAL;
-		goto out;
-	}
+	ret = -EINVAL;
+	if (!buf)
+		goto fail_stop;
 
 	pt_buffer_reset_offsets(buf, pt->handle.head);
 	if (!buf->snapshot) {
 		ret = pt_buffer_reset_markers(buf, &pt->handle);
-		if (ret) {
-			perf_aux_output_end(&pt->handle, 0, true);
-			goto out;
-		}
+		if (ret)
+			goto fail_end_stop;
 	}
 
 	if (mode & PERF_EF_START) {
 		pt_event_start(event, 0);
-		if (hwc->state = PERF_HES_STOPPED) {
-			pt_event_del(event, 0);
-			ret = -EBUSY;
-		}
+		ret = -EBUSY;
+		if (hwc->state = PERF_HES_STOPPED)
+			goto fail_end_stop;
 	} else {
 		hwc->state = PERF_HES_STOPPED;
 	}
 
-	ret = 0;
-out:
+	return 0;
 
-	if (ret)
-		hwc->state = PERF_HES_STOPPED;
+fail_end_stop:
+	perf_aux_output_end(&pt->handle, 0, true);
+fail_stop:
+	hwc->state = PERF_HES_STOPPED;
+fail:
 
 	return ret;
 }

Re: [PATCH] perf/x86/intel/pt: Clean up error handling in pt_event_add()

[Alexander Shishkin]

Ingo Molnar <mingo@kernel.org> writes:

> * Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:
>
> So it's still not obvious enough IMO - I wrote the patch below. 
> Untested.
>
> NOTE: I materially changed the existing clean up logic in the 
> pt_event_start() failure case to use the direct perf_aux_output_end() 
> path, not pt_event_del(). I could not convince myself that 
> pt_event_del() is really needed there - but I might be wrong.

No, you're right, perf_aux_output_end() is sufficient there.

> In any case, these functions are a mess and they are barely 
> documented! Please add proper comments about what the interaction and 
> expected rules of perf_aux_output_begin(), pt_buffer_reset_offsets(), 
> pt_buffer_reset_markers(), pt_event_start(), perf_aux_output_end() et 
> al is, right now it's a guessing game mostly. (in a separate patch 
> please)

Will do.

> Btw., pt_event_start() has weird error handling as well: it should 
> probably return an error code, instead of open coding event->hw.state 
> = PERF_HES_STOPPED. This would have to be changed in all PMU drivers, 
> with core perf setting hw.state to PERF_HES_STOPPED or so?

The difference is that normal performance counters can't really fail to
start if their pmu::add() succeeded and afaict that such is also the
assumption in the perf core; aux counters, however, can run out of room
in the aux buffer. For most things tracking hw.state seems sufficient.

What I could do is have something like do_pt_event_start() that returns
-ENOSPC for the buffer-full condition and call it from both
pt_event_add() and pt_event_start(), which would both set hw.state to
HES_STOPPED if it fails. I'm not sure how much of a readability
improvement that is, I suspect that the same can be achieved by adding
appropriate comments to these functions. What do you think?

Thanks,
--
Alex

[tip:perf/urgent] perf/x86/intel/pt: Fix and clean up error handling in pt_event_add()

[tip-bot for Ingo Molnar]

Commit-ID:  0c99241c93b8060441f3c8434848e54b5338f922
Gitweb:     http://git.kernel.org/tip/0c99241c93b8060441f3c8434848e54b5338f922
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Thu, 16 Apr 2015 12:38:30 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 18 Apr 2015 13:31:26 +0200

perf/x86/intel/pt: Fix and clean up error handling in pt_event_add()

Dan Carpenter reported that pt_event_add() has buggy
error handling logic: it returns 0 instead of -EBUSY when
it fails to start a newly added event.

Furthermore, the control flow in this function is messy,
with cleanup labels mixed with direct returns.

Fix the bug and clean up the code by converting it to
a straight fast path for the regular non-failing case,
plus a clear sequence of cascading goto labels to do
all cleanup.

NOTE: I materially changed the existing clean up logic in the
pt_event_start() failure case to use the direct
perf_aux_output_end() path, not pt_event_del(), because
perf_aux_output_end() is enough here.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20150416103830.GB7847@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_intel_pt.c | 33 ++++++++++++++-----------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
index f277064..ffe666c 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
@@ -988,39 +988,36 @@ static int pt_event_add(struct perf_event *event, int mode)
 	int ret = -EBUSY;
 
 	if (pt->handle.event)
-		goto out;
+		goto fail;
 
 	buf = perf_aux_output_begin(&pt->handle, event);
-	if (!buf) {
-		ret = -EINVAL;
-		goto out;
-	}
+	ret = -EINVAL;
+	if (!buf)
+		goto fail_stop;
 
 	pt_buffer_reset_offsets(buf, pt->handle.head);
 	if (!buf->snapshot) {
 		ret = pt_buffer_reset_markers(buf, &pt->handle);
-		if (ret) {
-			perf_aux_output_end(&pt->handle, 0, true);
-			goto out;
-		}
+		if (ret)
+			goto fail_end_stop;
 	}
 
 	if (mode & PERF_EF_START) {
 		pt_event_start(event, 0);
-		if (hwc->state == PERF_HES_STOPPED) {
-			pt_event_del(event, 0);
-			ret = -EBUSY;
-		}
+		ret = -EBUSY;
+		if (hwc->state == PERF_HES_STOPPED)
+			goto fail_end_stop;
 	} else {
 		hwc->state = PERF_HES_STOPPED;
 	}
 
-	ret = 0;
-out:
-
-	if (ret)
-		hwc->state = PERF_HES_STOPPED;
+	return 0;
 
+fail_end_stop:
+	perf_aux_output_end(&pt->handle, 0, true);
+fail_stop:
+	hwc->state = PERF_HES_STOPPED;
+fail:
 	return ret;
 }