diff mbox series

[v2,20/87] trace-cmd library: Compress part of the trace file

Message ID 20210729050959.12263-21-tz.stoyanov@gmail.com (mailing list archive)
State Superseded
Headers show
Series Trace file version 7 | expand

Commit Message

Tzvetomir Stoyanov (VMware) July 29, 2021, 5:08 a.m. UTC 
Compress part of the trace.dat file metadata. If there is compression
support, compress these parts of the file:
 - ftrace events format
 - format of recorded events
 - information of the mapping of function addresses to the function names
 - trace_printk() format strings
 - information of the mapping a PID to a process name

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-output.c | 130 +++++++++++++++++++++++++----------
 1 file changed, 92 insertions(+), 38 deletions(-)

Comments

Steven Rostedt Aug. 5, 2021, 9:27 p.m. UTC | #1 
On Thu, 29 Jul 2021 08:08:52 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> @@ -384,26 +384,30 @@ static int read_header_files(struct tracecmd_output *handle)
>  	if (!path)
>  		return -1;
>  
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = stat(path, &st);
>  	if (ret < 0) {


> -
> +	if (compress && out_compression_end(handle))
> +		goto out_close;
>  	handle->file_state = TRACECMD_FILE_HEADERS;
>  


> -
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = copy_event_system(handle, systems);
> -
> +	if (compress) {
> +		if (!ret)
> +			ret = out_compression_end(handle);
> +		else
> +			out_compression_reset(handle);
> +	}


> -
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = -1;
>  	endian4 = convert_endian_4(handle, count);
>  	if (do_write_check(handle, &endian4, 4))
> @@ -763,9 +777,19 @@ static int read_event_files(struct tracecmd_output *handle,
>  		}
>  		ret = copy_event_system(handle, slist);
>  	}
> -
> -	handle->file_state = TRACECMD_FILE_ALL_EVENTS;
> +	if (ret)
> +		goto out_free;
> +	if (compress) {
> +		ret = out_compression_end(handle);
> +		if (ret)
> +			goto out_free;
> +	}


> @@ -827,19 +851,21 @@ static int read_proc_kallsyms(struct tracecmd_output *handle)
>  	if (handle->kallsyms)
>  		path = handle->kallsyms;
>  
> +	if (compress)
> +		out_compression_start(handle);


> -
> -	return 0;
> +	if (compress) {
> +		ret = out_compression_end(handle);
> +		if (ret)
> +			goto out;
> +	}

>  
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = stat(path, &st);
>  	if (ret < 0) {
>  		/* not found */
> @@ -894,11 +931,14 @@ static int read_ftrace_printk(struct tracecmd_output *handle)
>  	}
>  
>   out:
> -	handle->file_state = TRACECMD_FILE_PRINTK;
>  	put_tracing_file(path);
> +	if (compress && out_compression_end(handle))
> +		return -1;
> +	handle->file_state = TRACECMD_FILE_PRINTK;

>  	}
> +
> +	if (handle->compress)
> +		out_compression_start(handle);
> +
>  	ret = save_tracing_file_data(handle, "saved_cmdlines");
> -	if (ret < 0)
> +	if (ret < 0) {
> +		out_compression_reset(handle);
>  		return ret;
> +	}
> +
> +	if (handle->compress && out_compression_end(handle))
> +		return -1;
> +
>  	handle->file_state = TRACECMD_FILE_CMD_LINES;
>  	return 0;
>  }


Pretty much all the out_compression_*() functions are enclosed in a:

	if (compress)
		out_compress_*()

condition.

Why not just add that to the parameter, and clean up the code where
this is used, because these branches are rather ugly and break up the
flow.

For example:

__hidden void out_compression_reset(struct tracecmd_output *handle, bool compress)
{
        if (!compress || !handle->compress)
                return;
        tracecmd_compress_reset(handle->compress);
        handle->do_compress = false;
}

__hidden int out_uncompress_block(struct tracecmd_output *handle, bool compress)
{
        int ret = 0;

        if (!compress || !handle->compress)
                return 0;
        ret = tracecmd_uncompress_block(handle->compress);
        if (!ret)
                handle->do_compress = true;
        return ret;
}

__hidden int out_compression_start(struct tracecmd_output *handle, bool compress)
{
        if (!compress || !handle->compress)
                return 0;
        tracecmd_compress_reset(handle->compress);
        handle->do_compress = true;
        return 0;
}

__hidden int out_compression_end(struct tracecmd_output *handle, bool compress)
{
        if (!compress || !handle->compress)
                return 0;
        handle->do_compress = false;
        return tracecmd_compress_block(handle->compress);
}

Then instead of all these:

        if (compress)
                out_compression_start(handle);

