Messages in this thread | | | Date | Thu, 13 Oct 2011 16:00:40 +0200 | From | Jiri Olsa <> | Subject | Re: [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/
| |