Re: [PATCH v7 11/25] trace-cmd library: Fit CPU latency trace data in the new trace file version 7 format

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

On Sat, Jan 15, 2022 at 5:20 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:34 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> More nitpicking about spacing ;-)
>
> > Trace file version 7 format is based on sections. To fit the latency
> > trace data in this structure, a new section and option for it is
> > defined:
> >   BUFFER_TEXT
>
> space
>
> > It is similar to the BUFFER section which holds the flyrecord binary
> > data, but has a latency specific design for text data. The BUFFER_TEXT
> > section has:
> >  - section header, as all other sections
> >  - compression of the trace data, optional
> >  - corresponding trace option, pointing to the section
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  .../include/private/trace-cmd-private.h       |  1 +
> >  lib/trace-cmd/trace-output.c                  | 24 ++++++++++++++++---
> >  2 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> > index 047fc26f..d8ee9b74 100644
> > --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> > +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> > @@ -145,6 +145,7 @@ enum {
> >       TRACECMD_OPTION_KALLSYMS,
> >       TRACECMD_OPTION_PRINTK,
> >       TRACECMD_OPTION_CMDLINES,
> > +     TRACECMD_OPTION_BUFFER_TEXT,
> >       TRACECMD_OPTION_MAX,
> >  };
> >
> > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> > index 44050dc8..47227728 100644
> > --- a/lib/trace-cmd/trace-output.c
> > +++ b/lib/trace-cmd/trace-output.c
> > @@ -1874,7 +1874,9 @@ out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
> >
> >  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
> >  {
> > +     enum tracecmd_section_flags flags = 0;
> >       struct tracecmd_output *handle;
> > +     tsize_t offset;
> >       char *path;
> >
> >       handle = tracecmd_output_create(output_file);
> > @@ -1891,7 +1893,8 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
> >
> >       if (tracecmd_write_cpus(handle, cpus) < 0)
> >               goto out_free;
> > -
> > +     if (tracecmd_write_buffer_info(handle) < 0)
> > +             goto out_free;
> >       if (tracecmd_write_options(handle) < 0)
> >               goto out_free;
> >
> > @@ -1901,23 +1904,38 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
> >               goto out_free;
> >       }
> >
> > -     if (do_write_check(handle, "latency  ", 10))
> > +     if (!HAS_SECTIONS(handle) && do_write_check(handle, "latency  ", 10))
> >               goto out_free;
> >
> >       path = get_tracing_file(handle, "trace");
> >       if (!path)
> >               goto out_free;
> >
> > +     offset = do_lseek(handle, 0, SEEK_CUR);
> > +     if (HAS_SECTIONS(handle) &&
> > +         !out_add_buffer_option_v7(handle, "", TRACECMD_OPTION_BUFFER_TEXT, offset, 0, NULL))
> > +             goto out_free;
> > +
> > +     offset = out_write_section_header(handle, TRACECMD_OPTION_BUFFER_TEXT,
> > +                                       "buffer latency", flags, false);
> > +
> >       copy_file(handle, path);
> > +     if (out_update_section_header(handle, offset))
> > +             goto out_free;
> >
> >       put_tracing_file(path);
> >
> >       handle->file_state = TRACECMD_FILE_CPU_LATENCY;
> >
> > +     if (HAS_SECTIONS(handle))
> > +             tracecmd_write_options(handle);
> > +
> >       return handle;
>
>
> >
> >  out_free:
> > -     tracecmd_output_close(handle);
> > +     if (handle)
> > +             tracecmd_output_close(handle);
> > +     unlink(output_file);
>
> Hmm, how does the above play a role in this patch?
>
> That is, what about this new BUFFER_TEXT required this change?
> I mean, output_file is being removed now, but I don't see anything in
> the rest of the patch to warrant that?
>

That looks like a leftover from a previous version of this patch.

> -- Steve
>
>
> >       return NULL;
> >  }
> >
>


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center