Re: [PATCH 2/2] sched: Trace point sched_stat_sleep should cover iowait case

[Oliver Yang]

On 2015/8/4 2:43, Peter Zijlstra wrote:
> On Mon, Aug 03, 2015 at 01:35:28PM -0400, Steven Rostedt wrote:
>> On Mon, 27 Jul 2015 09:11:52 -0400
>> yangoliver <yang_oliver@xxxxxxxxxxx> wrote:
>>
>>> Per sched_stat_sleep definition in sched.h, it should include
>>> iowait case. This can also relect the design of sum_sleep_runtime
>>> statistic, as this counter also includes the io_wait.
>>>
>>> Signed-off-by: Yong Yang <yangoliver@xxxxxxxxx>
>>> ---
>>>  kernel/sched/fair.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index d113c3b..85677bf 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -3018,6 +3018,8 @@ static void enqueue_sleeper(struct cfs_rq *cfs_rq, struct sched_entity *se)
>>>  		se->statistics.sum_sleep_runtime += delta;
>>>  
>>>  		if (tsk) {
>>> +			trace_sched_stat_sleep(tsk, delta);
>>> +
>>>  			if (tsk->in_iowait) {
>>>  				se->statistics.iowait_sum += delta;
>>>  				se->statistics.iowait_count++;
> No, that's broken in two ways. Firstly you don't change semantics of
> stuff just because of a comment and secondly iowait has nothing what all
> to do with INTERRUPTIBLE/sleep vs UNINTERRUPTIBLE/blocked.
Peter,

Sorry for missing key person in this mail thread.

Another reason I think sched_stat_sleep should cover UNINTERRUPTIBLE/blocked case
is, the sum_sleep_runtime counter get increased for both INTERRUPTIBLE and
UNINTERRUPTIBLE cases. We can find below statement for both cases in the code,
   
    se->statistics.sum_sleep_runtime += delta;

Plus below comments, I guessed the sched_stat_sleep trace point is originally
designed for cover all kind of sleep cases: interruptible and uninterruptible,

/*
 * Tracepoint for accounting sleep time (time the task is not runnable,
 * including iowait, see below).
 */
DEFINE_EVENT(sched_stat_template, sched_stat_sleep,
         TP_PROTO(struct task_struct *tsk, u64 delay),
         TP_ARGS(tsk, delay));

Do you think we should make sched_stat_sleep meaning similar with the
sum_sleep_runtime counter?

If not, we may need fix the comments in sched.h above.
>
> And wtf are you doing sending sched patches and not Cc maintainers.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/