linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Few small trace-cmd fixes
@ 2020-05-04  6:27 Tzvetomir Stoyanov (VMware)
  2020-05-04  6:27 ` [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case Tzvetomir Stoyanov (VMware)
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2020-05-04  6:27 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

A couple of trace-cmd bugfixes, good to have in the next trace-cmd
release.

[
 v2 changes:
  - Improve error handling in run_cmd() - clean semaphores on all error
    cases
  - Pass the child PID to update_task_filter() 
]


Tzvetomir Stoyanov (VMware) (4):
  trace-cmd: Fix trace-cmd report crash while displaying trace.dat in
    specific use case
  trace-cmd: Add "main" in the output of trace-cmd stat when displaying
    main instance
  trace-cmd: Create ftrace instances before using them.
  trace-cmd: Do not try to update parent's memory from a fork()-ed child

 lib/trace-cmd/trace-input.c |  2 ++
 tracecmd/trace-record.c     | 68 +++++++++++++++++++++++++++++--------
 tracecmd/trace-stat.c       |  4 ++-
 3 files changed, 59 insertions(+), 15 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case
  2020-05-04  6:27 [PATCH v2 0/4] Few small trace-cmd fixes Tzvetomir Stoyanov (VMware)
@ 2020-05-04  6:27 ` Tzvetomir Stoyanov (VMware)
  2020-05-04 20:00   ` Steven Rostedt
  2020-05-04  6:27 ` [PATCH v2 2/4] trace-cmd: Add "main" in the output of trace-cmd stat when displaying main instance Tzvetomir Stoyanov (VMware)
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2020-05-04  6:27 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

The trace-cmd report command crashes while displaying a file recorded with "--proc-map" and "-B" options:
#trace-cmd record --proc-map  -B test -e sched -F sleep 1
The "--proc-map" options saves the address map of "sleep" into the trace.dat file. This
information is used by KernelShark. The "-B" options traces the specified events into a
ftrace instance "test".
When such file is opened using libtracecmd APIs, the proc-map is parsed and saved into
a tracecmd_input handler, as linked list "pid_maps". Later, when the ftrace instance
"test" is parsed, a copy of this handler is used to fill it with the instance's trace data.
Both tracecmd_input handlers share the same "pid_maps" list, thus leads to a double
free of the list, when  handlers are destroyed.
As this "pid_maps" is not used in ftrace buffers, the "pid_maps" list of the copy can be
initialized to NULL.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 55c3d80a..7583d5cb 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3712,6 +3712,8 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
 
 	new_handle->flags |= TRACECMD_FL_BUFFER_INSTANCE;
 
