[v2,4/4] perf sched: Commit to evsel__taskstate() to parse task state info
Commit Message
Now that we have evsel__taskstate() which no longer relies on the
hardcoded task state string and has good backward compatibility,
we have a good reason to use it.
Note TASK_STATE_TO_CHAR_STR and task bitmasks are useless now so
we remove them for good. And now we pass the state info back and
forth in a symbolic char which explains itself well instead.
Signed-off-by: Ze Gao <zegao@tencent.com>
---
tools/perf/builtin-sched.c | 46 +++++++++-----------------------------
1 file changed, 10 insertions(+), 36 deletions(-)
Comments
Hello,
On Sun, Jan 21, 2024 at 11:11 PM Ze Gao <zegao2021@gmail.com> wrote:
>
> Now that we have evsel__taskstate() which no longer relies on the
> hardcoded task state string and has good backward compatibility,
> we have a good reason to use it.
>
> Note TASK_STATE_TO_CHAR_STR and task bitmasks are useless now so
> we remove them for good. And now we pass the state info back and
> forth in a symbolic char which explains itself well instead.
>
> Signed-off-by: Ze Gao <zegao@tencent.com>
> ---
> tools/perf/builtin-sched.c | 46 +++++++++-----------------------------
> 1 file changed, 10 insertions(+), 36 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index ced6fffe8110..f1d62f6b6cab 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -92,13 +92,6 @@ struct sched_atom {
> struct task_desc *wakee;
> };
>
> -#define TASK_STATE_TO_CHAR_STR "RSDTtXZPI"
> -
> -/* task state bitmask, copied from include/linux/sched.h */
> -#define TASK_RUNNING 0
> -#define TASK_INTERRUPTIBLE 1
> -#define TASK_UNINTERRUPTIBLE 2
> -
> enum thread_state {
> THREAD_SLEEPING = 0,
> THREAD_WAIT_CPU,
> @@ -255,7 +248,7 @@ struct thread_runtime {
> u64 total_preempt_time;
> u64 total_delay_time;
>
> - int last_state;
> + char last_state;
>
> char shortname[3];
> bool comm_changed;
> @@ -425,7 +418,7 @@ static void add_sched_event_wakeup(struct perf_sched *sched, struct task_desc *t
> }
>
> static void add_sched_event_sleep(struct perf_sched *sched, struct task_desc *task,
> - u64 timestamp, u64 task_state __maybe_unused)
> + u64 timestamp, const char task_state __maybe_unused)
> {
> struct sched_atom *event = get_new_event(task, timestamp);
>
> @@ -849,7 +842,7 @@ static int replay_switch_event(struct perf_sched *sched,
> *next_comm = evsel__strval(evsel, sample, "next_comm");
> const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
> next_pid = evsel__intval(evsel, sample, "next_pid");
> - const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
> + const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
> struct task_desc *prev, __maybe_unused *next;
> u64 timestamp0, timestamp = sample->time;
> int cpu = sample->cpu;
> @@ -1039,13 +1032,6 @@ static int thread_atoms_insert(struct perf_sched *sched, struct thread *thread)
> return 0;
> }
>
> -static char sched_out_state(u64 prev_state)
> -{
> - const char *str = TASK_STATE_TO_CHAR_STR;
> -
> - return str[prev_state];
> -}
> -
> static int
> add_sched_out_event(struct work_atoms *atoms,
> char run_state,
> @@ -1121,7 +1107,7 @@ static int latency_switch_event(struct perf_sched *sched,
> {
> const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
> next_pid = evsel__intval(evsel, sample, "next_pid");
> - const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
> + const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
> struct work_atoms *out_events, *in_events;
> struct thread *sched_out, *sched_in;
> u64 timestamp0, timestamp = sample->time;
> @@ -1157,7 +1143,7 @@ static int latency_switch_event(struct perf_sched *sched,
> goto out_put;
> }
> }
> - if (add_sched_out_event(out_events, sched_out_state(prev_state), timestamp))
> + if (add_sched_out_event(out_events, prev_state, timestamp))
> return -1;
>
> in_events = thread_atoms_search(&sched->atom_root, sched_in, &sched->cmp_pid);
> @@ -2022,24 +2008,12 @@ static void timehist_header(struct perf_sched *sched)
> printf("\n");
> }
>
> -static char task_state_char(struct thread *thread, int state)
> -{
> - static const char state_to_char[] = TASK_STATE_TO_CHAR_STR;
> - unsigned bit = state ? ffs(state) : 0;
> -
> - /* 'I' for idle */
> - if (thread__tid(thread) == 0)
> - return 'I';
> -
> - return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
> -}
> -
> static void timehist_print_sample(struct perf_sched *sched,
> struct evsel *evsel,
> struct perf_sample *sample,
> struct addr_location *al,
> struct thread *thread,
> - u64 t, int state)
> + u64 t, const char state)
> {
> struct thread_runtime *tr = thread__priv(thread);
> const char *next_comm = evsel__strval(evsel, sample, "next_comm");
> @@ -2080,7 +2054,7 @@ static void timehist_print_sample(struct perf_sched *sched,
> print_sched_time(tr->dt_run, 6);
>
> if (sched->show_state)
> - printf(" %5c ", task_state_char(thread, state));
> + printf(" %5c ", thread->tid == 0 ? 'I' : state);
This resulted in a build error with reference count checker.
$ make EXTRA_CFLAGS=-DREFCNT_CHECKING=1
...
builtin-sched.c: In function ‘timehist_print_sample’:
builtin-sched.c:2057:39: error: ‘struct thread’ has no member named ‘tid’
2057 | printf(" %5c ", thread->tid == 0 ? 'I' : state);
|
The struct thread is protected by the refcount checker so
you should not access the members directly. Instead,
please use a help function like thread__tid().
Thanks,
Namhyung
>
> if (sched->show_next) {
> snprintf(nstr, sizeof(nstr), "next: %s[%d]", next_comm, next_pid);
> @@ -2152,9 +2126,9 @@ static void timehist_update_runtime_stats(struct thread_runtime *r,
> else if (r->last_time) {
> u64 dt_wait = tprev - r->last_time;
>
> - if (r->last_state == TASK_RUNNING)
> + if (r->last_state == 'R')
> r->dt_preempt = dt_wait;
> - else if (r->last_state == TASK_UNINTERRUPTIBLE)
> + else if (r->last_state == 'D')
> r->dt_iowait = dt_wait;
> else
> r->dt_sleep = dt_wait;
> @@ -2579,7 +2553,7 @@ static int timehist_sched_change_event(struct perf_tool *tool,
> struct thread_runtime *tr = NULL;
> u64 tprev, t = sample->time;
> int rc = 0;
> - int state = evsel__intval(evsel, sample, "prev_state");
> + const char state = evsel__taskstate(evsel, sample, "prev_state");
>
> addr_location__init(&al);
> if (machine__resolve(machine, &al, sample) < 0) {
> --
> 2.41.0
>
On Tue, Jan 23, 2024 at 8:38 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Sun, Jan 21, 2024 at 11:11 PM Ze Gao <zegao2021@gmail.com> wrote:
> >
> > Now that we have evsel__taskstate() which no longer relies on the
> > hardcoded task state string and has good backward compatibility,
> > we have a good reason to use it.
> >
> > Note TASK_STATE_TO_CHAR_STR and task bitmasks are useless now so
> > we remove them for good. And now we pass the state info back and
> > forth in a symbolic char which explains itself well instead.
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> > tools/perf/builtin-sched.c | 46 +++++++++-----------------------------
> > 1 file changed, 10 insertions(+), 36 deletions(-)
> >
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index ced6fffe8110..f1d62f6b6cab 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -92,13 +92,6 @@ struct sched_atom {
> > struct task_desc *wakee;
> > };
> >
> > -#define TASK_STATE_TO_CHAR_STR "RSDTtXZPI"
> > -
> > -/* task state bitmask, copied from include/linux/sched.h */
> > -#define TASK_RUNNING 0
> > -#define TASK_INTERRUPTIBLE 1
> > -#define TASK_UNINTERRUPTIBLE 2
> > -
> > enum thread_state {
> > THREAD_SLEEPING = 0,
> > THREAD_WAIT_CPU,
> > @@ -255,7 +248,7 @@ struct thread_runtime {
> > u64 total_preempt_time;
> > u64 total_delay_time;
> >
> > - int last_state;
> > + char last_state;
> >
> > char shortname[3];
> > bool comm_changed;
> > @@ -425,7 +418,7 @@ static void add_sched_event_wakeup(struct perf_sched *sched, struct task_desc *t
> > }
> >
> > static void add_sched_event_sleep(struct perf_sched *sched, struct task_desc *task,
> > - u64 timestamp, u64 task_state __maybe_unused)
> > + u64 timestamp, const char task_state __maybe_unused)
> > {
> > struct sched_atom *event = get_new_event(task, timestamp);
> >
> > @@ -849,7 +842,7 @@ static int replay_switch_event(struct perf_sched *sched,
> > *next_comm = evsel__strval(evsel, sample, "next_comm");
> > const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
> > next_pid = evsel__intval(evsel, sample, "next_pid");
> > - const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
> > + const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
> > struct task_desc *prev, __maybe_unused *next;
> > u64 timestamp0, timestamp = sample->time;
> > int cpu = sample->cpu;
> > @@ -1039,13 +1032,6 @@ static int thread_atoms_insert(struct perf_sched *sched, struct thread *thread)
> > return 0;
> > }
> >
> > -static char sched_out_state(u64 prev_state)
> > -{
> > - const char *str = TASK_STATE_TO_CHAR_STR;
> > -
> > - return str[prev_state];
> > -}
> > -
> > static int
> > add_sched_out_event(struct work_atoms *atoms,
> > char run_state,
> > @@ -1121,7 +1107,7 @@ static int latency_switch_event(struct perf_sched *sched,
> > {
> > const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
> > next_pid = evsel__intval(evsel, sample, "next_pid");
> > - const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
> > + const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
> > struct work_atoms *out_events, *in_events;
> > struct thread *sched_out, *sched_in;
> > u64 timestamp0, timestamp = sample->time;
> > @@ -1157,7 +1143,7 @@ static int latency_switch_event(struct perf_sched *sched,
> > goto out_put;
> > }
> > }
> > - if (add_sched_out_event(out_events, sched_out_state(prev_state), timestamp))
> > + if (add_sched_out_event(out_events, prev_state, timestamp))
> > return -1;
> >
> > in_events = thread_atoms_search(&sched->atom_root, sched_in, &sched->cmp_pid);
> > @@ -2022,24 +2008,12 @@ static void timehist_header(struct perf_sched *sched)
> > printf("\n");
> > }
> >
> > -static char task_state_char(struct thread *thread, int state)
> > -{
> > - static const char state_to_char[] = TASK_STATE_TO_CHAR_STR;
> > - unsigned bit = state ? ffs(state) : 0;
> > -
> > - /* 'I' for idle */
> > - if (thread__tid(thread) == 0)
> > - return 'I';
> > -
> > - return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
> > -}
> > -
> > static void timehist_print_sample(struct perf_sched *sched,
> > struct evsel *evsel,
> > struct perf_sample *sample,
> > struct addr_location *al,
> > struct thread *thread,
> > - u64 t, int state)
> > + u64 t, const char state)
> > {
> > struct thread_runtime *tr = thread__priv(thread);
> > const char *next_comm = evsel__strval(evsel, sample, "next_comm");
> > @@ -2080,7 +2054,7 @@ static void timehist_print_sample(struct perf_sched *sched,
> > print_sched_time(tr->dt_run, 6);
> >
> > if (sched->show_state)
> > - printf(" %5c ", task_state_char(thread, state));
> > + printf(" %5c ", thread->tid == 0 ? 'I' : state);
>
> This resulted in a build error with reference count checker.
>
> $ make EXTRA_CFLAGS=-DREFCNT_CHECKING=1
> ...
> builtin-sched.c: In function ‘timehist_print_sample’:
> builtin-sched.c:2057:39: error: ‘struct thread’ has no member named ‘tid’
> 2057 | printf(" %5c ", thread->tid == 0 ? 'I' : state);
> |
>
> The struct thread is protected by the refcount checker so
> you should not access the members directly. Instead,
> please use a help function like thread__tid().
Thanks for pointing this out. Commit ee84a3032b74("
perf thread: Add accessor functions for thread") introduced
this accessor which i overlooked. My bad :(.
Will send a fix in-reply-to this patch separately.
Thanks,
-- Ze
> Thanks,
> Namhyung
>
> >
> > if (sched->show_next) {
> > snprintf(nstr, sizeof(nstr), "next: %s[%d]", next_comm, next_pid);
> > @@ -2152,9 +2126,9 @@ static void timehist_update_runtime_stats(struct thread_runtime *r,
> > else if (r->last_time) {
> > u64 dt_wait = tprev - r->last_time;
> >
> > - if (r->last_state == TASK_RUNNING)
> > + if (r->last_state == 'R')
> > r->dt_preempt = dt_wait;
> > - else if (r->last_state == TASK_UNINTERRUPTIBLE)
> > + else if (r->last_state == 'D')
> > r->dt_iowait = dt_wait;
> > else
> > r->dt_sleep = dt_wait;
> > @@ -2579,7 +2553,7 @@ static int timehist_sched_change_event(struct perf_tool *tool,
> > struct thread_runtime *tr = NULL;
> > u64 tprev, t = sample->time;
> > int rc = 0;
> > - int state = evsel__intval(evsel, sample, "prev_state");
> > + const char state = evsel__taskstate(evsel, sample, "prev_state");
> >
> > addr_location__init(&al);
> > if (machine__resolve(machine, &al, sample) < 0) {
> > --
> > 2.41.0
> >
@@ -92,13 +92,6 @@ struct sched_atom {
struct task_desc *wakee;
};
-#define TASK_STATE_TO_CHAR_STR "RSDTtXZPI"
-
-/* task state bitmask, copied from include/linux/sched.h */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-
enum thread_state {
THREAD_SLEEPING = 0,
THREAD_WAIT_CPU,
@@ -255,7 +248,7 @@ struct thread_runtime {
u64 total_preempt_time;
u64 total_delay_time;
- int last_state;
+ char last_state;
char shortname[3];
bool comm_changed;
@@ -425,7 +418,7 @@ static void add_sched_event_wakeup(struct perf_sched *sched, struct task_desc *t
}
static void add_sched_event_sleep(struct perf_sched *sched, struct task_desc *task,
- u64 timestamp, u64 task_state __maybe_unused)
+ u64 timestamp, const char task_state __maybe_unused)
{
struct sched_atom *event = get_new_event(task, timestamp);
@@ -849,7 +842,7 @@ static int replay_switch_event(struct perf_sched *sched,
*next_comm = evsel__strval(evsel, sample, "next_comm");
const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
next_pid = evsel__intval(evsel, sample, "next_pid");
- const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
+ const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
struct task_desc *prev, __maybe_unused *next;
u64 timestamp0, timestamp = sample->time;
int cpu = sample->cpu;
@@ -1039,13 +1032,6 @@ static int thread_atoms_insert(struct perf_sched *sched, struct thread *thread)
return 0;
}
-static char sched_out_state(u64 prev_state)
-{
- const char *str = TASK_STATE_TO_CHAR_STR;
-
- return str[prev_state];
-}
-
static int
add_sched_out_event(struct work_atoms *atoms,
char run_state,
@@ -1121,7 +1107,7 @@ static int latency_switch_event(struct perf_sched *sched,
{
const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
next_pid = evsel__intval(evsel, sample, "next_pid");
- const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
+ const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
struct work_atoms *out_events, *in_events;
struct thread *sched_out, *sched_in;
u64 timestamp0, timestamp = sample->time;
@@ -1157,7 +1143,7 @@ static int latency_switch_event(struct perf_sched *sched,
goto out_put;
}
}
- if (add_sched_out_event(out_events, sched_out_state(prev_state), timestamp))
+ if (add_sched_out_event(out_events, prev_state, timestamp))
return -1;
in_events = thread_atoms_search(&sched->atom_root, sched_in, &sched->cmp_pid);
@@ -2022,24 +2008,12 @@ static void timehist_header(struct perf_sched *sched)
printf("\n");
}
-static char task_state_char(struct thread *thread, int state)
-{
- static const char state_to_char[] = TASK_STATE_TO_CHAR_STR;
- unsigned bit = state ? ffs(state) : 0;
-
- /* 'I' for idle */
- if (thread__tid(thread) == 0)
- return 'I';
-
- return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
-}
-
static void timehist_print_sample(struct perf_sched *sched,
struct evsel *evsel,
struct perf_sample *sample,
struct addr_location *al,
struct thread *thread,
- u64 t, int state)
+ u64 t, const char state)
{
struct thread_runtime *tr = thread__priv(thread);
const char *next_comm = evsel__strval(evsel, sample, "next_comm");
@@ -2080,7 +2054,7 @@ static void timehist_print_sample(struct perf_sched *sched,
print_sched_time(tr->dt_run, 6);
if (sched->show_state)
- printf(" %5c ", task_state_char(thread, state));
+ printf(" %5c ", thread->tid == 0 ? 'I' : state);
if (sched->show_next) {
snprintf(nstr, sizeof(nstr), "next: %s[%d]", next_comm, next_pid);
@@ -2152,9 +2126,9 @@ static void timehist_update_runtime_stats(struct thread_runtime *r,
else if (r->last_time) {
u64 dt_wait = tprev - r->last_time;
- if (r->last_state == TASK_RUNNING)
+ if (r->last_state == 'R')
r->dt_preempt = dt_wait;
- else if (r->last_state == TASK_UNINTERRUPTIBLE)
+ else if (r->last_state == 'D')
r->dt_iowait = dt_wait;
else
r->dt_sleep = dt_wait;
@@ -2579,7 +2553,7 @@ static int timehist_sched_change_event(struct perf_tool *tool,
struct thread_runtime *tr = NULL;
u64 tprev, t = sample->time;
int rc = 0;
- int state = evsel__intval(evsel, sample, "prev_state");
+ const char state = evsel__taskstate(evsel, sample, "prev_state");
addr_location__init(&al);
if (machine__resolve(machine, &al, sample) < 0) {