diff mbox series

[RFC,v6,2/5] perf sched: reorganize sched-out task state report code

Message ID 20230803083352.1585-3-zegao@tencent.com (mailing list archive)
State Handled Elsewhere
Headers show
Series fix task state report from sched tracepoint | expand

Commit Message

Ze Gao Aug. 3, 2023, 8:33 a.m. UTC 
Mainly does housekeeping work and not introduce any
functional change.

Signed-off-by: Ze Gao <zegao@tencent.com>
---
 tools/perf/builtin-sched.c | 59 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

Comments

Steven Rostedt Aug. 3, 2023, 9:10 a.m. UTC | #1 
On Thu,  3 Aug 2023 04:33:49 -0400
Ze Gao <zegao2021@gmail.com> wrote:

> Mainly does housekeeping work and not introduce any
> functional change.

This change log doesn't explain at all why this patch is needed, let alone
what it is even doing.

-- Steve


> 
> Signed-off-by: Ze Gao <zegao@tencent.com>
> ---
>  tools/perf/builtin-sched.c | 59 +++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 8dc8f071721c..5042874ba204 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -94,11 +94,6 @@ struct sched_atom {
>  
>  #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 +250,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 +420,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, char task_state __maybe_unused)
>  {
>  	struct sched_atom *event = get_new_event(task, timestamp);
>  
> @@ -840,6 +835,22 @@ replay_wakeup_event(struct perf_sched *sched,
>  	return 0;
>  }
>  
> +static inline char task_state_char(int state)
> +{
> +	static const char state_to_char[] = "RSDTtXZPI";
> +	unsigned int bit = state ? ffs(state) : 0;
> +
> +	return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
> +}
> +
> +static inline char get_task_prev_state(struct evsel *evsel,
> +				       struct perf_sample *sample)
> +{
> +	const int prev_state = evsel__intval(evsel, sample, "prev_state");
> +
> +	return task_state_char(prev_state);
> +}
> +
>  static int replay_switch_event(struct perf_sched *sched,
>  			       struct evsel *evsel,
>  			       struct perf_sample *sample,
> @@ -849,7 +860,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 = get_task_prev_state(evsel, sample);
>  	struct task_desc *prev, __maybe_unused *next;
>  	u64 timestamp0, timestamp = sample->time;
>  	int cpu = sample->cpu;
> @@ -1039,12 +1050,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,
> @@ -1121,7 +1126,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 = get_task_prev_state(evsel, sample);
>  	struct work_atoms *out_events, *in_events;
>  	struct thread *sched_out, *sched_in;
>  	u64 timestamp0, timestamp = sample->time;
> @@ -1157,7 +1162,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 +2027,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, char state)
>  {
>  	struct thread_runtime *tr = thread__priv(thread);
>  	const char *next_comm = evsel__strval(evsel, sample, "next_comm");
> @@ -2080,7 +2073,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 +2145,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 +2572,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 = get_task_prev_state(evsel, sample);
>  
>  	addr_location__init(&al);
>  	if (machine__resolve(machine, &al, sample) < 0) {
Ze Gao Aug. 3, 2023, 12:37 p.m. UTC | #2 
Hi Steven,

THIS IS THE NEW CHANGELOG FOR THIS PATCH:

    perf sched: reorganize sched-out task state report code

    Passing around the task state reported by tracepoint as an
    integer creates dependencies both on the task state macros
    and TASK_STATE_TO_CHAR_STR.

    Actually we can simplify this by computing the state based
    on TASK_STATE_TO_CHAR_STR and then pass the result as a
    'char', which saves us from using these macros anymore.
    So we can remove them for good.

    Note that sched_out_state() is basically doing the same
    thing as task_state_char(), so combine them into one
    and provide an intended helper get_task_prev_state() for
    extracting task state from perf record.

    IOW, this patch does not introduce any functional changes.
    mainly for housekeeping.

    Signed-off-by: Ze Gao <zegao@tencent.com>

Suggestions?

Regards,
Ze

On Thu, Aug 3, 2023 at 5:10 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu,  3 Aug 2023 04:33:49 -0400
> Ze Gao <zegao2021@gmail.com> wrote:
>
> > Mainly does housekeeping work and not introduce any
> > functional change.
>
> This change log doesn't explain at all why this patch is needed, let alone
> what it is even doing.
>
> -- Steve
>
>
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> >  tools/perf/builtin-sched.c | 59 +++++++++++++++++---------------------
> >  1 file changed, 26 insertions(+), 33 deletions(-)
> >
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 8dc8f071721c..5042874ba204 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -94,11 +94,6 @@ struct sched_atom {
> >
> >  #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 +250,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 +420,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, char task_state __maybe_unused)
> >  {
> >       struct sched_atom *event = get_new_event(task, timestamp);
> >
> > @@ -840,6 +835,22 @@ replay_wakeup_event(struct perf_sched *sched,
> >       return 0;
> >  }
> >
> > +static inline char task_state_char(int state)
> > +{
> > +     static const char state_to_char[] = "RSDTtXZPI";
> > +     unsigned int bit = state ? ffs(state) : 0;
> > +
> > +     return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
> > +}
> > +
> > +static inline char get_task_prev_state(struct evsel *evsel,
> > +                                    struct perf_sample *sample)
> > +{
> > +     const int prev_state = evsel__intval(evsel, sample, "prev_state");
> > +
> > +     return task_state_char(prev_state);
> > +}
> > +
> >  static int replay_switch_event(struct perf_sched *sched,
> >                              struct evsel *evsel,
> >                              struct perf_sample *sample,
> > @@ -849,7 +860,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 = get_task_prev_state(evsel, sample);
> >       struct task_desc *prev, __maybe_unused *next;
> >       u64 timestamp0, timestamp = sample->time;
> >       int cpu = sample->cpu;
> > @@ -1039,12 +1050,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,
> > @@ -1121,7 +1126,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 = get_task_prev_state(evsel, sample);
> >       struct work_atoms *out_events, *in_events;
> >       struct thread *sched_out, *sched_in;
> >       u64 timestamp0, timestamp = sample->time;
> > @@ -1157,7 +1162,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 +2027,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, char state)
> >  {
> >       struct thread_runtime *tr = thread__priv(thread);
> >       const char *next_comm = evsel__strval(evsel, sample, "next_comm");
> > @@ -2080,7 +2073,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 +2145,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 +2572,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 = get_task_prev_state(evsel, sample);
> >
> >       addr_location__init(&al);
> >       if (machine__resolve(machine, &al, sample) < 0) {
>
diff mbox series

Patch

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 8dc8f071721c..5042874ba204 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -94,11 +94,6 @@  struct sched_atom {
 
 #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 +250,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 +420,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, char task_state __maybe_unused)
 {
 	struct sched_atom *event = get_new_event(task, timestamp);
 
@@ -840,6 +835,22 @@  replay_wakeup_event(struct perf_sched *sched,
 	return 0;
 }
 
+static inline char task_state_char(int state)
+{
+	static const char state_to_char[] = "RSDTtXZPI";
+	unsigned int bit = state ? ffs(state) : 0;
+
+	return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
+}
+
+static inline char get_task_prev_state(struct evsel *evsel,
+				       struct perf_sample *sample)
+{
+	const int prev_state = evsel__intval(evsel, sample, "prev_state");
+
+	return task_state_char(prev_state);
+}
+
 static int replay_switch_event(struct perf_sched *sched,
 			       struct evsel *evsel,
 			       struct perf_sample *sample,
@@ -849,7 +860,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 = get_task_prev_state(evsel, sample);
 	struct task_desc *prev, __maybe_unused *next;
 	u64 timestamp0, timestamp = sample->time;
 	int cpu = sample->cpu;
@@ -1039,12 +1050,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,
@@ -1121,7 +1126,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 = get_task_prev_state(evsel, sample);
 	struct work_atoms *out_events, *in_events;
 	struct thread *sched_out, *sched_in;
 	u64 timestamp0, timestamp = sample->time;
@@ -1157,7 +1162,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 +2027,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, char state)
 {
 	struct thread_runtime *tr = thread__priv(thread);
 	const char *next_comm = evsel__strval(evsel, sample, "next_comm");
@@ -2080,7 +2073,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 +2145,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 +2572,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 = get_task_prev_state(evsel, sample);
 
 	addr_location__init(&al);
 	if (machine__resolve(machine, &al, sample) < 0) {