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

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

[yangoliver]

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@gmail.com>
---
 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++;
-- 
2.4.0

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

[Steven Rostedt]

[ Adding Peter Zijlstra to this ]

-- Steve


On Mon, 27 Jul 2015 09:11:52 -0400
yangoliver <yang_oliver@hotmail.com> 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@gmail.com>
> ---
>  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++;

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

[Peter Zijlstra]

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@hotmail.com> 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@gmail.com>
> > ---
> >  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.

And wtf are you doing sending sched patches and not Cc maintainers.

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@hotmail.com> 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@gmail.com>
>>> ---
>>>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/