[PATCH 0/2] Split reading the trace.dat options from trace data

[PATCH 0/2] Split reading the trace.dat options from trace data

[Tzvetomir Stoyanov (VMware)]

Add functionality which allows to implement opening a trace file on
stages - reading the trace headers and reading the trace data.

Tzvetomir Stoyanov (VMware) (2):
  trace-cmd: Move reading of trace.dat options to
    tracecmd_read_headers()
  trace-cmd: Add new API tracecmd_open_head()

 include/trace-cmd/trace-cmd.h |   2 +
 lib/trace-cmd/trace-input.c   | 117 +++++++++++++++++++++++++---------
 2 files changed, 90 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH 1/2] trace-cmd: Move reading of trace.dat options to tracecmd_read_headers()

[Tzvetomir Stoyanov (VMware)]

The options section from trace.dat file is logically part of trace headers.
In some use cases it is useful first to read all headers from the file,
analyze the information and do some processing, before reading the tracing
data. In the current implementation, reading of options is just before
reading the tracing data.
Moved the reading of options and a CPU count from read_cpu_data() to
tracecmd_read_headers(). This allows to implement APIs for trace.dat
file reading on stages - headers stage and tracing data stage.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/trace-cmd/trace-cmd.h |  1 +
 lib/trace-cmd/trace-input.c   | 89 +++++++++++++++++++++++------------
 2 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 595919d5..3f96bbde 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -119,6 +119,7 @@ enum {
 	TRACECMD_FL_BUFFER_INSTANCE	= (1 << 1),
 	TRACECMD_FL_LATENCY		= (1 << 2),
 	TRACECMD_FL_IN_USECS		= (1 << 3),
+	TRACECMD_FL_FLYRECORD		= (1 << 4),
 };
 
 struct tracecmd_ftrace {
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index f04528ae..ee9bfb52 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -141,6 +141,8 @@ struct tracecmd_input {
 
 __thread struct tracecmd_input *tracecmd_curr_thread_handle;
 
+static int read_options_type(struct tracecmd_input *handle);
+
 void tracecmd_set_flag(struct tracecmd_input *handle, int flag)
 {
 	handle->flags |= flag;
@@ -747,6 +749,19 @@ int tracecmd_get_parsing_failures(struct tracecmd_input *handle)
 	return 0;
 }
 
+static int read_cpus(struct tracecmd_input *handle)
+{
+	unsigned int cpus;
+
+	if (read4(handle, &cpus) < 0)
+		return -1;
+
+	handle->cpus = cpus;
+	tep_set_cpus(handle->pevent, handle->cpus);
+
+	return 0;
+}
+
 /**
  * tracecmd_read_headers - read the header information from trace.dat
  * @handle: input handle for the trace.dat file
@@ -784,6 +799,12 @@ int tracecmd_read_headers(struct tracecmd_input *handle)
 	if (read_and_parse_cmdlines(handle) < 0)
 		return -1;
 
+	if (read_cpus(handle) < 0)
+		return -1;
+
+	if (read_options_type(handle) < 0)
+		return -1;
+
 	tep_set_long_size(handle->pevent, handle->long_size);
 
 	return 0;
@@ -2576,23 +2597,13 @@ static int handle_options(struct tracecmd_input *handle)
 	return 0;
 }
 
-static int read_cpu_data(struct tracecmd_input *handle)
+static int read_options_type(struct tracecmd_input *handle)
 {
-	struct tep_handle *pevent = handle->pevent;
-	enum kbuffer_long_size long_size;
-	enum kbuffer_endian endian;
-	unsigned long long size;
-	unsigned long long max_size = 0;
-	unsigned long long pages;
 	char buf[10];
-	int cpus;
-	int cpu;
 
 	if (do_read_check(handle, buf, 10))
 		return -1;
 
-	cpus = handle->cpus;
-
 	/* check if this handles options */
 	if (strncmp(buf, "options", 7) == 0) {
 		if (handle_options(handle) < 0)
@@ -2602,17 +2613,41 @@ static int read_cpu_data(struct tracecmd_input *handle)
 	}
 
 	/*
-	 * Check if this is a latency report or not.
+	 * Check if this is a latency report or flyrecord.
 	 */
-	if (strncmp(buf, "latency", 7) == 0) {
+	if (strncmp(buf, "latency", 7) == 0)
 		handle->flags |= TRACECMD_FL_LATENCY;
+	else if (strncmp(buf, "flyrecord", 9) == 0)
+		handle->flags |= TRACECMD_FL_FLYRECORD;
+	else
+		return -1;
+
+	return 0;
+}
+
+static int read_cpu_data(struct tracecmd_input *handle)
+{
+	struct tep_handle *pevent = handle->pevent;
+	enum kbuffer_long_size long_size;
+	enum kbuffer_endian endian;
+	unsigned long long size;
+	unsigned long long max_size = 0;
+	unsigned long long pages;
+	int cpus;
+	int cpu;
+
+	/*
+	 * Check if this is a latency report or not.
+	 */
+	if (handle->flags & TRACECMD_FL_LATENCY)
 		return 1;
-	}
 
 	/* We expect this to be flyrecord */
-	if (strncmp(buf, "flyrecord", 9) != 0)
+	if (!(handle->flags & TRACECMD_FL_FLYRECORD))
 		return -1;
 
+	cpus = handle->cpus;
+
 	handle->cpu_data = malloc(sizeof(*handle->cpu_data) * handle->cpus);
 	if (!handle->cpu_data)
 		return -1;
@@ -2795,15 +2830,8 @@ static int read_and_parse_trace_clock(struct tracecmd_input *handle,
 int tracecmd_init_data(struct tracecmd_input *handle)
 {
 	struct tep_handle *pevent = handle->pevent;
-	unsigned int cpus;
 	int ret;
 
-	if (read4(handle, &cpus) < 0)
-		return -1;
-	handle->cpus = cpus;
-
-	tep_set_cpus(pevent, handle->cpus);
-
 	ret = read_cpu_data(handle);
 	if (ret < 0)
 		return ret;
@@ -3579,25 +3607,28 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
 	if (ret < 0) {
 		warning("could not seek to buffer %s offset %ld\n",
 			buffer->name, buffer->offset);
-		tracecmd_close(new_handle);
-		return NULL;
+		goto error;
 	}
 
-	ret = read_cpu_data(new_handle);
+	ret = read_options_type(new_handle);
+	if (!ret)
+		ret = read_cpu_data(new_handle);
 	if (ret < 0) {
 		warning("failed to read sub buffer %s\n", buffer->name);
-		tracecmd_close(new_handle);
-		return NULL;
+		goto error;
 	}
 
 	ret = lseek64(handle->fd, offset, SEEK_SET);
 	if (ret < 0) {
 		warning("could not seek to back to offset %ld\n", offset);
-		tracecmd_close(new_handle);
-		return NULL;
+		goto error;
 	}
 
 	return new_handle;
+
+error:
+	tracecmd_close(new_handle);
+	return NULL;
 }
 
 int tracecmd_is_buffer_instance(struct tracecmd_input *handle)
-- 
2.25.1

[PATCH 2/2] trace-cmd: Add new API tracecmd_open_head()

[Tzvetomir Stoyanov (VMware)]

Add an API to create a tracecmd_handle from a file,
read and parse only the trace headers from the file.
This allows to implement opening a trace file on
stages - reading the trace headers and reading the trace data.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/trace-cmd/trace-cmd.h |  1 +
 lib/trace-cmd/trace-input.c   | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 3f96bbde..e22aa251 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -143,6 +143,7 @@ typedef void (*tracecmd_handle_init_func)(struct tracecmd_input *handle,
 struct tracecmd_input *tracecmd_alloc(const char *file);
 struct tracecmd_input *tracecmd_alloc_fd(int fd);
 struct tracecmd_input *tracecmd_open(const char *file);
+struct tracecmd_input *tracecmd_open_head(const char *file);
 struct tracecmd_input *tracecmd_open_fd(int fd);
 void tracecmd_ref(struct tracecmd_input *handle);
 void tracecmd_close(struct tracecmd_input *handle);
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index ee9bfb52..efc8d4bd 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3175,6 +3175,34 @@ struct tracecmd_input *tracecmd_open(const char *file)
 	return tracecmd_open_fd(fd);
 }
 
+/**
+ * tracecmd_open_head - create a tracecmd_handle from a given file, read
+ *			read and parse only the trace headers from the file
+ * @file: the file name of the file that is of tracecmd data type.
+ */
+struct tracecmd_input *tracecmd_open_head(const char *file)
+{
+	struct tracecmd_input *handle;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+
+	handle = tracecmd_alloc_fd(fd);
+	if (!handle)
+		return NULL;
+
+	if (tracecmd_read_headers(handle) < 0)
+		goto fail;
+
+	return handle;
+
+fail:
+	tracecmd_close(handle);
+	return NULL;
+}
+
 /**
  * tracecmd_ref - add a reference to the handle
  * @handle: input handle for the trace.dat file
-- 
2.25.1

Re: [PATCH 2/2] trace-cmd: Add new API tracecmd_open_head()

[Steven Rostedt]

On Thu,  9 Apr 2020 16:28:25 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Add an API to create a tracecmd_handle from a file,
> read and parse only the trace headers from the file.
> This allows to implement opening a trace file on
> stages - reading the trace headers and reading the trace data.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  include/trace-cmd/trace-cmd.h |  1 +
>  lib/trace-cmd/trace-input.c   | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index 3f96bbde..e22aa251 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -143,6 +143,7 @@ typedef void (*tracecmd_handle_init_func)(struct tracecmd_input *handle,
>  struct tracecmd_input *tracecmd_alloc(const char *file);
>  struct tracecmd_input *tracecmd_alloc_fd(int fd);
>  struct tracecmd_input *tracecmd_open(const char *file);
> +struct tracecmd_input *tracecmd_open_head(const char *file);
>  struct tracecmd_input *tracecmd_open_fd(int fd);
>  void tracecmd_ref(struct tracecmd_input *handle);
>  void tracecmd_close(struct tracecmd_input *handle);

Hi Tzvetomir,

I was thinking, before we release libtracecmd, we should add
tracecmd_open() and tracecmd_open_fd() to the library. Just because I find
that tracecmd_open_head() is a awkward name to have alone and only makes
sense if there's already a tracecmd_open(). It will also help with
differentiating the different functions.

I know we were going to only have functions that kernelshark uses, but that
may be a little too limiting.

Thanks!

-- Steve

Re: [PATCH 2/2] trace-cmd: Add new API tracecmd_open_head()

[Tzvetomir Stoyanov]

On Thu, Jan 14, 2021 at 12:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu,  9 Apr 2020 16:28:25 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > Add an API to create a tracecmd_handle from a file,
> > read and parse only the trace headers from the file.
> > This allows to implement opening a trace file on
> > stages - reading the trace headers and reading the trace data.
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  include/trace-cmd/trace-cmd.h |  1 +
> >  lib/trace-cmd/trace-input.c   | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> > index 3f96bbde..e22aa251 100644
> > --- a/include/trace-cmd/trace-cmd.h
> > +++ b/include/trace-cmd/trace-cmd.h
> > @@ -143,6 +143,7 @@ typedef void (*tracecmd_handle_init_func)(struct tracecmd_input *handle,
> >  struct tracecmd_input *tracecmd_alloc(const char *file);
> >  struct tracecmd_input *tracecmd_alloc_fd(int fd);
> >  struct tracecmd_input *tracecmd_open(const char *file);
> > +struct tracecmd_input *tracecmd_open_head(const char *file);
> >  struct tracecmd_input *tracecmd_open_fd(int fd);
> >  void tracecmd_ref(struct tracecmd_input *handle);
> >  void tracecmd_close(struct tracecmd_input *handle);
>
> Hi Tzvetomir,
>
> I was thinking, before we release libtracecmd, we should add
> tracecmd_open() and tracecmd_open_fd() to the library. Just because I find
> that tracecmd_open_head() is a awkward name to have alone and only makes
> sense if there's already a tracecmd_open(). It will also help with
> differentiating the different functions.

I'll add tracecmd_open() and tracecmd_open_fd() as APIs. The reason
why tracecmd_open_head()
is used by KernelShark is that tracecmd_open() reads headers + tracing
data and calculates the
timestamps. Thus makes it hard to open multiple tracing files from a
single trace session and adjust
timestamps to be in a same time space. The current KernelShark open flow is:
 - Open a trace file and parse only the headers with tracecmd_open_head()
 - Based on the metadata information from the headers, check if there
is already an opened file from
  the same tracing session. If such is found, pair both files with
tracecmd_pair_peer(), before reading
  the tracing data.
 - Read the tracing data from the file with tracecmd_init_data(). This
will adjust timestamps accordingly,
   if there is a paired peer to the file.

We can rename tracecmd_open_head() to something else, or redesign the
whole flow of opening trace files

>
> I know we were going to only have functions that kernelshark uses, but that
> may be a little too limiting.
>
> Thanks!
>
> -- Steve



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