Ingo Molnar <mingo@xxxxxxxxxx> writes: > * Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
