Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754610Ab3JXJan (ORCPT ); Thu, 24 Oct 2013 05:30:43 -0400 Received: from mail-lb0-f178.google.com ([209.85.217.178]:44489 "EHLO mail-lb0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754054Ab3JXJal (ORCPT ); Thu, 24 Oct 2013 05:30:41 -0400 MIME-Version: 1.0 In-Reply-To: <20130914161626.GD1718@localhost.localdomain> References: <1378496221-61525-1-git-send-email-dsahern@gmail.com> <20130914161626.GD1718@localhost.localdomain> Date: Thu, 24 Oct 2013 10:30:39 +0100 Message-ID: Subject: Re: [PATCH] perf session: Add option to copy events when queueing From: Frederic Weisbecker To: David Ahern Cc: Arnaldo Melo , LKML , Ingo Molnar , Jiri Olsa , Mike Galbraith , Namhyung Kim , Peter Zijlstra , Stephane Eranian Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6020 Lines: 132 2013/9/14 Frederic Weisbecker : > 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 >> Cc: Frederic Weisbecker >> Cc: Ingo Molnar >> Cc: Jiri Olsa >> Cc: Mike Galbraith >> Cc: Namhyung Kim >> Cc: Peter Zijlstra >> Cc: Stephane Eranian >> --- >> 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: Bah, checking that again, there don't seem to be a bug there. Actually the sample buffer is reset after we pick the last entry. So it looks all fine. I got confused as usual. Nevermind. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/