Re: [PATCH v2] perf: Timehist account sch delay for scheduled out running

[Madadi Vineeth Reddy]

Hi Fernand,

On 18/06/24 14:33, Fernand Sieber wrote:
> When using perf timehist, sch delay is only computed for a waking task,
> not for a pre empted task. This patches changes sch delay to account for
> both. This makes sense as testing scheduling policy need to consider the
> effect of scheduling delay globally, not only for waking tasks.
> 
> Example of `perf timehist` report before the patch for `stress` task
> competing with each other.
> 
> First column is wait time, second column sch delay, third column
> runtime.
> 
> 1.492060 [0000]  s    stress[81]                          1.999      0.000      2.000      R  next: stress[83]
> 1.494060 [0000]  s    stress[83]                          2.000      0.000      2.000      R  next: stress[81]
> 1.496060 [0000]  s    stress[81]                          2.000      0.000      2.000      R  next: stress[83]
> 1.498060 [0000]  s    stress[83]                          2.000      0.000      1.999      R  next: stress[81]
> 
> After the patch, it looks like this (note that all wait time is not zero
> anymore):
> 
> 1.492060 [0000]  s    stress[81]                          1.999      1.999      2.000      R  next: stress[83]
> 1.494060 [0000]  s    stress[83]                          2.000      2.000      2.000      R  next: stress[81]
> 1.496060 [0000]  s    stress[81]                          2.000      2.000      2.000      R  next: stress[83]
> 1.498060 [0000]  s    stress[83]                          2.000      2.000      1.999      R  next: stress[81]
> 
> Signed-off-by: Fernand Sieber <sieberf@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Madadi Vineeth Reddy <vineethr@xxxxxxxxxxxxx>
> ---
>  tools/perf/Documentation/perf-sched.txt | 4 ++--
>  tools/perf/builtin-sched.c              | 5 ++++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-sched.txt b/tools/perf/Documentation/perf-sched.txt
> index a216d2991b19..74c812f7a4a4 100644
> --- a/tools/perf/Documentation/perf-sched.txt
> +++ b/tools/perf/Documentation/perf-sched.txt
> @@ -64,8 +64,8 @@ There are several variants of 'perf sched':
>      
>     By default it shows the individual schedule events, including the wait
>     time (time between sched-out and next sched-in events for the task), the
> -   task scheduling delay (time between wakeup and actually running) and run
> -   time for the task:
> +   task scheduling delay (time between runnable and actually running) and
> +   run time for the task:

LGTM.

Reviewed-by: Madadi Vineeth Reddy <vineethr@xxxxxxxxxxxxx>

Thanks and Regards
Madadi Vineeth Reddy

>      
>                  time    cpu  task name             wait time  sch delay   run time
>                               [tid/pid]                (msec)     (msec)     (msec)
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 5977c49ae2c7..7422c930abaf 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2659,7 +2659,10 @@ static int timehist_sched_change_event(struct perf_tool *tool,
>  		tr->last_state = state;
>  
>  		/* sched out event for task so reset ready to run time */
> -		tr->ready_to_run = 0;
> +		if (state == 'R')
> +			tr->ready_to_run = t;
> +		else
> +			tr->ready_to_run = 0;
>  	}
>  
>  	evsel__save_time(evsel, sample->time, sample->cpu);