[PATCH] tracing: Fix issues recording task information

[PATCH] tracing: Fix issues recording task information

[Joel Fernandes]

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: kernel-team@android.com
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Reported-by: Michael Sartain <mikesart@gmail.com>
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
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(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b66a1c8805f3..27b981a46389 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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);
-- 
2.13.2.725.g09c95d1e9-goog

Re: [PATCH] tracing: Fix issues recording task information

[Steven Rostedt]

On Wed,  5 Jul 2017 17:11:47 -0700
Joel Fernandes <joelaf@google.com> 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.

> 
> Cc: kernel-team@android.com
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Reported-by: Michael Sartain <mikesart@gmail.com>
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> ---
> 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(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b66a1c8805f3..27b981a46389 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -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.

>  
>  	/*
> @@ -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.

>  
>  	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;

Should probably add some comments before the 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);

Some comments here too.

Thanks!

-- Steve

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

Re: [PATCH] tracing: Fix issues recording task information

[Joel Fernandes]

Hi Steven,

On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed,  5 Jul 2017 17:11:47 -0700
> Joel Fernandes <joelaf@google.com> 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

Re: [PATCH] tracing: Fix issues recording task information

[Joel Fernandes]

Hi Steven,

On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
[..]
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index b66a1c8805f3..27b981a46389 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -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.

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

Re: [PATCH] tracing: Fix issues recording task information

[Steven Rostedt]

On Thu, 6 Jul 2017 16:17:18 -0700
Joel Fernandes <joelaf@google.com> wrote:

> Hi Steven,
> 
> On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> [..]
> >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> >> index b66a1c8805f3..27b981a46389 100644
> >> --- a/kernel/trace/trace.c
> >> +++ b/kernel/trace/trace.c
> >> @@ -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.  
> 
> 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.

> 
> 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