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(-)
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
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.
>
> 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(-)
>
> 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);
Hi Steven,
On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <[email protected]> wrote:
> 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
Hi Steven,
On Thu, Jul 6, 2017 at 5:59 AM, Steven Rostedt <[email protected]> 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
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:
> [..]
> >> 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