tracing: Fix issues recording task information

PATCH In review Joel Fernandes 2 participants 5 messages Jul 6, 2017 – Jul 7, 2017 Tracing

Share & tools

Joel Fernandes

author

2017-07-06 00:11 UTC #0 lore raw

PATCH 1/1 tracing: Fix issues recording task information

Currently we stop recording task information if any of them error out or are not being recorded. Further if switching to/from the idle thread, we bail out early on. Fix these issues.

Cc [email protected]

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

Reported-by Michael Sartain <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
Hi Steve,
This fix is based on ftrace/core branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
Could you pick it up for -rc cycle if it looks good to you? Thanks.

 kernel/trace/trace.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

kernel/trace/trace.c +23 ~~-11~~

index b66a1c8805f3..27b981a46389 100644
@@ -1918,7 +1918,11 @@ static int trace_save_cmdline(struct task_struct *tsk)
 {
 	unsigned pid, idx;
 
-	if (!tsk->pid || unlikely(tsk->pid > PID_MAX_DEFAULT))
+	/* treat recording of idle task as a success */
+	if (!tsk->pid)
+		return 1;
+
+	if (unlikely(tsk->pid > PID_MAX_DEFAULT))
 		return 0;
 
 	/*

@@ -2004,7 +2008,11 @@ int trace_find_tgid(int pid)
 
 static int trace_save_tgid(struct task_struct *tsk)
 {
-	if (unlikely(!tgid_map || !tsk->pid || tsk->pid > PID_MAX_DEFAULT))
+	/* treat recording of idle task as a success */
+	if (!tsk->pid)
+		return 1;
+
+	if (unlikely(!tgid_map || tsk->pid > PID_MAX_DEFAULT))
 		return 0;
 
 	tgid_map[tsk->pid] = tsk->tgid;

@@ -2031,11 +2039,14 @@ static bool tracing_record_taskinfo_skip(int flags)
  */
 void tracing_record_taskinfo(struct task_struct *task, int flags)
 {
+	bool done;
+
 	if (tracing_record_taskinfo_skip(flags))
 		return;
-	if ((flags & TRACE_RECORD_CMDLINE) && !trace_save_cmdline(task))
-		return;
-	if ((flags & TRACE_RECORD_TGID) && !trace_save_tgid(task))
+
+	done = !(flags & TRACE_RECORD_CMDLINE) || trace_save_cmdline(task);
+	done &= !(flags & TRACE_RECORD_TGID) || trace_save_tgid(task);
+	if (!done)
 		return;
 
 	__this_cpu_write(trace_taskinfo_save, false);

@@ -2052,15 +2063,16 @@ void tracing_record_taskinfo(struct task_struct *task, int flags)
 void tracing_record_taskinfo_sched_switch(struct task_struct *prev,
 					  struct task_struct *next, int flags)
 {
-	if (tracing_record_taskinfo_skip(flags))
-		return;
+	bool done;
 
-	if ((flags & TRACE_RECORD_CMDLINE) &&
-	    (!trace_save_cmdline(prev) || !trace_save_cmdline(next)))
+	if (tracing_record_taskinfo_skip(flags))
 		return;
 
-	if ((flags & TRACE_RECORD_TGID) &&
-	    (!trace_save_tgid(prev) || !trace_save_tgid(next)))
+	done  = !(flags & TRACE_RECORD_CMDLINE) || trace_save_cmdline(prev);
+	done &= !(flags & TRACE_RECORD_CMDLINE) || trace_save_cmdline(next);
+	done &= !(flags & TRACE_RECORD_TGID) || trace_save_tgid(prev);
+	done &= !(flags & TRACE_RECORD_TGID) || trace_save_tgid(next);
+	if (!done)
 		return;
 
 	__this_cpu_write(trace_taskinfo_save, false);

Signature · 1 line

2.13.2.725.g09c95d1e9-goog

Steven Rostedt

maintainer

2017-07-06 13:04 UTC #1 lore raw

Quoted 3 lines · On Wed, 5 Jul 2017 17:11:47 -0700 Joel Fernandes <[email protected]> wrote:

Currently we stop recording task information if any of them error out or are not being recorded. Further if switching to/from the idle thread, we bail out early on. Fix these issues.

Can you break this up into three patches. And yeah, I'll send these after my initial pull request to Linus.

Quoted 20 lines

Cc [email protected]
Cc Steven Rostedt <[email protected]>
Cc Ingo Molnar <[email protected]>
Reported-by Michael Sartain <[email protected]>
Signed-off-by Joel Fernandes <[email protected]>
---
Hi Steve,
This fix is based on ftrace/core branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
Could you pick it up for -rc cycle if it looks good to you? Thanks.
 kernel/trace/trace.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)
kernel/trace/trace.c +0 -0
index b66a1c8805f3..27b981a46389 100644

kernel/trace/trace.c static int trace_save_cmdline(struct task_struct *tsk) · line 1926

Quoted 10 lines

kernel/trace/trace.c +5 -1

@@ -1918,7 +1918,11 @@ static int trace_save_cmdline(struct task_struct *tsk)
 {
 	unsigned pid, idx;
 
-	if (!tsk->pid || unlikely(tsk->pid > PID_MAX_DEFAULT))
+	/* treat recording of idle task as a success */
+	if (!tsk->pid)
+		return 1;
+
+	if (unlikely(tsk->pid > PID_MAX_DEFAULT))
 		return 0;

Make this a separate patch. This should go to stable.

kernel/trace/trace.c int trace_find_tgid(int pid) · line 2016

Quoted 13 lines

kernel/trace/trace.c +5 -1

/*

@@ -2004,7 +2008,11 @@ int trace_find_tgid(int pid)
 
 static int trace_save_tgid(struct task_struct *tsk)
 {
-	if (unlikely(!tgid_map || !tsk->pid || tsk->pid > PID_MAX_DEFAULT))
+	/* treat recording of idle task as a success */
+	if (!tsk->pid)
+		return 1;
+
+	if (unlikely(!tgid_map || tsk->pid > PID_MAX_DEFAULT))
 		return 0;

Make this a separate patch as it fixes a different issue than what is below.

Quoted 8 lines

kernel/trace/trace.c +2 -0

 	tgid_map[tsk->pid] = tsk->tgid;

@@ -2031,11 +2039,14 @@ static bool tracing_record_taskinfo_skip(int flags)
  */
 void tracing_record_taskinfo(struct task_struct *task, int flags)
 {
+	bool done;
+

kernel/trace/trace.c static bool tracing_record_taskinfo_skip(int flags) · line 2050

Quoted 10 lines

kernel/trace/trace.c +4 -3

 	if (tracing_record_taskinfo_skip(flags))
 		return;
-	if ((flags & TRACE_RECORD_CMDLINE) && !trace_save_cmdline(task))
-		return;
-	if ((flags & TRACE_RECORD_TGID) && !trace_save_tgid(task))
+
+	done = !(flags & TRACE_RECORD_CMDLINE) || trace_save_cmdline(task);
+	done &= !(flags & TRACE_RECORD_TGID) || trace_save_tgid(task);
+	if (!done)
 		return;

Should probably add some comments before the return.

Quoted 11 lines

kernel/trace/trace.c +1 -3

 	__this_cpu_write(trace_taskinfo_save, false);

@@ -2052,15 +2063,16 @@ void tracing_record_taskinfo(struct task_struct *task, int flags)
 void tracing_record_taskinfo_sched_switch(struct task_struct *prev,
 					  struct task_struct *next, int flags)
 {
-	if (tracing_record_taskinfo_skip(flags))
-		return;
+	bool done;
 
-	if ((flags & TRACE_RECORD_CMDLINE) &&

kernel/trace/trace.c void tracing_record_taskinfo(struct task_struct *task, int flags) · line 2074

Quoted 10 lines

kernel/trace/trace.c +5 -3

-	    (!trace_save_cmdline(prev) || !trace_save_cmdline(next)))
+	if (tracing_record_taskinfo_skip(flags))
 		return;
 
-	if ((flags & TRACE_RECORD_TGID) &&
-	    (!trace_save_tgid(prev) || !trace_save_tgid(next)))
+	done  = !(flags & TRACE_RECORD_CMDLINE) || trace_save_cmdline(prev);
+	done &= !(flags & TRACE_RECORD_CMDLINE) || trace_save_cmdline(next);
+	done &= !(flags & TRACE_RECORD_TGID) || trace_save_tgid(prev);
+	done &= !(flags & TRACE_RECORD_TGID) || trace_save_tgid(next);

Some comments here too.

Thanks!

-- Steve

Quoted 4 lines

kernel/trace/trace.c +1 -0

+	if (!done)
 		return;
 
 	__this_cpu_write(trace_taskinfo_save, false);

Joel Fernandes

author

2017-07-06 23:05 UTC #2 lore raw

Hi Steven,

Quoted 9 lines · Steven Rostedt

Quoted 3 lines · On Wed, 5 Jul 2017 17:11:47 -0700 Joel Fernandes <[email protected]> wrote:
Currently we stop recording task information if any of them error out or are not being recorded. Further if switching to/from the idle thread, we bail out early on. Fix these issues.
Can you break this up into three patches. And yeah, I'll send these after my initial pull request to Linus.

Just sent them out again after breaking them up.

Thanks,

-Joel

Joel Fernandes

author

2017-07-06 23:17 UTC #3 lore raw

In reply to Steven Rostedt

Hi Steven,

On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <[email protected]> wrote:
[..]

Quoted 8 lines

kernel/trace/trace.c +0 -0

index b66a1c8805f3..27b981a46389 100644
@@ -1918,7 +1918,11 @@ static int trace_save_cmdline(struct task_struct *tsk)
 {
      unsigned pid, idx;

Quoted 9 lines

kernel/trace/trace.c static int trace_save_cmdline(struct task_struct *tsk) · line 1926
Quoted 7 lines
kernel/trace/trace.c +5 -1
-     if (!tsk->pid || unlikely(tsk->pid > PID_MAX_DEFAULT))
+     /* treat recording of idle task as a success */
+     if (!tsk->pid)
+             return 1;
+
+     if (unlikely(tsk->pid > PID_MAX_DEFAULT))
              return 0;
Make this a separate patch. This should go to stable.

Actually I am not sure if this should be marked for stable since before we were doing:
tracing_record_cmdline(prev); tracing_record_cmdline(next);
in probe_sched_switch, thus even if prev was the idle task, it would still goto record the next.

IMO it becomes an issue only with the new stuff where we record cmdline for prev and next in the same call (tracing_record_taskinfo_sched_switch). Anyway I already broke it up into a different patch and sent it out. Let me know if anything else needs to be done here, thanks.

Regards,
-Joel

Steven Rostedt

maintainer

2017-07-07 01:29 UTC #4 lore raw

Quoted 22 lines · On Thu, 6 Jul 2017 16:17:18 -0700 Joel Fernandes <[email protected]> wrote:

Hi Steven,
On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <[email protected]> wrote:
[..]
Quoted 8 lines
Quoted 8 lines
kernel/trace/trace.c +0 -0
index b66a1c8805f3..27b981a46389 100644
@@ -1918,7 +1918,11 @@ static int trace_save_cmdline(struct task_struct *tsk)
 {
      unsigned pid, idx;
 
Quoted 9 lines
kernel/trace/trace.c static int trace_save_cmdline(struct task_struct *tsk) · line 1926
Quoted 7 lines
kernel/trace/trace.c +5 -1
-     if (!tsk->pid || unlikely(tsk->pid > PID_MAX_DEFAULT))
+     /* treat recording of idle task as a success */
+     if (!tsk->pid)
+             return 1;
+
+     if (unlikely(tsk->pid > PID_MAX_DEFAULT))
              return 0;  
Make this a separate patch. This should go to stable.

Quoted 6 lines

Actually I am not sure if this should be marked for stable since before we were doing:
tracing_record_cmdline(prev); tracing_record_cmdline(next);
in probe_sched_switch, thus even if prev was the idle task, it would still goto record the next.

Ah right. I wasn't sure when that change was made. I should have looked deeper into it.

Quoted 6 lines

IMO it becomes an issue only with the new stuff where we record cmdline for prev and next in the same call (tracing_record_taskinfo_sched_switch). Anyway I already broke it up into a different patch and sent it out. Let me know if anything else needs to be done here, thanks.

OK, I'll take a look at it tomorrow.

Thanks,

-- Steve