Re: [PATCH] perf_events: fix errors path in perf_output_begin()

[Stephane Eranian]

On Mon, May 17, 2010 at 1:46 PM, Frederic Weisbecker <fweisbec@xxxxxxxxx> wrote:
> On Mon, May 17, 2010 at 12:46:01PM +0200, Stephane Eranian wrote:
>> Â In case the sampling buffer has no "payload" pages, nr_pages is 0.
>> Â The problem is that the error path in perf_output_begin() skips to
>> Â a label which assumes perf_output_lock() has been issued which is
>> Â not the case. That triggers a WARN_ON() is perf_output_unlock().
>>
>> Â This patch fixes the problem by adding a new label and skipping
>> Â perf_task_unlock() in case data->nr_pages is 0.
>>
>> Â Signed-off-by: Stephane Eranian <eranian@xxxxxxxxxx>
>>
>> --
>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
>> index a4fa381..95137b6 100644
>> --- a/kernel/perf_event.c
>> +++ b/kernel/perf_event.c
>> @@ -3035,8 +3035,10 @@ int perf_output_begin(struct perf_output_handle *handle,
>>    handle->nmi   = nmi;
>> Â Â Â handle->sample Â= sample;
>>
>> - Â Â if (!data->nr_pages)
>> - Â Â Â Â Â Â goto fail;
>> + Â Â if (!data->nr_pages) {
>> + Â Â Â Â Â Â atomic_inc(&data->lost);
>> + Â Â Â Â Â Â goto out;
>> + Â Â }
>
>
>
> Oh indeed, handle->lock is in a random state.
> Whatever its value we have an unbalanced put_cpu()
> anyway.
>
Yes, I caught this problem while debugging the perf record -c
issue.

> And we don't race with someone else, data->lock = -1
> and will then warn.
>
> I just have a tiny doubt: should we really count this
> path to the lost events? I'm not sure when we can have
> data->nr_pages == 0, does this happen if we mmap after
> enabling the event?
>
Well, nr_pages = 0 means all you have is the sampling buffer
header page. You cannot save any sample, so you  actually
loose everything. Perf reports a dozen samples but those
are synthetic. The kernel cannot reject an mmap() for
just one page because of the remapped counters, thus it
has to handle this gracefully and at least report the lost
samples.

> All I know is that I observed I already lost events in
> this path using perf lock.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/