[PATCH] Fix delay accounting regression

[PATCH] Fix delay accounting regression

[Balbir Singh]

Fix the delay accounting regression introduced by commit
75d4ef16a6aa84f708188bada182315f80aab6fa. rq no longer has sched_info
data associated with it. task_struct sched_info structure is used by delay
accounting to provide back statistics to user space.

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 kernel/sched_stats.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN kernel/sched_stats.h~delayacct-define-bug kernel/sched_stats.h
--- linux-2.6-latest/kernel/sched_stats.h~delayacct-define-bug	2007-11-02 19:02:07.000000000 +0530
+++ linux-2.6-latest-balbir/kernel/sched_stats.h	2007-11-02 19:51:30.000000000 +0530
@@ -127,7 +127,7 @@ rq_sched_info_depart(struct rq *rq, unsi
 # define schedstat_set(var, val)	do { } while (0)
 #endif
 
-#ifdef CONFIG_SCHEDSTATS
+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
 /*
  * Called when a process is dequeued from the active array and given
  * the cpu.  We should note that with the exception of interactive
@@ -231,5 +231,5 @@ sched_info_switch(struct task_struct *pr
 #else
 #define sched_info_queued(t)		do { } while (0)
 #define sched_info_switch(t, next)	do { } while (0)
-#endif /* CONFIG_SCHEDSTATS */
+#endif /* CONFIG_SCHEDSTATS || CONFIG_TASK_DELAY_ACCT */
 
_

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [PATCH] Fix delay accounting regression

[Ingo Molnar]

* Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> Fix the delay accounting regression introduced by commit 
> 75d4ef16a6aa84f708188bada182315f80aab6fa. rq no longer has sched_info 
> data associated with it. task_struct sched_info structure is used by 
> delay accounting to provide back statistics to user space.

no, this really is broken code. sched_clock() is not a generally 
reliable time source so at a minimum the rq->clock fix i mentioned is 
needed. What user-space tools are utilizing delay-accounting by the way? 
We have most of this data available under SCHEDSTATS - how about 
consolidating CONFIG_TASK_DELAY_ACCT into CONFIG_SCHEDSTATS?

	Ingo

Re: [PATCH] Fix delay accounting regression

[Guillaume Chazarain]

On 11/2/07, Ingo Molnar <mingo@elte.hu> wrote:

> What user-space tools are utilizing delay-accounting by the way?

Thanks for the plugging opportunity ;-)
http://guichaz.free.fr/misc/#iotop uses the I/O side of delay-accounting.

-- 
Guillaume

Re: [PATCH] Fix delay accounting regression

[Balbir Singh]

Ingo Molnar wrote:
> * Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
>> Fix the delay accounting regression introduced by commit 
>> 75d4ef16a6aa84f708188bada182315f80aab6fa. rq no longer has sched_info 
>> data associated with it. task_struct sched_info structure is used by 
>> delay accounting to provide back statistics to user space.
> 
> no, this really is broken code. sched_clock() is not a generally 
> reliable time source so at a minimum the rq->clock fix i mentioned is 
> needed. What user-space tools are utilizing delay-accounting by the way? 

Applications I can think of are

1. iotop
2. Some typical applications are mentioned in
   https://ols2006.108.redhat.com/2007/Reprints/nagar-Reprint.pdf

> We have most of this data available under SCHEDSTATS - how about 
> consolidating CONFIG_TASK_DELAY_ACCT into CONFIG_SCHEDSTATS?
> 
> 	Ingo


Yes, we do use schedstats - except for two cases that I know of?

1. To accumulate information in taskstats to share with user space
   (we copy the data from schedstats into taskstats)
2. We accumulate the data for dead threads in a thread group

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [PATCH] Fix delay accounting regression

[Balbir Singh]

