diff mbox series

[RFC,v6,5/5] perf sched: prefer to use prev_state_char introduced in sched_switch

Message ID 20230803083352.1585-6-zegao@tencent.com
State New
Headers
Series fix task state report from sched tracepoint |

Commit Message

Ze Gao Aug. 3, 2023, 8:33 a.m. UTC 
  Since the sched_switch tracepoint introduces a new variable to
report sched-out task state in symbolic char, we prefer to use
it to spare from knowing internal implementations in kernel.

Also we keep the old parsing logic intact but sync the state char
array with the latest kernel.

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

Comments

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

> Since the sched_switch tracepoint introduces a new variable to
> report sched-out task state in symbolic char, we prefer to use
> it to spare from knowing internal implementations in kernel.

The above needs to be rewritten to be more comprehensive.

Feel free to reply to this thread with new versions of the change log and
such. No need to send a new patch series to fix this before you know that
the new version is acceptable or not.

Replying to a current patch series is fine, but sending out a new one
causes the spam as it's much easier to ignore a thread than to ignore a new
thread with a new series.

Thanks,

-- Steve


> 
> Also we keep the old parsing logic intact but sync the state char
> array with the latest kernel.
> 
> Signed-off-by: Ze Gao <zegao@tencent.com>
> ---
>  tools/perf/builtin-sched.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 5042874ba204..6f99a36206c9 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -837,7 +837,7 @@ replay_wakeup_event(struct perf_sched *sched,
>  
>  static inline char task_state_char(int state)
>  {
> -	static const char state_to_char[] = "RSDTtXZPI";
> +	static const char state_to_char[] = "RSDTtXZPIp";
>  	unsigned int bit = state ? ffs(state) : 0;
>  
>  	return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
> @@ -846,9 +846,20 @@ static inline char task_state_char(int state)
>  static inline char get_task_prev_state(struct evsel *evsel,
>  				       struct perf_sample *sample)
>  {
> -	const int prev_state = evsel__intval(evsel, sample, "prev_state");
> +	char prev_state_char;
> +	int prev_state;
>  
> -	return task_state_char(prev_state);
> +	/* prefer to use prev_state_char */
> +	if (evsel__field(evsel, "prev_state_char"))
> +		prev_state_char = (char) evsel__intval(evsel,
> +				sample, "prev_state_char");
> +	else {
> +		prev_state = (int) evsel__intval(evsel,
> +				sample, "prev_state");
> +		prev_state_char = task_state_char(prev_state);
> +	}
> +
> +	return prev_state_char;
>  }
>  
>  static int replay_switch_event(struct perf_sched *sched,
> @@ -2145,7 +2156,7 @@ 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 == 'R')
> +			if (r->last_state == 'R' || r->last_state == 'p')
>  				r->dt_preempt = dt_wait;
>  			else if (r->last_state == 'D')
>  				r->dt_iowait = dt_wait;
Ze Gao Aug. 3, 2023, 11:01 a.m. UTC | #2 
On Thu, Aug 3, 2023 at 5:34 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu,  3 Aug 2023 04:33:52 -0400
> Ze Gao <zegao2021@gmail.com> wrote:
>
> > Since the sched_switch tracepoint introduces a new variable to
> > report sched-out task state in symbolic char, we prefer to use
> > it to spare from knowing internal implementations in kernel.
>
> The above needs to be rewritten to be more comprehensive.
> Feel free to reply to this thread with new versions of the change log and
> such. No need to send a new patch series to fix this before you know that
> the new version is acceptable or not.

Copy that! Thank you.

> Replying to a current patch series is fine, but sending out a new one
> causes the spam as it's much easier to ignore a thread than to ignore a new
> thread with a new series.

Thanks Steven!  Lesson learned.

Regards,
Ze

>
>
> >
> > Also we keep the old parsing logic intact but sync the state char
> > array with the latest kernel.
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> >  tools/perf/builtin-sched.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 5042874ba204..6f99a36206c9 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -837,7 +837,7 @@ replay_wakeup_event(struct perf_sched *sched,
> >
> >  static inline char task_state_char(int state)
> >  {
> > -     static const char state_to_char[] = "RSDTtXZPI";
> > +     static const char state_to_char[] = "RSDTtXZPIp";
> >       unsigned int bit = state ? ffs(state) : 0;
> >
> >       return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
> > @@ -846,9 +846,20 @@ static inline char task_state_char(int state)
> >  static inline char get_task_prev_state(struct evsel *evsel,
> >                                      struct perf_sample *sample)
> >  {
> > -     const int prev_state = evsel__intval(evsel, sample, "prev_state");
> > +     char prev_state_char;
> > +     int prev_state;
> >
> > -     return task_state_char(prev_state);
> > +     /* prefer to use prev_state_char */
> > +     if (evsel__field(evsel, "prev_state_char"))
> > +             prev_state_char = (char) evsel__intval(evsel,
> > +                             sample, "prev_state_char");
> > +     else {
> > +             prev_state = (int) evsel__intval(evsel,
> > +                             sample, "prev_state");
> > +             prev_state_char = task_state_char(prev_state);
> > +     }
> > +
> > +     return prev_state_char;
> >  }
> >
> >  static int replay_switch_event(struct perf_sched *sched,
> > @@ -2145,7 +2156,7 @@ 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 == 'R')
> > +                     if (r->last_state == 'R' || r->last_state == 'p')
> >                               r->dt_preempt = dt_wait;
> >                       else if (r->last_state == 'D')
> >                               r->dt_iowait = dt_wait;
>
diff mbox series

Patch

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 5042874ba204..6f99a36206c9 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -837,7 +837,7 @@  replay_wakeup_event(struct perf_sched *sched,
 
 static inline char task_state_char(int state)
 {
-	static const char state_to_char[] = "RSDTtXZPI";
+	static const char state_to_char[] = "RSDTtXZPIp";
 	unsigned int bit = state ? ffs(state) : 0;
 
 	return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
@@ -846,9 +846,20 @@  static inline char task_state_char(int state)
 static inline char get_task_prev_state(struct evsel *evsel,
 				       struct perf_sample *sample)
 {
-	const int prev_state = evsel__intval(evsel, sample, "prev_state");
+	char prev_state_char;
+	int prev_state;
 
-	return task_state_char(prev_state);
+	/* prefer to use prev_state_char */
+	if (evsel__field(evsel, "prev_state_char"))
+		prev_state_char = (char) evsel__intval(evsel,
+				sample, "prev_state_char");
+	else {
+		prev_state = (int) evsel__intval(evsel,
+				sample, "prev_state");
+		prev_state_char = task_state_char(prev_state);
+	}
+
+	return prev_state_char;
 }
 
 static int replay_switch_event(struct perf_sched *sched,
@@ -2145,7 +2156,7 @@  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 == 'R')
+			if (r->last_state == 'R' || r->last_state == 'p')
 				r->dt_preempt = dt_wait;
 			else if (r->last_state == 'D')
 				r->dt_iowait = dt_wait;