Messages in this thread | | | From | 関山友輝 / SEKIYAMA,TOMOKI <> | Subject | RE: [PATCH] perf sched: fix wrong conversion of task state | Date | Wed, 27 Jul 2016 15:58:21 +0000 |
| |
Hi David,
Thank you for the review.
On 2016/7/27 23:18, David Ahern wrote: > On 7/27/16 6:54 AM, Tomoki Sekiyama wrote: >> sched_out_state() converts the prev_state u64 bitmask to a char in >> a wrong way, which may cause wrong results of 'perf sched latency'. >> This patch fixes the conversion. >> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> >> Cc: Jiri Olsa <jolsa@kernel.org> >> Cc: David Ahern <dsahern@gmail.com> >> Cc: Namhyung Kim <namhyung@kernel.org> >> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> >> Cc: Masami Hiramatsu <mhiramat@kernel.org> >> --- >> tools/perf/builtin-sched.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c >> index 0dfe8df..eb2f7f4 100644 >> --- a/tools/perf/builtin-sched.c >> +++ b/tools/perf/builtin-sched.c >> @@ -71,6 +71,7 @@ struct sched_atom { >> }; >> >> #define TASK_STATE_TO_CHAR_STR "RSDTtZXxKWP" >> +#define TASK_STATE_MASK 0x7ff > >The mask should not be needed and looking at top of tree there are 2 new >states (N and n) that need to be added.
I couldn't find the state for 'n'. Where can I find it? I have rechecked the code and found that __trace_sched_switch_state() uses TASK_STATE_MAX for preempted tasks. I will add 'N', and '+' (instead of "R+" ftrace shows) for preempted tasks. (Also add_sched_out_event should set THREAD_WAIT_CPU for preempted tasks.)
>> >> enum thread_state { >> THREAD_SLEEPING = 0, >> @@ -899,7 +900,7 @@ static char sched_out_state(u64 prev_state) >> { >> const char *str = TASK_STATE_TO_CHAR_STR; >> >> - return str[prev_state]; >> + return str[ffs(prev_state & TASK_STATE_MASK)]; >> } >> >> static int >> > > Handle unknown bits with '?' like the kernel does (see task_state_char > and sched_show_task).
OK, I will make it return '?' for unknown states.
Will update the patch soon.
Thanks, Tomoki Sekiyama
| |