On Fri, Nov 02, 2007 at 04:53:58PM +0100, Ingo Molnar wrote:
> 
> * Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > Fix the delay accounting regression introduced by commit 
> > 75d4ef16a6aa84f708188bada182315f80aab6fa. rq no longer has sched_info 
> > data associated with it. task_struct sched_info structure is used by 
> > delay accounting to provide back statistics to user space.
> 
> no, this really is broken code. sched_clock() is not a generally 
> reliable time source so at a minimum the rq->clock fix i mentioned is 
> needed. What user-space tools are utilizing delay-accounting by the way? 
> We have most of this data available under SCHEDSTATS - how about 
> consolidating CONFIG_TASK_DELAY_ACCT into CONFIG_SCHEDSTATS?
> 
> 	Ingo

Hi, Ingo,

How about this?


Changelog
1. As per Ingo Molnar's recommendation, use rq->clock instead of sched_clock().


Fix the delay accounting regression introduced by commit
75d4ef16a6aa84f708188bada182315f80aab6fa. rq no longer has sched_info
data associated with it. task_struct sched_info structure is used by delay
accounting to provide back statistics to user space.


Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 kernel/sched_stats.h |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff -puN kernel/sched_stats.h~delayacct-define-bug kernel/sched_stats.h
--- linux-2.6-latest/kernel/sched_stats.h~delayacct-define-bug	2007-11-02 19:02:07.000000000 +0530
+++ linux-2.6-latest-balbir/kernel/sched_stats.h	2007-11-02 22:11:21.000000000 +0530
@@ -127,7 +127,7 @@ rq_sched_info_depart(struct rq *rq, unsi
 # define schedstat_set(var, val)	do { } while (0)
 #endif
 
-#ifdef CONFIG_SCHEDSTATS
+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
 /*
  * Called when a process is dequeued from the active array and given
  * the cpu.  We should note that with the exception of interactive
@@ -155,7 +155,7 @@ static inline void sched_info_dequeued(s
  */
 static void sched_info_arrive(struct task_struct *t)
 {
-	unsigned long long now = sched_clock(), delta = 0;
+	unsigned long long now = task_rq(t)->clock, delta = 0;
 
 	if (t->sched_info.last_queued)
 		delta = now - t->sched_info.last_queued;
@@ -186,7 +186,7 @@ static inline void sched_info_queued(str
 {
 	if (unlikely(sched_info_on()))
 		if (!t->sched_info.last_queued)
-			t->sched_info.last_queued = sched_clock();
+			t->sched_info.last_queued = task_rq(t)->clock;
 }
 
 /*
@@ -195,7 +195,8 @@ static inline void sched_info_queued(str
  */
 static inline void sched_info_depart(struct task_struct *t)
 {
-	unsigned long long delta = sched_clock() - t->sched_info.last_arrival;
+	unsigned long long delta = task_rq(t)->clock -
+					t->sched_info.last_arrival;
 
 	t->sched_info.cpu_time += delta;
 	rq_sched_info_depart(task_rq(t), delta);
@@ -231,5 +232,5 @@ sched_info_switch(struct task_struct *pr
 #else
 #define sched_info_queued(t)		do { } while (0)
 #define sched_info_switch(t, next)	do { } while (0)
-#endif /* CONFIG_SCHEDSTATS */
+#endif /* CONFIG_SCHEDSTATS || CONFIG_TASK_DELAY_ACCT */
 
_

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [PATCH] Fix delay accounting regression

[Ingo Molnar]

* Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> On Fri, Nov 02, 2007 at 04:53:58PM +0100, Ingo Molnar wrote:
> > 
> > * Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> > 
> > > Fix the delay accounting regression introduced by commit 
> > > 75d4ef16a6aa84f708188bada182315f80aab6fa. rq no longer has sched_info 
> > > data associated with it. task_struct sched_info structure is used by 
> > > delay accounting to provide back statistics to user space.
> > 
> > no, this really is broken code. sched_clock() is not a generally 
> > reliable time source so at a minimum the rq->clock fix i mentioned is 
> > needed. What user-space tools are utilizing delay-accounting by the way? 
> > We have most of this data available under SCHEDSTATS - how about 
> > consolidating CONFIG_TASK_DELAY_ACCT into CONFIG_SCHEDSTATS?
> > 
> > 	Ingo
> 
> Hi, Ingo,
> 
> How about this?

thanks, applied.

	Ingo