DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7EAA72156A
Date: Thu, 6 Jul 2017 08:59:02 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Joel Fernandes <joelaf@google.com>
Cc: linux-kernel@vger.kernel.org, mikesart@gmail.com,
        kernel-team@android.com, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] tracing: Fix issues recording task information
Message-ID: <20170706085902.0af6c9a6@gandalf.local.home>
In-Reply-To: <20170706001147.29213-1-joelaf@google.com>
References: <20170706001147.29213-1-joelaf@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3574
Lines: 114

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