On Fri, Sep 06, 2013 at 01:37:01PM -0600, David Ahern wrote:
> When processing events the session code has an ordered samples queue which is
> used to time-sort events coming in across multiple mmaps. At a later point in
> time samples on the queue are flushed up to some timestamp at which point the
> event is actually processed.
>
> When analyzing events live (ie., record/analysis path in the same command)
> there is a race that leads to corrupted events and parse errors which cause
> perf to terminate. The problem is that when the event is placed in the ordered
> samples queue it is only a reference to the event which is really sitting in
> the mmap buffer. Even though the event is queued for later processing the mmap
> tail pointer is updated which indicates to the kernel that the event has been
> processed. The race is flushing the event from the queue before it gets
> overwritten by some other event. For commands trying to process events live
> (versus just writing to a file) and processing a high rate of events this
> leads
> to parse failures and perf terminates.
>
> Examples hitting this problem are 'perf kvm stat live', especially with nested
> VMs which generate 100,000+ traces per second, and a command processing
> scheduling events with a high rate of context switching -- e.g., running
> 'perf bench sched pipe'.
>
> This patch offers live commands an option to copy the event when it is placed
> in
> the ordered samples queue.
>
> Signed-off-by: David Ahern <dsah...@gmail.com>
> Cc: Frederic Weisbecker <fweis...@gmail.com>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: Jiri Olsa <jo...@redhat.com>
> Cc: Mike Galbraith <efa...@gmx.de>
> Cc: Namhyung Kim <namhy...@kernel.org>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Stephane Eranian <eran...@google.com>
> ---
> tools/perf/util/session.c | 17 ++++++++++++++---
> tools/perf/util/session.h | 1 +
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 1b185ca..71f16db 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -483,6 +483,8 @@ static void perf_session_free_sample_buffers(struct
> perf_session *session)
>
> sq = list_entry(os->to_free.next, struct sample_queue, list);
> list_del(&sq->list);
> + if (session->copy_on_queue)
> + free(sq->event);
> free(sq);
> }
> }
> @@ -513,11 +515,15 @@ static int flush_sample_queue(struct perf_session *s,
> break;
>
> ret = perf_evlist__parse_sample(s->evlist, iter->event,
> &sample);
> - if (ret)
> + if (ret) {
> pr_err("Can't parse sample, err = %d\n", ret);
> - else {
> + if (s->copy_on_queue)
> + free(iter->event);
> + } else {
> ret = perf_session_deliver_event(s, iter->event,
> &sample, tool,
> iter->file_offset);
> + if (s->copy_on_queue)
> + free(iter->event);
> if (ret)
> return ret;
> }
> @@ -676,7 +682,12 @@ int perf_session_queue_event(struct perf_session *s,
> union perf_event *event,
>
> new->timestamp = timestamp;
> new->file_offset = file_offset;
> - new->event = event;
> +
> + if (s->copy_on_queue) {
> + new->event = malloc(event->header.size);
> + memcpy(new->event, event, event->header.size);
> + } else
> + new->event = event;
>
> __queue_event(new, s);
>
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index 3aa75fb..4adfcbb 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -38,6 +38,7 @@ struct perf_session {
> bool fd_pipe;
> bool repipe;
> struct ordered_samples ordered_samples;
> + bool copy_on_queue;
So do you think it should stay optional? This looks like a global problem, I
mean
the event can be unmapped anytime for any builtin tool mapping it, right?
Also we already allocate the sample list node (struct sample_queue) from
os->sample
buffer. ie: we have our own allocator there.
Probably we should reuse that and include the copied event space in "struct
sample_queue"?
Also looking at it now, it seems we have a bug on the existing code:
if (!list_empty(sc)) {
new = list_entry(sc->next, struct sample_queue, list);
list_del(&new->list);
} else if (os->sample_buffer) {
new = os->sample_buffer + os->sample_buffer_idx;
if (++os->sample_buffer_idx == MAX_SAMPLE_BUFFER)
os->sample_buffer = NULL;
} else {
os->sample_buffer = malloc(MAX_SAMPLE_BUFFER * sizeof(*new));
if (!os->sample_buffer)
return -ENOMEM;
list_add(&os->sample_buffer->list, &os->to_free);
os->sample_buffer_idx = 2;
new = os->sample_buffer + 1;
}
If we actually run out of buffer rooms, we should realloc right after and not
wait for the next entry, otherwise we loose an event:
if (!list_empty(sc)) {
new = list_entry(sc->next, struct sample_queue, list);
list_del(&new->list);
} else {
if (os->sample_buffer) {
new = os->sample_buffer + os->sample_buffer_idx;
if (++os->sample_buffer_idx == MAX_SAMPLE_BUFFER)
os->sample_buffer = NULL;
}
if (!os->sample_buffer) {
os->sample_buffer = malloc(MAX_SAMPLE_BUFFER *
sizeof(*new));
if (!os->sample_buffer)
return -ENOMEM;
list_add(&os->sample_buffer->list, &os->to_free);
os->sample_buffer_idx = 2;
new = os->sample_buffer + 1;
}
Although the mirrored os->sample_buffer condition check is a bit ugly and
should move to
a function. But the idea is there.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