You just have:

        out_compression_start(handle, compress);


-- Steve
diff mbox series

Patch

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index d348d6a5..5d3fd58f 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -366,12 +366,12 @@  int tracecmd_ftrace_enable(int set)
 	return ret;
 }
 
-static int read_header_files(struct tracecmd_output *handle)
+static int read_header_files(struct tracecmd_output *handle, bool compress)
 {
 	tsize_t size, check_size, endian8;
 	struct stat st;
 	char *path;
-	int fd;
+	int fd = -1;
 	int ret;
 
 	if (!check_out_state(handle, TRACECMD_FILE_HEADERS)) {
@@ -384,26 +384,30 @@  static int read_header_files(struct tracecmd_output *handle)
 	if (!path)
 		return -1;
 
+	if (compress)
+		out_compression_start(handle);
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* old style did not show this info, just add zero */
 		put_tracing_file(path);
 		if (do_write_check(handle, "header_page", 12))
-			return -1;
+			goto out_close;
 		size = 0;
 		if (do_write_check(handle, &size, 8))
-			return -1;
+			goto out_close;
 		if (do_write_check(handle, "header_event", 13))
-			return -1;
+			goto out_close;
 		if (do_write_check(handle, &size, 8))
-			return -1;
+			goto out_close;
+		if (compress && out_compression_end(handle))
+			goto out_close;
 		return 0;
 	}
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-		return -1;
+		goto out_close;
 	}
 
 	/* unfortunately, you can not stat debugfs files for size */
@@ -419,18 +423,18 @@  static int read_header_files(struct tracecmd_output *handle)
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s' size=%lld read=%lld", path, size, check_size);
 		errno = EINVAL;
-		return -1;
+		goto out_close;
 	}
 	put_tracing_file(path);
 
 	path = get_tracing_file(handle, "events/header_event");
 	if (!path)
-		return -1;
+		goto out_close;
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-		return -1;
+		goto out_close;
 	}
 
 	size = get_size_fd(fd);
@@ -444,16 +448,19 @@  static int read_header_files(struct tracecmd_output *handle)
 	close(fd);
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s'", path);
-		return -1;
+		goto out_close;
 	}
 	put_tracing_file(path);
-
+	if (compress && out_compression_end(handle))
+		goto out_close;
 	handle->file_state = TRACECMD_FILE_HEADERS;
 
 	return 0;
 
  out_close:
-	close(fd);
+	out_compression_reset(handle);
+	if (fd >= 0)
+		close(fd);
 	return -1;
 }
 
@@ -680,7 +687,7 @@  create_event_list_item(struct tracecmd_output *handle,
 	 tracecmd_warning("Insufficient memory");
 }
 
-static int read_ftrace_files(struct tracecmd_output *handle)
+static int read_ftrace_files(struct tracecmd_output *handle, bool compress)
 {
 	struct list_event_system *systems = NULL;
 	struct tracecmd_event_list list = { .glob = "ftrace/*" };
@@ -693,9 +700,15 @@  static int read_ftrace_files(struct tracecmd_output *handle)
 	}
 
 	create_event_list_item(handle, &systems, &list);
-
+	if (compress)
+		out_compression_start(handle);
 	ret = copy_event_system(handle, systems);
-
+	if (compress) {
+		if (!ret)
+			ret = out_compression_end(handle);
+		else
+			out_compression_reset(handle);
+	}
 	free_list_events(systems);
 
 	handle->file_state = TRACECMD_FILE_FTRACE_EVENTS;
@@ -717,7 +730,7 @@  create_event_list(struct tracecmd_output *handle,
 }
 
 static int read_event_files(struct tracecmd_output *handle,
-			    struct tracecmd_event_list *event_list)
+			    struct tracecmd_event_list *event_list, bool compress)
 {
 	struct list_event_system *systems;
 	struct list_event_system *slist;
@@ -748,7 +761,8 @@  static int read_event_files(struct tracecmd_output *handle,
 
 	for (slist = systems; slist; slist = slist->next)
 		count++;
-
+	if (compress)
+		out_compression_start(handle);
 	ret = -1;
 	endian4 = convert_endian_4(handle, count);
 	if (do_write_check(handle, &endian4, 4))
@@ -763,9 +777,19 @@  static int read_event_files(struct tracecmd_output *handle,
 		}
 		ret = copy_event_system(handle, slist);
 	}
-
-	handle->file_state = TRACECMD_FILE_ALL_EVENTS;
+	if (ret)
+		goto out_free;
+	if (compress) {
+		ret = out_compression_end(handle);
+		if (ret)
+			goto out_free;
+	}
  out_free:
+	if (!ret)
+		handle->file_state = TRACECMD_FILE_ALL_EVENTS;
+	else
+		out_compression_reset(handle);
+
 	free_list_events(systems);
 
 	return ret;
@@ -811,7 +835,7 @@  err:
 		tracecmd_warning("can't set kptr_restrict");
 }
 
