lkml.org 
[lkml]   [2006]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Patch 3/8] cpu delays
Andrew Morton wrote:

>Shailabh Nagar <nagar@watson.ibm.com> wrote:
>
>
>>delayacct-schedstats.patch
>>
>>Make the task-related schedstats functions
>>callable by delay accounting even if schedstats
>>collection isn't turned on. This removes the
>>dependency of delay accounting on schedstats and allows
>>cpu delays to be exported.
>>
>>..
>>
>>Index: linux-2.6.16/include/linux/sched.h
>>===================================================================
>>--- linux-2.6.16.orig/include/linux/sched.h 2006-03-29 18:13:13.000000000 -0500
>>+++ linux-2.6.16/include/linux/sched.h 2006-03-29 18:13:15.000000000 -0500
>>@@ -525,7 +525,7 @@ typedef struct prio_array prio_array_t;
>> struct backing_dev_info;
>> struct reclaim_state;
>>
>>-#ifdef CONFIG_SCHEDSTATS
>>+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
>> struct sched_info {
>> /* cumulative counters */
>> unsigned long cpu_time, /* time spent on the cpu */
>>@@ -537,10 +537,14 @@ struct sched_info {
>> last_queued; /* when we were last queued to run */
>> };
>>
>>+#ifdef CONFIG_SCHEDSTATS
>> extern struct file_operations proc_schedstat_operations;
>>-#endif
>>+#endif /* CONFIG_SCHEDSTATS */
>>+#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */
>>
>> #ifdef CONFIG_TASK_DELAY_ACCT
>>+extern int delayacct_on;
>>+
>>
>>
>
>This was already declared in delayacct.h and nothing in sched.h needs it.
>So it's better if .c files pick this declaration up from delayacct.h.
>
>
>
>>+static inline void rq_sched_info_arrive(struct runqueue *rq,
>>+ unsigned long diff)
>>+{
>>+ if (rq) {
>>+ rq->rq_sched_info.run_delay += diff;
>>+ rq->rq_sched_info.pcnt++;
>>+ }
>>+}
>>
>>
>
>The nonatomic updates need locking. I assume it's runqueue.lock, but a
>comment describing the rules would be good.
>
>
>
Yes, it is rq.lock. Will add comment.

>>+static inline void rq_sched_info_depart(struct runqueue *rq,
>>+ unsigned long diff)
>>+{
>>+ if (rq)
>>+ rq->rq_sched_info.cpu_time += diff;
>>+}
>>
>>
>
>Ditto.
>
>
Will add comment.

>
>
>> static inline void sched_info_queued(task_t *t)
>> {
>>- if (!t->sched_info.last_queued)
>>- t->sched_info.last_queued = jiffies;
>>+ if (unlikely(sched_info_on()))
>>+ if (!t->sched_info.last_queued)
>>+ t->sched_info.last_queued = jiffies;
>> }
>>
>>
>
>It might be better to put the unlikely() into sched_info_on() itself.
>
>
The function sched_info_on has only got a return statement so putting
the unlikely within
it doesn't seem possible.

This is a rather aggressive use of the unlikely. In the case where
CONFIG_SCHEDSTATS is
defined, sched_info_on always returns 1 so the unlikely will always
cause a mispredict. I'm assuming
the extra penalty for that is acceptable for those using schedstats
since its never turned on in a production
kernel.
If only CONFIG_TASK_DELAY_ACCT is configured, the value of delayacct_on,
which is very
likely going to be 0, will be returned so the unlikely is potentially
helpful to reduce overhead.

Thoughts ?

As I write this, I realize the function sched_info_on() is also wrongly
written and doesn't account for the
case where both SCHEDSTATS and TASK_DELAY_ACCT are configured. Will fix
that. But thats
orthogonal to the above discussion of whether the unlikely should be used.

--Shailabh


-
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/

\
 
 \ /
  Last update: 2006-03-30 18:03    [W:0.285 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site