Re: [PATCH] perf tools: Fix tracing info recording

Frederic Weisbecker
2011-09-21T15:30:33Z

On Wed, Sep 14, 2011 at 03:58:40PM +0200, Jiri Olsa wrote:
> 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.
>
> Fixing this behaviour by using temp file in case of pipe output. The
> debugfs/proc files are being read only once, ensuring the integrity of
> the tracing data.
>
> Also changing the way the event files are searched by quering specified
> event files directly, instead of walking the events directory.
>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Looks good overall, but I have some comments about details:

> ---
> tools/perf/util/header.c | 39 ++++-
> tools/perf/util/parse-events.h | 1 +
> tools/perf/util/trace-event-info.c | 333 +++++++++++++++++++-----------------
> tools/perf/util/trace-event.h | 7 +
> 4 files changed, 215 insertions(+), 165 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index b6c1ad1..6a9fd5b 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -387,13 +387,21 @@ static int perf_header__adds_write(struct perf_header *header,
>
> 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, we can write data
> + * directly, not temp file is needed.

s/not/no

May be also briefly explain why we can write directly here. The fact
we have reserved space to write the size in the headers already.

> + */
> + tdata = tracing_data_get(&evlist->entries, fd, false);
> + if (!tdata)
> + goto out_free;
> +
> + trace_sec->size = tracing_data_size(tdata);
> + tracing_data_put(tdata);
> }
>
> if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
> @@ -1100,15 +1108,25 @@ 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;
>
> + /*
> + * The fd descriptor is pipe, se we need to:
> + * - 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
> + */

That also needs a brief explanation on why we need to do that.

> + 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 = tracing_data_size(tdata);
> aligned_size = ALIGN(size, sizeof(u64));
> padding = aligned_size - size;
> ev.tracing_data.header.size = sizeof(ev.tracing_data);
<snip>
> -int read_tracing_data(int fd, struct list_head *pattrs)
> +#define FILENAME_SIZE 50
> +
> +struct tracing_data {
> + ssize_t size;
> + bool temp;
> + char temp_file[FILENAME_SIZE];
> +};
> +
> +ssize_t tracing_data_size(struct tracing_data *td)
> {
> - char buf[BUFSIZ];
> - struct tracepoint_path *tps = get_tracepoints_path(pattrs);
> + return td->size;
> +}

May be inline it. Or rather directly access td->size from calling code.

>
> - /*
> - * What? No tracepoints? No sense writing anything here, bail out.
> - */
> - if (tps == NULL)
> - return -1;
> +static char *get_format_file(char *sys, char *name)
> +{
> + char *file, *path;
>
> - output_fd = fd;
> + path = get_tracing_file("events");
> + if (!path)
> + return NULL;
> +
> + file = malloc_or_die(strlen(path) + strlen(sys) +
> + strlen(name) + strlen("format") + 10);

Why "10" ?

> +
> + sprintf(file, "%s/%s/%s/format", path, sys, name);
> + return file;
> +}
> +
> +static void put_format_file(char *file)
> +{
> + free(file);
> +}
> +
> +/*
> + * Walk tracepoint event objects and store them.
> + * Only those matching the sys parameter are stored
> + * and marked as done.
> + */
> +static void read_event_files_system(struct tracepoint_path *tps,
> + const char *sys)
> +{
> + off_t count_pos;
> + u32 count = 0;
> +
> + /* specified events count under single system */
> + count_pos = lseek(output_fd, 0, SEEK_CUR);
> + write_or_die(&count, 4);
> +
> + while (tps) {
> + if ((!tps->done) &&
> + (!strcmp(sys, tps->system))) {
> + char *file;
> +
> + file = get_format_file(tps->system, tps->name);
> + if (file) {
> + record_file(file, 8);
> + count++;
> + }
> +
> + put_format_file(file);
> + tps->done = 1;
> + }
> +
> + tps = tps->next;
> + }
> +
> + if (pwrite(output_fd, &count, 4, count_pos) < 0)
> + die("pwrite");
> +}
> +
> +/*
> + * We have all needed tracepoint event files stored in
> + * the tracepoint_path objects.
> + *
> + * - first we store ftrace system events
> + * - then we walk all '!done' event objects
> + * and process them
> + */
> +static void read_event_files(struct tracepoint_path *tps)
> +{
> + off_t count_pos;
> + u32 count = 0;
> +
> + read_event_files_system(tps, "ftrace");
> +
> + /* systems count */
> + count_pos = lseek(output_fd, 0, SEEK_CUR);
> + write_or_die(&count, 4);
> +
> + while (tps) {
> + if (!tps->done) {
> + write_or_die(tps->system, strlen(tps->system) + 1);
> + read_event_files_system(tps, tps->system);
> + count++;
> + }
> +
> + tps = tps->next;
> + }
>
> + if (pwrite(output_fd, &count, 4, count_pos) < 0)
> + die("write");
> +}

There seem to be a significant reorganization of the code in that file
that seem to overlap the initial scope of this patch. Should
it go to a seperate single purpose patch?

Thanks.