+	new_handle->pid_maps = NULL;
+
 	/* Save where we currently are */
 	offset = lseek64(handle->fd, 0, SEEK_CUR);
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] trace-cmd: Add "main" in the output of trace-cmd stat when displaying main instance
  2020-05-04  6:27 [PATCH v2 0/4] Few small trace-cmd fixes Tzvetomir Stoyanov (VMware)
  2020-05-04  6:27 ` [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case Tzvetomir Stoyanov (VMware)
@ 2020-05-04  6:27 ` Tzvetomir Stoyanov (VMware)
  2020-05-04 13:41   ` Steven Rostedt
  2020-05-04  6:27 ` [PATCH v2 3/4] trace-cmd: Create ftrace instances before using them Tzvetomir Stoyanov (VMware)
  2020-05-04  6:27 ` [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child Tzvetomir Stoyanov (VMware)
  3 siblings, 1 reply; 9+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2020-05-04  6:27 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

By default, "trace-cmd stat" command prints status of the main ftrace instance. When more
instances are configured, the user could be confused which status is displayed.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-stat.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tracecmd/trace-stat.c b/tracecmd/trace-stat.c
index c5057978..dcf2789c 100644
--- a/tracecmd/trace-stat.c
+++ b/tracecmd/trace-stat.c
@@ -920,8 +920,10 @@ static void stat_instance(struct buffer_instance *instance)
 			printf("---------------\n");
 		printf("Instance: %s\n",
 			tracefs_instance_get_name(instance->tracefs));
-	} else
+	} else {
 		report_instances();
+		printf("\nInstance: main\n\n");
+	}
 
 	report_plugin(instance);
 	report_events(instance);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] trace-cmd: Create ftrace instances before using them.
  2020-05-04  6:27 [PATCH v2 0/4] Few small trace-cmd fixes Tzvetomir Stoyanov (VMware)
  2020-05-04  6:27 ` [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case Tzvetomir Stoyanov (VMware)
  2020-05-04  6:27 ` [PATCH v2 2/4] trace-cmd: Add "main" in the output of trace-cmd stat when displaying main instance Tzvetomir Stoyanov (VMware)
@ 2020-05-04  6:27 ` Tzvetomir Stoyanov (VMware)
  2020-05-04  6:27 ` [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child Tzvetomir Stoyanov (VMware)
  3 siblings, 0 replies; 9+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2020-05-04  6:27 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

Ftrace instances should be created physically in the tracefs, before
trying to access its files. In record_trace() function, the call to
make_instances() should be before the logic which reads the instance's
tracing file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index d8c24ebf..1e4d38fa 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -6222,6 +6222,8 @@ static void record_trace(int argc, char **argv,
 	if (!ctx->output)
 		ctx->output = DEFAULT_INPUT_FILE;
 
+	make_instances();
+
 	/* Save the state of tracing_on before starting */
 	for_all_instances(instance) {
 		instance->output_file = strdup(ctx->output);
@@ -6236,8 +6238,6 @@ static void record_trace(int argc, char **argv,
 			instance->tracing_on_init_val = 1;
 	}
 
-	make_instances();
-
 	if (ctx->events)
 		expand_event_list();
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child
  2020-05-04  6:27 [PATCH v2 0/4] Few small trace-cmd fixes Tzvetomir Stoyanov (VMware)
                   ` (2 preceding siblings ...)
  2020-05-04  6:27 ` [PATCH v2 3/4] trace-cmd: Create ftrace instances before using them Tzvetomir Stoyanov (VMware)
@ 2020-05-04  6:27 ` Tzvetomir Stoyanov (VMware)
  2020-05-04 20:30   ` Steven Rostedt
  3 siblings, 1 reply; 9+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2020-05-04  6:27 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

When trace-cmd runs a command, specified with the "-F" flag, it forks a
child process and executes the command in its context. This child process
receives a full copy of the parents memory at the moment of fork(). When
it modifies this copy, the parent memory is not affected. Calling the
function update_task_filter() in the child context will operate on a valid
data, but will not update anything in the parent's databases.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 64 +++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 1e4d38fa..ae8a5745 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -11,6 +11,7 @@
 #include <stdarg.h>
 #include <getopt.h>
 #include <time.h>
+#include <semaphore.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/time.h>
@@ -1187,10 +1188,12 @@ static void get_filter_pid_maps(void)
 	}
 }
 
-static void update_task_filter(void)
+static void update_task_filter(int pid)
 {
 	struct buffer_instance *instance;
-	int pid = getpid();
+
+	if (pid < 0)
+		pid = getpid();
 
 	if (no_filter)
 		return;
@@ -1487,18 +1490,40 @@ static int change_user(const char *user)
 	return 0;
 }
 
+#define TRACE_RUN_SEM	"trace_cmd_semaphore_run"
+#define TRACE_INIT_SEM	"trace_cmd_semaphore_init"
 static void run_cmd(enum trace_type type, const char *user, int argc, char **argv)
 {
+	sem_t *sem_init = SEM_FAILED;
+	sem_t *sem_run = SEM_FAILED;
+	char *die_log = NULL;
 	int status;
 	int pid;
 
-	if ((pid = fork()) < 0)
-		die("failed to fork");
+	sem_init = sem_open(TRACE_INIT_SEM, O_CREAT, S_IRUSR|S_IWUSR, 0);
+	if (sem_init == SEM_FAILED) {
+		die_log = "failed to init trace_cmd init semaphore";
+		goto out;
+	}
+
+	sem_run = sem_open(TRACE_RUN_SEM, O_CREAT, S_IRUSR|S_IWUSR, 0);
+	if (sem_run == SEM_FAILED) {
+		die_log = "failed to init trace_cmd run semaphore";
+		goto out;
+
+	}
+
+	if ((pid = fork()) < 0) {
+		die_log = "failed to fork";
+		goto out;
+	}
 	if (!pid) {
 		/* child */
-		update_task_filter();
+		sem_wait(sem_init);
 		tracecmd_enable_tracing();
 		enable_ptrace();
+		sem_post(sem_run);
+
 		/*
 		 * If we are using stderr for stdout, switch
 		 * it back to the saved stdout for the code we run.
@@ -1519,12 +1544,27 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
 			die("Failed to exec %s", argv[0]);
 		}
 	}
-	if (do_ptrace) {
-		add_filter_pid(pid, 0);
-		ptrace_attach(pid);
+	update_task_filter(pid);
+	sem_post(sem_init);
+	sem_wait(sem_run);
+
+	if (do_ptrace)
 		ptrace_wait(type);
-	} else
+	else
 		trace_waitpid(type, pid, &status, 0);
+
+out:
+	if (sem_init != SEM_FAILED) {
+		sem_close(sem_init);
+		sem_unlink(TRACE_INIT_SEM);
+	}
+	if (sem_run != SEM_FAILED) {
+		sem_close(sem_run);
+		sem_unlink(TRACE_RUN_SEM);
+	}
+
+	if (die_log)
+		die(die_log);
 }
 
 static void
@@ -6285,7 +6325,7 @@ static void record_trace(int argc, char **argv,
 		if (!latency)
 			start_threads(type, ctx);
 	} else {
-		update_task_filter();
+		update_task_filter(-1);
 		tracecmd_enable_tracing();
 		exit(0);
 	}
@@ -6293,11 +6333,11 @@ static void record_trace(int argc, char **argv,
 	if (ctx->run_command) {
 		run_cmd(type, ctx->user, (argc - optind) - 1, &argv[optind + 1]);
 	} else if (ctx->instance && is_agent(ctx->instance)) {
-		update_task_filter();
+		update_task_filter(-1);
 		tracecmd_enable_tracing();
 		tracecmd_msg_wait_close(ctx->instance->msg_handle);
 	} else {
-		update_task_filter();
+		update_task_filter(-1);
 		tracecmd_enable_tracing();
 		/* We don't ptrace ourself */
 		if (do_ptrace && filter_pids) {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/4] trace-cmd: Add "main" in the output of trace-cmd stat when displaying main instance
  2020-05-04  6:27 ` [PATCH v2 2/4] trace-cmd: Add "main" in the output of trace-cmd stat when displaying main instance Tzvetomir Stoyanov (VMware)
@ 2020-05-04 13:41   ` Steven Rostedt
  0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2020-05-04 13:41 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: linux-trace-devel

On Mon,  4 May 2020 09:27:09 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> By default, "trace-cmd stat" command prints status of the main ftrace instance. When more
> instances are configured, the user could be confused which status is displayed.

Hmm, I'm not so big on this one. Has anyone complained about it? We never
actually gave a name to the top level. In fact, it can get more confusing
if someone were to create an instance called "main" then it would get even
more confusing.

I think the better answer is to just move the instance reporting to the
end, and not at the beginning. That would make more sense. I was thinking
of recommending that when I first applied the patch, but now I think it
should be done.

-- Steve

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  tracecmd/trace-stat.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tracecmd/trace-stat.c b/tracecmd/trace-stat.c
> index c5057978..dcf2789c 100644
> --- a/tracecmd/trace-stat.c
> +++ b/tracecmd/trace-stat.c
> @@ -920,8 +920,10 @@ static void stat_instance(struct buffer_instance *instance)
>  			printf("---------------\n");
>  		printf("Instance: %s\n",
>  			tracefs_instance_get_name(instance->tracefs));
> -	} else
> +	} else {
>  		report_instances();
> +		printf("\nInstance: main\n\n");
> +	}
>  
>  	report_plugin(instance);
>  	report_events(instance);


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case
  2020-05-04  6:27 ` [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case Tzvetomir Stoyanov (VMware)
@ 2020-05-04 20:00   ` Steven Rostedt
  0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2020-05-04 20:00 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: linux-trace-devel

On Mon,  4 May 2020 09:27:08 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The trace-cmd report command crashes while displaying a file recorded with "--proc-map" and "-B" options:
> #trace-cmd record --proc-map  -B test -e sched -F sleep 1
> The "--proc-map" options saves the address map of "sleep" into the trace.dat file. This
> information is used by KernelShark. The "-B" options traces the specified events into a
> ftrace instance "test".
> When such file is opened using libtracecmd APIs, the proc-map is parsed and saved into
> a tracecmd_input handler, as linked list "pid_maps". Later, when the ftrace instance
> "test" is parsed, a copy of this handler is used to fill it with the instance's trace data.
> Both tracecmd_input handlers share the same "pid_maps" list, thus leads to a double
> free of the list, when  handlers are destroyed.
> As this "pid_maps" is not used in ftrace buffers, the "pid_maps" list of the copy can be
> initialized to NULL.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>


FYI, I changed the subject and body to this:

trace-cmd: Fix trace-cmd report crash while displaying trace.dat with --proc-map and -B options
  
The trace-cmd report command crashes while displaying a file recorded with
"--proc-map" and "-B" options:

 # trace-cmd record --proc-map  -B test -e sched -F sleep 1

The "--proc-map" options saves the address map of "sleep" into the trace.dat
file. This information is used by KernelShark. The "-B" option traces the
specified events into a ftrace instance "test".

When such a file is opened using libtracecmd APIs, the proc-map is parsed and
saved into a tracecmd_input handler, as linked list "pid_maps". Later, when
the ftrace instance "test" is parsed, a copy of this handler is used to fill
it with the instance's trace data.  Both tracecmd_input handlers share the
same "pid_maps" list, thus leads to a double free of the list when the
handlers are destroyed.  As this "pid_maps" is not used in ftrace buffers,
the "pid_maps" list of the copy can be initialized to NULL.

-- Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child
  2020-05-04  6:27 ` [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child Tzvetomir Stoyanov (VMware)
@ 2020-05-04 20:30   ` Steven Rostedt
  2020-05-05  8:39     ` Tzvetomir Stoyanov
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2020-05-04 20:30 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: linux-trace-devel

On Mon,  4 May 2020 09:27:11 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> When trace-cmd runs a command, specified with the "-F" flag, it forks a
> child process and executes the command in its context. This child process
> receives a full copy of the parents memory at the moment of fork(). When
> it modifies this copy, the parent memory is not affected. Calling the
> function update_task_filter() in the child context will operate on a valid
> data, but will not update anything in the parent's databases.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  tracecmd/trace-record.c | 64 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 1e4d38fa..ae8a5745 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -11,6 +11,7 @@
>  #include <stdarg.h>
>  #include <getopt.h>
>  #include <time.h>
> +#include <semaphore.h>

OK, first things first. Do not use semaphores. I think I mentioned this to
you before. Semaphores are a horrible interface, and should be avoided at
all costs! ;-)

Also, I don't think you are solving the bug you think you are ;-)

With today's code (without this patch), I can run:

 # trace-cmd record -e exceptions -e sched -e irq --proc-map ls

And for that result I can do:

 # trace-cmd dump --options
[..]
                [Option PROCMAPS, 2383 bytes]
a10 30 /usr/bin/ls
556850495000 556850499000 /usr/bin/ls
556850499000 5568504ad000 /usr/bin/ls
5568504ad000 5568504b6000 /usr/bin/ls
5568504b6000 5568504b8000 /usr/bin/ls
5568504b8000 5568504b9000 /usr/bin/ls
556850c60000 556850c81000 [heap]
7efce4a9b000 7efcf1a45000 /usr/lib/locale/locale-archive
7efcf1a49000 7efcf1a4f000 /usr/lib64/libpthread-2.28.so
7efcf1a4f000 7efcf1a5f000 /usr/lib64/libpthread-2.28.so
7efcf1a5f000 7efcf1a65000 /usr/lib64/libpthread-2.28.so
7efcf1a65000 7efcf1a66000 /usr/lib64/libpthread-2.28.so
7efcf1a66000 7efcf1a67000 /usr/lib64/libpthread-2.28.so
[..]

What is it that you are fixing? Remember, if we run --proc-map, we enable
ptrace. Which at the end of its execution we have:

			case PTRACE_EVENT_EXIT:
				if (get_procmap)
					get_pid_addr_maps(pid);

Where the code records the proc_map of the -F process when it exits.

The only thing this patch is saving, is the wasted time of updating the
procmaps from the child. And to stop that, all you need is this:

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 1e4d38fa..4d647887 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1187,7 +1187,7 @@ static void get_filter_pid_maps(void)
 	}
 }
 