-static int read_proc_kallsyms(struct tracecmd_output *handle)
+static int read_proc_kallsyms(struct tracecmd_output *handle, bool compress)
 {
 	unsigned int size, check_size, endian4;
 	const char *path = "/proc/kallsyms";
@@ -827,19 +851,21 @@  static int read_proc_kallsyms(struct tracecmd_output *handle)
 	if (handle->kallsyms)
 		path = handle->kallsyms;
 
+	if (compress)
+		out_compression_start(handle);
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* not found */
 		size = 0;
 		endian4 = convert_endian_4(handle, size);
-		if (do_write_check(handle, &endian4, 4))
-			return -1;
-		return 0;
+		ret = do_write_check(handle, &endian4, 4);
+		goto out;
 	}
 	size = get_size(path);
 	endian4 = convert_endian_4(handle, size);
-	if (do_write_check(handle, &endian4, 4))
-		return -1;
+	ret = do_write_check(handle, &endian4, 4);
+	if (ret)
+		goto out;
 
 	set_proc_kptr_restrict(0);
 	check_size = copy_file(handle, path);
@@ -847,16 +873,25 @@  static int read_proc_kallsyms(struct tracecmd_output *handle)
 		errno = EINVAL;
 		tracecmd_warning("error in size of file '%s'", path);
 		set_proc_kptr_restrict(1);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 	set_proc_kptr_restrict(1);
 
-	handle->file_state = TRACECMD_FILE_KALLSYMS;
-
-	return 0;
+	if (compress) {
+		ret = out_compression_end(handle);
+		if (ret)
+			goto out;
+	}
+out:
+	if (!ret)
+		handle->file_state = TRACECMD_FILE_KALLSYMS;
+	else
+		out_compression_reset(handle);
+	return ret;
 }
 
-static int read_ftrace_printk(struct tracecmd_output *handle)
+static int read_ftrace_printk(struct tracecmd_output *handle, bool compress)
 {
 	unsigned int size, check_size, endian4;
 	struct stat st;
@@ -873,6 +908,8 @@  static int read_ftrace_printk(struct tracecmd_output *handle)
 	if (!path)
 		return -1;
 
+	if (compress)
+		out_compression_start(handle);
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* not found */
@@ -894,11 +931,14 @@  static int read_ftrace_printk(struct tracecmd_output *handle)
 	}
 
  out:
-	handle->file_state = TRACECMD_FILE_PRINTK;
 	put_tracing_file(path);
+	if (compress && out_compression_end(handle))
+		return -1;
+	handle->file_state = TRACECMD_FILE_PRINTK;
 	return 0;
  fail:
 	put_tracing_file(path);
+	out_compression_reset(handle);
 	return -1;
 }
 
@@ -1270,21 +1310,25 @@  int tracecmd_output_write_init(struct tracecmd_output *handler)
 int tracecmd_output_write_headers(struct tracecmd_output *handler,
 				  struct tracecmd_event_list *list)
 {
+	bool compress = false;
+
 	if (!handler || handler->file_state < TRACECMD_FILE_ALLOCATED)
 		return -1;
 
 	/* Write init data, if not written yet */
 	if (handler->file_state < TRACECMD_FILE_INIT && tracecmd_output_write_init(handler))
 		return -1;
-	if (read_header_files(handler))
+	if (handler->compress)
+		compress = true;
+	if (read_header_files(handler, compress))
 		return -1;
-	if (read_ftrace_files(handler))
+	if (read_ftrace_files(handler, compress))
 		return -1;
-	if (read_event_files(handler, list))
+	if (read_event_files(handler, list, compress))
 		return -1;
-	if (read_proc_kallsyms(handler))
+	if (read_proc_kallsyms(handler, compress))
 		return -1;
-	if (read_ftrace_printk(handler))
+	if (read_ftrace_printk(handler, compress))
 		return -1;
 	return 0;
 }
@@ -1529,9 +1573,19 @@  int tracecmd_write_cmdlines(struct tracecmd_output *handle)
 				 handle->file_state);
 		return -1;
 	}
+
+	if (handle->compress)
+		out_compression_start(handle);
+
 	ret = save_tracing_file_data(handle, "saved_cmdlines");
-	if (ret < 0)
+	if (ret < 0) {
+		out_compression_reset(handle);
 		return ret;
+	}
+
+	if (handle->compress && out_compression_end(handle))
+		return -1;
+
 	handle->file_state = TRACECMD_FILE_CMD_LINES;
 	return 0;
 }