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

Quoted 3 lines · Dan Carpenter

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.

Quoted 8 lines

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;
                                ^^^^^^^^^^^^

Quoted 8 lines

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 <[email protected]>
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.

* Alexander Shishkin <[email protected]> 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.

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?