Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681965 > unrolled thread

[PATCH] tracing: Fix issues recording task information

Back to article view | Back to linux.kernel

Contents

  [PATCH] tracing: Fix issues recording task information Joel Fernandes <joelaf@google.com> - 2017-07-06 02:20 +0200
    Re: [PATCH] tracing: Fix issues recording task information Steven Rostedt <rostedt@goodmis.org> - 2017-07-06 15:10 +0200
      Re: [PATCH] tracing: Fix issues recording task information Joel Fernandes <joelaf@google.com> - 2017-07-07 01:10 +0200
      Re: [PATCH] tracing: Fix issues recording task information Joel Fernandes <joelaf@google.com> - 2017-07-07 01:20 +0200
        Re: [PATCH] tracing: Fix issues recording task information Steven Rostedt <rostedt@goodmis.org> - 2017-07-07 03:30 +0200

#1681965 — [PATCH] 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: 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

[toc] | [next] | [standalone]

#1682415

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

[toc] | [prev] | [next] | [standalone]

#1682796

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

[toc] | [prev] | [next] | [standalone]

#1682804

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

[toc] | [prev] | [next] | [standalone]

#1682852

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web