-static void update_task_filter(void)
+static void update_task_filter(bool do_procmaps)
 {
 	struct buffer_instance *instance;
 	int pid = getpid();
@@ -1195,7 +1195,7 @@ static void update_task_filter(void)
 	if (no_filter)
 		return;
 
-	if (get_procmap && filter_pids)
+	if (do_procmaps && get_procmap && filter_pids)
 		get_filter_pid_maps();
 
 	if (filter_task)
@@ -1496,7 +1496,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
 		die("failed to fork");
 	if (!pid) {
 		/* child */
-		update_task_filter();
+		update_task_filter(false);
 		tracecmd_enable_tracing();
 		enable_ptrace();
 		/*
@@ -6285,7 +6285,7 @@ static void record_trace(int argc, char **argv,
 		if (!latency)
 			start_threads(type, ctx);
 	} else {
-		update_task_filter();
+		update_task_filter(true);
 		tracecmd_enable_tracing();
 		exit(0);
 	}
@@ -6293,11 +6293,11 @@ static void record_trace(int argc, char **argv,
 	if (ctx->run_command) {
 		run_cmd(type, ctx->user, (argc - optind) - 1, &argv[optind + 1]);
 	} else if (ctx->instance && is_agent(ctx->instance)) {
-		update_task_filter();
+		update_task_filter(true);
 		tracecmd_enable_tracing();
 		tracecmd_msg_wait_close(ctx->instance->msg_handle);
 	} else {
-		update_task_filter();
+		update_task_filter(true);
 		tracecmd_enable_tracing();
 		/* We don't ptrace ourself */
 		if (do_ptrace && filter_pids) {

-- Steve

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child
  2020-05-04 20:30   ` Steven Rostedt
@ 2020-05-05  8:39     ` Tzvetomir Stoyanov
  0 siblings, 0 replies; 9+ messages in thread
From: Tzvetomir Stoyanov @ 2020-05-05  8:39 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel

Hi Steven,

On Mon, May 4, 2020 at 11:30 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon,  4 May 2020 09:27:11 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > When trace-cmd runs a command, specified with the "-F" flag, it forks a
> > child process and executes the command in its context. This child process
> > receives a full copy of the parents memory at the moment of fork(). When
> > it modifies this copy, the parent memory is not affected. Calling the
> > function update_task_filter() in the child context will operate on a valid
> > data, but will not update anything in the parent's databases.
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  tracecmd/trace-record.c | 64 +++++++++++++++++++++++++++++++++--------
> >  1 file changed, 52 insertions(+), 12 deletions(-)
> >
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index 1e4d38fa..ae8a5745 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -11,6 +11,7 @@
> >  #include <stdarg.h>
> >  #include <getopt.h>
> >  #include <time.h>
> > +#include <semaphore.h>
>
> OK, first things first. Do not use semaphores. I think I mentioned this to
> you before. Semaphores are a horrible interface, and should be avoided at
> all costs! ;-)
>
> Also, I don't think you are solving the bug you think you are ;-)
>

While modifying the code for per instance PID filtering, I changed the ptrace()
related logic and decided to test these changes with "--proc-map". I looked at
the implementation to find out how ptrace() works when a program is filtered and
reached to this update_task_filter() call in the child context. I
thought it should
update the filtered tasks with child's PID and realized it does not update the
parent's list because it runs in the child context.
That's the reason for this patch, I agree there is no actual bug. As I
understand,
the update_task_filter() is called in child context to update ftrace
filter configuration
with the child's PID, not to update some trace-cmd internal state. That's why
add_filter_pid() is called again in the parent's context.
As there is no actual bug, and you prefer to avoid any semaphores in trace-cmd,
I'll withdraw the patch.

> With today's code (without this patch), I can run:
>
>  # trace-cmd record -e exceptions -e sched -e irq --proc-map ls
>
> And for that result I can do:
>
>  # trace-cmd dump --options
> [..]
>                 [Option PROCMAPS, 2383 bytes]
> a10 30 /usr/bin/ls
> 556850495000 556850499000 /usr/bin/ls
> 556850499000 5568504ad000 /usr/bin/ls
> 5568504ad000 5568504b6000 /usr/bin/ls
> 5568504b6000 5568504b8000 /usr/bin/ls
> 5568504b8000 5568504b9000 /usr/bin/ls
> 556850c60000 556850c81000 [heap]
> 7efce4a9b000 7efcf1a45000 /usr/lib/locale/locale-archive
> 7efcf1a49000 7efcf1a4f000 /usr/lib64/libpthread-2.28.so
> 7efcf1a4f000 7efcf1a5f000 /usr/lib64/libpthread-2.28.so
> 7efcf1a5f000 7efcf1a65000 /usr/lib64/libpthread-2.28.so
> 7efcf1a65000 7efcf1a66000 /usr/lib64/libpthread-2.28.so
> 7efcf1a66000 7efcf1a67000 /usr/lib64/libpthread-2.28.so
> [..]
>
> What is it that you are fixing? Remember, if we run --proc-map, we enable
> ptrace. Which at the end of its execution we have:
>
>                         case PTRACE_EVENT_EXIT:
>                                 if (get_procmap)
>                                         get_pid_addr_maps(pid);
>
> Where the code records the proc_map of the -F process when it exits.
>
> The only thing this patch is saving, is the wasted time of updating the
> procmaps from the child. And to stop that, all you need is this:
>
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 1e4d38fa..4d647887 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -1187,7 +1187,7 @@ static void get_filter_pid_maps(void)
>         }
>  }
>
> -static void update_task_filter(void)
> +static void update_task_filter(bool do_procmaps)
>  {
>         struct buffer_instance *instance;
>         int pid = getpid();
> @@ -1195,7 +1195,7 @@ static void update_task_filter(void)
>         if (no_filter)
>                 return;
>
> -       if (get_procmap && filter_pids)
> +       if (do_procmaps && get_procmap && filter_pids)
>                 get_filter_pid_maps();
>
>         if (filter_task)
> @@ -1496,7 +1496,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
>                 die("failed to fork");
>         if (!pid) {
>                 /* child */
> -               update_task_filter();
> +               update_task_filter(false);
>                 tracecmd_enable_tracing();
>                 enable_ptrace();
>                 /*
> @@ -6285,7 +6285,7 @@ static void record_trace(int argc, char **argv,
>                 if (!latency)
>                         start_threads(type, ctx);
>         } else {
> -               update_task_filter();
> +               update_task_filter(true);
>                 tracecmd_enable_tracing();
>                 exit(0);
>         }
> @@ -6293,11 +6293,11 @@ static void record_trace(int argc, char **argv,
>         if (ctx->run_command) {
>                 run_cmd(type, ctx->user, (argc - optind) - 1, &argv[optind + 1]);
>         } else if (ctx->instance && is_agent(ctx->instance)) {
> -               update_task_filter();
> +               update_task_filter(true);
>                 tracecmd_enable_tracing();
>                 tracecmd_msg_wait_close(ctx->instance->msg_handle);
>         } else {
> -               update_task_filter();
> +               update_task_filter(true);
>                 tracecmd_enable_tracing();
>                 /* We don't ptrace ourself */
>                 if (do_ptrace && filter_pids) {
>
> -- Steve



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-05-05  8:40 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-04  6:27 [PATCH v2 0/4] Few small trace-cmd fixes Tzvetomir Stoyanov (VMware)
2020-05-04  6:27 ` [PATCH v2 1/4] trace-cmd: Fix trace-cmd report crash while displaying trace.dat in specific use case Tzvetomir Stoyanov (VMware)
2020-05-04 20:00   ` Steven Rostedt
2020-05-04  6:27 ` [PATCH v2 2/4] trace-cmd: Add "main" in the output of trace-cmd stat when displaying main instance Tzvetomir Stoyanov (VMware)
2020-05-04 13:41   ` Steven Rostedt
2020-05-04  6:27 ` [PATCH v2 3/4] trace-cmd: Create ftrace instances before using them Tzvetomir Stoyanov (VMware)
2020-05-04  6:27 ` [PATCH v2 4/4] trace-cmd: Do not try to update parent's memory from a fork()-ed child Tzvetomir Stoyanov (VMware)
2020-05-04 20:30   ` Steven Rostedt
2020-05-05  8:39     ` Tzvetomir Stoyanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).