lkml.org 
[lkml]   [2011]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv3 2/2] perf tools: Fix tracing info recording
    hi,
    any feedback on this?

    I cut off changes colliding with Steven's change,
    now it just fixies the pipe/file issue..

    thanks,
    jirka


    On Thu, Sep 29, 2011 at 05:05:09PM +0200, Jiri Olsa wrote:
    > Fixing the way the tracing information is stored within record command.
    > The current implementation is causing issues for pipe output.
    >
    > Following commands fail currently:
    > perf script syscall-counts ls
    > perf record -e syscalls:sys_exit_read ls | ./perf report -i -
    >
    > The tracing information is part of the perf data file. It contains
    > several files from within the tracing debugfs and procs directories.
    >
    > Beside some static header files, for each tracing event the format
    > file is added. The /proc/kallsyms file is also added.
    >
    > The tracing data are stored with preceeding size. This is causing some
    > dificulties for pipe output, since there's no way to tell debugfs/proc
    > file size before reading it. So, for pipe output, all the debugfs files
    > were read twice. Once to get the overall size and once to store the
    > content itself. This can cause problem in case any of these file
    > changed, within the storage time.
    >
    > To fix this behaviour and ensure the integrity of the tracing data, we:
    > - read debugfs/proc file into the temp file
    > - get temp file size and dump it to the pipe
    > - dump the temp file contents to the pipe
    >
    > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
    > ---
    > tools/perf/util/header.c | 50 +++++++++++++++---
    > tools/perf/util/trace-event-info.c | 102 ++++++++++++++++++++++++++---------
    > tools/perf/util/trace-event.h | 11 +++-
    > 3 files changed, 127 insertions(+), 36 deletions(-)
    >
    > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
    > index b6c1ad1..477c73b 100644
    > --- a/tools/perf/util/header.c
    > +++ b/tools/perf/util/header.c
    > @@ -382,18 +382,33 @@ static int perf_header__adds_write(struct perf_header *header,
    >
    > sec_size = sizeof(*feat_sec) * nr_sections;
    >
    > + /*
    > + * We reserve header space for each section, and
    > + * update/store it later once we know the section size.
    > + */
    > sec_start = header->data_offset + header->data_size;
    > lseek(fd, sec_start + sec_size, SEEK_SET);
    >
    > if (perf_header__has_feat(header, HEADER_TRACE_INFO)) {
    > struct perf_file_section *trace_sec;
    > + struct tracing_data *tdata;
    >
    > trace_sec = &feat_sec[idx++];
    > -
    > - /* Write trace info */
    > trace_sec->offset = lseek(fd, 0, SEEK_CUR);
    > - read_tracing_data(fd, &evlist->entries);
    > - trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
    > +
    > + /*
    > + * We work over the real file, so we can write data
    > + * directly, no temp file is needed.
    > + */
    > + tdata = tracing_data_get(&evlist->entries, fd, false);
    > + if (!tdata)
    > + goto out_free;
    > +
    > + /*
    > + * Update the section header information.
    > + */
    > + trace_sec->size = tdata->size;
    > + tracing_data_put(tdata);
    > }
    >
    > if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
    > @@ -1100,15 +1115,29 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
    > struct perf_session *session __unused)
    > {
    > union perf_event ev;
    > + struct tracing_data *tdata;
    > ssize_t size = 0, aligned_size = 0, padding;
    > int err __used = 0;
    >
    > + /*
    > + * We are going to store the size of the data followed
    > + * by the data contents. Since the fd descriptor is a pipe,
    > + * we cannot seek back to store the size of the data once
    > + * we know it. Instead we:
    > + *
    > + * - write the tracing data to the temp file
    > + * - get/write the data size to pipe
    > + * - write the tracing data from the temp file
    > + * to the pipe
    > + */
    > + tdata = tracing_data_get(&evlist->entries, fd, true);
    > + if (!tdata)
    > + return -1;
    > +
    > memset(&ev, 0, sizeof(ev));
    >
    > ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
    > - size = read_tracing_data_size(fd, &evlist->entries);
    > - if (size <= 0)
    > - return size;
    > + size = tdata->size;
    > aligned_size = ALIGN(size, sizeof(u64));
    > padding = aligned_size - size;
    > ev.tracing_data.header.size = sizeof(ev.tracing_data);
    > @@ -1116,7 +1145,12 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
    >
    > process(&ev, NULL, session);
    >
    > - err = read_tracing_data(fd, &evlist->entries);
    > + /*
    > + * The put function will copy all the tracing data
    > + * stored in temp file to the pipe.
    > + */
    > + tracing_data_put(tdata);
    > +
    > write_padded(fd, NULL, 0, padding);
    >
    > return aligned_size;
    > diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
    > index 3403f81..c9aa4c9 100644
    > --- a/tools/perf/util/trace-event-info.c
    > +++ b/tools/perf/util/trace-event-info.c
    > @@ -196,7 +196,8 @@ static void record_file(const char *file, size_t hdr_sz)
    > die("Can't read '%s'", file);
    >
    > /* put in zeros for file size, then fill true size later */
    > - write_or_die(&size, hdr_sz);
    > + if (hdr_sz)
    > + write_or_die(&size, hdr_sz);
    >
    > do {
    > r = read(fd, buf, BUFSIZ);
    > @@ -212,7 +213,7 @@ static void record_file(const char *file, size_t hdr_sz)
    > if (bigendian())
    > sizep += sizeof(u64) - hdr_sz;
    >
    > - if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
    > + if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
    > die("writing to %s", output_file);
    > }
    >
    > @@ -428,6 +429,19 @@ get_tracepoints_path(struct list_head *pattrs)
    > return nr_tracepoints > 0 ? path.next : NULL;
    > }
    >
    > +static void
    > +put_tracepoints_path(struct tracepoint_path *tps)
    > +{
    > + while (tps) {
    > + struct tracepoint_path *t = tps;
    > +
    > + tps = tps->next;
    > + free(t->name);
    > + free(t->system);
    > + free(t);
    > + }
    > +}
    > +
    > bool have_tracepoints(struct list_head *pattrs)
    > {
    > struct perf_evsel *pos;
    > @@ -439,19 +453,11 @@ bool have_tracepoints(struct list_head *pattrs)
    > return false;
    > }
    >
    > -int read_tracing_data(int fd, struct list_head *pattrs)
    > +static void tracing_data_header(void)
    > {
    > - char buf[BUFSIZ];
    > - struct tracepoint_path *tps = get_tracepoints_path(pattrs);
    > -
    > - /*
    > - * What? No tracepoints? No sense writing anything here, bail out.
    > - */
    > - if (tps == NULL)
    > - return -1;
    > -
    > - output_fd = fd;
    > + char buf[50];
    >
    > + /* just guessing this is someone's birthday.. ;) */
    > buf[0] = 23;
    > buf[1] = 8;
    > buf[2] = 68;
    > @@ -476,28 +482,72 @@ int read_tracing_data(int fd, struct list_head *pattrs)
    > /* save page_size */
    > page_size = sysconf(_SC_PAGESIZE);
    > write_or_die(&page_size, 4);
    > +}
    > +
    > +struct tracing_data *tracing_data_get(struct list_head *pattrs,
    > + int fd, bool temp)
    > +{
    > + struct tracepoint_path *tps;
    > + struct tracing_data *tdata;
    > +
    > + output_fd = fd;
    > +
    > + tps = get_tracepoints_path(pattrs);
    > + if (!tps)
    > + return NULL;
    > +
    > + tdata = malloc_or_die(sizeof(*tdata));
    > + tdata->temp = temp;
    > + tdata->size = 0;
    > +
    > + if (temp) {
    > + int temp_fd;
    > +
    > + snprintf(tdata->temp_file, sizeof(tdata->temp_file),
    > + "/tmp/perf-XXXXXX");
    > + if (!mkstemp(tdata->temp_file))
    > + die("Can't make temp file");
    > +
    > + temp_fd = open(tdata->temp_file, O_RDWR);
    > + if (temp_fd < 0)
    > + die("Can't read '%s'", tdata->temp_file);
    > +
    > + /*
    > + * Set the temp file the default output, so all the
    > + * tracing data are stored into it.
    > + */
    > + output_fd = temp_fd;
    > + } else
    > + tdata->size = -lseek(output_fd, 0, SEEK_CUR);
    >
    > + tracing_data_header();
    > read_header_files();
    > read_ftrace_files(tps);
    > read_event_files(tps);
    > read_proc_kallsyms();
    > read_ftrace_printk();
    >
    > - return 0;
    > -}
    > + tdata->size += lseek(output_fd, 0, SEEK_CUR);
    >
    > -ssize_t read_tracing_data_size(int fd, struct list_head *pattrs)
    > -{
    > - ssize_t size;
    > - int err = 0;
    > + /*
    > + * All tracing data are stored by now, we can restore
    > + * the default output file in case we used temp file.
    > + */
    > + if (temp) {
    > + close(output_fd);
    > + output_fd = fd;
    > + }
    >
    > - calc_data_size = 1;
    > - err = read_tracing_data(fd, pattrs);
    > - size = calc_data_size - 1;
    > - calc_data_size = 0;
    > + put_tracepoints_path(tps);
    > + return tdata;
    > +}
    >
    > - if (err < 0)
    > - return err;
    > +void tracing_data_put(struct tracing_data *tdata)
    > +{
    > + if (tdata->temp) {
    > + record_file(tdata->temp_file, 0);
    > + unlink(tdata->temp_file);
    > + }
    >
    > - return size;
    > + free(tdata);
    > }
    > diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
    > index f674dda..92274b9 100644
    > --- a/tools/perf/util/trace-event.h
    > +++ b/tools/perf/util/trace-event.h
    > @@ -262,8 +262,15 @@ raw_field_value(struct event *event, const char *name, void *data);
    > void *raw_field_ptr(struct event *event, const char *name, void *data);
    > unsigned long long eval_flag(const char *flag);
    >
    > -int read_tracing_data(int fd, struct list_head *pattrs);
    > -ssize_t read_tracing_data_size(int fd, struct list_head *pattrs);
    > +struct tracing_data {
    > + ssize_t size;
    > + bool temp;
    > + char temp_file[50];
    > +};
    > +
    > +struct tracing_data *tracing_data_get(struct list_head *pattrs,
    > + int fd, bool temp);
    > +void tracing_data_put(struct tracing_data *tdata);
    >
    > /* taken from kernel/trace/trace.h */
    > enum trace_flag_type {
    > --
    > 1.7.4
    >
    > --
    > 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/


    \
     
     \ /
      Last update: 2011-10-13 16:03    [W:0.065 / U:0.212 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site