Re: [Patch 3/8] cpu delays

From: Shailabh Nagar
Date: Thu Mar 30 2006 - 10:58:32 EST


Andrew Morton wrote:

Shailabh Nagar <nagar@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/